Repository navigation
choose configuration profile UI (WP-1022) - #638
vsolovei-smartling wants to merge 10 commits into
Conversation
First step of letting a user choose which Smartling profile/project a translation request goes to, instead of relying on whichever profile is flagged "active": the request model itself now requires the caller to say which profile it's for, validated the same way the existing required job/source fields are. Updated every constructor/fromArray() call site (TestRunController, and the test suites exercising UserTranslationRequest/ ContentRelationsDiscoveryService/ContentRelationsHandler) to pass one. Note: ContentRelationsDiscoveryService::createSubmissions() doesn't read getProfileId() yet - it still resolves the active profile unconditionally. That wiring, plus the frontend profile selector that will actually populate this field in real AJAX requests, are separate steps in the same ticket.
createSubmissions() always resolved the profile via getSingleSettingsProfile($curBlogId) - whichever one happens to be flagged active - ignoring UserTranslationRequest::getProfileId() entirely. Now resolves the requested profile explicitly (validating it exists, belongs to the current blog, and is active), falling back to today's active-profile behavior only if the requested id is stale (profile deactivated/deleted client-side after the page loaded) rather than hard-failing the whole request. This is the single resolution point both the bulk and non-bulk branches already share, so the profile-stamping added for WP-1021 (createBatch(), setConfigurationProfileId() at enqueue time) now automatically uses the user's explicit choice with no further changes needed at those call sites.
list-jobs/create-job previously always resolved the blog's active profile via getSingleSettingsProfile(), ignoring any profileId the client may have selected. Mirror the fallback behavior already added to ContentRelationsDiscoveryService: use the requested profile when it exists, belongs to the current blog, and is active; otherwise fall back to the active profile. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Surface all active profiles for the current blog (not just the first one) to the React job wizard shared by the post/taxonomy edit screen and bulk submit page, each with its own target-locale set. Add a profile dropdown (shown only when more than one profile applies), remember the chosen profile per-blog in localStorage, and send its id with list-jobs/create-job/smartling-create-submissions so the request is routed to the profile the user picked instead of whichever one happens to be flagged active. Switching profiles reconciles the selected target locales and refreshes the existing-jobs list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The per-post "Smartling Widget" (side metabox) only showed a link to a target placeholder post once its row had been rendered server-side on page load, so after queuing a new upload from the job wizard the user had to manually reload the page to get a link to the freshly created placeholder. Add an AJAX endpoint that re-renders that metabox's markup on demand, and have the job wizard poll it with a short backoff after a successful (non bulk-submit) upload, swapping in the refreshed widget until every requested target locale has an edit link or the poll window runs out. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…1022) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… client (WP-1022) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
sl-mmuradov
left a comment
There was a problem hiding this comment.
Thanks, centralizing the lookup in resolveRequestedProfile() with a strict check (exists, same blog, active) is the right approach, and keeping error details server-side is good. A few things block merging, though:
Blocking
- Related content still uses the default profile.
SubmissionManager::getSubmissionEntity()always stampsgetSingleSettingsProfile(), even on existing submissions. Related content created during upload or download (referenced posts and terms, images,downloadTranslation) therefore goes to profile A while the parent and job are on B, and existing B submissions are re-stamped to A. This defeats the main goal of the ticket for any content with relations. - Target blogs aren't validated against the chosen profile in
createSubmissions,create-jobor instant translation. The result is an empty Smartling locale and a failure later on. - The widget refresh breaks the side widget. The
outerHTMLswap drops the Download and checkbox handlers. The AJAX action is registered by every post-type controller, so only the first post type renders correct submissions. Taxonomy screens poll with a term id aspostId. ContentRelationsHandlerreports everySmartlingDbExceptionas "Invalid translation profile", which hides real DB errors.
Should fix
- Re-stamping in-progress submissions under a different profile leaves them pointing at the wrong project. Block or warn.
- The wizard leaves the old profile's jobs selectable after a switch, doesn't clear its error, and re-fires all relation requests.
- The side widget merges locales from all profiles, but its legacy upload path uses only the first profile.
Scope question: the ticket asks for a per-blog default profile. Right now it's "first active by id" plus localStorage. Is that a follow-up ticket?
Tests: please add tests for ajaxRefreshWidgetHandler, the job proxy's profile 400 path, the handler's exception mapping, related-content stamping when the requested profile isn't the default, and Playwright coverage for the dropdown, the reset on switch and the widget refresh.
Inline comments have details and suggested fixes.
| // them with the profile this request's batch is being created under (below), so | ||
| // UploadJob never has to guess at a profile for them later. | ||
| // New submissions built from this template bypass getSubmissionEntity(), stamp them with the requested profile. | ||
| $submissionTemplateArray[SubmissionEntity::FIELD_CONFIGURATION_PROFILE_ID] = $profile->getId(); |
There was a problem hiding this comment.
[High] Parent submissions are stamped with the requested profile here, but related content created later still gets the default profile. SubmissionManager::getSubmissionEntity() → stampConfigurationProfile() → getSingleSettingsProfile() (SubmissionManager.php ~489) runs for new and existing submissions. It's reached from TranslationHelper::tryPrepareRelatedContent() via ReferencedContentProcessor (referenced posts/terms in meta), SmartlingCoreExportApi::sendAttachmentForTranslation() (Gutenberg/Elementor images) and SmartlingCoreDownloadTrait::downloadTranslation().
With profile B chosen, children get profile A's credentials but B's job (JobEntityWithBatchUid::fromJob($submission->getJobInfo())), and existing B submissions are silently re-stamped to A. This was harmless while "active" meant "requested". Now that several profiles can be active, it isn't. Suggest: pass the parent's profile id into related-content creation, and in getSubmissionEntity() stamp only when getConfigurationProfileId() === null.
| { | ||
| $curBlogId = $this->wordpressProxy->get_current_blog_id(); | ||
| $profile = $this->settingsManager->getSingleSettingsProfile($curBlogId); | ||
| $profile = $this->settingsManager->resolveRequestedProfile($request->getProfileId(), $curBlogId); |
There was a problem hiding this comment.
[High] targetBlogIds aren't checked against the resolved profile's enabled target locales. A blog outside profile B (stale tab, crafted request, or a locale picked in the side widget, which now merges all profiles) is accepted. getSmartlingLocaleIdBySettingsProfile() then returns '', so the upload fails later, away from the request. Can we reject it here with a 400? The same applies to create-job (ContentEditJobController.php:161) and instant translation (InstantTranslationController.php:58).
| if (!widget) { | ||
| return; | ||
| } | ||
| widget.outerHTML = response.html; |
There was a problem hiding this comment.
[High] widget.outerHTML = response.html destroys the handlers smartling-connector-admin.js bound at page load. #smartling-download is bound directly (:196), and the checkbox handler is delegated from the widget node itself (:54). After the first refresh, Download does nothing and checkbox value syncing stops until the page is reloaded. Options: replace only the inner content and re-init the handlers, or move those handlers to $(document).on(...) delegation.
| add_action('save_post', [$this, 'save']); // old logic 2 be refactored | ||
| add_action('wp_ajax_' . 'smartling_force_download_handler', [$this, 'ajaxDownloadHandler']); | ||
| add_action('wp_ajax_' . 'smartling_upload_handler', [$this, 'ajaxUploadHandler']); | ||
| add_action('wp_ajax_' . 'smartling_refresh_post_widget', [$this, 'ajaxRefreshWidgetHandler']); |
There was a problem hiding this comment.
[High] CustomPostType::registerWidgetHandler() (CustomPostType.php:63) creates one controller per post type, and each registers this same wp_ajax_smartling_refresh_post_widget. The first registered controller handles every request and wp_send_json ends the request. Its preView() then filters submissions by its own servedContentType, so on a page or CPT the refreshed widget has no statuses. allReady never becomes true, so it polls all 5 times. Suggest registering the action once and using $post->post_type, or checking $post->post_type === $this->servedContentType and returning early so the matching controller can answer.
| throw new Error(submissionResponse.message?.global || 'Failed to add content to upload queue.'); | ||
| } | ||
| setSuccess('Content successfully added to upload queue.'); | ||
| if (!isBulkSubmitPage) { |
There was a problem hiding this comment.
[High] !isBulkSubmitPage is also true on taxonomy term edit screens, which render the wizard (ContentEditJobController::box for WP_Term) and a #smartling-post-widget (taxonomy-based-content-type.php:61). The term id goes out as postId, so get_post(<termId>) either swaps in an unrelated post's widget or returns a 404 on every poll. Please limit polling to the post base type, for example by passing data-base-type to the wizard.
| try { | ||
| $this->service->createSubmissions(UserTranslationRequest::fromArray($data)); | ||
| $this->returnResponse(['status' => BaseAjaxServiceAbstract::RESPONSE_SUCCESS]); | ||
| } catch (SmartlingDbException $e) { |
There was a problem hiding this comment.
[Medium] SmartlingDbException is also thrown by DB, Queue, TaxonomyEntityStd, GravityFormsFormHandler, SmartlingCore and others, all reachable from createSubmissions(). Real DB or content errors will now show as "Invalid translation profile" and the original message is lost. It's also misleading when no profileId is sent and the blog has no active profile. Suggest a dedicated exception for profile resolution, or resolving the profile before this try block like the other two controllers do.
| // that method, so it would otherwise keep whatever profile (or none) it had. | ||
| // Resubmitting an existing submission is an explicit new translation request, so restamp it with the | ||
| // profile it was requested with. | ||
| $submission->setConfigurationProfileId($profile->getId()); |
There was a problem hiding this comment.
[Medium] (also applies to :110 and InstantTranslationController.php:323) Re-stamping an existing submission unconditionally means content that is IN_PROGRESS in project A and resubmitted under B loses its link to A's file and job. Status checks and download then go to B. Should we block or warn when the stored profile differs and the submission isn't completed or failed yet?
| action: 'smartling_job_api_proxy', | ||
| _wpnonce: nonce, | ||
| innerAction: 'list-jobs', | ||
| params: { profileId: selectedProfileId } |
There was a problem hiding this comment.
[Medium] On a profile switch the old job list stays selectable until the new one arrives (and stays forever if the request fails). selectedJob is cleared, but the user can still pick one of A's jobs and submit with profileId=B, which fails at createBatch. Suggest setJobs([]); setLoading(true); setError(''); at the start of the effect. Also, setError('Failed to load jobs') (:111) is never cleared after a later successful load.
| const [activeTab, setActiveTab] = useState('new'); | ||
| const [selectedProfileId, setSelectedProfileId] = useState(() => { | ||
| const stored = getStoredProfileId(blogId); | ||
| return (stored !== null && profiles.some(p => p.id === stored)) ? stored : profiles[0]?.id; |
There was a problem hiding this comment.
[Medium] The ticket says "One profile can be marked default for a blog; the user can override it." Here the default is profiles[0] (DB id order) overridden by per-browser localStorage. The server fallback is also "first active by id", as are cron (JobAbstract::getActiveProfile()), test run and the legacy widget upload. Is an admin-configurable default planned as a follow-up ticket?
| */ | ||
| $locales = $data['profile']->getTargetLocales(); | ||
| $locales = []; | ||
| foreach ($data['profiles'] ?? [$data['profile']] as $profile) { |
There was a problem hiding this comment.
[Medium] The side widget now lists locales from all profiles (first one wins per blog), but its legacy upload path ajaxUploadHandler() still uses ArrayHelper::first($this->getProfiles()) (PostBasedWidgetControllerStd.php:220). Choosing a blog that only belongs to profile 2 uploads through profile 1 with an empty Smartling locale. The "settings" link in the empty state (:150) also points only at the first profile.
No description provided.