From b517e7c03ab0314e2b6c779641afa605ab80cc97 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Wed, 2 Sep 2026 10:50:10 +0200 Subject: [PATCH 01/20] remove cloning poll from UploadJob, clone attachments synchronously (WP-1015) The "Clone attachment" profile option flagged an attachment is_cloned=1 and deferred the actual clone to UploadJob's processCloning() poll (findSubmissionForCloning()). Cloning makes no Smartling API calls and is a synchronous local operation, so sendForTranslation() now calls cloneContent() directly at the point it already holds the submission, instead of deferring it to a separate unclaimed poll that raced under concurrent cron runs. The standalone clone-request feature (ContentRelationsHandler's formAction=clone) has no live trigger in either UI (the React tab bar has no clone tab, and the legacy jQuery form is permanently display:none with nothing that reveals it), so it's left untouched as a separate dead-code cleanup. findSubmissionForCloning() stays on SubmissionManager: QueueManagerTableWidget still uses it to detect a clone stuck from a crashed prior run. Co-Authored-By: Claude Sonnet 5 --- .../Base/SmartlingCoreUploadTrait.php | 20 ++- inc/Smartling/Jobs/UploadJob.php | 15 --- tests/Smartling/Base/SmartlingCoreTest.php | 127 ++++++++++++++++++ tests/Smartling/Jobs/UploadJobTest.php | 46 ++----- 4 files changed, 154 insertions(+), 54 deletions(-) diff --git a/inc/Smartling/Base/SmartlingCoreUploadTrait.php b/inc/Smartling/Base/SmartlingCoreUploadTrait.php index 86d68534..5f5dc064 100644 --- a/inc/Smartling/Base/SmartlingCoreUploadTrait.php +++ b/inc/Smartling/Base/SmartlingCoreUploadTrait.php @@ -532,8 +532,10 @@ public function sendForTranslation(UploadQueueItem $item): void $configurationProfile = $this->getSettingsManager()->getSingleSettingsProfile($item->getSubmissions()[0]->getSourceBlogId()); - // Mark attachment submission as "Cloned" if there is "Clone attachment" - // option is enabled in configuration profile. + // Clone attachment submission instead of uploading it, if "Clone attachment" + // option is enabled in configuration profile. Cloning is a local, synchronous + // operation (no Smartling API calls), so it happens right here instead of being + // deferred to a separate poll. foreach ($item->getSubmissions() as $submission) { if (1 === $configurationProfile->getCloneAttachment() && $submission->getContentType() === 'attachment') { $submission->setIsCloned(1); @@ -541,7 +543,7 @@ public function sendForTranslation(UploadQueueItem $item): void $this->getLogger()->info( sprintf( - 'Attachment submissionId="%s" marked as cloned (sourceBlogId="%s", sourceId="%s", contentType="%s", batchUid="%s").', + 'Cloning attachment submissionId="%s" (sourceBlogId="%s", sourceId="%s", contentType="%s", batchUid="%s").', $submission->getId(), $submission->getSourceBlogId(), $submission->getSourceId(), @@ -549,6 +551,18 @@ public function sendForTranslation(UploadQueueItem $item): void $item->getBatchUid(), ) ); + try { + $this->cloneContent($submission); + } catch (\Throwable $e) { + $this->getSubmissionManager()->setErrorMessage( + $submission, vsprintf('Error occurred while cloning: %s', [$e->getMessage()]) + ); + $this->getLogger()->error(sprintf( + 'Failed cloning attachment submissionId="%s": %s', + $submission->getId(), + $e->getMessage(), + )); + } $item = $item->removeSubmission($submission); } } diff --git a/inc/Smartling/Jobs/UploadJob.php b/inc/Smartling/Jobs/UploadJob.php index c903cf36..d5494563 100644 --- a/inc/Smartling/Jobs/UploadJob.php +++ b/inc/Smartling/Jobs/UploadJob.php @@ -48,8 +48,6 @@ public function run(string $source): void $this->processUploadQueue($blogId); - $this->processCloning($blogId); - $this->getLogger()->debug("Finished $message"); } @@ -124,17 +122,4 @@ private function failItem(UploadQueueItem $item, string $logVerb, string $errorM $this->submissionManager->setErrorMessage($submission, $errorMessage); } } - - private function processCloning(int $blogId): void - { - while (($submission = $this->submissionManager->findSubmissionForCloning($blogId)) !== null) { - try { - $this->wpProxy->do_action(ExportedAPI::ACTION_SMARTLING_CLONE_CONTENT, $submission); - } catch (\Throwable $e) { - $this->submissionManager->setErrorMessage($submission, $e->getMessage()); - continue; - } - $this->placeLockFlag(true); - } - } } diff --git a/tests/Smartling/Base/SmartlingCoreTest.php b/tests/Smartling/Base/SmartlingCoreTest.php index 9a0815c6..619b08dd 100644 --- a/tests/Smartling/Base/SmartlingCoreTest.php +++ b/tests/Smartling/Base/SmartlingCoreTest.php @@ -17,9 +17,15 @@ use Smartling\Helpers\SiteHelper; use Smartling\Helpers\TestRunHelper; use Smartling\Helpers\XmlHelper; +use Smartling\Models\IntStringPair; +use Smartling\Models\IntStringPairCollection; +use Smartling\Models\LoggerWithStringContext; +use Smartling\Models\UploadQueueItem; use Smartling\Replacers\ReplacerFactory; +use Smartling\Settings\ConfigurationProfileEntity; use Smartling\Settings\SettingsManager; use Smartling\Submissions\SubmissionEntity; +use Smartling\Submissions\SubmissionManager; use Smartling\Tests\Mocks\WordpressFunctionsMockHelper; use Smartling\Tests\Traits\DbAlMock; use Smartling\Tests\Traits\DummyLoggerMock; @@ -383,4 +389,125 @@ public function testExceptionOnTargetPlaceholderCreationFail() $obj->getXMLFiltered($submission); } + + /** + * The "Clone attachment" profile option used to only flag the submission is_cloned=1 and + * defer the actual clone to UploadJob's separate processCloning() poll. That poll is gone, + * so sendForTranslation() must clone the attachment itself, synchronously, right where it + * already holds the submission. + */ + public function testSendForTranslationClonesAttachmentSynchronously() + { + $attachment = $this->createMock(SubmissionEntity::class); + $attachment->method('getId')->willReturn(1); + $attachment->method('getContentType')->willReturn('attachment'); + $attachment->method('getSourceBlogId')->willReturn(1); + $attachment->method('getSourceId')->willReturn(10); + + $item = new UploadQueueItem( + [$attachment], + 'batchUid', + new IntStringPairCollection([new IntStringPair(1, 'de-DE')]), + 42, + ); + + $core = $this->buildCoreForSendForTranslation($this->cloneAttachmentProfile()); + $core->expects(self::once())->method('cloneContent')->with($attachment); + $core->expects(self::never())->method('bulkSubmit'); + + $core->sendForTranslation($item); + } + + /** + * A clone failure must be recorded as a visible error, the same way a failed upload is, + * instead of silently disappearing or aborting the whole cron run. + */ + public function testSendForTranslationRecordsErrorWhenAttachmentCloneFails() + { + $attachment = $this->createMock(SubmissionEntity::class); + $attachment->method('getId')->willReturn(1); + $attachment->method('getContentType')->willReturn('attachment'); + $attachment->method('getSourceBlogId')->willReturn(1); + $attachment->method('getSourceId')->willReturn(10); + + $item = new UploadQueueItem( + [$attachment], + 'batchUid', + new IntStringPairCollection([new IntStringPair(1, 'de-DE')]), + 42, + ); + + $submissionManager = $this->createMock(SubmissionManager::class); + $submissionManager->method('storeEntity')->willReturnArgument(0); + $submissionManager->expects(self::once())->method('setErrorMessage') + ->with($attachment, $this->stringContains('boom')); + + $core = $this->buildCoreForSendForTranslation($this->cloneAttachmentProfile(), $submissionManager); + $core->method('cloneContent')->willThrowException(new \RuntimeException('boom')); + $core->expects(self::never())->method('bulkSubmit'); + + $core->sendForTranslation($item); + } + + /** + * Guards against a regression where cloning is attempted for content it was never meant + * for: non-attachment submissions must still go through the normal upload path. + */ + public function testSendForTranslationDoesNotCloneNonAttachmentContent() + { + $post = $this->createMock(SubmissionEntity::class); + $post->method('getId')->willReturn(1); + $post->method('getContentType')->willReturn('post'); + $post->method('getSourceBlogId')->willReturn(1); + $post->method('getSourceId')->willReturn(10); + + $item = new UploadQueueItem( + [$post], + 'batchUid', + new IntStringPairCollection([new IntStringPair(1, 'de-DE')]), + 42, + ); + + $core = $this->buildCoreForSendForTranslation($this->cloneAttachmentProfile()); + $core->expects(self::never())->method('cloneContent'); + $core->expects(self::once())->method('bulkSubmit')->with($item); + + $core->sendForTranslation($item); + } + + private function cloneAttachmentProfile(): ConfigurationProfileEntity + { + $profile = $this->createMock(ConfigurationProfileEntity::class); + $profile->method('getCloneAttachment')->willReturn(1); + + return $profile; + } + + private function buildCoreForSendForTranslation( + ConfigurationProfileEntity $profile, + ?SubmissionManager $submissionManager = null, + ): SmartlingCore|\PHPUnit\Framework\MockObject\MockObject { + $settingsManager = $this->createMock(SettingsManager::class); + $settingsManager->method('getSingleSettingsProfile')->willReturn($profile); + + $submissionManager ??= $this->createMock(SubmissionManager::class); + + $logger = $this->createMock(LoggerWithStringContext::class); + $logger->method('withStringContext')->willReturnCallback( + static fn(array $context, callable $callable) => $callable(), + ); + + $core = $this->createPartialMock(SmartlingCore::class, [ + 'getSettingsManager', + 'getSubmissionManager', + 'getLogger', + 'cloneContent', + 'bulkSubmit', + ]); + $core->method('getSettingsManager')->willReturn($settingsManager); + $core->method('getSubmissionManager')->willReturn($submissionManager); + $core->method('getLogger')->willReturn($logger); + + return $core; + } } diff --git a/tests/Smartling/Jobs/UploadJobTest.php b/tests/Smartling/Jobs/UploadJobTest.php index aca6ae28..54d79c65 100644 --- a/tests/Smartling/Jobs/UploadJobTest.php +++ b/tests/Smartling/Jobs/UploadJobTest.php @@ -5,7 +5,6 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Smartling\ApiWrapperInterface; -use Smartling\Base\ExportedAPI; use Smartling\DbAl\UploadQueueManager; use Smartling\Exception\SmartlingDbException; use Smartling\Helpers\Cache; @@ -206,45 +205,21 @@ private function buildTwoSubmissionItem(): array } /** - * processUploadQueue() dispatches through the WordPress function proxy so the hook - * call can be mocked in tests; processCloning() must do the same, or bugs in the - * cloning dispatch have no unit-test coverage. + * Cloning used to be polled here via findSubmissionForCloning(), deferring the actual + * clone to a separate loop. The "Clone attachment" profile option now clones + * synchronously inside sendForTranslation(), and the standalone clone-request feature + * has no live trigger anywhere in the UI, so UploadJob must not poll for cloning work. */ - public function testCloningDispatchesThroughWordpressProxy() + public function testRunDoesNotPollForCloningWork() { - $uploadQueueManager = $this->createMock(UploadQueueManager::class); - $uploadQueueManager->method('length')->willReturn(0); - $uploadQueueManager->method('dequeue')->willReturn(null); + $item = $this->buildItem(); + $uploadQueueManager = $this->buildQueueManager($item); + $uploadQueueManager->method('complete'); - $submission = $this->createMock(SubmissionEntity::class); $submissionManager = $this->createMock(SubmissionManager::class); - $calls = 0; - $submissionManager->method('findSubmissionForCloning')->willReturnCallback( - function () use ($submission, &$calls) { - return $calls++ === 0 ? $submission : null; - }, - ); - - $settingsManager = $this->createMock(SettingsManager::class); - $settingsManager->method('getActiveProfile') - ->willReturn($this->createMock(ConfigurationProfileEntity::class)); + $submissionManager->expects($this->never())->method('findSubmissionForCloning'); - $wpProxy = $this->createMock(WordpressFunctionProxyHelper::class); - $wpProxy->method('get_current_blog_id')->willReturn(1); - $wpProxy->expects($this->once())->method('do_action') - ->with(ExportedAPI::ACTION_SMARTLING_CLONE_CONTENT, $submission); - - (new UploadJob( - $this->createMock(ApiWrapperInterface::class), - $this->createMock(Cache::class), - $this->createMock(FileUriHelper::class), - $settingsManager, - $submissionManager, - $uploadQueueManager, - $wpProxy, - 0, - 'hourly', - ))->run(''); + $this->buildJob($uploadQueueManager, $submissionManager)->run(''); } private function buildItem(?SubmissionEntity $submission = null): UploadQueueItem @@ -294,7 +269,6 @@ private function buildJob( ->willReturn($this->createMock(ConfigurationProfileEntity::class)); $submissionManager ??= $this->createMock(SubmissionManager::class); - $submissionManager->method('findSubmissionForCloning')->willReturn(null); $wpProxy = $this->createMock(WordpressFunctionProxyHelper::class); $wpProxy->method('get_current_blog_id')->willReturn(1); From 64a716b3ad9744cff72bb5f9a9a53de36e318921 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Wed, 2 Sep 2026 11:28:24 +0200 Subject: [PATCH 02/20] make UploadQueueManager::claim() a real compare-and-swap (WP-1015) claim() updated a row by id alone, never re-checking that it was still unclaimed (or stale) at write time. Two concurrent dequeue() calls that both selected the same unclaimed row could both succeed in claiming it, since nothing in the UPDATE's WHERE would make the second one lose - each is dispatched to Smartling independently, so a lost race meant a real duplicate upload, not just a local bookkeeping error. claim() now re-checks the same unclaimed-or-stale condition dequeue() selected on, in the same UPDATE that writes the new claim: InnoDB serializes concurrent writers to a row and re-evaluates the WHERE against current data, so only one concurrent claim can ever match. Also fixes a related bug this exposed: the old `!== false` check on the query result treated an UPDATE matching zero rows as success, since PHP's `0 !== false` is true. Affected-rows is checked with `> 0` now, so a lost race is correctly treated as "not claimed" instead of being handed out anyway. The stale-claim condition dequeue()'s SELECT already used is extracted into staleClaimCondition() and shared with claim(), so the two queries can't drift out of sync on what counts as an abandoned claim. Co-Authored-By: Claude Sonnet 5 --- inc/Smartling/DbAl/UploadQueueManager.php | 54 ++++++++++---- .../Smartling/DbAl/UploadQueueManagerTest.php | 74 +++++++++++++++++++ 2 files changed, 113 insertions(+), 15 deletions(-) diff --git a/inc/Smartling/DbAl/UploadQueueManager.php b/inc/Smartling/DbAl/UploadQueueManager.php index 5e393a9c..d122192e 100644 --- a/inc/Smartling/DbAl/UploadQueueManager.php +++ b/inc/Smartling/DbAl/UploadQueueManager.php @@ -50,19 +50,7 @@ public function dequeue(int $blogId): ?UploadQueueItem // Get queue items with the first submission having its source blog id = $blogId. // It should be impossible to create a single queue item with submissions from multiple source blog ids, // so only checking one is enough. - $staleClaimCondition = new ConditionBlock(ConditionBuilder::CONDITION_BLOCK_LEVEL_OPERATOR_OR); - $staleClaimCondition->addCondition(new Condition( - ConditionBuilder::CONDITION_IS_NULL, - 'q.' . UploadQueueEntity::FIELD_CLAIMED, - [], - false, - )); - $staleClaimCondition->addCondition(new Condition( - ConditionBuilder::CONDITION_SIGN_LESS, - 'q.' . UploadQueueEntity::FIELD_CLAIMED, - $this->getStaleClaimThreshold(), - false, - )); + $staleClaimCondition = $this->staleClaimCondition('q.'); $query = sprintf(<<<'SQL' select q.%1$s, q.%2$s, q.%3$s, q.%9$s, q.%10$s from %7$s q left join %8$s s @@ -171,6 +159,33 @@ private function getStaleClaimThreshold(): string ); } + /** + * A row is eligible to be (re)claimed when nobody holds a claim on it, or the claim is + * old enough to have been abandoned by a crashed process. + * + * @param string $fieldPrefix Table alias prefix (e.g. 'q.') to use in a joined query. + * Left empty for an unqualified column reference. + */ + private function staleClaimCondition(string $fieldPrefix = ''): ConditionBlock + { + $escapeField = $fieldPrefix === ''; + $block = new ConditionBlock(ConditionBuilder::CONDITION_BLOCK_LEVEL_OPERATOR_OR); + $block->addCondition(new Condition( + ConditionBuilder::CONDITION_IS_NULL, + $fieldPrefix . UploadQueueEntity::FIELD_CLAIMED, + [], + $escapeField, + )); + $block->addCondition(new Condition( + ConditionBuilder::CONDITION_SIGN_LESS, + $fieldPrefix . UploadQueueEntity::FIELD_CLAIMED, + $this->getStaleClaimThreshold(), + $escapeField, + )); + + return $block; + } + public function complete(UploadQueueItem $item): void { if (!$this->delete($item->getId())) { @@ -179,18 +194,27 @@ public function complete(UploadQueueItem $item): void } /** + * Claims a row by id, but only if it is still unclaimed (or stale) at the moment of the + * write. Matching by id alone would let two concurrent dequeue() calls that both selected + * the same unclaimed row both succeed in claiming it; re-checking the claim in the same + * UPDATE makes this a real compare-and-swap, since InnoDB serializes concurrent writers + * to the same row and re-evaluates the WHERE clause against the current data. + * * @return bool Whether the row was actually claimed. */ private function claim(int $id, int $attempts): bool { + $conditions = $this->idCondition($id); + $conditions->addConditionBlock($this->staleClaimCondition()); + return $this->db->query(QueryBuilder::buildUpdateQuery( $this->tableName, [ UploadQueueEntity::FIELD_CLAIMED => DateTimeHelper::nowAsString(), UploadQueueEntity::FIELD_ATTEMPTS => $attempts + 1, ], - $this->idCondition($id), - )) !== false; + $conditions, + )) > 0; } public function enqueue(IntegerIterator $submissionIds, string $batchUid): void diff --git a/tests/Smartling/DbAl/UploadQueueManagerTest.php b/tests/Smartling/DbAl/UploadQueueManagerTest.php index db75522d..d033fa68 100644 --- a/tests/Smartling/DbAl/UploadQueueManagerTest.php +++ b/tests/Smartling/DbAl/UploadQueueManagerTest.php @@ -262,6 +262,80 @@ public function query() {} $this->assertNull($uploadQueueManager->dequeue(1), 'Must not hand out an item whose claim could not be confirmed'); } + /** + * $wpdb->query() returns the number of affected rows for a successful UPDATE - 0 when + * the WHERE matched nothing. If claim() re-checks the row is still unclaimed (a real + * compare-and-swap) instead of updating by id alone, a concurrent dequeue() that claimed + * the row first makes this UPDATE affect zero rows without erroring. Treating that as + * "not claimed" is the whole point of the fix: naively checking `!== false` would treat + * int 0 as success (0 !== false is true) and hand out a row someone else already claimed. + */ + public function testDequeueDoesNotHandOutItemWhenClaimLosesRaceToAnotherProcess() + { + $this->mockDbAl(); + $db = $this->getMockBuilder(DB::class) + ->setConstructorArgs([new class { + public string $base_prefix = ''; + public function getRowArray() {} + public function query() {} + }]) + ->onlyMethods(['getRowArray', 'query']) + ->getMock(); + $db->method('getRowArray')->willReturnOnConsecutiveCalls( + ['id' => 7, 'batch_uid' => '', 'submission_ids' => '1', 'claimed' => null, 'attempts' => 0], + null, + ); + $db->method('query')->willReturn(0); // matched zero rows: another process claimed it first + + $submission = $this->createMock(SubmissionEntity::class); + $submission->method('getId')->willReturn(1); + $submission->method('getSourceId')->willReturn(1); + $submission->method('getSourceBlogId')->willReturn(1); + $submissionManager = $this->createMock(SubmissionManager::class); + $submissionManager->method('getEntityById')->willReturn($submission); + + $settingsManager = $this->createMock(SettingsManager::class); + $settingsManager->method('getSmartlingLocaleBySubmission')->willReturn('de-DE'); + + $uploadQueueManager = new UploadQueueManager( + $this->createMock(ApiWrapperInterface::class), + $settingsManager, + $db, + $submissionManager, + ); + + $this->assertNull($uploadQueueManager->dequeue(1), 'A lost claim race must not be handed out'); + } + + /** + * claim() must re-check the row is still unclaimed (or stale) in the same UPDATE that + * writes the new claim, not just match by id - otherwise two concurrent dequeue() calls + * that both selected the same unclaimed row would both succeed in claiming it. + */ + public function testClaimQueryRechecksRowIsStillUnclaimed() + { + $queries = []; + $manager = $this->buildManager( + [['id' => 7, 'batch_uid' => '', 'submission_ids' => '1', 'claimed' => null, 'attempts' => 0], null], + [1 => 1], + $queries, + ); + + $manager->dequeue(1); + + $this->assertStringStartsWith('UPDATE', $queries[0]); + $this->assertStringContainsString( + 'is null', + strtolower($queries[0]), + 'Expected the claim to re-check the row is still unclaimed', + ); + $this->assertMatchesRegularExpression( + '/claimed`? <|<.*claimed/i', + $queries[0], + 'Expected the claim to re-check the claim has not gone stale', + ); + } + public function testDequeueOnlyConsidersUnclaimedOrStaleRows() { $queries = []; From e5bffed52868f76a16d3e5784704f4a7530e012f Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Wed, 2 Sep 2026 11:35:26 +0200 Subject: [PATCH 03/20] let UploadJob opt out of the distributed lock (WP-1015) UploadQueueManager::claim() is now a real compare-and-swap, so concurrent UploadJob runs can no longer double-process the same queue row. The account-level distributed lock (placeLockFlag()/dropLockFlag(), one Smartling API round trip per acquire, and another per renew - which processUploadQueue() does after every single processed item) was the only thing serializing concurrent runs before that fix; it's no longer needed for correctness here. Added JobAbstract::usesDistributedLock() (default true, unchanged for every other job) so a job can skip the acquireLock()/renewLock()/ releaseLock() calls while keeping the local throttle-cache check and the "no active profile" guard placeLockFlag() also does. UploadJob overrides it to false. Known side effect, discussed and accepted: QueueManagerTableWidget's "Running, please wait..." cell for the upload queue probes this same lock, so it will no longer report the upload job as running. A live-refreshing queue count is planned to replace that indicator, as a separate change. Co-Authored-By: Claude Sonnet 5 --- inc/Smartling/Jobs/JobAbstract.php | 17 ++++++++ inc/Smartling/Jobs/UploadJob.php | 10 +++++ tests/Jobs/AbstractJobTest.php | 54 ++++++++++++++++++++++++++ tests/Smartling/Jobs/UploadJobTest.php | 18 +++++++++ 4 files changed, 99 insertions(+) diff --git a/inc/Smartling/Jobs/JobAbstract.php b/inc/Smartling/Jobs/JobAbstract.php index 8f1ce056..91daebbf 100644 --- a/inc/Smartling/Jobs/JobAbstract.php +++ b/inc/Smartling/Jobs/JobAbstract.php @@ -85,6 +85,17 @@ protected function getCronFlagName(): string return self::CRON_FLAG_PREFIX . $this->getJobHookName(); } + /** + * Whether this job needs the Smartling-account-level distributed lock to serialize + * concurrent cron runs. Override to return false for a job whose own state (e.g. an + * atomically-claimed queue) already makes concurrent runs safe, to avoid paying for + * the acquireLock()/renewLock()/releaseLock() API round trips. + */ + protected function usesDistributedLock(): bool + { + return true; + } + /** * @throws EntityNotFoundException * @throws SmartlingApiException @@ -117,6 +128,9 @@ public function placeLockFlag(bool $renew = false, string $source = ''): void if ($this->throttleIntervalSeconds > 0) { $this->cache->set($flagName, 1, $this->throttleIntervalSeconds); } + if (!$this->usesDistributedLock()) { + return; + } if ($renew) { $this->api->renewLock($profile, $flagName, $this->cronLockTtl); } else { @@ -130,6 +144,9 @@ public function placeLockFlag(bool $renew = false, string $source = ''): void */ public function dropLockFlag(): void { + if (!$this->usesDistributedLock()) { + return; + } $profile = $this->settingsManager->getActiveProfile(); $flagName = $this->getCronFlagName(); $this->getLogger()->debug( diff --git a/inc/Smartling/Jobs/UploadJob.php b/inc/Smartling/Jobs/UploadJob.php index d5494563..611e9295 100644 --- a/inc/Smartling/Jobs/UploadJob.php +++ b/inc/Smartling/Jobs/UploadJob.php @@ -36,6 +36,16 @@ public function getJobHookName(): string return self::JOB_HOOK_NAME; } + /** + * The upload queue claims rows with a compare-and-swap (see UploadQueueManager::claim()), + * so concurrent runs of this job can no longer double-process the same item. The + * account-level distributed lock is no longer needed for correctness here. + */ + protected function usesDistributedLock(): bool + { + return false; + } + public function run(string $source): void { $message = 'UploadJob'; diff --git a/tests/Jobs/AbstractJobTest.php b/tests/Jobs/AbstractJobTest.php index 6db0c3c7..eb3a0768 100644 --- a/tests/Jobs/AbstractJobTest.php +++ b/tests/Jobs/AbstractJobTest.php @@ -57,6 +57,36 @@ public function testRunCronJobUserSource() $this->fail('Should throw exception when source is user'); } + /** + * A job that opts out of the distributed lock (usesDistributedLock() === false) must + * not pay for the Smartling API round trips acquireLock()/renewLock() would otherwise + * make, whether placing the flag for the first time or renewing it mid-run. + */ + public function testPlaceLockFlagSkipsDistributedLockApiWhenDisabled() + { + $api = $this->createMock(ApiWrapperInterface::class); + $api->expects($this->never())->method('acquireLock'); + $api->expects($this->never())->method('renewLock'); + + $x = $this->getJobAbstractMockWithoutDistributedLock($api); + + $x->placeLockFlag(); + $x->placeLockFlag(true); + } + + /** + * Same as above for releasing the flag. + */ + public function testDropLockFlagSkipsDistributedLockApiWhenDisabled() + { + $api = $this->createMock(ApiWrapperInterface::class); + $api->expects($this->never())->method('releaseLock'); + + $x = $this->getJobAbstractMockWithoutDistributedLock($api); + + $x->dropLockFlag(); + } + /** * @return MockObject|JobAbstract */ @@ -77,4 +107,28 @@ private function getJobAbstractMock(ApiWrapperInterface $api) ]) ->getMockForAbstractClass(); } + + /** + * @return MockObject|JobAbstract + */ + private function getJobAbstractMockWithoutDistributedLock(ApiWrapperInterface $api) + { + $settingsManager = $this->createMock(SettingsManager::class); + $settingsManager->method('getActiveProfile')->willReturn($this->profile); + + $x = $this->getMockBuilder(JobAbstract::class) + ->setConstructorArgs([ + $api, + $this->createMock(Cache::class), + $settingsManager, + $this->submissionManager, + 0, + '5m', + ]) + ->onlyMethods(['usesDistributedLock']) + ->getMockForAbstractClass(); + $x->method('usesDistributedLock')->willReturn(false); + + return $x; + } } diff --git a/tests/Smartling/Jobs/UploadJobTest.php b/tests/Smartling/Jobs/UploadJobTest.php index 54d79c65..b6f9e9ab 100644 --- a/tests/Smartling/Jobs/UploadJobTest.php +++ b/tests/Smartling/Jobs/UploadJobTest.php @@ -204,6 +204,24 @@ private function buildTwoSubmissionItem(): array return [$item, $submission1, $submission2]; } + /** + * The distributed lock's correctness role is now handled by UploadQueueManager's + * compare-and-swap claim(); UploadJob must not pay for the Smartling API round trips + * placeLockFlag()/dropLockFlag() would otherwise make on every processed item. + */ + public function testRunDoesNotUseDistributedLockApi() + { + $item = $this->buildItem(); + $uploadQueueManager = $this->buildQueueManager($item); + + $api = $this->createMock(ApiWrapperInterface::class); + $api->expects($this->never())->method('acquireLock'); + $api->expects($this->never())->method('renewLock'); + $api->expects($this->never())->method('releaseLock'); + + $this->buildJob($uploadQueueManager, null, null, null, $api)->run(''); + } + /** * Cloning used to be polled here via findSubmissionForCloning(), deferring the actual * clone to a separate loop. The "Clone attachment" profile option now clones From 348217c1d0c8981a7c2039bf71b6eed043259f1d Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Wed, 2 Sep 2026 11:47:23 +0200 Subject: [PATCH 04/20] replace upload row's lock probe with a live-refreshing queue count (WP-1015) UploadJob no longer holds the distributed lock, so QueueManagerTableWidget's "Running, please wait..." indicator for the Upload row could never fire again - the trial acquireLock() call would always succeed. Rather than keep a pointless API round trip on every page load, the Upload row now renders a counter span that JS polls every second via a new AJAX endpoint, so the admin can see the queue actually draining instead of a lock-derived state that no longer means anything. Added UploadQueueCountController (wp_ajax_smartling_upload_queue_count), mirroring InstantTranslationController's shape: nonce + capability checked, returns UploadQueueManager::count(). Reuses the smartling_connector_ajax nonce already localized to smartling-connector-admin.js for this page, so no new nonce plumbing was needed. The other rows (Download, Check Status, Check Status Helper) are unchanged - they still hold the lock and still need the "Running" probe, so tests exercising that behavior were retargeted from the Upload row to Download rather than deleted. Co-Authored-By: Claude Sonnet 5 --- .../Controller/UploadQueueCountController.php | 50 +++++++++++ .../WP/Table/QueueManagerTableWidget.php | 26 +++--- inc/config/register-on-startup.yml | 1 + inc/config/services.yml | 6 ++ js/smartling-connector-admin.js | 23 ++++++ .../UploadQueueCountControllerTest.php | 82 +++++++++++++++++++ .../WP/Table/QueueManagerTableWidgetTest.php | 69 +++++++++++++--- 7 files changed, 234 insertions(+), 23 deletions(-) create mode 100644 inc/Smartling/WP/Controller/UploadQueueCountController.php create mode 100644 tests/Smartling/WP/Controller/UploadQueueCountControllerTest.php diff --git a/inc/Smartling/WP/Controller/UploadQueueCountController.php b/inc/Smartling/WP/Controller/UploadQueueCountController.php new file mode 100644 index 00000000..53adb91a --- /dev/null +++ b/inc/Smartling/WP/Controller/UploadQueueCountController.php @@ -0,0 +1,50 @@ +wpProxy->add_action('wp_ajax_' . self::ACTION_NAME, [$this, 'handleGetCount']); + } + + public function handleGetCount(): void + { + if ($this->wpProxy->check_ajax_referer('smartling_connector_ajax', '_wpnonce', false) === false) { + $this->getLogger()->warning('Invalid nonce for action "' . self::ACTION_NAME . '"'); + $this->wpProxy->wp_send_json_error(['message' => 'Invalid nonce'], 403); + return; + } + + if (!$this->wpProxy->current_user_can(SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP)) { + $this->getLogger()->warning('User lacks capability "' . SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP . '" for action "' . self::ACTION_NAME . '"'); + $this->wpProxy->wp_send_json_error(['message' => 'Insufficient permissions'], 403); + return; + } + + $this->wpProxy->wp_send_json_success(['count' => $this->uploadQueueManager->count()]); + } +} diff --git a/inc/Smartling/WP/Table/QueueManagerTableWidget.php b/inc/Smartling/WP/Table/QueueManagerTableWidget.php index 5bc497eb..578c5443 100644 --- a/inc/Smartling/WP/Table/QueueManagerTableWidget.php +++ b/inc/Smartling/WP/Table/QueueManagerTableWidget.php @@ -81,7 +81,7 @@ public function prepare_items(): void $data = [ [ 'cron_name' => __('Upload'), - 'run_cron' => $this->getUploadCronActionCell($profile, $newSubmissionsCount), + 'run_cron' => $this->getUploadCronActionCell($newSubmissionsCount), 'queue_name' => __(' '), 'queue_purge' => 0 === $newSubmissionsCount ? __('Nothing to purge') @@ -140,23 +140,23 @@ public function prepare_items(): void $this->items = $data; } - private function getUploadCronActionCell(?ConfigurationProfileEntity $profile, int $count): string + /** + * UploadJob no longer holds the distributed lock (see UploadJob::usesDistributedLock()), + * so unlike the other cron rows this one can't detect "running" via a lock probe - that + * probe would now always succeed and cost a Smartling API round trip for nothing. Instead, + * this shows a live counter span that JS polls and refreshes every second. + */ + private function getUploadCronActionCell(int $count): string { if ($count === 0 && $this->submissionManager->findSubmissionForCloning($this->wpProxy->get_current_blog_id()) === null) { return self::MESSAGE_NOTHING_TO_DO; } - $jobName = UploadJob::JOB_HOOK_NAME; - try { - $this->testLock($profile, $jobName); - return sprintf( - '%s (%s submissions waiting)', - $this->getLockTag($jobName), - $count, - ); - } catch (SmartlingApiException $e) { - return sprintf('%s (%s submissions queued)', $this->getRunningMessage($e), $count); - } + return sprintf( + '%s (%s submissions waiting)', + $this->getLockTag(UploadJob::JOB_HOOK_NAME), + $count, + ); } private function getCheckStatusHelperCronActionCell(?ConfigurationProfileEntity $profile, int $count): string diff --git a/inc/config/register-on-startup.yml b/inc/config/register-on-startup.yml index 8fe42c5a..7e9b325c 100644 --- a/inc/config/register-on-startup.yml +++ b/inc/config/register-on-startup.yml @@ -16,6 +16,7 @@ services: - [ "addService", [ "@wp.test.run" ]] - [ "addService", [ "@wp.bulkSubmit" ]] - [ "addService", [ "@wp.instant.translation" ]] + - [ "addService", [ "@wp.upload-queue-count" ]] - [ "addService", [ "@wp.settings" ]] - [ "addService", [ "@wp.settings.edit" ]] - [ "addService", [ "@smartling.helper.relative-image-path-support" ]] diff --git a/inc/config/services.yml b/inc/config/services.yml index a18e52c7..0bad8513 100644 --- a/inc/config/services.yml +++ b/inc/config/services.yml @@ -491,6 +491,12 @@ services: - "@file.uri.helper" - "@wp.proxy" + wp.upload-queue-count: + class: Smartling\WP\Controller\UploadQueueCountController + arguments: + - "@manager.upload.queue" + - "@wp.proxy" + helper.gutenberg: class: Smartling\Helpers\GutenbergBlockHelper arguments: diff --git a/js/smartling-connector-admin.js b/js/smartling-connector-admin.js index 36ccb183..46bc3075 100644 --- a/js/smartling-connector-admin.js +++ b/js/smartling-connector-admin.js @@ -234,6 +234,29 @@ jQuery(document).ready(function () { }); }) + /** + * UploadJob no longer holds the distributed lock (see UploadJob::usesDistributedLock()), + * so the Queue Manager screen can't show "running" for it anymore. Poll the current + * upload queue count instead, so the number visibly drains while a run is in progress. + */ + if (jQuery('#smartling-upload-queue-count').length > 0 && typeof smartlingConnector !== 'undefined') { + var uploadQueueCountInterval = setInterval(function () { + var $counter = jQuery('#smartling-upload-queue-count'); + if ($counter.length === 0) { + clearInterval(uploadQueueCountInterval); + return; + } + jQuery.getJSON(ajaxurl, { + action: 'smartling_upload_queue_count', + _wpnonce: smartlingConnector.nonce + }).done(function (response) { + if (response && response.success && response.data && typeof response.data.count !== 'undefined') { + jQuery('#smartling-upload-queue-count').text(response.data.count); + } + }); + }, 1000); + } + }); function ajaxDownload() { diff --git a/tests/Smartling/WP/Controller/UploadQueueCountControllerTest.php b/tests/Smartling/WP/Controller/UploadQueueCountControllerTest.php new file mode 100644 index 00000000..68085ab2 --- /dev/null +++ b/tests/Smartling/WP/Controller/UploadQueueCountControllerTest.php @@ -0,0 +1,82 @@ +uploadQueueManager = $this->createMock(UploadQueueManager::class); + $this->wpProxy = $this->createMock(WordpressFunctionProxyHelper::class); + + $this->controller = new UploadQueueCountController( + $this->uploadQueueManager, + $this->wpProxy, + ); + } + + public function testHandleGetCountReturns403WhenNonceInvalid(): void + { + $this->wpProxy->method('check_ajax_referer')->willReturn(false); + + $errorArgs = null; + $this->wpProxy->method('wp_send_json_error')->willReturnCallback( + function (array $data, int $status) use (&$errorArgs) { + $errorArgs = ['data' => $data, 'status' => $status]; + } + ); + $this->uploadQueueManager->expects($this->never())->method('count'); + + $this->controller->handleGetCount(); + + $this->assertNotNull($errorArgs); + $this->assertSame(403, $errorArgs['status']); + } + + public function testHandleGetCountReturns403WhenCapabilityMissing(): void + { + $this->wpProxy->method('check_ajax_referer')->willReturn(true); + $this->wpProxy->method('current_user_can')->willReturn(false); + + $errorArgs = null; + $this->wpProxy->method('wp_send_json_error')->willReturnCallback( + function (array $data, int $status) use (&$errorArgs) { + $errorArgs = ['data' => $data, 'status' => $status]; + } + ); + $this->uploadQueueManager->expects($this->never())->method('count'); + + $this->controller->handleGetCount(); + + $this->assertNotNull($errorArgs); + $this->assertSame(403, $errorArgs['status']); + } + + public function testHandleGetCountReturnsCurrentQueueCount(): void + { + $this->wpProxy->method('check_ajax_referer')->willReturn(true); + $this->wpProxy->method('current_user_can')->willReturn(true); + $this->uploadQueueManager->method('count')->willReturn(7); + + $successArgs = null; + $this->wpProxy->method('wp_send_json_success')->willReturnCallback( + function (array $data) use (&$successArgs) { + $successArgs = $data; + } + ); + + $this->controller->handleGetCount(); + + $this->assertSame(['count' => 7], $successArgs); + } +} diff --git a/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php b/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php index 94ecdf32..adb6d1a0 100644 --- a/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php +++ b/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php @@ -63,6 +63,7 @@ class QueueManagerTableWidgetTest extends TestCase private function buildWidget( ApiWrapperInterface $api, int $uploadQueueCount = 1, + int $downloadQueueCount = 0, ): QueueManagerTableWidget { $profile = $this->createMock(ConfigurationProfileEntity::class); $profile->method('getProjectId')->willReturn('testProject'); @@ -77,7 +78,7 @@ private function buildWidget( $queue = $this->createMock(QueueInterface::class); $queue->method('stats')->willReturn([ QueueInterface::QUEUE_NAME_LAST_MODIFIED_CHECK_QUEUE => 0, - QueueInterface::QUEUE_NAME_DOWNLOAD_QUEUE => 0, + QueueInterface::QUEUE_NAME_DOWNLOAD_QUEUE => $downloadQueueCount, ]); $uploadQueueManager = $this->createMock(UploadQueueManager::class); @@ -115,6 +116,11 @@ public function __construct( }; } + /** + * Download still holds the distributed lock (only UploadJob opted out via + * usesDistributedLock()), so its cell must still reflect an invalid-credentials + * error from the lock probe. + */ public function testPrepareItemsDoesNotThrowWhenApiCredentialsAreInvalid(): void { $authError = new SmartlingApiException( @@ -125,16 +131,20 @@ public function testPrepareItemsDoesNotThrowWhenApiCredentialsAreInvalid(): void $api = $this->createMock(ApiWrapperInterface::class); $api->method('acquireLock')->willThrowException($authError); - $widget = $this->buildWidget($api, uploadQueueCount: 3); + $widget = $this->buildWidget($api, downloadQueueCount: 3); $widget->prepare_items(); $this->assertNotEmpty($widget->items); - $uploadRow = $widget->items[0]; - $this->assertStringContainsString('API error', $uploadRow['run_cron']); - $this->assertStringContainsString('Invalid credentials', $uploadRow['run_cron']); + $downloadRow = $widget->items[3]; + $this->assertStringContainsString('API error', $downloadRow['run_cron']); + $this->assertStringContainsString('Invalid credentials', $downloadRow['run_cron']); } + /** + * Download still holds the distributed lock, so its cell must still show + * "Running" when the lock probe reports the resource is locked. + */ public function testPrepareItemsShowsRunningMessageWhenLockHeld(): void { $lockError = new SmartlingApiException( @@ -145,14 +155,18 @@ public function testPrepareItemsShowsRunningMessageWhenLockHeld(): void $api = $this->createMock(ApiWrapperInterface::class); $api->method('acquireLock')->willThrowException($lockError); - $widget = $this->buildWidget($api, uploadQueueCount: 3); + $widget = $this->buildWidget($api, downloadQueueCount: 3); $widget->prepare_items(); - $uploadRow = $widget->items[0]; - $this->assertStringContainsString('Running', $uploadRow['run_cron']); + $downloadRow = $widget->items[3]; + $this->assertStringContainsString('Running', $downloadRow['run_cron']); } + /** + * Download still holds the distributed lock, so its cell must still show + * "Running" for the SDK's wrapped-Guzzle-423 shape too. + */ public function testPrepareItemsShowsRunningMessageWhenSdkWrapsGuzzle423(): void { // Reproduces the real SDK behavior: BaseApiAbstract::sendRequest() catches @@ -170,13 +184,48 @@ public function testPrepareItemsShowsRunningMessageWhenSdkWrapsGuzzle423(): void $guzzleException, )); + $widget = $this->buildWidget($api, downloadQueueCount: 3); + + $widget->prepare_items(); + + $downloadRow = $widget->items[3]; + $this->assertStringContainsString('Running', $downloadRow['run_cron']); + $this->assertStringNotContainsString('API error', $downloadRow['run_cron']); + } + + /** + * UploadJob no longer holds the distributed lock (see UploadJob::usesDistributedLock()), + * so its cell must not probe it at all - the probe would now always "succeed" and never + * detect a real background upload run, making the check pointless while still costing a + * Smartling API round trip on every page load. + */ + public function testUploadRowDoesNotProbeDistributedLock(): void + { + $api = $this->createMock(ApiWrapperInterface::class); + $api->expects($this->never())->method('acquireLock'); + + $widget = $this->buildWidget($api, uploadQueueCount: 3); + + $widget->prepare_items(); + + $uploadRow = $widget->items[0]; + $this->assertStringNotContainsString('Running', $uploadRow['run_cron']); + } + + /** + * The upload row shows a live counter span that JS polls and refreshes every + * second instead of the (no-longer-meaningful) "Running" indicator. + */ + public function testUploadRowShowsLiveCounterSpanWithCurrentCount(): void + { + $api = $this->createMock(ApiWrapperInterface::class); + $widget = $this->buildWidget($api, uploadQueueCount: 3); $widget->prepare_items(); $uploadRow = $widget->items[0]; - $this->assertStringContainsString('Running', $uploadRow['run_cron']); - $this->assertStringNotContainsString('API error', $uploadRow['run_cron']); + $this->assertStringContainsString('3', $uploadRow['run_cron']); } } } From 1aaac568384aa25b2bdbcce15d35b962cff4160a Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Wed, 2 Sep 2026 12:06:43 +0200 Subject: [PATCH 05/20] animate upload queue count on change (WP-1015) Only when the polled count actually differs from what's displayed: snap the text to a highlight color (a CSS custom property, --smartling-queue-count-highlight) with transitions disabled, force a reflow, then remove that class so the base rule's transition calmly fades the color back over 900ms. No flash/scale - just a color that settles back to normal, so repeated identical polls stay inert. Co-Authored-By: Claude Sonnet 5 --- css/smartling-connector-admin.css | 11 +++++++++++ js/smartling-connector-admin.js | 11 ++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/css/smartling-connector-admin.css b/css/smartling-connector-admin.css index eeedb158..e89d29bd 100644 --- a/css/smartling-connector-admin.css +++ b/css/smartling-connector-admin.css @@ -455,3 +455,14 @@ span.circle { .smartling-border-table .numeric { text-align: right; } + +#smartling-upload-queue-count { + --smartling-queue-count-highlight: #2271b1; + color: inherit; + transition: color 900ms ease; +} + +#smartling-upload-queue-count.smartling-queue-count-changed { + color: var(--smartling-queue-count-highlight); + transition: none; +} diff --git a/js/smartling-connector-admin.js b/js/smartling-connector-admin.js index 46bc3075..c5cf3cc3 100644 --- a/js/smartling-connector-admin.js +++ b/js/smartling-connector-admin.js @@ -251,7 +251,16 @@ jQuery(document).ready(function () { _wpnonce: smartlingConnector.nonce }).done(function (response) { if (response && response.success && response.data && typeof response.data.count !== 'undefined') { - jQuery('#smartling-upload-queue-count').text(response.data.count); + var $current = jQuery('#smartling-upload-queue-count'); + var newCount = String(response.data.count); + if ($current.text() !== newCount) { + $current.text(newCount); + // Snap to the highlight color, then let the transition on the base + // rule (below) calmly fade it back once the class is removed. + $current.addClass('smartling-queue-count-changed'); + void $current.get(0).offsetWidth; // force reflow so removal transitions + $current.removeClass('smartling-queue-count-changed'); + } } }); }, 1000); From 2e54b2a3818951f9b7a26721be63d784335d1353 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Wed, 2 Sep 2026 13:02:54 +0200 Subject: [PATCH 06/20] switch upload cell to "Nothing to do" when the polled count hits zero (WP-1015) Wrapped the cell's link+counter state in a stable #smartling-upload-cron-cell span so JS can replace the whole cell, not just the counter. When a poll reports count === 0, the cell now swaps to the same "Nothing to do" text page load would render, and polling stops (the interval's own "element gone" check would have caught it a tick later regardless, but stopping immediately avoids one wasted request). Known simplification: page load's "Nothing to do" state also depends on findSubmissionForCloning() (a lingering pending clone), which the polled endpoint doesn't check. That state is rare now that cloning is synchronous (only a mid-clone crash leaves one behind) and reachable only via a fresh page load, same as before this change. Co-Authored-By: Claude Sonnet 5 --- .../WP/Table/QueueManagerTableWidget.php | 2 +- js/smartling-connector-admin.js | 7 +++++++ .../WP/Table/QueueManagerTableWidgetTest.php | 20 +++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/inc/Smartling/WP/Table/QueueManagerTableWidget.php b/inc/Smartling/WP/Table/QueueManagerTableWidget.php index 578c5443..0179b2c2 100644 --- a/inc/Smartling/WP/Table/QueueManagerTableWidget.php +++ b/inc/Smartling/WP/Table/QueueManagerTableWidget.php @@ -153,7 +153,7 @@ private function getUploadCronActionCell(int $count): string } return sprintf( - '%s (%s submissions waiting)', + '%s (%s submissions waiting)', $this->getLockTag(UploadJob::JOB_HOOK_NAME), $count, ); diff --git a/js/smartling-connector-admin.js b/js/smartling-connector-admin.js index c5cf3cc3..2f1dabb9 100644 --- a/js/smartling-connector-admin.js +++ b/js/smartling-connector-admin.js @@ -251,6 +251,13 @@ jQuery(document).ready(function () { _wpnonce: smartlingConnector.nonce }).done(function (response) { if (response && response.success && response.data && typeof response.data.count !== 'undefined') { + if (response.data.count === 0) { + // Same state QueueManagerTableWidget::MESSAGE_NOTHING_TO_DO renders + // on page load when the queue is empty. + jQuery('#smartling-upload-cron-cell').text('Nothing to do'); + clearInterval(uploadQueueCountInterval); + return; + } var $current = jQuery('#smartling-upload-queue-count'); var newCount = String(response.data.count); if ($current.text() !== newCount) { diff --git a/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php b/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php index adb6d1a0..eb6a445d 100644 --- a/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php +++ b/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php @@ -227,5 +227,25 @@ public function testUploadRowShowsLiveCounterSpanWithCurrentCount(): void $uploadRow = $widget->items[0]; $this->assertStringContainsString('3', $uploadRow['run_cron']); } + + /** + * JS needs a stable element wrapping the whole cell state (link + counter) so that + * when the polled count reaches zero, it can swap the entire cell to "Nothing to do" + * - the same state page load would render - rather than just zeroing the counter. + */ + public function testUploadRowWrapsCellStateInStableContainer(): void + { + $api = $this->createMock(ApiWrapperInterface::class); + + $widget = $this->buildWidget($api, uploadQueueCount: 3); + + $widget->prepare_items(); + + $uploadRow = $widget->items[0]; + $this->assertStringContainsString( + '', + $uploadRow['run_cron'], + ); + } } } From 9a03f730d01bf89f901ff3c82804551b101347fe Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Wed, 2 Sep 2026 13:14:37 +0200 Subject: [PATCH 07/20] fix bulk uploads (WP-1015) --- inc/Smartling/Models/UserCloneRequest.php | 1 - .../Models/UserTranslationRequest.php | 3 +- tests/Models/TranslationRequestTest.php | 28 +++++++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/inc/Smartling/Models/UserCloneRequest.php b/inc/Smartling/Models/UserCloneRequest.php index 4942c1ed..f56f8807 100644 --- a/inc/Smartling/Models/UserCloneRequest.php +++ b/inc/Smartling/Models/UserCloneRequest.php @@ -56,7 +56,6 @@ public static function fromArray(array $array): self return new self(self::getSourceId($array), $array['source']['contentType'], $array['relations'] ?? [], explode(',', $array['targetBlogIds'])); } - // Might be 0 in case of bulk upload protected static function getSourceId(array $array): int { $id = $array['source']['id'][0] ?? null; diff --git a/inc/Smartling/Models/UserTranslationRequest.php b/inc/Smartling/Models/UserTranslationRequest.php index dec87596..a23096d4 100644 --- a/inc/Smartling/Models/UserTranslationRequest.php +++ b/inc/Smartling/Models/UserTranslationRequest.php @@ -30,9 +30,10 @@ public static function fromArray(array $array): self { self::validate($array); $ids = self::toIntegerArray($array['ids'] ?? []); + $contentId = count($ids) > 0 ? 0 : self::getSourceId($array); return new self( - self::getSourceId($array), + $contentId, $array['source']['contentType'] ?? '', $array['relations'] ?? [], explode(',', $array['targetBlogIds']), diff --git a/tests/Models/TranslationRequestTest.php b/tests/Models/TranslationRequestTest.php index 77fb47dc..c5e7baa8 100644 --- a/tests/Models/TranslationRequestTest.php +++ b/tests/Models/TranslationRequestTest.php @@ -46,4 +46,32 @@ public function testFromArray() $this->assertEquals($jobTimeZone, $x->getJobInformation()->getTimeZone()); $this->assertEquals($jobUid, $x->getJobInformation()->getId()); } + + /** + * Bulk submit sends an empty source.id array (the actual content ids live in `ids`), + * so fromArray() must not require source.id[0] to be present when `ids` is populated. + */ + public function testFromArrayBulkUploadWithEmptySourceId() + { + $targetBlogId = 2; + $ids = [13, 14, 15]; + $x = UserTranslationRequest::fromArray([ + 'job' => [ + 'id' => '', + 'name' => '', + 'description' => '', + 'dueDate' => '', + 'timeZone' => 'Europe/Kyiv', + 'authorize' => 'true', + ], + 'formAction' => ContentRelationsHandler::FORM_ACTION_UPLOAD, + 'source' => ['id' => [], 'contentType' => 'post'], + 'relations' => [], + 'targetBlogIds' => (string)$targetBlogId, + 'ids' => $ids, + ]); + $this->assertTrue($x->isBulk()); + $this->assertEquals($ids, $x->getIds()); + $this->assertEquals('post', $x->getContentType()); + } } From 49e9b29cde31c019b74698e542cbfb2a7f7e6be1 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Thu, 3 Sep 2026 11:05:23 +0200 Subject: [PATCH 08/20] cleanup (WP-1015) --- .../Base/SmartlingCoreUploadTrait.php | 4 +-- inc/Smartling/Jobs/JobAbstract.php | 6 ---- inc/Smartling/Jobs/UploadJob.php | 5 --- .../Controller/UploadQueueCountController.php | 6 ---- .../WP/Table/QueueManagerTableWidget.php | 6 ---- js/smartling-connector-admin.js | 11 +------ tests/Jobs/AbstractJobTest.php | 8 ----- tests/Models/TranslationRequestTest.php | 4 --- tests/Smartling/Base/SmartlingCoreTest.php | 14 -------- .../Smartling/DbAl/UploadQueueManagerTest.php | 33 ------------------- tests/Smartling/Jobs/UploadJobTest.php | 11 ------- .../WP/Table/QueueManagerTableWidgetTest.php | 29 ---------------- 12 files changed, 2 insertions(+), 135 deletions(-) diff --git a/inc/Smartling/Base/SmartlingCoreUploadTrait.php b/inc/Smartling/Base/SmartlingCoreUploadTrait.php index 5f5dc064..ea922973 100644 --- a/inc/Smartling/Base/SmartlingCoreUploadTrait.php +++ b/inc/Smartling/Base/SmartlingCoreUploadTrait.php @@ -533,9 +533,7 @@ public function sendForTranslation(UploadQueueItem $item): void $configurationProfile = $this->getSettingsManager()->getSingleSettingsProfile($item->getSubmissions()[0]->getSourceBlogId()); // Clone attachment submission instead of uploading it, if "Clone attachment" - // option is enabled in configuration profile. Cloning is a local, synchronous - // operation (no Smartling API calls), so it happens right here instead of being - // deferred to a separate poll. + // option is enabled in configuration profile. foreach ($item->getSubmissions() as $submission) { if (1 === $configurationProfile->getCloneAttachment() && $submission->getContentType() === 'attachment') { $submission->setIsCloned(1); diff --git a/inc/Smartling/Jobs/JobAbstract.php b/inc/Smartling/Jobs/JobAbstract.php index 91daebbf..e17cb078 100644 --- a/inc/Smartling/Jobs/JobAbstract.php +++ b/inc/Smartling/Jobs/JobAbstract.php @@ -85,12 +85,6 @@ protected function getCronFlagName(): string return self::CRON_FLAG_PREFIX . $this->getJobHookName(); } - /** - * Whether this job needs the Smartling-account-level distributed lock to serialize - * concurrent cron runs. Override to return false for a job whose own state (e.g. an - * atomically-claimed queue) already makes concurrent runs safe, to avoid paying for - * the acquireLock()/renewLock()/releaseLock() API round trips. - */ protected function usesDistributedLock(): bool { return true; diff --git a/inc/Smartling/Jobs/UploadJob.php b/inc/Smartling/Jobs/UploadJob.php index 611e9295..5a1a9339 100644 --- a/inc/Smartling/Jobs/UploadJob.php +++ b/inc/Smartling/Jobs/UploadJob.php @@ -36,11 +36,6 @@ public function getJobHookName(): string return self::JOB_HOOK_NAME; } - /** - * The upload queue claims rows with a compare-and-swap (see UploadQueueManager::claim()), - * so concurrent runs of this job can no longer double-process the same item. The - * account-level distributed lock is no longer needed for correctness here. - */ protected function usesDistributedLock(): bool { return false; diff --git a/inc/Smartling/WP/Controller/UploadQueueCountController.php b/inc/Smartling/WP/Controller/UploadQueueCountController.php index 53adb91a..f4e72102 100644 --- a/inc/Smartling/WP/Controller/UploadQueueCountController.php +++ b/inc/Smartling/WP/Controller/UploadQueueCountController.php @@ -8,12 +8,6 @@ use Smartling\Helpers\WordpressFunctionProxyHelper; use Smartling\WP\WPHookInterface; -/** - * Backs the live-refreshing upload queue count shown on the Queue Manager screen. - * UploadJob no longer holds the distributed lock (see UploadJob::usesDistributedLock()), - * so the widget can no longer detect "running" via a lock probe; polling the current - * queue size instead lets the admin see it draining without that indicator. - */ class UploadQueueCountController implements WPHookInterface { use LoggerSafeTrait; diff --git a/inc/Smartling/WP/Table/QueueManagerTableWidget.php b/inc/Smartling/WP/Table/QueueManagerTableWidget.php index 0179b2c2..356a1e14 100644 --- a/inc/Smartling/WP/Table/QueueManagerTableWidget.php +++ b/inc/Smartling/WP/Table/QueueManagerTableWidget.php @@ -140,12 +140,6 @@ public function prepare_items(): void $this->items = $data; } - /** - * UploadJob no longer holds the distributed lock (see UploadJob::usesDistributedLock()), - * so unlike the other cron rows this one can't detect "running" via a lock probe - that - * probe would now always succeed and cost a Smartling API round trip for nothing. Instead, - * this shows a live counter span that JS polls and refreshes every second. - */ private function getUploadCronActionCell(int $count): string { if ($count === 0 && $this->submissionManager->findSubmissionForCloning($this->wpProxy->get_current_blog_id()) === null) { diff --git a/js/smartling-connector-admin.js b/js/smartling-connector-admin.js index 2f1dabb9..6adc7235 100644 --- a/js/smartling-connector-admin.js +++ b/js/smartling-connector-admin.js @@ -234,11 +234,6 @@ jQuery(document).ready(function () { }); }) - /** - * UploadJob no longer holds the distributed lock (see UploadJob::usesDistributedLock()), - * so the Queue Manager screen can't show "running" for it anymore. Poll the current - * upload queue count instead, so the number visibly drains while a run is in progress. - */ if (jQuery('#smartling-upload-queue-count').length > 0 && typeof smartlingConnector !== 'undefined') { var uploadQueueCountInterval = setInterval(function () { var $counter = jQuery('#smartling-upload-queue-count'); @@ -252,8 +247,6 @@ jQuery(document).ready(function () { }).done(function (response) { if (response && response.success && response.data && typeof response.data.count !== 'undefined') { if (response.data.count === 0) { - // Same state QueueManagerTableWidget::MESSAGE_NOTHING_TO_DO renders - // on page load when the queue is empty. jQuery('#smartling-upload-cron-cell').text('Nothing to do'); clearInterval(uploadQueueCountInterval); return; @@ -262,10 +255,8 @@ jQuery(document).ready(function () { var newCount = String(response.data.count); if ($current.text() !== newCount) { $current.text(newCount); - // Snap to the highlight color, then let the transition on the base - // rule (below) calmly fade it back once the class is removed. $current.addClass('smartling-queue-count-changed'); - void $current.get(0).offsetWidth; // force reflow so removal transitions + void $current.get(0).offsetWidth; // force reflow $current.removeClass('smartling-queue-count-changed'); } } diff --git a/tests/Jobs/AbstractJobTest.php b/tests/Jobs/AbstractJobTest.php index eb3a0768..424002cf 100644 --- a/tests/Jobs/AbstractJobTest.php +++ b/tests/Jobs/AbstractJobTest.php @@ -57,11 +57,6 @@ public function testRunCronJobUserSource() $this->fail('Should throw exception when source is user'); } - /** - * A job that opts out of the distributed lock (usesDistributedLock() === false) must - * not pay for the Smartling API round trips acquireLock()/renewLock() would otherwise - * make, whether placing the flag for the first time or renewing it mid-run. - */ public function testPlaceLockFlagSkipsDistributedLockApiWhenDisabled() { $api = $this->createMock(ApiWrapperInterface::class); @@ -74,9 +69,6 @@ public function testPlaceLockFlagSkipsDistributedLockApiWhenDisabled() $x->placeLockFlag(true); } - /** - * Same as above for releasing the flag. - */ public function testDropLockFlagSkipsDistributedLockApiWhenDisabled() { $api = $this->createMock(ApiWrapperInterface::class); diff --git a/tests/Models/TranslationRequestTest.php b/tests/Models/TranslationRequestTest.php index c5e7baa8..d0872ce3 100644 --- a/tests/Models/TranslationRequestTest.php +++ b/tests/Models/TranslationRequestTest.php @@ -47,10 +47,6 @@ public function testFromArray() $this->assertEquals($jobUid, $x->getJobInformation()->getId()); } - /** - * Bulk submit sends an empty source.id array (the actual content ids live in `ids`), - * so fromArray() must not require source.id[0] to be present when `ids` is populated. - */ public function testFromArrayBulkUploadWithEmptySourceId() { $targetBlogId = 2; diff --git a/tests/Smartling/Base/SmartlingCoreTest.php b/tests/Smartling/Base/SmartlingCoreTest.php index 619b08dd..d7194833 100644 --- a/tests/Smartling/Base/SmartlingCoreTest.php +++ b/tests/Smartling/Base/SmartlingCoreTest.php @@ -390,12 +390,6 @@ public function testExceptionOnTargetPlaceholderCreationFail() $obj->getXMLFiltered($submission); } - /** - * The "Clone attachment" profile option used to only flag the submission is_cloned=1 and - * defer the actual clone to UploadJob's separate processCloning() poll. That poll is gone, - * so sendForTranslation() must clone the attachment itself, synchronously, right where it - * already holds the submission. - */ public function testSendForTranslationClonesAttachmentSynchronously() { $attachment = $this->createMock(SubmissionEntity::class); @@ -418,10 +412,6 @@ public function testSendForTranslationClonesAttachmentSynchronously() $core->sendForTranslation($item); } - /** - * A clone failure must be recorded as a visible error, the same way a failed upload is, - * instead of silently disappearing or aborting the whole cron run. - */ public function testSendForTranslationRecordsErrorWhenAttachmentCloneFails() { $attachment = $this->createMock(SubmissionEntity::class); @@ -449,10 +439,6 @@ public function testSendForTranslationRecordsErrorWhenAttachmentCloneFails() $core->sendForTranslation($item); } - /** - * Guards against a regression where cloning is attempted for content it was never meant - * for: non-attachment submissions must still go through the normal upload path. - */ public function testSendForTranslationDoesNotCloneNonAttachmentContent() { $post = $this->createMock(SubmissionEntity::class); diff --git a/tests/Smartling/DbAl/UploadQueueManagerTest.php b/tests/Smartling/DbAl/UploadQueueManagerTest.php index d033fa68..b4481dc8 100644 --- a/tests/Smartling/DbAl/UploadQueueManagerTest.php +++ b/tests/Smartling/DbAl/UploadQueueManagerTest.php @@ -262,14 +262,6 @@ public function query() {} $this->assertNull($uploadQueueManager->dequeue(1), 'Must not hand out an item whose claim could not be confirmed'); } - /** - * $wpdb->query() returns the number of affected rows for a successful UPDATE - 0 when - * the WHERE matched nothing. If claim() re-checks the row is still unclaimed (a real - * compare-and-swap) instead of updating by id alone, a concurrent dequeue() that claimed - * the row first makes this UPDATE affect zero rows without erroring. Treating that as - * "not claimed" is the whole point of the fix: naively checking `!== false` would treat - * int 0 as success (0 !== false is true) and hand out a row someone else already claimed. - */ public function testDequeueDoesNotHandOutItemWhenClaimLosesRaceToAnotherProcess() { $this->mockDbAl(); @@ -307,11 +299,6 @@ public function query() {} $this->assertNull($uploadQueueManager->dequeue(1), 'A lost claim race must not be handed out'); } - /** - * claim() must re-check the row is still unclaimed (or stale) in the same UPDATE that - * writes the new claim, not just match by id - otherwise two concurrent dequeue() calls - * that both selected the same unclaimed row would both succeed in claiming it. - */ public function testClaimQueryRechecksRowIsStillUnclaimed() { $queries = []; @@ -362,13 +349,6 @@ public function testDequeueOnlyConsidersUnclaimedOrStaleRows() ); } - /** - * A queue row groups submissions that share the same content, so one submission - * with an unresolvable locale takes the whole row down. Every submission that - * still exists - the one that failed to resolve and any sibling that resolved - * just fine - must not just vanish: each needs a visible error instead of being - * left in New status with no queue row and no explanation. - */ public function testDequeueSetsErrorOnResolvedSiblingsWhenGroupIsUnprocessable() { $resolvableSubmission = $this->createMock(SubmissionEntity::class); @@ -473,12 +453,6 @@ public function testDequeueFailsSubmissionsOnceAttemptsAreExhausted() ); } - /** - * $wpdb->query() returns false on failure (deadlock, lock-wait timeout, connection - * blip) without throwing. If discardQueueItem()'s delete() silently fails, dequeue() - * must not treat the row as gone and re-select: the row comes back unchanged, so - * continuing the while loop would spin on it forever inside a single dequeue() call. - */ public function testDequeueStopsInsteadOfSpinningWhenDiscardFailsToDelete() { $this->mockDbAl(); @@ -517,13 +491,6 @@ public function query() {} ); } - /** - * dequeue() claims a row only after resolving every submission in it. If that - * resolution throws anything unexpected, the row must still end up discarded - * rather than left permanently unclaimed - otherwise a single misbehaving - * submission blocks the entire per-blog queue forever, since every future - * dequeue() call would hit the same exception before ever reaching claim(). - */ public function testDequeueDiscardsItemWhenResolvingASubmissionThrowsUnexpectedException() { $this->mockDbAl(); diff --git a/tests/Smartling/Jobs/UploadJobTest.php b/tests/Smartling/Jobs/UploadJobTest.php index b6f9e9ab..2f2b86f7 100644 --- a/tests/Smartling/Jobs/UploadJobTest.php +++ b/tests/Smartling/Jobs/UploadJobTest.php @@ -204,11 +204,6 @@ private function buildTwoSubmissionItem(): array return [$item, $submission1, $submission2]; } - /** - * The distributed lock's correctness role is now handled by UploadQueueManager's - * compare-and-swap claim(); UploadJob must not pay for the Smartling API round trips - * placeLockFlag()/dropLockFlag() would otherwise make on every processed item. - */ public function testRunDoesNotUseDistributedLockApi() { $item = $this->buildItem(); @@ -222,12 +217,6 @@ public function testRunDoesNotUseDistributedLockApi() $this->buildJob($uploadQueueManager, null, null, null, $api)->run(''); } - /** - * Cloning used to be polled here via findSubmissionForCloning(), deferring the actual - * clone to a separate loop. The "Clone attachment" profile option now clones - * synchronously inside sendForTranslation(), and the standalone clone-request feature - * has no live trigger anywhere in the UI, so UploadJob must not poll for cloning work. - */ public function testRunDoesNotPollForCloningWork() { $item = $this->buildItem(); diff --git a/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php b/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php index eb6a445d..3568a54a 100644 --- a/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php +++ b/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php @@ -115,12 +115,6 @@ public function __construct( } }; } - - /** - * Download still holds the distributed lock (only UploadJob opted out via - * usesDistributedLock()), so its cell must still reflect an invalid-credentials - * error from the lock probe. - */ public function testPrepareItemsDoesNotThrowWhenApiCredentialsAreInvalid(): void { $authError = new SmartlingApiException( @@ -141,10 +135,6 @@ public function testPrepareItemsDoesNotThrowWhenApiCredentialsAreInvalid(): void $this->assertStringContainsString('Invalid credentials', $downloadRow['run_cron']); } - /** - * Download still holds the distributed lock, so its cell must still show - * "Running" when the lock probe reports the resource is locked. - */ public function testPrepareItemsShowsRunningMessageWhenLockHeld(): void { $lockError = new SmartlingApiException( @@ -163,10 +153,6 @@ public function testPrepareItemsShowsRunningMessageWhenLockHeld(): void $this->assertStringContainsString('Running', $downloadRow['run_cron']); } - /** - * Download still holds the distributed lock, so its cell must still show - * "Running" for the SDK's wrapped-Guzzle-423 shape too. - */ public function testPrepareItemsShowsRunningMessageWhenSdkWrapsGuzzle423(): void { // Reproduces the real SDK behavior: BaseApiAbstract::sendRequest() catches @@ -193,12 +179,6 @@ public function testPrepareItemsShowsRunningMessageWhenSdkWrapsGuzzle423(): void $this->assertStringNotContainsString('API error', $downloadRow['run_cron']); } - /** - * UploadJob no longer holds the distributed lock (see UploadJob::usesDistributedLock()), - * so its cell must not probe it at all - the probe would now always "succeed" and never - * detect a real background upload run, making the check pointless while still costing a - * Smartling API round trip on every page load. - */ public function testUploadRowDoesNotProbeDistributedLock(): void { $api = $this->createMock(ApiWrapperInterface::class); @@ -212,10 +192,6 @@ public function testUploadRowDoesNotProbeDistributedLock(): void $this->assertStringNotContainsString('Running', $uploadRow['run_cron']); } - /** - * The upload row shows a live counter span that JS polls and refreshes every - * second instead of the (no-longer-meaningful) "Running" indicator. - */ public function testUploadRowShowsLiveCounterSpanWithCurrentCount(): void { $api = $this->createMock(ApiWrapperInterface::class); @@ -228,11 +204,6 @@ public function testUploadRowShowsLiveCounterSpanWithCurrentCount(): void $this->assertStringContainsString('3', $uploadRow['run_cron']); } - /** - * JS needs a stable element wrapping the whole cell state (link + counter) so that - * when the polled count reaches zero, it can swap the entire cell to "Nothing to do" - * - the same state page load would render - rather than just zeroing the counter. - */ public function testUploadRowWrapsCellStateInStableContainer(): void { $api = $this->createMock(ApiWrapperInterface::class); From b127d1cc91b3efc7410db2cf615471b6b6c72d4b Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Thu, 3 Sep 2026 13:46:32 +0200 Subject: [PATCH 09/20] fix bulk-submit description precedence bug, remove dead clone-request flow (WP-1015) UserTranslationRequest::fromArray()'s description fallback mixed ?? and ?: without parens: `$array['description'] ?? count($ids) > 0 ? 'From Bulk Submit' : 'From Widget'` parses as `($array['description'] ?? (count($ids) > 0)) ? 'From Bulk Submit' : 'From Widget'`, since ?? binds tighter than ?:. Verified with a PHP repro: a real caller-supplied description was silently discarded and replaced by one of the two canned labels. Parenthesized the intended grouping instead. ContentRelationsDiscoveryService::clone() (the standalone "clone" formAction, reachable via ContentRelationsHandler::createSubmissionsHandler() regardless of UI exposure) stored submissions with isCloned=1/status=NEW and relied entirely on UploadJob::processCloning() to actually clone them. That executor was removed in an earlier commit on this branch, so clone() has been silently broken since: submissions it creates are now permanently stuck, never cloned, with no error surfaced. Removed clone() and ContentRelationsHandler's formAction=clone dispatch branch (FORM_ACTION_CLONE constant included) rather than fix an already-dead feature - the React tab bar has no clone tab (confirmed dead via its TabPanel definition), so nothing can reach this path. UserCloneRequest becomes pointless once clone() is gone: it was only a base class for UserTranslationRequest and a type hint on getSources(), which is only ever called with a UserTranslationRequest now. Merged its properties/getters into UserTranslationRequest directly and deleted the class, retyping getSources() accordingly. Also dropped the now-provably-dead 'clone' tab conditionals in js/app.js (tab.name can only ever be 'new'/'existing'/'instant'). inc/Smartling/WP/View/ContentEditJob.php's one remaining FORM_ACTION_CLONE reference is left untouched - that legacy jQuery view is documented as kept for backwards compatibility only and is already unreachable (wrapped in display:none). Found during code review of PR #630. Co-Authored-By: Claude Sonnet 5 --- inc/Smartling/Models/UserCloneRequest.php | 67 --------------- .../Models/UserTranslationRequest.php | 54 +++++++++++- .../ContentRelationsDiscoveryService.php | 53 +----------- .../Services/ContentRelationsHandler.php | 8 +- js/app.js | 8 +- tests/IntegrationTests/tests/CloneTest.php | 84 ------------------- tests/Models/CloneRequestTest.php | 29 ------- tests/Models/TranslationRequestTest.php | 36 ++++++++ .../ContentRelationsDiscoveryServiceTest.php | 74 ---------------- .../Services/ContentRelationsHandlerTest.php | 42 ++++++---- 10 files changed, 121 insertions(+), 334 deletions(-) delete mode 100644 inc/Smartling/Models/UserCloneRequest.php delete mode 100644 tests/Models/CloneRequestTest.php diff --git a/inc/Smartling/Models/UserCloneRequest.php b/inc/Smartling/Models/UserCloneRequest.php deleted file mode 100644 index f56f8807..00000000 --- a/inc/Smartling/Models/UserCloneRequest.php +++ /dev/null @@ -1,67 +0,0 @@ -contentId = $contentId; - $this->contentType = $contentType; - $this->description = $description; - krsort($relations); - $this->relations = $relations; - $this->targetBlogIds = ArrayHelper::toArrayOfIntegers($targetBlogIds, 'Target blog id expected to be numeric'); - } - - public function getContentId(): int - { - return $this->contentId; - } - - public function getContentType(): string - { - return $this->contentType; - } - - public function getDescription(): string - { - return $this->description; - } - - public function getRelationsOrdered(): array - { - return $this->relations; - } - - /** - * @return int[] - */ - public function getTargetBlogIds(): array - { - return $this->targetBlogIds; - } - - public static function fromArray(array $array): self - { - return new self(self::getSourceId($array), $array['source']['contentType'], $array['relations'] ?? [], explode(',', $array['targetBlogIds'])); - } - - protected static function getSourceId(array $array): int - { - $id = $array['source']['id'][0] ?? null; - if ($id === null) { - throw new SmartlingHumanReadableException('Source content id is empty, please save content prior to uploading', 'source.id.empty', 400); - } - return (int)$id; - } -} diff --git a/inc/Smartling/Models/UserTranslationRequest.php b/inc/Smartling/Models/UserTranslationRequest.php index a23096d4..a2a2991b 100644 --- a/inc/Smartling/Models/UserTranslationRequest.php +++ b/inc/Smartling/Models/UserTranslationRequest.php @@ -2,20 +2,59 @@ namespace Smartling\Models; +use Smartling\Exception\SmartlingHumanReadableException; use Smartling\Helpers\ArrayHelper; -class UserTranslationRequest extends UserCloneRequest +class UserTranslationRequest { + private int $contentId; + private string $contentType; + private string $description; + private array $relations; + private array $targetBlogIds; private JobInformation $jobInformation; private array $ids; public function __construct(int $contentId, string $contentType, array $relations, array $targetBlogIds, JobInformation $jobInformation, array $ids = [], string $description = '') { - parent::__construct($contentId, $contentType, $relations, $targetBlogIds, $description); + $this->contentId = $contentId; + $this->contentType = $contentType; + $this->description = $description; + krsort($relations); + $this->relations = $relations; + $this->targetBlogIds = ArrayHelper::toArrayOfIntegers($targetBlogIds, 'Target blog id expected to be numeric'); $this->jobInformation = $jobInformation; $this->ids = self::toIntegerArray($ids); } + public function getContentId(): int + { + return $this->contentId; + } + + public function getContentType(): string + { + return $this->contentType; + } + + public function getDescription(): string + { + return $this->description; + } + + public function getRelationsOrdered(): array + { + return $this->relations; + } + + /** + * @return int[] + */ + public function getTargetBlogIds(): array + { + return $this->targetBlogIds; + } + public function getJobInformation(): JobInformation { return $this->jobInformation; @@ -39,7 +78,7 @@ public static function fromArray(array $array): self explode(',', $array['targetBlogIds']), new JobInformation($array['job']['id'], $array['job']['authorize'] === 'true', $array['job']['name'], $array['job']['description'], $array['job']['dueDate'], $array['job']['timeZone']), $ids, - $array['description'] ?? count($ids) > 0 ? 'From Bulk Submit' : 'From Widget', + $array['description'] ?? (count($ids) > 0 ? 'From Bulk Submit' : 'From Widget'), ); } @@ -48,6 +87,15 @@ public function isBulk(): bool return count($this->ids) > 0; } + private static function getSourceId(array $array): int + { + $id = $array['source']['id'][0] ?? null; + if ($id === null) { + throw new SmartlingHumanReadableException('Source content id is empty, please save content prior to uploading', 'source.id.empty', 400); + } + return (int)$id; + } + private static function validate(array $array): void { if (!array_key_exists('source', $array)) { diff --git a/inc/Smartling/Services/ContentRelationsDiscoveryService.php b/inc/Smartling/Services/ContentRelationsDiscoveryService.php index 108c2dec..663ee8db 100644 --- a/inc/Smartling/Services/ContentRelationsDiscoveryService.php +++ b/inc/Smartling/Services/ContentRelationsDiscoveryService.php @@ -34,7 +34,6 @@ use Smartling\Helpers\WordpressFunctionProxyHelper; use Smartling\Jobs\JobEntity; use Smartling\Models\IntegerIterator; -use Smartling\Models\UserCloneRequest; use Smartling\Models\DetectedRelation; use Smartling\Models\DetectedRelations; use Smartling\Models\GutenbergBlock; @@ -146,56 +145,6 @@ public function bulkUpload( return $queueIds; } - public function clone(UserCloneRequest $request): void - { - $sourceBlogId = $this->wordpressProxy->get_current_blog_id(); - $submissionArray = [ - SubmissionEntity::FIELD_SOURCE_BLOG_ID => $sourceBlogId, - ]; - $submissions = []; - - foreach ($request->getTargetBlogIds() as $targetBlogId) { - $submissionArray[SubmissionEntity::FIELD_TARGET_BLOG_ID] = $targetBlogId; - $sources = $this->getSources($request, $targetBlogId); - - $sources[] = [ - 'id' => $request->getContentId(), - 'type' => $request->getContentType(), - ]; - - foreach ($sources as $source) { - $submissionArray[SubmissionEntity::FIELD_CONTENT_TYPE] = $source['type']; - $submissionArray[SubmissionEntity::FIELD_SOURCE_ID] = (int)$source['id']; - $existing = $this->submissionManager->findTargetBlogSubmission( - $submissionArray[SubmissionEntity::FIELD_CONTENT_TYPE], - $submissionArray[SubmissionEntity::FIELD_SOURCE_BLOG_ID], - $submissionArray[SubmissionEntity::FIELD_SOURCE_ID], - $submissionArray[SubmissionEntity::FIELD_TARGET_BLOG_ID], - ); - if ($existing instanceof SubmissionEntity) { - $submission = $existing; - if ($submission->isLocked()) { - $this->getLogger()->debug('Skipping cloning for submissionId=' . $submission->getId() . ', because it is locked'); - continue; - } - $submission->setStatus(SubmissionEntity::SUBMISSION_STATUS_NEW); - } else { - $submissionArray[SubmissionEntity::FIELD_STATUS] = SubmissionEntity::SUBMISSION_STATUS_NEW; - $submissionArray[SubmissionEntity::FIELD_SUBMISSION_DATE] = DateTimeHelper::nowAsString(); - $submission = $this->submissionFactory->fromArray($submissionArray); - $title = $this->getTitle($submission); - if ($title !== '') { - $submission->setSourceTitle($title); - } - $submission->setFileUri($this->fileUriHelper->generateFileUri($submission)); - } - $submission->setIsCloned(1); - $submissions[] = $submission; - } - } - $this->submissionManager->storeSubmissions($submissions); - } - public function createSubmissions(UserTranslationRequest $request): void { $curBlogId = $this->wordpressProxy->get_current_blog_id(); @@ -652,7 +601,7 @@ public function getTitle(SubmissionEntity $submission): string } } - private function getSources(UserCloneRequest $request, int $targetBlogId): array + private function getSources(UserTranslationRequest $request, int $targetBlogId): array { $sources = []; diff --git a/inc/Smartling/Services/ContentRelationsHandler.php b/inc/Smartling/Services/ContentRelationsHandler.php index 62d8a935..43cde2f4 100644 --- a/inc/Smartling/Services/ContentRelationsHandler.php +++ b/inc/Smartling/Services/ContentRelationsHandler.php @@ -7,7 +7,6 @@ use Smartling\Helpers\LoggerSafeTrait; use Smartling\Helpers\SmartlingUserCapabilities; use Smartling\Helpers\WordpressFunctionProxyHelper; -use Smartling\Models\UserCloneRequest; use Smartling\Models\UserTranslationRequest; /** @@ -42,7 +41,6 @@ class ContentRelationsHandler extends BaseAjaxServiceAbstract public const ACTION_NAME_CREATE_SUBMISSIONS = 'smartling-create-submissions'; - public const FORM_ACTION_CLONE = 'clone'; public const FORM_ACTION_UPLOAD = 'upload'; private ContentRelationsDiscoveryService $service; @@ -96,11 +94,7 @@ public function createSubmissionsHandler(array $data = null): void $data = $_POST; } try { - if ($data['formAction'] === self::FORM_ACTION_CLONE) { - $this->service->clone(UserCloneRequest::fromArray($data)); - } else { - $this->service->createSubmissions(UserTranslationRequest::fromArray($data)); - } + $this->service->createSubmissions(UserTranslationRequest::fromArray($data)); $this->returnResponse(['status' => BaseAjaxServiceAbstract::RESPONSE_SUCCESS]); } catch (Exception $e) { $this->returnError('content.submission.failed', $e->getMessage()); diff --git a/js/app.js b/js/app.js index 3e8e12fe..b687bf31 100644 --- a/js/app.js +++ b/js/app.js @@ -249,7 +249,7 @@ function JobWizard({ isBulkSubmitPage, contentType, contentId, locales, ajaxUrl, const data = { _wpnonce: nonce, - formAction: activeTab === 'clone' ? 'clone' : 'upload', + formAction: 'upload', source: { contentType, id: isBulkSubmitPage ? [] : [contentId] }, job: { id: activeTab === 'new' ? '' : selectedJob, @@ -372,7 +372,7 @@ function JobWizard({ isBulkSubmitPage, contentType, contentId, locales, ajaxUrl, ) ), - tab.name !== 'clone' && tab.name !== 'instant' && el('div', {}, + tab.name !== 'instant' && el('div', {}, tab.name === 'new' && el(TextControl, { label: 'Name', value: jobName, onChange: setJobName }), el(TextareaControl, { label: 'Description', value: description, onChange: setDescription, rows: 3 }), el(TextControl, { @@ -385,7 +385,7 @@ function JobWizard({ isBulkSubmitPage, contentType, contentId, locales, ajaxUrl, el(CheckboxControl, { label: 'Authorize Job', checked: authorize, onChange: setAuthorize }) ), - (tab.name === 'instant' || tab.name !== 'clone') && el('div', {}, + el('div', {}, el('fieldset', { style: { marginTop: '16px', border: '1px solid #ddd', padding: '12px', borderRadius: '4px' } }, el('legend', { style: { fontWeight: 600, padding: '0 8px' } }, 'Target Locales'), el('div', { style: { display: 'flex', gap: '8px', marginBottom: '8px' } }, @@ -495,7 +495,7 @@ function JobWizard({ isBulkSubmitPage, contentType, contentId, locales, ajaxUrl, isBusy: submitting, disabled: submitting || pendingRequests > 0 || selectedLocales.length === 0, onClick: tab.name === 'instant' ? handleInstantTranslation : handleSubmit - }, tab.name === 'instant' ? 'Request Instant Translation' : tab.name === 'new' ? 'Create Job' : tab.name === 'clone' ? 'Clone' : 'Add to selected Job') + }, tab.name === 'instant' ? 'Request Instant Translation' : tab.name === 'new' ? 'Create Job' : 'Add to selected Job') ) )) ) diff --git a/tests/IntegrationTests/tests/CloneTest.php b/tests/IntegrationTests/tests/CloneTest.php index 1a3bef62..9ea2075a 100644 --- a/tests/IntegrationTests/tests/CloneTest.php +++ b/tests/IntegrationTests/tests/CloneTest.php @@ -2,90 +2,13 @@ namespace IntegrationTests\tests; -use Smartling\Helpers\ArrayHelper; use Smartling\Helpers\DateTimeHelper; use Smartling\Jobs\JobEntity; -use Smartling\Models\UserCloneRequest; use Smartling\Submissions\SubmissionEntity; use Smartling\Tests\IntegrationTests\SmartlingUnitTestCaseAbstract; use Smartling\Vendor\Smartling\Exceptions\SmartlingApiException; class CloneTest extends SmartlingUnitTestCaseAbstract { - public function testNoMediaDuplication(): void - { - $this->markTestSkipped('TODO'); - $content = ''; - $currentBlogId = get_current_blog_id(); - $targetBlogId = 2; - switch_to_blog($targetBlogId); - $attachmentCount = count($this->getAttachments()); - restore_current_blog(); - - $childPostId = $this->createPost('post', 'embedded post', 'embedded content'); - $imageId = $this->createAttachment(); - set_post_thumbnail($childPostId, $imageId); - wp_update_post([ - 'ID' => $imageId, - 'post_parent' => $childPostId, - ]); // Force ReferencedStdBasedContentProcessorAbstract change that caused regression initially - - $relationsDiscoveryService = $this->getContentRelationsDiscoveryService(); - $rootPostId = $this->createPost('post', 'root post', sprintf($content, $childPostId)); - $addedMetaKey = 'contribute_slug_to_childpage_url'; - $addedMetaValue = [ - 'use_page_name' => true, - $addedMetaKey => false, - ]; - add_post_meta($rootPostId, $addedMetaKey, $addedMetaValue); - - $this->withBlockRules($this->getRulesManager(), [ - 'test' => [ - 'block' => 'test/post', - 'path' => 'id', - 'replacerId' => 'related|post', - ], - ], function () use ($childPostId, $imageId, $relationsDiscoveryService, $rootPostId, $targetBlogId) { - $references = $relationsDiscoveryService->getRelations('post', $rootPostId, [$targetBlogId]); - $postReferences = array_filter($references->getReferences(), static fn($rel) => $rel->getContentType() === 'post'); - $this->assertCount(1, $postReferences); - $this->assertEquals($childPostId, $postReferences[0]->getId()); - $relationsDiscoveryService->clone(new UserCloneRequest($rootPostId, 'post', [ - $targetBlogId => [ - 'post' => [$childPostId], - 'attachment' => [$imageId], - ], - ], [$targetBlogId])); - $this->executeUpload(); - }); - - switch_to_blog($targetBlogId); - $this->assertCount($attachmentCount + 1, $this->getAttachments(), 'Expected exactly one more attachment in target blog after cloning'); - $rootSubmission = ArrayHelper::first($this->getSubmissionManager()->find([ - SubmissionEntity::FIELD_SOURCE_BLOG_ID => $currentBlogId, - SubmissionEntity::FIELD_SOURCE_ID => $rootPostId, - ])); - $childSubmission = ArrayHelper::first($this->getSubmissionManager()->find([ - SubmissionEntity::FIELD_SOURCE_BLOG_ID => $currentBlogId, - SubmissionEntity::FIELD_SOURCE_ID => $childPostId, - ])); - $imageSubmission = ArrayHelper::first($this->getSubmissionManager()->find([ - SubmissionEntity::FIELD_SOURCE_BLOG_ID => $currentBlogId, - SubmissionEntity::FIELD_SOURCE_ID => $imageId, - ])); - $this->assertInstanceOf(SubmissionEntity::class, $rootSubmission); - $this->assertInstanceOf(SubmissionEntity::class, $childSubmission); - $this->assertInstanceOf(SubmissionEntity::class, $imageSubmission); - $childPostTargetId = $childSubmission->getTargetId(); - $post = get_post($rootSubmission->getTargetId()); - $this->assertEquals(sprintf($content, $childPostTargetId), $post->post_content, 'Expected root post to reference child post id at the target blog'); - $this->assertEquals($addedMetaValue, get_post_meta($rootSubmission->getTargetId(), $addedMetaKey, true), 'Expected boolean values in array metadata to be preserved'); - $imageTargetId = $imageSubmission->getTargetId(); - $this->assertEquals($imageTargetId, get_post_meta($childPostTargetId, '_thumbnail_id', true), 'Expected child post to reference attachment id at the target blog'); - $this->assertNotEquals($childPostId, $childPostTargetId, 'Expected child post id to change in translation'); - $this->assertNotEquals($imageId, $imageTargetId, 'Expected attachment id to change in translation'); - restore_current_blog(); - } - public function testLocking(): void { $content = << 'attachment']); - } } diff --git a/tests/Models/CloneRequestTest.php b/tests/Models/CloneRequestTest.php deleted file mode 100644 index e76553a3..00000000 --- a/tests/Models/CloneRequestTest.php +++ /dev/null @@ -1,29 +0,0 @@ - ContentRelationsHandler::FORM_ACTION_UPLOAD, - 'source' => ['id' => [$sourceId], 'contentType' => $sourceContentType], - 'relations' => [ - 1 => [$targetBlogId => ['post' => [3]]], - 2 => [$targetBlogId => ['attachment' => [5]]], - ], - 'targetBlogIds' => (string)$targetBlogId, - ]); - $this->assertEquals($sourceId, $x->getContentId()); - $this->assertEquals($sourceContentType, $x->getContentType()); - $this->assertEquals([1 => [$targetBlogId => ['post' => [3]]], 2 => [$targetBlogId => ['attachment' => [5]]]], $x->getRelationsOrdered()); - } -} diff --git a/tests/Models/TranslationRequestTest.php b/tests/Models/TranslationRequestTest.php index d0872ce3..6084683a 100644 --- a/tests/Models/TranslationRequestTest.php +++ b/tests/Models/TranslationRequestTest.php @@ -70,4 +70,40 @@ public function testFromArrayBulkUploadWithEmptySourceId() $this->assertEquals($ids, $x->getIds()); $this->assertEquals('post', $x->getContentType()); } + + public function testFromArrayDefaultsDescriptionToBulkSubmitWhenBulk() + { + $x = UserTranslationRequest::fromArray($this->buildArray(['ids' => [13, 14, 15]])); + $this->assertEquals('From Bulk Submit', $x->getDescription()); + } + + public function testFromArrayDefaultsDescriptionToWidgetWhenNotBulk() + { + $x = UserTranslationRequest::fromArray($this->buildArray()); + $this->assertEquals('From Widget', $x->getDescription()); + } + + public function testFromArrayPreservesExplicitTopLevelDescription() + { + $x = UserTranslationRequest::fromArray($this->buildArray(['description' => 'My custom description'])); + $this->assertEquals('My custom description', $x->getDescription()); + } + + private function buildArray(array $overrides = []): array + { + return array_merge([ + 'job' => [ + 'id' => '', + 'name' => '', + 'description' => '', + 'dueDate' => '', + 'timeZone' => 'Europe/Kyiv', + 'authorize' => 'true', + ], + 'formAction' => ContentRelationsHandler::FORM_ACTION_UPLOAD, + 'source' => ['id' => [5], 'contentType' => 'post'], + 'relations' => [], + 'targetBlogIds' => '2', + ], $overrides); + } } diff --git a/tests/Services/ContentRelationsDiscoveryServiceTest.php b/tests/Services/ContentRelationsDiscoveryServiceTest.php index 8ea39c0d..a480947f 100644 --- a/tests/Services/ContentRelationsDiscoveryServiceTest.php +++ b/tests/Services/ContentRelationsDiscoveryServiceTest.php @@ -52,7 +52,6 @@ function apply_filters($a, ...$b) { use Smartling\Jobs\SubmissionJobEntity; use Smartling\Jobs\SubmissionsJobsManager; use Smartling\Models\GutenbergBlock; - use Smartling\Models\UserCloneRequest; use Smartling\Models\UserTranslationRequest; use Smartling\Processors\ContentEntitiesIOFactory; use Smartling\Replacers\ContentIdReplacer; @@ -519,79 +518,6 @@ private function restoreDependencyInjection(): void $containerBuilder->set('factory.contentIO', $this->factory); } - public function testCloningNoDuplication() - { - $this->prepareDependencyInjection(VirtualEntityAbstract::class); - - $contentType = 'post'; - $childPostId = 2; - $rootPostId = 1; - $sourceBlogId = 1; - $targetBlogId = 2; - - $siteHelper = $this->createMock(SiteHelper::class); - $siteHelper->method('getCurrentBlogId')->willReturn($sourceBlogId); - - $contentHelper = $this->createMock(ContentHelper::class); - $contentHelper->method('getSiteHelper')->willReturn($siteHelper); - $submissionManager = $this->createMock(SubmissionManager::class); - - $matcher = $this->exactly(2); - $submissionManager->expects($matcher)->method('findTargetBlogSubmission')->willReturnCallback(function ($actualContentType, $actualSourceBlogId, $contentId, $actualTargetBlogId) use ($contentType, $childPostId, $rootPostId, $sourceBlogId, $targetBlogId, $matcher) { - $this->assertEquals($contentType, $actualContentType); - $this->assertEquals($sourceBlogId, $actualSourceBlogId); - $this->assertEquals($targetBlogId, $actualTargetBlogId); - switch ($matcher->getInvocationCount()) { - case 1: - $this->assertEquals($childPostId, $contentId); - break; - case 2: - $this->assertEquals($rootPostId, $contentId); - break; - } - }); - - $wpProxy = $this->createMock(WordpressFunctionProxyHelper::class); - $wpProxy->method('get_current_blog_id')->willReturn($sourceBlogId); - - $x = $this->getContentRelationDiscoveryService( - $this->createMock(ApiWrapper::class), - $contentHelper, - $this->createMock(SettingsManager::class), - $submissionManager, - wpProxy: $wpProxy, - ); - $x->clone(new UserCloneRequest($rootPostId, $contentType, [$targetBlogId => [$contentType => [$childPostId]]], [$targetBlogId])); - - $this->restoreDependencyInjection(); - } - - public function testCloningSkipsLockedSubmissions() - { - $contentType = 'post'; - $contentId = 1; - $targetBlogId = 2; - $existing = $this->createMock(SubmissionEntity::class); - $existing->method('isLocked')->willReturn(true); - $existing->expects($this->never())->method('setStatus'); - - $submissionManager = $this->createMock(SubmissionManager::class); - $submissionManager->expects($this->once())->method('findTargetBlogSubmission')->willReturn($existing); - $submissionManager->expects($this->once())->method('storeSubmissions')->with([]); - - $wpProxy = $this->createMock(WordpressFunctionProxyHelper::class); - $wpProxy->method('get_current_blog_id')->willReturn(1); - - $x = $this->getContentRelationDiscoveryService( - $this->createMock(ApiWrapper::class), - $this->createMock(ContentHelper::class), - $this->createMock(SettingsManager::class), - $submissionManager, - wpProxy: $wpProxy, - ); - $x->clone(new UserCloneRequest($contentId, $contentType, [], [$targetBlogId])); - } - public function testParentPageReferenceDetected() { $parentId = 10001; diff --git a/tests/Services/ContentRelationsHandlerTest.php b/tests/Services/ContentRelationsHandlerTest.php index 32b35603..9636dff7 100644 --- a/tests/Services/ContentRelationsHandlerTest.php +++ b/tests/Services/ContentRelationsHandlerTest.php @@ -5,7 +5,7 @@ use PHPUnit\Framework\TestCase; use Smartling\Helpers\ArrayHelper; use Smartling\Helpers\WordpressFunctionProxyHelper; -use Smartling\Models\UserCloneRequest; +use Smartling\Models\UserTranslationRequest; use Smartling\Services\ContentRelationsDiscoveryService; use Smartling\Services\ContentRelationsHandler; @@ -20,10 +20,10 @@ private function makeWpProxy(bool $currentUserCan = true): WordpressFunctionProx return $proxy; } - public function testCreateSubmissionsHandlerCloneNoRelations() + public function testCreateSubmissionsHandlerUploadNoRelations() { $service = $this->createMock(ContentRelationsDiscoveryService::class); - $service->expects($this->once())->method('clone')->willReturnCallback(function (UserCloneRequest $request) { + $service->expects($this->once())->method('createSubmissions')->willReturnCallback(function (UserTranslationRequest $request) { $this->request = $request; }); $proxy = $this->makeWpProxy(); @@ -37,18 +37,18 @@ public function returnError($key, $message, $responseCode = 400): void TestCase::fail('Should not return error, got ' . $message); } }; - $x->createSubmissionsHandler(['formAction' => ContentRelationsHandler::FORM_ACTION_CLONE, 'source' => ['id' => [13], 'contentType' => 'post'], 'targetBlogIds' => '2,3']); - $this->assertInstanceOf(UserCloneRequest::class, $this->request); + $x->createSubmissionsHandler($this->buildData(['source' => ['id' => [13], 'contentType' => 'post'], 'targetBlogIds' => '2,3'])); + $this->assertInstanceOf(UserTranslationRequest::class, $this->request); $this->assertEquals(13, $this->request->getContentId()); $this->assertEquals('post', $this->request->getContentType()); $this->assertEquals([], $this->request->getRelationsOrdered(), 'Should be empty array if no relations specified'); $this->assertEquals([2, 3], $this->request->getTargetBlogIds()); } - public function testCreateSubmissionsHandlerCloneRelations() + public function testCreateSubmissionsHandlerUploadRelations() { $service = $this->createMock(ContentRelationsDiscoveryService::class); - $service->expects($this->once())->method('clone')->willReturnCallback(function (UserCloneRequest $request) { + $service->expects($this->once())->method('createSubmissions')->willReturnCallback(function (UserTranslationRequest $request) { $this->request = $request; }); $targetBlogId = 2; @@ -63,16 +63,15 @@ public function returnError($key, $message, $responseCode = 400): void TestCase::fail('Should not return error, got ' . $message); } }; - $x->createSubmissionsHandler([ - 'formAction' => ContentRelationsHandler::FORM_ACTION_CLONE, + $x->createSubmissionsHandler($this->buildData([ 'source' => ['id' => [13], 'contentType' => 'post'], 'relations' => [ 1 => [$targetBlogId => ['post' => 3]], 2 => [$targetBlogId => ['attachment' => 5]], ], - 'targetBlogIds' => (string)$targetBlogId - ]); - $this->assertInstanceOf(UserCloneRequest::class, $this->request); + 'targetBlogIds' => (string)$targetBlogId, + ])); + $this->assertInstanceOf(UserTranslationRequest::class, $this->request); $this->assertEquals([1 => [$targetBlogId => ['post' => 3]], 2 => [$targetBlogId => ['attachment' => 5]]], $this->request->getRelationsOrdered()); $this->assertEquals([$targetBlogId => ['attachment' => 5]], ArrayHelper::first($this->request->getRelationsOrdered()), 'Should return deepest level first'); } @@ -80,7 +79,7 @@ public function returnError($key, $message, $responseCode = 400): void public function testCreateSubmissionsHandlerReturns403WhenCapabilityMissing(): void { $service = $this->createMock(ContentRelationsDiscoveryService::class); - $service->expects($this->never())->method('clone'); + $service->expects($this->never())->method('createSubmissions'); $proxy = $this->makeWpProxy(false); @@ -97,9 +96,24 @@ public function returnError($key, $message, $responseCode = 400): void } }; - $x->createSubmissionsHandler(['formAction' => ContentRelationsHandler::FORM_ACTION_CLONE, 'source' => ['id' => [1], 'contentType' => 'post'], 'targetBlogIds' => '2']); + $x->createSubmissionsHandler($this->buildData(['source' => ['id' => [1], 'contentType' => 'post'], 'targetBlogIds' => '2'])); $this->assertSame('permission.denied', $x->capturedErrorKey); $this->assertSame(403, $x->capturedErrorCode); } + + private function buildData(array $overrides = []): array + { + return array_merge([ + 'formAction' => ContentRelationsHandler::FORM_ACTION_UPLOAD, + 'job' => [ + 'id' => '', + 'name' => '', + 'description' => '', + 'dueDate' => '', + 'timeZone' => 'Europe/Kyiv', + 'authorize' => 'true', + ], + ], $overrides); + } } From 31741e4d69eb150eb84cf5d587196aaab05a96d4 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Thu, 3 Sep 2026 17:47:42 +0200 Subject: [PATCH 10/20] fix unused constant, add tests, remove cloneContent hook (WP-1015) --- inc/Smartling/Base/ExportedAPI.php | 6 - inc/Smartling/Base/SmartlingCore.php | 1 - inc/Smartling/WP/View/ContentEditJob.php | 2 +- .../WP/View/ContentEditJobViewTest.php | 260 ++++++++++++------ 4 files changed, 173 insertions(+), 96 deletions(-) diff --git a/inc/Smartling/Base/ExportedAPI.php b/inc/Smartling/Base/ExportedAPI.php index 61f1a2bc..8ec603b8 100755 --- a/inc/Smartling/Base/ExportedAPI.php +++ b/inc/Smartling/Base/ExportedAPI.php @@ -68,12 +68,6 @@ interface ExportedAPI */ public const ACTION_SMARTLING_SEND_FOR_TRANSLATION = 'smartling_send_for_translation'; - /** - * Action that clones content of given SubmissionEntity without translation - * @param SubmissionEntity - */ - public const ACTION_SMARTLING_CLONE_CONTENT = 'smartling_clone_content'; - /** * Action that downloads translation for given SubmissionEntity * @param SubmissionEntity diff --git a/inc/Smartling/Base/SmartlingCore.php b/inc/Smartling/Base/SmartlingCore.php index 8a3ea7c3..f08ffa05 100644 --- a/inc/Smartling/Base/SmartlingCore.php +++ b/inc/Smartling/Base/SmartlingCore.php @@ -40,7 +40,6 @@ public function __construct( ) { parent::__construct(); - $this->wpProxy->add_action(ExportedAPI::ACTION_SMARTLING_CLONE_CONTENT, [$this, 'cloneContent']); $this->wpProxy->add_action(ExportedAPI::ACTION_SMARTLING_PREPARE_SUBMISSION_UPLOAD, [$this, 'prepareUpload']); $this->wpProxy->add_action(ExportedAPI::ACTION_SMARTLING_SEND_FOR_TRANSLATION, [$this, 'sendForTranslation']); $this->wpProxy->add_action(ExportedAPI::ACTION_SMARTLING_DOWNLOAD_TRANSLATION, [$this, 'downloadTranslationBySubmission',]); diff --git a/inc/Smartling/WP/View/ContentEditJob.php b/inc/Smartling/WP/View/ContentEditJob.php index 60bb4298..f9a0a181 100644 --- a/inc/Smartling/WP/View/ContentEditJob.php +++ b/inc/Smartling/WP/View/ContentEditJob.php @@ -626,7 +626,7 @@ function (i, e) { }); var data = { - formAction: e.target.id === 'cloneButton' ? '' : '', + formAction: '', source: currentContent, job: { id: $("#jobSelect").val(), diff --git a/tests/Smartling/WP/View/ContentEditJobViewTest.php b/tests/Smartling/WP/View/ContentEditJobViewTest.php index 6c508dd7..74b506c6 100644 --- a/tests/Smartling/WP/View/ContentEditJobViewTest.php +++ b/tests/Smartling/WP/View/ContentEditJobViewTest.php @@ -1,102 +1,186 @@ resolveTemplate(true); - $this->assertDivBalanced($html, 'taxonomy edit screen ($needWrapper=true)'); - } - - public function testHtmlDivTagsBalanceOnPostEditScreen(): void - { - $html = $this->resolveTemplate(false); - $this->assertDivBalanced($html, 'post edit screen ($needWrapper=false)'); - } - - public function testHtmlDivTagsBalanceOnBulkSubmitScreen(): void - { - $html = $this->resolveTemplate(false, true); - $this->assertDivBalanced($html, 'bulk submit screen ($isBulkSubmitPage=true)'); +namespace { + if (!function_exists('plugin_dir_path')) { + function plugin_dir_path($file) + { + return rtrim(dirname($file), '/\\') . '/'; + } } - public function testHiddenWrapperHasClosingTag(): void - { - $html = $this->resolveTemplate(false); - - $this->assertSame( - 1, - preg_match( - '##i', - $html, - $matches, - PREG_OFFSET_CAPTURE - ), - 'Hidden display:none wrapper opening
should exist' - ); - - $tail = substr($html, $matches[0][1]); - $opens = preg_match_all('##i', $tail); - - $this->assertSame( - $opens, - $closes, - 'Hidden display:none wrapper must have a matching closing
' . - '(an unclosed wrapper here is what caused WP-1004).' - ); + if (!function_exists('get_current_screen')) { + function get_current_screen() + { + return null; + } } - private function resolveTemplate(bool $needWrapper, bool $isBulkSubmitPage = false): string - { - $source = $this->stripNonHtml(file_get_contents(self::VIEW_FILE)); - - $source = preg_replace_callback( - '#<\?php\s+if\s*\(\s*\$needWrapper\s*\)\s*:\s*\?>((?:(?!<\?php\s+endif).)*)<\?php\s+endif\s*;\s*\?>#s', - static fn(array $m): string => $needWrapper ? $m[1] : '', - $source - ); - - $source = preg_replace_callback( - '#<\?php\s.*?if\s*\(\s*!\$isBulkSubmitPage\s*\)\s*:\s*\?>((?:(?!<\?php\s+endif).)*)<\?php\s+endif\s*;\s*\?>#s', - static fn(array $m): string => $isBulkSubmitPage ? '' : $m[1], - $source - ); - - $source = preg_replace('#<\?(?:php|=).*?\?>#s', '', $source); - - return $source; + if (!function_exists('admin_url')) { + function admin_url($path = '') + { + return 'http://example.com/wp-admin/' . ltrim($path, '/'); + } } - private function stripNonHtml(string $source): string - { - $source = preg_replace('#]*>.*?#is', '', $source); - $source = preg_replace('#]*>.*?#is', '', $source); - - return $source; + if (!function_exists('wp_create_nonce')) { + function wp_create_nonce($action = -1) + { + return 'test-nonce'; + } } +} - private function assertDivBalanced(string $html, string $context): void +namespace Smartling\Tests\Smartling\WP\View { + + use PHPUnit\Framework\TestCase; + use Smartling\ApiWrapperInterface; + use Smartling\DbAl\LocalizationPluginProxyInterface; + use Smartling\Helpers\Cache; + use Smartling\Helpers\PluginInfo; + use Smartling\Helpers\SiteHelper; + use Smartling\Helpers\WordpressFunctionProxyHelper; + use Smartling\Settings\ConfigurationProfileEntity; + use Smartling\Settings\SettingsManager; + use Smartling\Submissions\SubmissionManager; + use Smartling\Tests\Mocks\WordpressFunctionsMockHelper; + use Smartling\WP\Controller\ContentEditJobController; + + class ContentEditJobViewTest extends TestCase { - $opens = preg_match_all('##i', $html); - - $this->assertSame( - $opens, - $closes, - sprintf( - 'Unbalanced
tags on %s: %d opens vs %d closes. ' . - 'A mismatch here is what caused WP-1004 (postboxes below the Smartling box refusing to open).', - $context, + private const VIEW_FILE = __DIR__ . '/../../../../inc/Smartling/WP/View/ContentEditJob.php'; + + public static function setUpBeforeClass(): void + { + WordpressFunctionsMockHelper::injectFunctionsMocks(); + } + + /** + * Regression test for a fatal error shipped alongside the clone-request removal: + * the legacy jQuery view (rendered unconditionally by every post/taxonomy edit + * screen and the bulk submit page, only visually hidden via CSS) kept referencing + * ContentRelationsHandler::FORM_ACTION_CLONE after that constant was deleted, + * which throws "Undefined constant" as soon as the view is actually rendered. + * Unlike the other tests in this file, this exercises the real PHP template + * (via WPAbstract::view()) rather than a regex-stripped copy of its markup, so it + * would have caught that. + */ + public function testViewRendersWithoutFatalError(): void + { + $controller = new ContentEditJobController( + $this->createMock(ApiWrapperInterface::class), + $this->createMock(LocalizationPluginProxyInterface::class), + $this->createMock(PluginInfo::class), + $this->createMock(SettingsManager::class), + $this->createMock(SiteHelper::class), + $this->createMock(SubmissionManager::class), + $this->createMock(Cache::class), + $this->createMock(WordpressFunctionProxyHelper::class), + ); + + $profile = $this->createMock(ConfigurationProfileEntity::class); + + ob_start(); + try { + $controller->view(['profile' => $profile, 'contentType' => 'post']); + } finally { + $html = ob_get_clean(); + } + + $this->assertStringContainsString('id="smartling-app"', $html); + $this->assertStringContainsString('id="createJob"', $html); + $this->assertStringContainsString("formAction: 'upload'", $html); + } + + public function testHtmlDivTagsBalanceOnTaxonomyEditScreen(): void + { + $html = $this->resolveTemplate(true); + $this->assertDivBalanced($html, 'taxonomy edit screen ($needWrapper=true)'); + } + + public function testHtmlDivTagsBalanceOnPostEditScreen(): void + { + $html = $this->resolveTemplate(false); + $this->assertDivBalanced($html, 'post edit screen ($needWrapper=false)'); + } + + public function testHtmlDivTagsBalanceOnBulkSubmitScreen(): void + { + $html = $this->resolveTemplate(false, true); + $this->assertDivBalanced($html, 'bulk submit screen ($isBulkSubmitPage=true)'); + } + + public function testHiddenWrapperHasClosingTag(): void + { + $html = $this->resolveTemplate(false); + + $this->assertSame( + 1, + preg_match( + '##i', + $html, + $matches, + PREG_OFFSET_CAPTURE + ), + 'Hidden display:none wrapper opening
should exist' + ); + + $tail = substr($html, $matches[0][1]); + $opens = preg_match_all('##i', $tail); + + $this->assertSame( + $opens, + $closes, + 'Hidden display:none wrapper must have a matching closing
' . + '(an unclosed wrapper here is what caused WP-1004).' + ); + } + + private function resolveTemplate(bool $needWrapper, bool $isBulkSubmitPage = false): string + { + $source = $this->stripNonHtml(file_get_contents(self::VIEW_FILE)); + + $source = preg_replace_callback( + '#<\?php\s+if\s*\(\s*\$needWrapper\s*\)\s*:\s*\?>((?:(?!<\?php\s+endif).)*)<\?php\s+endif\s*;\s*\?>#s', + static fn(array $m): string => $needWrapper ? $m[1] : '', + $source + ); + + $source = preg_replace_callback( + '#<\?php\s.*?if\s*\(\s*!\$isBulkSubmitPage\s*\)\s*:\s*\?>((?:(?!<\?php\s+endif).)*)<\?php\s+endif\s*;\s*\?>#s', + static fn(array $m): string => $isBulkSubmitPage ? '' : $m[1], + $source + ); + + $source = preg_replace('#<\?(?:php|=).*?\?>#s', '', $source); + + return $source; + } + + private function stripNonHtml(string $source): string + { + $source = preg_replace('#]*>.*?#is', '', $source); + $source = preg_replace('#]*>.*?#is', '', $source); + + return $source; + } + + private function assertDivBalanced(string $html, string $context): void + { + $opens = preg_match_all('##i', $html); + + $this->assertSame( $opens, - $closes - ) - ); + $closes, + sprintf( + 'Unbalanced
tags on %s: %d opens vs %d closes. ' . + 'A mismatch here is what caused WP-1004 (postboxes below the Smartling box refusing to open).', + $context, + $opens, + $closes + ) + ); + } } } From 14ea14a200aeb2c44be19e05260f4b3c36cb20e6 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Thu, 3 Sep 2026 20:16:56 +0200 Subject: [PATCH 11/20] fix upload queue delete race, dead clone UI, poller resilience, ajax dedup (WP-1015) Findings from code review of PR #630: - UploadQueueManager::delete() checked `!== false` instead of affected-rows > 0, the same pitfall claim() was already fixed for in this branch: a successful DELETE/UPDATE matching zero rows returns int(0), and `0 !== false` is true in PHP, so a lost race was reported as success. Now checked the same way as claim(). - ContentEditJob.php's legacy jQuery view still rendered a live Clone tab and #cloneButton wired to the create-submissions AJAX call with formAction hardcoded to upload, even though all backend clone handling was removed earlier on this branch. Removed the dead tab, button, and click-handler wiring; the view is otherwise unreachable (display:none) so this is a pure risk-reduction cleanup, not a behavior change. - The new upload-queue-count poller (js/smartling-connector-admin.js) had no .fail() handler, so a persistent AJAX error (nonce rotation, 5xx, network blip) left it silently polling admin-ajax.php once a second forever with no visible progress. Added a consecutive-failure counter that stops the interval after 5 failures. - The nonce + capability check block was duplicated near-verbatim across UploadQueueCountController and ContentRelationsHandler (2 handlers). Extracted into AjaxSecurityTrait::checkAjaxNonceAndCapability(), with failure reasons in a plain AjaxAuthorizationFailure class rather than trait constants, since trait constants require PHP 8.2 and this project targets PHP 8.0. Co-Authored-By: Claude Sonnet 5 --- inc/Smartling/DbAl/UploadQueueManager.php | 5 ++- .../Helpers/AjaxAuthorizationFailure.php | 16 +++++++ inc/Smartling/Helpers/AjaxSecurityTrait.php | 45 +++++++++++++++++++ .../Services/ContentRelationsHandler.php | 25 +++++++---- .../Controller/UploadQueueCountController.php | 15 ++++--- inc/Smartling/WP/View/ContentEditJob.php | 16 +------ js/smartling-connector-admin.js | 11 +++++ 7 files changed, 104 insertions(+), 29 deletions(-) create mode 100644 inc/Smartling/Helpers/AjaxAuthorizationFailure.php create mode 100644 inc/Smartling/Helpers/AjaxSecurityTrait.php diff --git a/inc/Smartling/DbAl/UploadQueueManager.php b/inc/Smartling/DbAl/UploadQueueManager.php index d122192e..45775a67 100644 --- a/inc/Smartling/DbAl/UploadQueueManager.php +++ b/inc/Smartling/DbAl/UploadQueueManager.php @@ -305,7 +305,10 @@ private function getSmartlingLocale(SubmissionEntity $submission): ?string */ private function delete(int $id): bool { - return $this->db->query(QueryBuilder::buildDeleteQuery($this->tableName, $this->idCondition($id))) !== false; + // Affected-rows is checked with `> 0`, not `!== false`: a successful DELETE matching + // zero rows returns int(0), and `0 !== false` is true in PHP, which would report a + // no-op delete as success (see claim()'s equivalent check for the same pitfall). + return $this->db->query(QueryBuilder::buildDeleteQuery($this->tableName, $this->idCondition($id))) > 0; } private function idCondition(int $id): ConditionBlock diff --git a/inc/Smartling/Helpers/AjaxAuthorizationFailure.php b/inc/Smartling/Helpers/AjaxAuthorizationFailure.php new file mode 100644 index 00000000..306ee374 --- /dev/null +++ b/inc/Smartling/Helpers/AjaxAuthorizationFailure.php @@ -0,0 +1,16 @@ +wpProxy->check_ajax_referer($nonceAction, '_wpnonce', false) === false) { + $this->getLogger()->warning(sprintf( + 'Invalid nonce for action "%s" from userId=%d', + $actionName, + $this->wpProxy->get_current_user_id(), + )); + + return AjaxAuthorizationFailure::INVALID_NONCE; + } + + if (!$this->wpProxy->current_user_can($capability)) { + $this->getLogger()->warning(sprintf( + 'User %d lacks capability "%s" for action "%s"', + $this->wpProxy->get_current_user_id(), + $capability, + $actionName, + )); + + return AjaxAuthorizationFailure::INSUFFICIENT_CAPABILITY; + } + + return null; + } +} diff --git a/inc/Smartling/Services/ContentRelationsHandler.php b/inc/Smartling/Services/ContentRelationsHandler.php index 43cde2f4..72d685c3 100644 --- a/inc/Smartling/Services/ContentRelationsHandler.php +++ b/inc/Smartling/Services/ContentRelationsHandler.php @@ -4,6 +4,8 @@ use Exception; use Smartling\Exception\SmartlingHumanReadableException; +use Smartling\Helpers\AjaxAuthorizationFailure; +use Smartling\Helpers\AjaxSecurityTrait; use Smartling\Helpers\LoggerSafeTrait; use Smartling\Helpers\SmartlingUserCapabilities; use Smartling\Helpers\WordpressFunctionProxyHelper; @@ -35,6 +37,7 @@ */ class ContentRelationsHandler extends BaseAjaxServiceAbstract { + use AjaxSecurityTrait; use LoggerSafeTrait; public const ACTION_NAME = 'smartling-get-relations'; @@ -79,13 +82,16 @@ public function register(): void */ public function createSubmissionsHandler(array $data = null): void { - if ($this->wpProxy->check_ajax_referer('smartling_translation', '_wpnonce', false) === false) { - $this->getLogger()->warning(sprintf('Invalid nonce for action "%s" from userId=%d', 'smartling_translation', get_current_user_id())); + $authFailure = $this->checkAjaxNonceAndCapability( + 'smartling_translation', + SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP, + self::ACTION_NAME_CREATE_SUBMISSIONS, + ); + if ($authFailure === AjaxAuthorizationFailure::INVALID_NONCE) { $this->returnError('invalid.nonce', 'Invalid nonce', 403); return; } - if (!$this->wpProxy->current_user_can(SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP)) { - $this->getLogger()->warning(sprintf('User %d lacks capability "%s"', get_current_user_id(), SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP)); + if ($authFailure === AjaxAuthorizationFailure::INSUFFICIENT_CAPABILITY) { $this->returnError('permission.denied', 'Insufficient permissions', 403); return; } @@ -103,13 +109,16 @@ public function createSubmissionsHandler(array $data = null): void public function actionHandler(): void { - if ($this->wpProxy->check_ajax_referer('smartling_translation', '_wpnonce', false) === false) { - $this->getLogger()->warning(sprintf('Invalid nonce for action "%s" from userId=%d', 'smartling_translation', get_current_user_id())); + $authFailure = $this->checkAjaxNonceAndCapability( + 'smartling_translation', + SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP, + static::ACTION_NAME, + ); + if ($authFailure === AjaxAuthorizationFailure::INVALID_NONCE) { $this->returnError('invalid.nonce', 'Invalid nonce', 403); return; } - if (!$this->wpProxy->current_user_can(SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP)) { - $this->getLogger()->warning(sprintf('User %d lacks capability "%s"', get_current_user_id(), SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP)); + if ($authFailure === AjaxAuthorizationFailure::INSUFFICIENT_CAPABILITY) { $this->returnError('permission.denied', 'Insufficient permissions', 403); return; } diff --git a/inc/Smartling/WP/Controller/UploadQueueCountController.php b/inc/Smartling/WP/Controller/UploadQueueCountController.php index f4e72102..be379acc 100644 --- a/inc/Smartling/WP/Controller/UploadQueueCountController.php +++ b/inc/Smartling/WP/Controller/UploadQueueCountController.php @@ -3,6 +3,8 @@ namespace Smartling\WP\Controller; use Smartling\DbAl\UploadQueueManager; +use Smartling\Helpers\AjaxAuthorizationFailure; +use Smartling\Helpers\AjaxSecurityTrait; use Smartling\Helpers\LoggerSafeTrait; use Smartling\Helpers\SmartlingUserCapabilities; use Smartling\Helpers\WordpressFunctionProxyHelper; @@ -10,6 +12,7 @@ class UploadQueueCountController implements WPHookInterface { + use AjaxSecurityTrait; use LoggerSafeTrait; private const ACTION_NAME = 'smartling_upload_queue_count'; @@ -27,14 +30,16 @@ public function register(): void public function handleGetCount(): void { - if ($this->wpProxy->check_ajax_referer('smartling_connector_ajax', '_wpnonce', false) === false) { - $this->getLogger()->warning('Invalid nonce for action "' . self::ACTION_NAME . '"'); + $authFailure = $this->checkAjaxNonceAndCapability( + 'smartling_connector_ajax', + SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP, + self::ACTION_NAME, + ); + if ($authFailure === AjaxAuthorizationFailure::INVALID_NONCE) { $this->wpProxy->wp_send_json_error(['message' => 'Invalid nonce'], 403); return; } - - if (!$this->wpProxy->current_user_can(SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP)) { - $this->getLogger()->warning('User lacks capability "' . SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP . '" for action "' . self::ACTION_NAME . '"'); + if ($authFailure === AjaxAuthorizationFailure::INSUFFICIENT_CAPABILITY) { $this->wpProxy->wp_send_json_error(['message' => 'Insufficient permissions'], 403); return; } diff --git a/inc/Smartling/WP/View/ContentEditJob.php b/inc/Smartling/WP/View/ContentEditJob.php index f9a0a181..7906f5da 100644 --- a/inc/Smartling/WP/View/ContentEditJob.php +++ b/inc/Smartling/WP/View/ContentEditJob.php @@ -97,7 +97,6 @@
New Job Existing Job - Clone'?>
@@ -212,7 +211,6 @@ - @@ -393,24 +391,12 @@ function (i, e) { $("div#job-tabs span").on("click", function () { $("div#job-tabs span").removeClass("active"); $(this).addClass("active"); - const hideWhenCloning = $('.hideWhenCloning'); - const cloneButton = $('#cloneButton'); switch ($(this).attr("data-action")) { case "new": Helper.ui.createJobForm.show(); - hideWhenCloning.show(); - cloneButton.addClass('hidden'); - break; - case "clone": - Helper.ui.createJobForm.hide(); - hideWhenCloning.hide(); - $('#addToJob').addClass('hidden'); - cloneButton.removeClass('hidden'); break; case "existing": Helper.ui.createJobForm.hide(); - hideWhenCloning.show(); - cloneButton.addClass('hidden'); break; default: } @@ -608,7 +594,7 @@ function (i, e) { && hasProp(window.wp.data, "dispatch") ; - $("#addToJob, #cloneButton").on("click", function (e) { + $("#addToJob").on("click", function (e) { e.stopPropagation(); e.preventDefault(); const btn = $(e.target); diff --git a/js/smartling-connector-admin.js b/js/smartling-connector-admin.js index 6adc7235..e3ecac01 100644 --- a/js/smartling-connector-admin.js +++ b/js/smartling-connector-admin.js @@ -235,6 +235,8 @@ jQuery(document).ready(function () { }) if (jQuery('#smartling-upload-queue-count').length > 0 && typeof smartlingConnector !== 'undefined') { + var uploadQueueCountConsecutiveFailures = 0; + var uploadQueueCountMaxConsecutiveFailures = 5; var uploadQueueCountInterval = setInterval(function () { var $counter = jQuery('#smartling-upload-queue-count'); if ($counter.length === 0) { @@ -245,6 +247,7 @@ jQuery(document).ready(function () { action: 'smartling_upload_queue_count', _wpnonce: smartlingConnector.nonce }).done(function (response) { + uploadQueueCountConsecutiveFailures = 0; if (response && response.success && response.data && typeof response.data.count !== 'undefined') { if (response.data.count === 0) { jQuery('#smartling-upload-cron-cell').text('Nothing to do'); @@ -260,6 +263,14 @@ jQuery(document).ready(function () { $current.removeClass('smartling-queue-count-changed'); } } + }).fail(function () { + // Transient nonce rotation / 5xx / network blip: keep the last known count + // displayed and retry on the next tick, but give up after repeated failures + // instead of hammering admin-ajax.php forever with no visible progress. + uploadQueueCountConsecutiveFailures++; + if (uploadQueueCountConsecutiveFailures >= uploadQueueCountMaxConsecutiveFailures) { + clearInterval(uploadQueueCountInterval); + } }); }, 1000); } From e13f921616199a3f2bbe4e83a8b63504665f3991 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Thu, 3 Sep 2026 20:55:22 +0200 Subject: [PATCH 12/20] remove dead cloning poll, unify InstantTranslationController AJAX auth (WP-1015) findSubmissionForCloning() had no remaining callers other than QueueManagerTableWidget's "Nothing to do" check, and existed only to support the old async cloning poll that sendForTranslation() replaced with a synchronous, in-place clone earlier on this branch. Deleted it along with the now-pointless WordpressFunctionProxyHelper dependency it was the only reason QueueManagerTableWidget (and its constructor wiring in ConfigurationProfilesController/services.yml) carried. InstantTranslationController's two AJAX handlers hand-rolled the same nonce+capability check AjaxSecurityTrait was introduced for elsewhere on this branch (ContentRelationsHandler, UploadQueueCountController), including a raw, non-proxied get_current_user_id() call. Switched both handlers to the shared trait for consistency. Co-Authored-By: Claude Sonnet 5 --- .../Submissions/SubmissionManager.php | 18 ------------- .../ConfigurationProfilesController.php | 3 --- .../InstantTranslationController.php | 27 ++++++++++++------- .../WP/Table/QueueManagerTableWidget.php | 4 +-- inc/config/services.yml | 1 - tests/Smartling/Jobs/UploadJobTest.php | 12 --------- .../Submissions/SubmissionManagerTest.php | 18 ------------- .../WP/Table/QueueManagerTableWidgetTest.php | 7 ----- 8 files changed, 18 insertions(+), 72 deletions(-) diff --git a/inc/Smartling/Submissions/SubmissionManager.php b/inc/Smartling/Submissions/SubmissionManager.php index afd0b8ec..f2927825 100644 --- a/inc/Smartling/Submissions/SubmissionManager.php +++ b/inc/Smartling/Submissions/SubmissionManager.php @@ -288,24 +288,6 @@ public function findOne(array $parameters): ?SubmissionEntity return null; } - public function findSubmissionForCloning(int $blogId): ?SubmissionEntity - { - $block = new ConditionBlock(ConditionBuilder::CONDITION_BLOCK_LEVEL_OPERATOR_AND); - $block->addCondition(Condition::getCondition(ConditionBuilder::CONDITION_SIGN_EQ, SubmissionEntity::FIELD_STATUS, [SubmissionEntity::SUBMISSION_STATUS_NEW])); - $block->addCondition(Condition::getCondition(ConditionBuilder::CONDITION_SIGN_EQ, SubmissionEntity::FIELD_IS_CLONED, [1])); - $block->addCondition(Condition::getCondition(ConditionBuilder::CONDITION_SIGN_EQ, SubmissionEntity::FIELD_IS_LOCKED, [0])); - $block->addCondition(new Condition(ConditionBuilder::CONDITION_SIGN_EQ, SubmissionEntity::FIELD_SOURCE_BLOG_ID, [$blogId])); - - $data = $this->fetchData(QueryBuilder::buildSelectQuery( - $this->getDbal()->completeTableName(SubmissionEntity::getTableName()), - array_keys(SubmissionEntity::getFieldDefinitions()), - $block, - ['id' => 'asc'], - ['limit' => 1, 'page' => 1], - )); - - return ArrayHelper::first($data) ?: null; - } /** * @param int[] $ids * @return SubmissionEntity[] diff --git a/inc/Smartling/WP/Controller/ConfigurationProfilesController.php b/inc/Smartling/WP/Controller/ConfigurationProfilesController.php index aad4f779..f79ac5d7 100644 --- a/inc/Smartling/WP/Controller/ConfigurationProfilesController.php +++ b/inc/Smartling/WP/Controller/ConfigurationProfilesController.php @@ -11,7 +11,6 @@ use Smartling\Helpers\PluginInfo; use Smartling\Helpers\SiteHelper; use Smartling\Helpers\SmartlingUserCapabilities; -use Smartling\Helpers\WordpressFunctionProxyHelper; use Smartling\Jobs\JobAbstract; use Smartling\Queue\QueueInterface; use Smartling\Services\GlobalSettingsManager; @@ -39,7 +38,6 @@ public function __construct( Cache $cache, private QueueInterface $queue, private UploadQueueManager $uploadQueueManager, - private WordpressFunctionProxyHelper $wpProxy, ) { parent::__construct($api, $connector, $pluginInfo, $settingsManager, $siteHelper, $manager, $cache); } @@ -204,7 +202,6 @@ public function listProfiles(): void $this->settingsManager, $this->submissionManager, $this->uploadQueueManager, - $this->wpProxy, ), ]); } diff --git a/inc/Smartling/WP/Controller/InstantTranslationController.php b/inc/Smartling/WP/Controller/InstantTranslationController.php index 56067f0b..3fb201cf 100644 --- a/inc/Smartling/WP/Controller/InstantTranslationController.php +++ b/inc/Smartling/WP/Controller/InstantTranslationController.php @@ -3,6 +3,8 @@ namespace Smartling\WP\Controller; use Smartling\FTS\FtsService; +use Smartling\Helpers\AjaxAuthorizationFailure; +use Smartling\Helpers\AjaxSecurityTrait; use Smartling\Helpers\DateTimeHelper; use Smartling\Helpers\FileUriHelper; use Smartling\Helpers\LoggerSafeTrait; @@ -15,6 +17,7 @@ class InstantTranslationController implements WPHookInterface { + use AjaxSecurityTrait; use LoggerSafeTrait; private const ACTION_REQUEST_TRANSLATION = 'smartling_instant_translation'; @@ -37,14 +40,16 @@ public function register(): void public function handleRequestTranslation(): void { - if ($this->wpProxy->check_ajax_referer('smartling_translation', '_wpnonce', false) === false) { - $this->getLogger()->warning(sprintf('Invalid nonce for action "%s" from userId=%d', 'smartling_translation', get_current_user_id())); + $authFailure = $this->checkAjaxNonceAndCapability( + 'smartling_translation', + SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP, + self::ACTION_REQUEST_TRANSLATION, + ); + if ($authFailure === AjaxAuthorizationFailure::INVALID_NONCE) { $this->wpProxy->wp_send_json_error(['message' => 'Invalid nonce'], 403); return; } - - if (!$this->wpProxy->current_user_can(SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP)) { - $this->getLogger()->warning(sprintf('User %d lacks capability "%s"', get_current_user_id(), SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP)); + if ($authFailure === AjaxAuthorizationFailure::INSUFFICIENT_CAPABILITY) { $this->wpProxy->wp_send_json_error(['message' => 'Insufficient permissions'], 403); return; } @@ -144,14 +149,16 @@ public function handleRequestTranslation(): void public function handlePollStatus(): void { - if ($this->wpProxy->check_ajax_referer('smartling_translation', '_wpnonce', false) === false) { - $this->getLogger()->warning(sprintf('Invalid nonce for action "%s" from userId=%d', 'smartling_translation', get_current_user_id())); + $authFailure = $this->checkAjaxNonceAndCapability( + 'smartling_translation', + SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP, + self::ACTION_POLL_STATUS, + ); + if ($authFailure === AjaxAuthorizationFailure::INVALID_NONCE) { $this->wpProxy->wp_send_json_error(['message' => 'Invalid nonce'], 403); return; } - - if (!$this->wpProxy->current_user_can(SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP)) { - $this->getLogger()->warning(sprintf('User %d lacks capability "%s"', get_current_user_id(), SmartlingUserCapabilities::SMARTLING_CAPABILITY_WIDGET_CAP)); + if ($authFailure === AjaxAuthorizationFailure::INSUFFICIENT_CAPABILITY) { $this->wpProxy->wp_send_json_error(['message' => 'Insufficient permissions'], 403); return; } diff --git a/inc/Smartling/WP/Table/QueueManagerTableWidget.php b/inc/Smartling/WP/Table/QueueManagerTableWidget.php index 356a1e14..6f9cd79f 100644 --- a/inc/Smartling/WP/Table/QueueManagerTableWidget.php +++ b/inc/Smartling/WP/Table/QueueManagerTableWidget.php @@ -6,7 +6,6 @@ use Smartling\DbAl\UploadQueueManager; use Smartling\Exception\EntityNotFoundException; use Smartling\Helpers\HtmlTagGeneratorHelper; -use Smartling\Helpers\WordpressFunctionProxyHelper; use Smartling\Jobs\DownloadTranslationJob; use Smartling\Jobs\JobAbstract; use Smartling\Jobs\LastModifiedCheckJob; @@ -38,7 +37,6 @@ public function __construct( protected SettingsManager $settingsManager, protected SubmissionManager $submissionManager, protected UploadQueueManager $uploadQueueManager, - protected WordpressFunctionProxyHelper $wpProxy, ) { $this->setSource($_REQUEST); @@ -142,7 +140,7 @@ public function prepare_items(): void private function getUploadCronActionCell(int $count): string { - if ($count === 0 && $this->submissionManager->findSubmissionForCloning($this->wpProxy->get_current_blog_id()) === null) { + if ($count === 0) { return self::MESSAGE_NOTHING_TO_DO; } diff --git a/inc/config/services.yml b/inc/config/services.yml index 0bad8513..b3b7041e 100644 --- a/inc/config/services.yml +++ b/inc/config/services.yml @@ -400,7 +400,6 @@ services: - "@site.cache" - "@queue.db" - "@manager.upload.queue" - - "@wp.proxy" wp.settings.edit: class: Smartling\WP\Controller\ConfigurationProfileFormController diff --git a/tests/Smartling/Jobs/UploadJobTest.php b/tests/Smartling/Jobs/UploadJobTest.php index 2f2b86f7..86ffbbaa 100644 --- a/tests/Smartling/Jobs/UploadJobTest.php +++ b/tests/Smartling/Jobs/UploadJobTest.php @@ -217,18 +217,6 @@ public function testRunDoesNotUseDistributedLockApi() $this->buildJob($uploadQueueManager, null, null, null, $api)->run(''); } - public function testRunDoesNotPollForCloningWork() - { - $item = $this->buildItem(); - $uploadQueueManager = $this->buildQueueManager($item); - $uploadQueueManager->method('complete'); - - $submissionManager = $this->createMock(SubmissionManager::class); - $submissionManager->expects($this->never())->method('findSubmissionForCloning'); - - $this->buildJob($uploadQueueManager, $submissionManager)->run(''); - } - private function buildItem(?SubmissionEntity $submission = null): UploadQueueItem { if ($submission === null) { diff --git a/tests/Smartling/Submissions/SubmissionManagerTest.php b/tests/Smartling/Submissions/SubmissionManagerTest.php index 4f425421..21d61e49 100644 --- a/tests/Smartling/Submissions/SubmissionManagerTest.php +++ b/tests/Smartling/Submissions/SubmissionManagerTest.php @@ -107,24 +107,6 @@ public function testSearch_ConditionBlockWithBlocksAndConditions() $x->searchByCondition($block); } - /** - * Locked submissions should not get cloned - */ - public function testFindSubmissionsForCloning() - { - $db = $this->db; - $x = $this->subject; - $x->method('getDbal')->willReturn($db); - $x->expects($this->once())->method('fetchData')->willReturnCallback(function(string $query) { - $this->assertStringContainsString("`is_cloned` = '1'", $query); - $this->assertStringContainsString("`is_locked` = '0'", $query); - $this->assertStringContainsString("`source_blog_id` = '1'", $query); - return []; - }); - - $x->findSubmissionForCloning(1); - } - public function testStoreEmptyEntity() { $x = $this->subject; diff --git a/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php b/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php index 3568a54a..fb2e4d30 100644 --- a/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php +++ b/tests/Smartling/WP/Table/QueueManagerTableWidgetTest.php @@ -46,7 +46,6 @@ function admin_url($path = '') use PHPUnit\Framework\TestCase; use Smartling\ApiWrapperInterface; use Smartling\DbAl\UploadQueueManager; - use Smartling\Helpers\WordpressFunctionProxyHelper; use Smartling\Queue\QueueInterface; use Smartling\Settings\ConfigurationProfileEntity; use Smartling\Settings\Locale; @@ -86,10 +85,6 @@ private function buildWidget( $submissionManager = $this->createMock(SubmissionManager::class); $submissionManager->method('getTotalInCheckStatusHelperQueue')->willReturn(0); - $submissionManager->method('findSubmissionForCloning')->willReturn(null); - - $wpProxy = $this->createMock(WordpressFunctionProxyHelper::class); - $wpProxy->method('get_current_blog_id')->willReturn(1); // Use an anonymous subclass to bypass WP_List_Table::__construct(), which // calls convert_to_screen() / get_current_screen() and requires a fully @@ -100,7 +95,6 @@ private function buildWidget( $settingsManager, $submissionManager, $uploadQueueManager, - $wpProxy, ) extends QueueManagerTableWidget { /** @noinspection PhpMissingParentConstructorInspection */ public function __construct( @@ -109,7 +103,6 @@ public function __construct( protected SettingsManager $settingsManager, protected SubmissionManager $submissionManager, protected UploadQueueManager $uploadQueueManager, - protected WordpressFunctionProxyHelper $wpProxy, ) { $this->setSource([]); } From ef06a23648acabe325683fa68bb23c09b61e49b7 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Thu, 3 Sep 2026 22:15:57 +0200 Subject: [PATCH 13/20] remove dead cloning path from bulk submit page (WP-1015) processBulkAction()'s $action==='clone' branch created isCloned=1 submissions and deliberately excluded them from the upload queue, relying on UploadJob::processCloning() to clone them later. That executor was already removed earlier on this branch (its trigger, UploadJob's async cloning poll, was replaced with a synchronous clone in sendForTranslation()), so any submission built via this path was silently orphaned forever: isCloned=1, status stuck, no error, no retry. Removed the $clone branch from processBulkAction() - every submission is now always prepared as a normal upload and enqueued. prepareForUpload() lost its now-always-false bool $clone parameter, and always sets isCloned(0). Also removed the dead UI that was the only way to reach this: the 'Clone' tab, its locale checkboxes and button in BulkSubmit.php, the action=clone hidden field, and WPAbstract::bulkSubmitCloneButton() - same class of dead legacy jQuery markup (wrapped in display:none, superseded by the React #smartling-app widget) already cleaned up for ContentEditJob.php earlier on this branch. ACTION_SMARTLING_CLONE_CONTENT and its documentation were already fully removed by an earlier commit on this branch; verified zero remaining references anywhere in the repo, so no further action needed there. Found during code review of PR #630. Co-Authored-By: Claude Sonnet 5 --- .../Base/SmartlingCoreUploadTrait.php | 5 +-- .../WP/Table/BulkSubmitTableWidget.php | 10 ++--- inc/Smartling/WP/View/BulkSubmit.php | 39 ------------------- inc/Smartling/WP/WPAbstract.php | 14 ------- .../WP/Table/BulkSubmitTableWidgetTest.php | 28 ++++++++++--- 5 files changed, 28 insertions(+), 68 deletions(-) diff --git a/inc/Smartling/Base/SmartlingCoreUploadTrait.php b/inc/Smartling/Base/SmartlingCoreUploadTrait.php index ea922973..f81f2357 100644 --- a/inc/Smartling/Base/SmartlingCoreUploadTrait.php +++ b/inc/Smartling/Base/SmartlingCoreUploadTrait.php @@ -622,7 +622,7 @@ public function sendForTranslation(UploadQueueItem $item): void } } - public function prepareForUpload(string $contentType, int $sourceBlog, int $sourceEntity, int $targetBlog, JobEntityWithBatchUid $jobInfo, bool $clone): SubmissionEntity + public function prepareForUpload(string $contentType, int $sourceBlog, int $sourceEntity, int $targetBlog, JobEntityWithBatchUid $jobInfo): SubmissionEntity { $translationHelper = $this->getTranslationHelper(); $submission = $translationHelper @@ -641,8 +641,7 @@ public function prepareForUpload(string $contentType, int $sourceBlog, int $sour $submission->setStatus(SubmissionEntity::SUBMISSION_STATUS_NEW); } - $isCloned = true === $clone ? 1 : 0; - $submission->setIsCloned($isCloned); + $submission->setIsCloned(0); $submission->setJobInfo($jobInfo->getJobInformationEntity()); return $this->getSubmissionManager()->storeEntity($submission); diff --git a/inc/Smartling/WP/Table/BulkSubmitTableWidget.php b/inc/Smartling/WP/Table/BulkSubmitTableWidget.php index e717c322..33e125f1 100644 --- a/inc/Smartling/WP/Table/BulkSubmitTableWidget.php +++ b/inc/Smartling/WP/Table/BulkSubmitTableWidget.php @@ -228,23 +228,19 @@ public function processBulkAction(): void $queueIds = new IntegerIterator(); if (is_array($submissions) && count($locales) > 0) { - $clone = 'clone' === $action; foreach ($submissions as $submission) { [$id] = explode('-', $submission); $type = $this->getContentTypeFilterValue(); $curBlogId = $this->getProfile()->getSourceLocale()->getBlogId(); foreach ($locales as $blogId => $blogName) { - $submissionId = $this->core->prepareForUpload( + $submissionId = $this->core->prepareForUpload( $type, $curBlogId, $id, (int)$blogId, - new JobEntityWithBatchUid($batchUid, $jobName, $clone ? '' : $smartlingData['jobId'], $profile->getProjectId()), - $clone, + new JobEntityWithBatchUid($batchUid, $jobName, $smartlingData['jobId'] ?? '', $profile->getProjectId()), )->getId(); - if (!$clone) { - $queueIds[] = $submissionId; - } + $queueIds[] = $submissionId; } } diff --git a/inc/Smartling/WP/View/BulkSubmit.php b/inc/Smartling/WP/View/BulkSubmit.php index 689240bb..563abbe2 100644 --- a/inc/Smartling/WP/View/BulkSubmit.php +++ b/inc/Smartling/WP/View/BulkSubmit.php @@ -3,7 +3,6 @@ use Smartling\Helpers\ArrayHelper; use Smartling\WP\Controller\BulkSubmitController; use Smartling\WP\Table\BulkSubmitTableWidget; -use Smartling\WP\WPAbstract; /** * @var BulkSubmitController $this @@ -76,7 +75,6 @@
Translate - Clone
@@ -88,48 +86,11 @@ $this->renderViewScript('ContentEditJob.php'); ?>
-
- - - -
-

-
- -
- getProfile() - ->getTargetLocales(); - - ArrayHelper::sortLocales($locales); - - foreach ($locales as $locale) { - if (!$locale->isEnabled()) { - continue; - } - ?> -

- getBlogId(), - $locale->getLabel(), - false - ); ?> -

- -
-
- -
- -
-