KAN-294: ops docs → Drive/NotebookLM (+ staff offboarding fix, gemini-flash-latest) - #489
Merged
Conversation
KAN-297 relocated the "Main line" PhoneEndpoint out of the company_defaults fixture (to satisfy the instance.sh config validator) into initial_data.json. The PDF golden tests load only company_defaults, so the test DB lost the endpoint and the delivery-docket letterhead rendered without the shop phone (+6496365131) — 11 fewer bytes than the committed golden, failing test_delivery_docket_matches_golden in CI. The golden is correct; the test's fixture set had drifted. build_golden_job documents itself as the single source of truth for every field that influences PDF output, so create the MAIN_LINE endpoint there rather than leaning on an external seed fixture. Restores byte-identity with no golden regeneration and decouples the golden tests from seed-fixture churn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D3hi37oW5tWkjGjtZV1Jbz
Deleting a staff member was a hard delete the DB blocks (CostLine.staff is on_delete=PROTECT) for anyone who has logged time, so it failed for every real employee -- and the staff form exposed no way to set date_left, the actual offboarding mechanism. There was no working way to offboard someone. - StaffFormModal: add a Date Left field (always sent, so clearing it reinstates an employee) - AdminStaffView/useStaffApi: remove the Delete button and its accounts_staff_destroy caller - staff_api: StaffRetrieveUpdateDestroyAPIView -> StaffRetrieveUpdateAPIView (retrieve + update only); regenerate schema.yml and api.ts - test: DELETE on a staff member returns 405 and the record survives Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The manual content now lives in the Operations Manual on Google Drive, surfaced to staff via a NotebookLM notebook. Remove the in-repo VitePress manual and its build/serve wiring, and repoint the navbar at the notebook. - delete frontend/manual/ (VitePress site) and dist-manual/ - drop manual:dev/build/preview npm scripts (keep manual:screenshots, the screenshot tool, to be repointed at the Docs separately) - remove the /manual vite dev proxy and the nginx /manual/ location - drop `npm run manual:build` from the release build - remove the "Frontend Manual Dev Server" VS Code task - repoint the "App Training" navbar links at the NotebookLM notebook (relabelled "Training Manual") - prune dead references (frontend README manual section, eslint globs) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
capture-screenshots.ts navigates to APP_DOMAIN, which in dev is an ngrok-free tunnel. ngrok's browser-warning interstitial hid the login page, so Playwright timed out waiting for #username. Send the ngrok-skip-browser-warning header on the browser context (harmless on non-ngrok hosts) so capture reaches the real app. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…h-latest Two settled bodies of work, committed together per the all-or-nothing rule. KAN-294 ops-manual tooling: - explore_google_drive.py: browse the MSM Shared Drive via delegated auth (GCP_CREDENTIALS + CompanyDefaults.company_email, GCP_DELEGATED_SUBJECT override); the old raw-SA version only ever saw an empty My Drive. - set_doc_screenshot.py: salvaged from throwaway migration scratch into a tracked tool — the push half of the screenshot pipeline (capture half is frontend/scripts/capture-screenshots.ts). - CLAUDE.md: docs/plans is ephemeral scratch — delete a plan at PR-open, migrate anything durable first, never leave a non-plan artifact there. Gemini model migration: - Migrate AIProvider rows off deprecated pinned model names onto gemini-flash-latest (migration 0012 + test), update the model help_text, providers, fixtures, templates, docs, and frontend examples. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DDssHZ17GwD2wtPyac7tEm
|
Warning Review limit reached
Next review available in: 56 seconds 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 (8)
📝 WalkthroughWalkthroughThe PR disables staff deletion, standardizes Gemini model configuration, removes the local VitePress manual, adds NotebookLM link management and Google Drive/Docs tooling, and updates deployment, fixture, documentation, and workflow guidance. ChangesStaff lifecycle
Gemini model standardization
Training manual removal
NotebookLM link management
Google Drive and Docs tooling
Supporting repository updates
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/accounts/tests/test_staff_api.py (1)
55-80: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a companion offboarding test.
This test class only exercises the DELETE-rejection path. Given the PR's stated goal of enabling offboarding via
date_left, a test thatPATCHesdate_lefton the detail endpoint and asserts it persists would directly cover the new intended workflow this endpoint change enables.🤖 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 `@apps/accounts/tests/test_staff_api.py` around lines 55 - 80, Add a companion test to StaffDetailAPIViewTests that authenticates an office staff user, PATCHes the target staff member’s date_left through the detail endpoint, and asserts a successful response plus the persisted date_left value. Reuse the existing Staff and APIClient setup from test_staff_cannot_be_deleted_via_api to cover the intended offboarding workflow.
🤖 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.
Inline comments:
In `@apps/job/tests/_pdf_golden_fixtures.py`:
- Around line 77-86: The golden fixture builder must make the rendered main-line
endpoint deterministic despite pre-existing MAIN_LINE fixtures. Update
build_golden_job’s setup to deactivate or otherwise exclude existing MAIN_LINE
rows and ensure the builder-created endpoint is active with number +6496365131,
or explicitly pin _primary_company_endpoint_number to that unique endpoint;
preserve the expected golden PDF output.
In `@apps/quoting/services/ai_price_extraction.py`:
- Line 22: Update MistralPriceExtractionProvider and the create_provider() path
so every returned provider satisfies the PriceExtractionProvider.model_name
contract. Store an appropriate Mistral model name on the Mistral instance, or
consistently narrow the shared contract to exclude provider-scoped model_name;
preserve the existing provider selection behavior.
In `@CLAUDE.md`:
- Around line 127-129: Reconcile the `docs/plans/` guidance in `CLAUDE.md` with
the root `.gitignore`: preserve the `_template.md` exception by adding an
explicit `!docs/plans/_template.md` negation after the `docs/plans/` ignore
rule, or remove the exception from the documentation.
In `@scripts/explore_google_drive.py`:
- Around line 41-44: Remove the unused Google Docs URL from the SCOPES list,
leaving only the Drive scope required by the Drive client construction in this
script.
In `@scripts/set_doc_screenshot.py`:
- Around line 91-104: The upload_png function leaves the publicly shared
temporary Drive image undeleted. Ensure the temporary file identified by fid is
deleted with drive.files().delete(...).execute() after the Docs image insertion
completes, while preserving the existing sharing setup needed for that
insertion.
---
Nitpick comments:
In `@apps/accounts/tests/test_staff_api.py`:
- Around line 55-80: Add a companion test to StaffDetailAPIViewTests that
authenticates an office staff user, PATCHes the target staff member’s date_left
through the detail endpoint, and asserts a successful response plus the
persisted date_left value. Reuse the existing Staff and APIClient setup from
test_staff_cannot_be_deleted_via_api to cover the intended offboarding workflow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc1aa5be-7c62-4522-a793-c5673ff2d679
⛔ Files ignored due to path filters (1)
frontend/src/api/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (51)
.vscode/tasks.jsonCLAUDE.mdapps/accounts/tests/test_staff_api.pyapps/accounts/urls.pyapps/accounts/views/__init__.pyapps/accounts/views/staff_api.pyapps/job/tests/_pdf_golden_fixtures.pyapps/quoting/services/ai_price_extraction.pyapps/quoting/services/providers/gemini_provider.pyapps/quoting/tests/test_ai_price_extraction.pyapps/workflow/fixtures/ai_providers.json.exampleapps/workflow/migrations/0012_use_latest_gemini_flash_model.pyapps/workflow/models/ai_provider.pyapps/workflow/tests/test_latest_gemini_model_migration.pydocs/client_onboarding.mddocs/urls/accounts.mdfrontend/README.mdfrontend/eslint.config.tsfrontend/manual/.vitepress/config.tsfrontend/manual/admin/manage-staff.mdfrontend/manual/admin/run-payroll.mdfrontend/manual/end-of-week/weekly-checklist.mdfrontend/manual/enquiries/new-customer-call.mdfrontend/manual/fieldwork/complete-a-job.mdfrontend/manual/index.mdfrontend/manual/invoicing/invoice-a-job.mdfrontend/manual/jobs/attach-files.mdfrontend/manual/jobs/understanding-job-finances.mdfrontend/manual/management/run-reports.mdfrontend/manual/purchasing/create-purchase-order.mdfrontend/manual/quoting/assess-and-price.mdfrontend/manual/quoting/send-quote.mdfrontend/manual/scheduling/schedule-a-job.mdfrontend/manual/timesheets/end-of-day-entry.mdfrontend/package.jsonfrontend/schema.ymlfrontend/scripts/capture-screenshots.tsfrontend/src/components/AppNavbar.vuefrontend/src/components/StaffFormModal.vuefrontend/src/components/admin/AIProviderFormModal.vuefrontend/src/components/chat/README.mdfrontend/src/composables/useStaffApi.tsfrontend/src/views/AdminStaffView.vuefrontend/vite.config.tsmypy-baseline.txtscripts/README.mdscripts/explore_google_drive.pyscripts/server/release-utils.shscripts/server/templates/ai-providers.json.templatescripts/server/templates/nginx-instance.conf.templatescripts/set_doc_screenshot.py
💤 Files with no reviewable changes (25)
- frontend/manual/index.md
- frontend/manual/jobs/attach-files.md
- frontend/manual/fieldwork/complete-a-job.md
- frontend/manual/enquiries/new-customer-call.md
- frontend/manual/quoting/assess-and-price.md
- frontend/manual/.vitepress/config.ts
- frontend/manual/management/run-reports.md
- frontend/manual/quoting/send-quote.md
- frontend/manual/end-of-week/weekly-checklist.md
- frontend/manual/purchasing/create-purchase-order.md
- frontend/manual/jobs/understanding-job-finances.md
- frontend/manual/scheduling/schedule-a-job.md
- frontend/manual/invoicing/invoice-a-job.md
- frontend/README.md
- frontend/eslint.config.ts
- frontend/manual/admin/run-payroll.md
- frontend/manual/timesheets/end-of-day-entry.md
- frontend/vite.config.ts
- scripts/server/release-utils.sh
- frontend/src/composables/useStaffApi.ts
- frontend/manual/admin/manage-staff.md
- frontend/package.json
- scripts/server/templates/nginx-instance.conf.template
- .vscode/tasks.json
- mypy-baseline.txt
The Training Manual navbar link hardcoded MSM's NotebookLM URL in the shared frontend, so every client (second customer onboarding next week) would ship pointing at MSM's notebook. Replace it with a per-instance managed table. Backend: - NotebookLmLink model (name / url / enabled / restriction / order) + migration 0013; restriction is a NotebookLmRestriction choice (all staff vs superusers only). - NotebookLmLinkViewSet: office-staff-gated CRUD for the admin UI, plus an all-staff 'menu' action that server-side-filters to the enabled links the caller may see (excludes superuser-restricted rows for non-superusers). - Tests: menu filtering by restriction/enabled/auth; CRUD permission gating. Frontend: - notebookLmLinks store loads the menu at startup; the navbar renders the links under a new Resources dropdown reorganised into Chatbots / Forms / Procedures. - AdminNotebookLmLinksView + NotebookLmLinkFormModal + service: full CRUD admin page (auto-routed under the superuser-gated /admin section). Table ships empty (no data-migration seed) so no client inherits another's URL; each client's superuser adds their rows via /admin/notebooklm-links. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DDssHZ17GwD2wtPyac7tEm
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In `@apps/workflow/__init__.py`:
- Line 4: Regenerate apps/workflow/__init__.py using python
scripts/update_init.py after adding the NotebookLM module, rather than manually
editing its exports. Commit the generated changes, including all affected export
entries such as AIProviderTypes and NotebookLmRestriction.
In `@frontend/src/components/AppNavbar.vue`:
- Around line 145-148: Update the Resources dropdown visibility around the
NotebookLM menu to allow all authenticated staff when NotebookLmRestriction.NONE
applies, rather than gating it with userInfo.is_office_staff. Keep Forms and
Procedures navigation restricted to office staff, and separate their visibility
conditions from the unrestricted NotebookLM link.
In `@frontend/src/stores/notebookLmLinks.ts`:
- Around line 1-28: Update loadLinks in useNotebookLmLinksStore to call a
menu-list method on notebookLmLinkService instead of invoking
api.workflow_notebook_lm_links_menu_list directly. Add or expose the
corresponding service method that delegates to the API, then use it in the store
while preserving the existing loading, error, and state-management behavior.
In `@frontend/src/views/AdminNotebookLmLinksView.vue`:
- Around line 181-185: Update the catch blocks in the save-link error handling
to narrow caught values with an instanceof Error check before reading message,
replacing the prohibited error as Error cast. Preserve the existing fallback
message for non-Error throws and apply the same change to both affected
handlers.
- Around line 163-180: Reload the shared useNotebookLmLinksStore menu after
successful mutations so navbar links stay current: in handleSave, refresh the
store after create or update and before closing or refreshing the local list; in
the deletion handler at frontend/src/views/AdminNotebookLmLinksView.vue lines
194-199, perform the same store reload after successful deletion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d577129-3ce0-446d-bdcb-83a0e12d91d2
⛔ Files ignored due to path filters (1)
frontend/src/api/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (18)
apps/workflow/__init__.pyapps/workflow/enums.pyapps/workflow/migrations/0013_notebooklmlink.pyapps/workflow/models/__init__.pyapps/workflow/models/notebook_lm_link.pyapps/workflow/serializers.pyapps/workflow/tests/test_notebook_lm_link_api.pyapps/workflow/urls.pyapps/workflow/views/notebook_lm_link_viewset.pyfrontend/schema.ymlfrontend/src/App.vuefrontend/src/components/AppNavbar.vuefrontend/src/components/__tests__/AppNavbar.test.tsfrontend/src/components/admin/NotebookLmLinkFormModal.vuefrontend/src/config/adminPages.tsfrontend/src/services/notebookLmLinkService.tsfrontend/src/stores/notebookLmLinks.tsfrontend/src/views/AdminNotebookLmLinksView.vue
Rebuild the manual-maintenance capability deleted with the KAN-294 migration scratch (only the screenshot pusher had been salvaged): - read_google_doc.py: export a Google Doc as Markdown. - write_google_doc.py: import Markdown as a Doc with a revisionId safety net that refuses to overwrite a human-edited doc (seed/import/trash/status); per-instance manifest is gitignored runtime state. Both use the app auth convention (GCP_CREDENTIALS + CompanyDefaults.company_email, GCP_DELEGATED_SUBJECT override), matching explore_google_drive.py. Supports the Operations Manual cleanup (KAN-302). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DDssHZ17GwD2wtPyac7tEm
Triaged all ten CodeRabbit comments; seven were valid. Resources menu is no longer office-staff gated. The `menu` endpoint serves any authenticated staff member and NotebookLmRestriction.NONE means "all staff", but the navbar hid the whole dropdown behind is_office_staff, so shop-floor staff could never reach an unrestricted link. The process read endpoints are already IsAuthenticated and the routes are requiresAuth only, so procedures and forms open up with it — that is the point of the ops manual. Covered by a navbar test that fails if the gate returns. NotebookLM frontend: the store called the generated client directly, so menu loading bypassed the service layer; and the navbar's shared store was loaded once at startup and never refreshed, leaving admin creates, edits and deletes invisible until a page reload. Price extraction: neither provider actually inherited PriceExtractionProvider, so the factory's return type was wrong for both and Mistral had no model_name at all. Both now inherit the ABC, Mistral's hardcoded "mistral-ocr-latest" becomes a constant the factory can override like Gemini's, and the two resolved violations are dropped from the mypy baseline. Scripts: explore_google_drive.py no longer requests the unused `documents` scope, and set_doc_screenshot.py deletes the temp upload rather than trashing it — trashing left the "anyone/reader" grant live. .gitignore now matches docs/plans/* so the !_template.md negation is reachable; git never descends into an excluded directory. Rejected two: the PDF golden fixture is already deterministic (BaseTestCase loads no PhoneEndpoint rows and normalized_number is unique), and apps/workflow/__init__.py was generated — re-running update_init.py leaves it unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wHxbSZWq7V7jMkBi3Lq1N
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
apps/quoting/tests/test_ai_price_extraction.py (1)
47-58: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact model selected by the factory.
assertTrue(provider.model_name)would pass for any non-empty model and will not catch a regression in the Mistral default wiring. AssertGEMINI_FLASH_MODELandMISTRAL_OCR_MODELexplicitly for their respective factory branches.Proposed test shape
+ expected_models = { + AIProviderTypes.GOOGLE: GEMINI_FLASH_MODEL, + AIProviderTypes.MISTRAL: MISTRAL_OCR_MODEL, + } for provider_type in (AIProviderTypes.GOOGLE, AIProviderTypes.MISTRAL): with self.subTest(provider_type=provider_type): provider = PriceExtractionFactory.create_provider( provider_type, "test-api-key", "", ) self.assertIsInstance(provider, PriceExtractionProvider) self.assertTrue(provider.provider_name) - self.assertTrue(provider.model_name) + self.assertEqual( + provider.model_name, + expected_models[provider_type], + )🤖 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 `@apps/quoting/tests/test_ai_price_extraction.py` around lines 47 - 58, Update test_factory_returns_a_provider_with_a_model_name to assert the exact model_name returned by PriceExtractionFactory for each provider_type: GEMINI_FLASH_MODEL for GOOGLE and MISTRAL_OCR_MODEL for MISTRAL. Replace the generic truthiness assertion while retaining the existing provider type and provider_name checks.apps/quoting/services/providers/mistral_provider.py (1)
313-313: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a boundary test for the Mistral OCR model override.
The existing Mistral OCR tests only check
provider.model_name; mockclient.ocr.processand assert that the configuredmodel=self.model_namevalue is forwarded to the OCR call so a regression to a hardcoded model would fail.🤖 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 `@apps/quoting/services/providers/mistral_provider.py` at line 313, Extend the Mistral OCR tests around the OCR invocation to mock client.ocr.process and assert it receives the configured model_name value as model. Keep the assertion focused on the forwarded argument so a hardcoded model override is detected, while preserving the existing provider.model_name checks.
🤖 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.
Inline comments:
In `@apps/quoting/services/providers/base.py`:
- Around line 1-2: Update the shared extraction result contract used by
extract_price_data() to replace Optional[Dict[str, Any]] with a named,
explicitly typed result type or TypedDict, and remove the Any import. Apply the
same named type consistently across the base interface and all concrete provider
overrides while preserving the existing tuple and optional-result semantics.
- Around line 11-24: Update the abstract method extract_price_data in the base
provider class so its body is explicitly non-returning after the docstring,
using raise NotImplementedError or an abstract ellipsis, while preserving its
current signature and documentation.
In `@scripts/read_google_doc.py`:
- Around line 26-29: Update the SCOPES configuration used by read_doc() to
request only the read-only Drive scope, replacing the broader Drive scope and
removing the unnecessary Docs API scope. Preserve the existing document export
behavior.
- Around line 32-56: Make the Google utilities strict-MyPy clean by adding typed
imports, a manifest TypedDict, parameterized containers, and explicit types for
Drive/Docs API and JSON responses. In scripts/read_google_doc.py lines 32-56,
update build_drive and read_doc plus related response handling; apply the
corresponding typing changes throughout scripts/write_google_doc.py lines
58-238, including its manifest and document response paths. Rerun bash
scripts/check_mypy.sh and resolve all reported typing errors.
In `@scripts/write_google_doc.py`:
- Around line 210-214: Update scripts/write_google_doc.py lines 210-214 in
status() so only the expected missing/trashed-document condition maps to
MISSING/TRASHED; let permission, network, and other API failures follow the
required AlreadyLoggedException/persist_app_error deduplicated persistence flow
before re-raising. Update lines 220-230 in main() so OverwriteRefused is
represented as a non-exception result, or is persisted exactly once through that
same two-arm flow before re-raising, preventing operational failures from being
silently reported as success.
- Around line 104-118: Update find_in_folder to escape both the CLI-controlled
title and folder_id as Google Drive query string literals before interpolating
them into q, including single quotes and backslashes. Use the escaped values
consistently in the name and parents predicates while preserving the existing
search filters and return behavior.
- Around line 138-171: Update do_import at scripts/write_google_doc.py lines
138-171 and 176-184 to make the trash/create workflow revision-safe: retain the
revision validated by check_unedited, revalidate or otherwise guard the remote
mutation against intervening edits, and persist a durable pending operation
before mutation with recovery or compensation if any Drive or save step fails.
Ensure the committed manifest state is saved only after the operation completes,
while pending state allows recovery so the newly created document cannot remain
unmanaged when save fails.
---
Nitpick comments:
In `@apps/quoting/services/providers/mistral_provider.py`:
- Line 313: Extend the Mistral OCR tests around the OCR invocation to mock
client.ocr.process and assert it receives the configured model_name value as
model. Keep the assertion focused on the forwarded argument so a hardcoded model
override is detected, while preserving the existing provider.model_name checks.
In `@apps/quoting/tests/test_ai_price_extraction.py`:
- Around line 47-58: Update test_factory_returns_a_provider_with_a_model_name to
assert the exact model_name returned by PriceExtractionFactory for each
provider_type: GEMINI_FLASH_MODEL for GOOGLE and MISTRAL_OCR_MODEL for MISTRAL.
Replace the generic truthiness assertion while retaining the existing provider
type and provider_name checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 021a7440-f543-45fe-af20-76cef898c91a
📒 Files selected for processing (18)
.gitignoreapps/quoting/services/ai_price_extraction.pyapps/quoting/services/providers/base.pyapps/quoting/services/providers/gemini_provider.pyapps/quoting/services/providers/mistral_provider.pyapps/quoting/tests/test_ai_price_extraction.pyfrontend/src/components/AppNavbar.vuefrontend/src/components/__tests__/AppNavbar.test.tsfrontend/src/services/notebookLmLinkService.tsfrontend/src/stores/__tests__/notebookLmLinks.test.tsfrontend/src/stores/notebookLmLinks.tsfrontend/src/views/AdminNotebookLmLinksView.vuemypy-baseline.txtscripts/README.mdscripts/explore_google_drive.pyscripts/read_google_doc.pyscripts/set_doc_screenshot.pyscripts/write_google_doc.py
💤 Files with no reviewable changes (1)
- mypy-baseline.txt
🚧 Files skipped from review as they are similar to previous changes (6)
- scripts/README.md
- frontend/src/stores/notebookLmLinks.ts
- frontend/src/views/AdminNotebookLmLinksView.vue
- frontend/src/services/notebookLmLinkService.ts
- scripts/set_doc_screenshot.py
- scripts/explore_google_drive.py
find_in_folder() interpolated the title and folder id straight into a Drive query, so an ordinary title like "Driver's Handbook" terminated the string literal early and made the whole query invalid. Escape both through a q_literal() helper. status() reported every failure as MISSING/TRASHED, so a 403 or a network blip looked like a deleted document. Only a 404 is a real "it isn't there"; anything else now propagates rather than being reported as a state we did not actually observe. read_google_doc.py no longer requests the `documents` scope — it exports through Drive and never touches a Docs API resource. Also parameterised the manifest containers with a ManifestEntry TypedDict instead of bare dict/list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wHxbSZWq7V7jMkBi3Lq1N
The scope, screenshot-cleanup and gitignore comments were narrating the edit that introduced them rather than explaining the code a later reader will find. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wHxbSZWq7V7jMkBi3Lq1N
MistralPriceExtractionProvider emitted `supplier_item_code` and a dict
`dimensions`. PDFDataValidationService._sanitize_single_product reads
`item_no` and runs `dimensions` through _clean_text, which stringifies
whatever it gets. So on the Mistral path every imported product lost its
supplier item code, and the dimensions column received a Python repr:
item_no : ''
dimensions : "{'width': '1200', 'length': '2400', 'thickness': '1.2mm', ...}"
Mistral is priority 1 in get_prioritized_active_providers and every
instance template provisions a Mistral row, so this affects any client
using it. Gemini was unaffected — it already emits the importer's names.
The provider is the producer, so the fix goes there rather than teaching
the sanitiser a second field name (ADR 0015). `dimensions` is now rendered
to the display string the importer stores, and `price_unit` is stated
explicitly at the value the sanitiser already defaulted it to.
The existing OCR test asserted the provider's own dict, which is why this
survived: rename the field on both sides and it stays green. Added a test
that crosses into the sanitiser, where the contract actually lives.
Annotating PDFDataValidationService.__init__ to call it from a typed test
resolved 4 baselined violations.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wHxbSZWq7V7jMkBi3Lq1N
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bundles this weekend's work on branch
feat/KAN-294-ops-manual-notebooklm(one PR, per the weekend-PR model). Five commits, net −1000 lines (mostly the removed VitePress manual).KAN-294 — Operations docs → Google Drive + NotebookLM
The app stops being a documentation silo. The in-repo VitePress manual is retired and its content migrated into MSM's
Operations Manualin Drive (01–03 SOPs), retrieved via a NotebookLM notebook.frontend/manual/; remove the nginx/manual/block, the Vite dev proxy,manual:build, and the manual VS Code task.scripts/:explore_google_drive.py— browse the MSM Shared Drive (the old raw-service-account version only ever saw an empty My Drive).set_doc_screenshot.py— push a captured PNG into a Doc at its{{screenshot:id}}marker (push half of the screenshot pipeline; capture half isfrontend/scripts/capture-screenshots.ts, now fixed to work behind the ngrok tunnel).GCP_CREDENTIALS+CompanyDefaults.company_email(app convention), with aGCP_DELEGATED_SUBJECToverride — no machine-specific literals.Remaining KAN-294 scope (notebooks creation, Estimator-fork resolution, screenshot-marker expansion) is tracked on the ticket.
Staff offboarding bug fix
Offboarding an employee had no working path. Offboard via
Staff.date_left(now surfaced in the UI); the broken hard-delete endpoint is removed — delete was blocked byon_delete=PROTECTon real staff anyway.Gemini model migration
Migrate
AIProviderrows off deprecated pinned model names ontogemini-flash-latest(data migration0012+ test); update the model help_text, providers, fixtures, templates, docs, and frontend examples.Repo hygiene
docs/plans/is ephemeral scratch — delete a plan at PR-open, migrate anything durable first (Jira /scripts// ADR), never leave a non-plan artifact there.mypy-baseline.txtshrunk by 3 (resolved violations, per the ratchet).🤖 Generated with Claude Code
https://claude.ai/code/session_01DDssHZ17GwD2wtPyac7tEm
Summary by CodeRabbit