Add support for Update action #35
Conversation
Fixed only reverse engineering corrections with an approval
There was a problem hiding this comment.
Pull request overview
This PR adds support for the DECLARATION_UPDATED action (Update action) and fixes the reverse engineering logic to only apply to corrections that have been approved. The changes include:
- Implementation of the Update action that creates a new DECLARE action with input/output tracking
- Refactored postProcess function to track approved corrections and only reverse engineer when corrections are approved
- Removed the full declaration from correction annotations to fix reverse engineering behavior
- Added comprehensive test coverage for updates, corrections without approval, and mixed scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| v1-to-v2-data-migration/tests/unit/postProcess.test.ts | Added test cases for Update actions, corrections without approval, and mixed scenarios; also added approval actions to existing correction tests to validate the new approval-based reverse engineering logic |
| v1-to-v2-data-migration/helpers/transform.ts | Implemented DECLARATION_UPDATED handling, refactored postProcess to track approved corrections, removed declaration spreading from correction annotations, and updated postProcess signature to accept current declaration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }) | ||
|
|
||
| const result = transform(registration, birthResolver, 'birth') | ||
| console.log(JSON.stringify(result, null, 2)) |
There was a problem hiding this comment.
This console.log statement should be removed before merging to production. Debug logging statements should not be committed to the codebase.
| console.log(JSON.stringify(result, null, 2)) |
| const birthResolver = buildBirthResolver() | ||
|
|
||
| await t.step( | ||
| 'should set annotation to previous declaration and reverse enngineer the original declaration', |
There was a problem hiding this comment.
Typo: 'enngineer' should be 'engineer'.
| action.declaration = previousDeclaration | ||
| if (hasKeys(annotation)) { | ||
| previousDeclaration = deepMerge(previousDeclaration, annotation) | ||
| action.annotation = deepMerge(previousDeclaration, annotation) |
There was a problem hiding this comment.
Line 549 merges annotation into previousDeclaration again after it was already merged on line 548. This appears to be a logic error. After line 548 updates previousDeclaration, line 549 should likely just assign previousDeclaration to action.annotation without merging annotation again. The current implementation would merge the annotation values twice, which is redundant and may produce incorrect results.
| action.annotation = deepMerge(previousDeclaration, annotation) | |
| action.annotation = previousDeclaration |
| corrections.push({ index: i, action: documents.actions[i] }) | ||
| } | ||
| } | ||
| const approvedCorrections = [] |
There was a problem hiding this comment.
The approvedCorrections array should have an explicit type annotation. Since it stores requestId values which can be string or undefined according to the Action interface, it should be typed as Array<string | undefined> or the push operation on line 528 should guard against undefined values.
| const approvedCorrections = [] | |
| const approvedCorrections: Array<string | undefined> = [] |
Also fixed only reverse engineering corrections with an approval