✨ feat: add segment disable/enable API for translation cancel requests#4488
✨ feat: add segment disable/enable API for translation cancel requests#4488
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an API v3 endpoint intended to “disable” a segment (cancel request) and propagates a disabled flag into segment-analysis responses, while introducing cache helpers around segment metadata and a shared “segment disabled” trait.
Changes:
- Added
/api/v3/jobs/:id_job/:password/segment/disable/:id_segmentroute and newCancelRequestControllerto mark a segment as disabled. - Introduced
SegmentDisabledTraitand wired it into segment analysis responses and translation submission flow. - Extended
SegmentMetadataDaowith cache-destruction helpers and a method to settranslation_disabledmetadata.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/Routes/api_v3_routes.php | Registers the new segment disable endpoint under API v3 jobs routes. |
| lib/Model/Segments/SegmentMetadataDao.php | Adds SQL constants, cache-destruction helpers, and a translation_disabled write helper. |
| lib/Controller/Traits/SegmentDisabledTrait.php | New trait providing cached “segment disabled” lookup and write-through cache helper. |
| lib/Controller/API/V3/SegmentAnalysisController.php | Adds disabled field to segment-analysis output using the new trait. |
| lib/Controller/API/V3/CancelRequestController.php | New controller implementing the disable/cancel-request behavior with rate limiting and checks. |
| lib/Controller/API/App/SetTranslationController.php | Blocks translation submission when a segment is marked disabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| // setting the cache to avoid multiple disable requests for the same segment in a short time frame | ||
| if (!$this->isSegmentDisabled($id_job, $id_segment)) { | ||
| SegmentMetadataDao::destroyGetAllCache($id_segment); | ||
| SegmentMetadataDao::setTranslationDisabled($id_segment); |
There was a problem hiding this comment.
When disabling a segment, you update DB via setTranslationDisabled() but only destroy the getAll() cache. If SegmentMetadataDao::get($id_segment, 'translation_disabled') was previously cached as empty (TTL=1 week), isSegmentDisabled()'s DB fallback can keep returning "enabled" after its 1h Redis TTL expires. Invalidate the key-specific DAO cache too (e.g. SegmentMetadataDao::destroyCache($id_segment, 'translation_disabled')) when writing the flag.
| SegmentMetadataDao::setTranslationDisabled($id_segment); | |
| SegmentMetadataDao::setTranslationDisabled($id_segment); | |
| SegmentMetadataDao::destroyCache($id_segment, 'translation_disabled'); |
| * Creates a controller with real performChecks but mocked external dependencies. | ||
| */ | ||
| private function createControllerWithPartialMock( | ||
| mixed $jobReturn = 'NOT_SET', | ||
| mixed $segmentReturn = 'NOT_SET', | ||
| ?UserStruct $user = null, | ||
| ?Response $rateLimitResponseIp = null, | ||
| ?Response $rateLimitResponseEmail = null, | ||
| ): CancelRequestController { | ||
| $controller = $this->getMockBuilder(CancelRequestController::class) |
There was a problem hiding this comment.
createControllerWithPartialMock() accepts $segmentReturn and multiple tests pass a SegmentTranslationStruct, but CancelRequestController::performChecks() reads the segment via the static SegmentTranslationDao::findBySegmentAndJob() and this helper never stubs that call. Those tests will be nondeterministic (and likely fail) unless the DB happens to contain the expected translation row. Consider wrapping the DAO call in an overridable/protected method (so it can be mocked) or set up explicit DB fixtures in the test.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…sts, reuse readonly in Segment.js Agent-Logs-Url: https://github.com/matecat/MateCat/sessions/3e5a31c8-de03-4d5e-bbf8-cb5f5f35701c Co-authored-by: mauretto78 <10035321+mauretto78@users.noreply.github.com>
## Summary Add early return guard after performChecks in cancelRequest to prevent further execution when the rate limit (429) has been triggered, matching the same pattern already used in enableRequest. ## Type - [x] `fix` — bug fix ## Changes | File | Change | |------|--------| | lib/Controller/API/V3/CancelRequestController.php | Add 429 early return in cancelRequest after performChecks | ## Testing - [x] New tests added for changed behavior
## Summary Fix all three test suites to pass in isolation without requiring a database connection. Introduce FakeRedisClient for Predis mock (PHPUnit 12 cannot mock __call magic methods), add findSegmentTranslation protected wrapper in CancelRequestController for testability. ## Type - [x] `test` — test coverage - [x] `refactor` — restructure without behavior change ## Changes | File | Change | |------|--------| | lib/Controller/API/V3/CancelRequestController.php | Extract findSegmentTranslation() protected wrapper for static DAO call | | tests/unit/Controllers/CancelRequestControllerTest.php | Fix all tests: use createStub, mock findSegmentTranslation, handle rate limit response code, remove non-existent 'not owner' test | | tests/unit/Traits/RateLimiterTraitTest.php | Replace Predis\Client mock with FakeRedisClient (in-memory fake) | | tests/unit/Traits/SegmentDisabledTraitTest.php | Use FakeRedisClient, skip destroySegmentDisabledCache (requires DB) | ## Testing - [x] New tests added for changed behavior - [x] All 68 tests pass (1 skipped due to DB dependency)
🧪 Test-Guard ReportCoverage Analysis: ❌ FAILNo changed source files found in coverage report (threshold: 80%)
Test File Matching: ❌ FAILFile matching: 7 pass, 1 warning, 8 fail
Per-File Evaluation:
|
| File | Verdict | Reason |
|---|---|---|
lib/Controller/API/App/SetTranslationController.php |
✅ pass | New segment disabled check and exception throwing tested indirectly via error code handling in JS tests. |
lib/Controller/API/V3/CancelRequestController.php |
✅ pass | Unit tests cover enableRequest and cancelRequest including validation and cache logic. |
lib/Controller/API/V3/SegmentAnalysisController.php |
✅ pass | Tests verify segment disabled flag inclusion in formatted segment output. |
lib/Controller/Traits/ChunkNotFoundHandlerTrait.php |
✅ pass | Only minor type hint changes, no behavioral changes needing tests. |
lib/Controller/Traits/RateLimiterTrait.php |
✅ pass | Tests cover TTL calculation and Redis interactions for rate limiting. |
lib/Controller/Traits/SegmentDisabledTrait.php |
✅ pass | Unit tests cover cache logic, database fallback, and cache clearing behavior. |
lib/Model/DataAccess/Database.php |
No tests added or modified; change is a safe transaction rollback guard. | |
lib/Model/ProjectCreation/SegmentStorageService.php |
✅ pass | Change is a minor type fix with no new behavior; no tests needed. |
lib/Model/Segments/SegmentMetadataDao.php |
⏭️ skip | AI analysis unavailable — deferred to Layer 2 |
lib/Model/Segments/SegmentMetadataStruct.php |
⏭️ skip | AI analysis unavailable — deferred to Layer 2 |
lib/Routes/api_v3_routes.php |
⏭️ skip | AI analysis unavailable — deferred to Layer 2 |
public/api/swagger-source.js |
⏭️ skip | AI analysis unavailable — deferred to Layer 2 |
public/js/actions/CatToolActions.js |
⏭️ skip | AI analysis unavailable — deferred to Layer 2 |
public/js/actions/SegmentActions.js |
⏭️ skip | AI analysis unavailable — deferred to Layer 2 |
public/js/components/segments/Segment.js |
⏭️ skip | AI analysis unavailable — deferred to Layer 2 |
public/js/utils/segmentUtils.js |
⏭️ skip | AI analysis unavailable — deferred to Layer 2 |
Result:
| public static function setTranslationDisabled(int $id_segment): void | ||
| { | ||
| $metadata = new SegmentMetadataStruct(); | ||
| $metadata->id_segment = $id_segment; | ||
| $metadata->meta_key = 'translation_disabled'; | ||
| $metadata->meta_value = "1"; | ||
|
|
||
| SegmentMetadataDao::save($metadata); | ||
| } |
There was a problem hiding this comment.
setTranslationDisabled() blindly inserts a new row each time. Since segment_metadata has no UNIQUE constraint on (id_segment, meta_key), concurrent disable requests can create duplicate translation_disabled rows. Consider deleting any existing row first or using an upsert strategy (and/or adding a unique index) to make this idempotent at the DB level.
| @@ -366,6 +370,7 @@ private function formatSegment( | |||
| 'notes' => (!empty($notesAggregate[$segmentForAnalysis->id]) ? $notesAggregate[$segmentForAnalysis->id] : []), | |||
| 'status' => $this->getStatusObject($segmentForAnalysis), | |||
| 'last_edit' => ($segmentForAnalysis->last_edit !== null) ? date(DATE_ATOM, strtotime($segmentForAnalysis->last_edit)) : null, | |||
| 'disabled' => $disabled, | |||
There was a problem hiding this comment.
formatSegment() calls isSegmentDisabled() per segment. On a cold cache this triggers one DB query per segment (up to 200/page) because isSegmentDisabled() falls back to SegmentMetadataDao::get(). Consider fetching all translation_disabled metadata in bulk (e.g. via SegmentMetadataDao::getBySegmentIds($segmentIds, 'translation_disabled') in getIssuesNotesAndIdRequests()) and deriving the disabled flag from that, avoiding an N+1 pattern.
| componentDidUpdate(prevProps) { | ||
| if (!isEqual(prevProps.segment, this.props.segment)) { | ||
| const readonly = SegmentUtils.isReadonlySegment(this.props.segment) | ||
| if (readonly !== this.state.readonly) { | ||
| this.setState({ | ||
| readonly, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
componentDidUpdate uses lodash.isEqual to deep-compare segment props on every update, even though shouldComponentUpdate already compares segImmutable. Deep equality here can be expensive across many Segment instances; consider using prevProps.segImmutable.equals(this.props.segImmutable) (or a cheap key/field comparison) as the change detector instead.
| const isTranslationDisabled = segment?.metadata?.some( | ||
| ({meta_key, meta_value}) => | ||
| meta_key === 'translation_disabled' && meta_value === '1', | ||
| ) | ||
|
|
||
| if (isTranslationDisabled) { | ||
| ModalsActions.showModalComponent( | ||
| AlertModal, | ||
| { | ||
| text: "This segment has been disabled by the project owner, so it cannot be translated.", | ||
| }, | ||
| 'Segment disabled', | ||
| ) | ||
| return |
There was a problem hiding this comment.
The translation_disabled metadata check is reimplemented inline here (and also inside SegmentUtils.isReadonlySegment). To avoid logic drift, consider extracting a single helper in segmentUtils (e.g. isTranslationDisabled(segment)) and reusing it from both places.
| require_once __DIR__ . '/RateLimiterTraitTest.php'; | ||
|
|
There was a problem hiding this comment.
This test file require_onces another test file to reuse FakeRedisClient. Test-to-test dependencies make the suite order-sensitive and harder to maintain. Consider moving FakeRedisClient into a shared test helper (e.g. tests/TestHelpers/FakeRedisClient.php) and autoloading it instead.
| /** | ||
| * Removes the "translation_disabled" metadata for a given segment and clears the related cache. | ||
| * | ||
| * @param int $id_segment The unique identifier of the segment to clear metadata and cache for. | ||
| * @return void | ||
| * @throws ReflectionException | ||
| */ | ||
| protected function destroySegmentDisabledCache(int $id_job, int $id_segment): void | ||
| { | ||
| SegmentMetadataDao::delete($id_segment, 'translation_disabled'); | ||
| SegmentMetadataDao::destroyCache($id_segment, 'translation_disabled'); | ||
| SegmentMetadataDao::destroyGetAllCache($id_segment); |
There was a problem hiding this comment.
The docblock for destroySegmentDisabledCache() lists only $id_segment, but the method signature also takes $id_job. Please update the PHPDoc params to match the signature to avoid misleading IDE/static analysis output.
| { | ||
|
|
||
| public ?string $id_segment = null; | ||
| public int $id_segment; |
There was a problem hiding this comment.
SegmentMetadataStruct::$id_segment was changed to a non-nullable int. With PDO::FETCH_CLASS, MySQL/PDO typically hydrates numeric columns as strings, which will cause a TypeError when assigning to a typed int property. Consider keeping it as string|int (or ?string as before), or introducing a casting layer during hydration instead of a strict typed property here.
| public int $id_segment; | |
| public int|string $id_segment; |
| // If the cache is empty, it means that the segment is not already disabled, so we can proceed with disabling it and | ||
| // setting the cache to avoid multiple disable requests for the same segment in a short time frame | ||
| if (!$this->isSegmentDisabled($id_job, $id_segment)) { | ||
| SegmentMetadataDao::destroyGetAllCache($id_segment); |
There was a problem hiding this comment.
cancelRequest() updates the DB by inserting translation_disabled, but it only clears the getAll() cache. If SegmentMetadataDao::get($id_segment, 'translation_disabled') was cached as empty before disabling, it can stay stale (TTL=1 week) and later make isSegmentDisabled() incorrectly return false after Redis eviction. Clear the per-key cache too (e.g. destroy the get() cache entry) when disabling.
| SegmentMetadataDao::destroyGetAllCache($id_segment); | |
| SegmentMetadataDao::destroyGetAllCache($id_segment); | |
| SegmentMetadataDao::destroyGetCache($id_segment, 'translation_disabled'); |
Summary
Adds a V3 API endpoint to disable (cancel) and re-enable segments within a job, preventing translations on cancelled segments. Includes backend validation, caching layer, rate limiting, and frontend UI integration.
Type
feat— new user-facing featurefix— bug fixrefactor— restructure without behavior changechore— build, deps, config, docsperf— performance improvementtest— test coverageChanges
lib/Controller/API/V3/CancelRequestController.phpcancelRequest()andenableRequest()actions, rate limiting, ownership and status checkslib/Controller/Traits/SegmentDisabledTrait.phpisSegmentDisabled,saveSegmentDisabledInCache,destroySegmentDisabledCachewith Redis-backed cachinglib/Controller/API/App/SetTranslationController.phpcheckIfSegmentIsNotDisabled()lib/Controller/API/V3/SegmentAnalysisController.phpdisabledflag in segment analysis responselib/Model/Segments/SegmentMetadataDao.phpget(),delete(),setTranslationDisabled(),destroyCache(),destroyGetAllCache()methodslib/Model/DataAccess/Database.phprollback()withinTransaction()checklib/Routes/api_v3_routes.phplib/Controller/Traits/ChunkNotFoundHandlerTrait.phpgetJob()paramslib/Controller/Traits/RateLimiterTrait.phpintlib/Model/ProjectCreation/SegmentStorageService.phpid_segmenttype (remove string cast)public/js/actions/SegmentActions.jspublic/js/actions/CatToolActions.jspublic/js/components/segments/Segment.jspublic/js/utils/segmentUtils.jspublic/api/swagger-source.jsTesting
vendor/bin/phpunit --exclude-group=ExternalServices --no-coveragepasses./vendor/bin/phpstanpasses (0 errors, with baseline)Endpoint tested manually:
POST /api/v3/jobs/:id_job/:password/segment/disable/:id_segment— disables segment, returns{ "id_segment": N }POST /api/v3/jobs/:id_job/:password/segment/enable/:id_segment— re-enables segmentSetTranslationControllerrejects translations on disabled segmentsAI Disclosure
GitHub Copilot (code suggestions applied via PR review)
Notes
NEWDatabase::rollback()fix prevents errors when no transaction is active (defensive guard)