fix: stop the Finish tab prescribing business process it invented (KAN-323) - #513
Conversation
An audit of user-facing strings in the last release found the Finish Job copy asserting processes KAN-323 never specified. Corrections, per the office's actual rules: - Over-invoiced is a diagnostic, not advice: a credit note is only one possible resolution — the job value being out of date is another — so the warning now names the fact and both likely causes, prescribing neither. - The load-error panel no longer instructs staff not to quote the customer; the T&M "get them right before invoicing" paragraph is deleted outright (the ticket forbids the checklist area nagging staff). - Timesheets only exist to invoice on a T&M job, so a quoted job is no longer asked "Have you collected the timesheet entries?". - "Has the job been released?" becomes "Has the job been handed over?", matching the field's meaning (collection, delivery, or on-site install). KAN-323 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe job finish tab updates financial and completion guidance. Its checklist now shows the timesheet question only for Time & Materials jobs and uses “handed over” for the final item. Tests verify both checklist variants. ChangesJob finish checklist updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/components/job/JobFinishTab.vue (1)
379-383: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an explicit
elsebranch for the checklist filter.This is a non-trivial conditional. Replace the ternary with an
if/elseblock. This makes the two checklist variants easier to audit.Proposed refactor
-const checklistItems = computed(() => - props.pricingMethodology === 'time_materials' - ? allChecklistItems - : allChecklistItems.filter((item) => item.key !== 'timesheets_collected'), -) +const checklistItems = computed(() => { + if (props.pricingMethodology === 'time_materials') { + return allChecklistItems + } else { + return allChecklistItems.filter((item) => item.key !== 'timesheets_collected') + } +})As per coding guidelines, use explicit
elsebranches for non-trivial conditionals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/job/JobFinishTab.vue` around lines 379 - 383, Update the checklistItems computed property to replace the ternary conditional with an explicit if/else branch: return allChecklistItems for the time_materials pricing method and the filtered list otherwise, preserving the existing filtering behavior.Source: Coding guidelines
frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts (1)
63-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the complete checklist contract.
The tests verify that expected keys exist, but they do not verify the total number of rendered items. An extra item would pass both tests. Add count assertions for five T&M items and four fixed-price items. Also assert the new “handed over” label.
Proposed test additions
const wrapper = mountTab('time_materials') await flushPromises() + expect( + wrapper.findAll('[data-automation-id^="JobFinishTab-checklist-"]'), + ).toHaveLength(5) + expect(wrapper.text()).toContain('Has the job been handed over?') + for (const key of ITEMS) { expect(item(wrapper, key).exists(), key).toBe(true) } const wrapper = mountTab('fixed_price') await flushPromises() + expect( + wrapper.findAll('[data-automation-id^="JobFinishTab-checklist-"]'), + ).toHaveLength(4) + expect(item(wrapper, 'timesheets_collected').exists()).toBe(false)As per coding guidelines, tests must verify enduring behavior or invariants by executing code and asserting observable outcomes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts` around lines 63 - 79, Add assertions in the T&M and fixed-price cases of the checklist test to verify exactly five and four rendered items respectively, preventing unexpected extras. Also assert the expected “handed over” checklist label using the existing item lookup or rendered text assertion.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts`:
- Around line 63-79: Add assertions in the T&M and fixed-price cases of the
checklist test to verify exactly five and four rendered items respectively,
preventing unexpected extras. Also assert the expected “handed over” checklist
label using the existing item lookup or rendered text assertion.
In `@frontend/src/components/job/JobFinishTab.vue`:
- Around line 379-383: Update the checklistItems computed property to replace
the ternary conditional with an explicit if/else branch: return
allChecklistItems for the time_materials pricing method and the filtered list
otherwise, preserving the existing filtering behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 70a5f1e7-a34c-4410-84e7-d6e1fcc10ebb
📒 Files selected for processing (2)
frontend/src/components/job/JobFinishTab.vuefrontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts
True, but not something the office needs telling; the behaviour is pinned by a test, not by copy. The subtitle keeps "Recorded against the job." so staff know ticks persist. KAN-323 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
- "Self-checklist." replaces "Recorded against the job." — every field is recorded against the job, so that said nothing. - Supervisor, not foreman — the term the workshop actually uses. The foreman_signed_off column keeps its name; only the label changes. - "today's timesheet entries" — says which timesheets are meant. - The materials question now asks what the check actually is: the workshop forgetting to write materials used on the job sheet, or writing them wrongly — not a vague "checked the materials". KAN-323 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
1 participant
Why
An audit of every user-facing string introduced in the latest production release (PR #512 vs PR #500) found the Finish Job workspace copy — written by the AI agent in
61fbc61d/1013c781— asserting business processes KAN-323 never specified. Ticket-backed copy ("Recorded against the job. Never blocks invoicing.", the invoice-mode labels, the balance labels) is untouched; only the invented policy is corrected, per the office's actual rules.What changed
All in
JobFinishTab.vue(+ its checklist test):Blessed as-is after review: the five-item checklist itself (the ticket specified three), including "Have you called the customer?".
Tests
vitest run src/components/job/__tests__— 7 files, 33 tests, all passing.🤖 Generated with Claude Code
https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
Summary by CodeRabbit