♻️ Refactor(current-result.service): Update readOnly logic to include additional status check#653
Conversation
|
There was a problem hiding this comment.
Review Notes
Overall, this is a clean, low-risk change — CI passes, SonarQube is green, and the intent is clear from reading the code. Just a few suggestions that could improve clarity and maintainability:
1. PR description
It would be helpful to add a brief description in the PR body explaining the business context — e.g., why "Approved" (status 6) results should now be editable. This makes it easier for reviewers (and future devs reading git history) to understand the reasoning behind the change.
2. Jira ticket reference
Consider linking a Jira ticket (P2-XXXX) in the commit message or PR title. This helps with traceability when looking back at changes later.
3. Test coverage for status_id == 6
The existing spec file (current-result.service.spec.ts) has tests for status_id == 1 (editable) and status_id == 2 (readOnly), but there's no test covering the new status_id == 6 scenario. Adding one would help document the expected behavior and prevent regressions.
4. Magic numbers
The values 1 and 6 are used directly in the condition. This is a pre-existing pattern, but as more status IDs get added to this logic, it might be worth considering extracting them into named constants (e.g., ResultStatus.Editing, ResultStatus.Approved) for readability. Just something to keep in mind for the future.
None of these are blockers — just ideas to consider for improving maintainability going forward. 👍
yecksin
left a comment
yecksin
left a comment
There was a problem hiding this comment.
Looks good — small, focused change with all CI checks passing. Left some suggestions for future improvements but nothing blocking. ✅
2 participants
No description provided.