fix: apply Xero sales branding theme to quotes and invoices - #464
Merged
Conversation
|
Warning Review limit reached
Next review available in: 4 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 ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Xero sales branding theme support across accounting contracts, setup and persistence, invoice and quote creation, authenticated APIs, OpenAPI schemas, frontend administration, tests, and deployment documentation. ChangesXero sales branding themes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant SectionForm
participant BrandingThemesAPI
participant XeroProvider
Admin->>SectionForm: Open Xero settings
SectionForm->>BrandingThemesAPI: GET branding themes
BrandingThemesAPI->>XeroProvider: list_document_themes()
XeroProvider-->>BrandingThemesAPI: Available DocumentTheme values
BrandingThemesAPI-->>SectionForm: Selector JSON
Admin->>SectionForm: Select and save theme ID
SectionForm-->>Admin: Persisted company setting
🚥 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 |
corrin
marked this pull request as ready for review
July 16, 2026 06:51
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
docs/restore-prod-to-nonprod.md (1)
255-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to the fenced code block.
As highlighted by static analysis tools, it is a best practice to specify a language (e.g.,
textorbash) for fenced code blocks to ensure proper rendering and avoid linter warnings.📝 Proposed change
-``` +```text🤖 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 `@docs/restore-prod-to-nonprod.md` at line 255, Update the fenced code block near the affected documentation section to include an explicit language specifier, using text or bash according to the block’s contents, while preserving the existing commands and formatting.Source: Linters/SAST tools
frontend/src/components/SectionForm.vue (2)
561-579: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hardcoding field keys in generic form components.
showBrandingThemePlaceholderandunavailableBrandingThemeIdhardcode'xero_sales_branding_theme_id'. Because the template iterates over genericfieldsand binds them tofield.key, hardcoding the specific field key here breaks the component's abstraction. If another field of type'xero_branding_theme'is added in the future, its dropdown will incorrectly use the value of'xero_sales_branding_theme_id'.Update the script to use parameterized functions instead of computed properties, and pass
field.keyfrom the template.♻️ Proposed fixes
-const showBrandingThemePlaceholder = computed( - () => - brandingThemesLoading.value || - brandingThemesUnavailable.value || - brandingThemeValue('xero_sales_branding_theme_id') === '', -) - const brandingThemePlaceholder = computed(() => { if (brandingThemesLoading.value) return 'Loading Xero branding themes…' if (brandingThemesUnavailable.value) return 'No Xero branding themes available' return 'Xero setup incomplete — select a branding theme' }) -const unavailableBrandingThemeId = computed(() => { - const configuredId = brandingThemeValue('xero_sales_branding_theme_id') - if (!configuredId) return null - const isAvailable = brandingThemes.value.some((theme) => theme.branding_theme_id === configuredId) - return isAvailable ? null : configuredId -}) +function showBrandingThemePlaceholder(fieldKey: string): boolean { + return ( + brandingThemesLoading.value || + brandingThemesUnavailable.value || + brandingThemeValue(fieldKey) === '' + ) +} + +function unavailableBrandingThemeId(fieldKey: string): string | null { + const configuredId = brandingThemeValue(fieldKey) + if (!configuredId) return null + const isAvailable = brandingThemes.value.some((theme) => theme.branding_theme_id === configuredId) + return isAvailable ? null : configuredId +}And update the template bindings accordingly:
- <option v-if="showBrandingThemePlaceholder" value="" disabled> + <option v-if="showBrandingThemePlaceholder(field.key)" value="" disabled> {{ brandingThemePlaceholder }} </option> - <option v-if="unavailableBrandingThemeId" :value="unavailableBrandingThemeId"> - Unavailable theme ({{ unavailableBrandingThemeId }}) + <option v-if="unavailableBrandingThemeId(field.key)" :value="unavailableBrandingThemeId(field.key)"> + Unavailable theme ({{ unavailableBrandingThemeId(field.key) }}) </option>🤖 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/SectionForm.vue` around lines 561 - 579, Replace the hardcoded-key computed properties showBrandingThemePlaceholder and unavailableBrandingThemeId with parameterized functions accepting a field key and reading brandingThemeValue(fieldKey). Update the template bindings to pass field.key for both placeholder visibility and unavailable-theme value/display, while preserving the existing loading, unavailable, and configured-theme behavior.
266-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
z.inferfor response types as per coding guidelines.While
Awaited<ReturnType<typeof ...>>correctly infers the type, the coding guidelines require explicitly usingz.infer<typeof schemas.X>for frontend response types derived from the schema.♻️ Proposed refactor
-const brandingThemes = ref<Awaited<ReturnType<typeof api.xero_branding_themes_list>>>([]) +const brandingThemes = ref<z.infer<typeof schemas.XeroBrandingTheme>[]>([])🤖 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/SectionForm.vue` around lines 266 - 282, The brandingThemes response type currently uses Awaited<ReturnType<typeof api.xero_branding_themes_list>>; update the brandingThemes declaration to use z.infer<typeof schemas.X> for the corresponding Xero branding themes response schema, following the frontend typing convention and preserving the existing array default.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.
Inline comments:
In `@apps/workflow/views/xero/xero_invoice_manager.py`:
- Around line 329-330: Update the generic exception handlers in
apps/workflow/views/xero/xero_invoice_manager.py:329-330 and
apps/workflow/views/xero/xero_quote_manager.py:245-246 to persist each exception
once with persist_app_error(exc), then construct AlreadyLoggedException(exc,
err.id) and re-raise it using the two-arm deduplication pattern instead of
returning an error dictionary.
In `@apps/workflow/views/xero/xero_view.py`:
- Line 1001: Update the fallback AssertionError in the persist_and_raise
exception-handling flow to chain it from the caught exception using the existing
exception variable exc. Preserve the current assertion message and behavior
while retaining the original failure as exception context.
In `@frontend/tests/company-defaults.spec.ts`:
- Around line 103-116: In the company-defaults test, move the selector enabled
assertion to after the availableThemeIds lookup and the no-theme test.skip
check. Keep the visibility and loading completion checks first, then inspect
options and skip when no themes are available before requiring the selector to
be enabled.
---
Nitpick comments:
In `@docs/restore-prod-to-nonprod.md`:
- Line 255: Update the fenced code block near the affected documentation section
to include an explicit language specifier, using text or bash according to the
block’s contents, while preserving the existing commands and formatting.
In `@frontend/src/components/SectionForm.vue`:
- Around line 561-579: Replace the hardcoded-key computed properties
showBrandingThemePlaceholder and unavailableBrandingThemeId with parameterized
functions accepting a field key and reading brandingThemeValue(fieldKey). Update
the template bindings to pass field.key for both placeholder visibility and
unavailable-theme value/display, while preserving the existing loading,
unavailable, and configured-theme behavior.
- Around line 266-282: The brandingThemes response type currently uses
Awaited<ReturnType<typeof api.xero_branding_themes_list>>; update the
brandingThemes declaration to use z.infer<typeof schemas.X> for the
corresponding Xero branding themes response schema, following the frontend
typing convention and preserving the existing array default.
🪄 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: 9c528ac7-c610-49de-8494-160b770ac76e
⛔ Files ignored due to path filters (1)
frontend/src/api/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (35)
apps/workflow/__init__.pyapps/workflow/accounting/document_theme_service.pyapps/workflow/accounting/provider.pyapps/workflow/accounting/types.pyapps/workflow/accounting/xero/provider.pyapps/workflow/fixtures/company_defaults.jsonapps/workflow/fixtures/company_defaults_prospect.jsonapps/workflow/management/commands/xero.pyapps/workflow/migrations/0011_companydefaults_xero_sales_branding_theme_id.pyapps/workflow/models/company_defaults.pyapps/workflow/models/settings_metadata.pyapps/workflow/serializers.pyapps/workflow/tests/test_company_defaults_api.pyapps/workflow/tests/test_company_defaults_schema.pyapps/workflow/tests/test_localdate_regression.pyapps/workflow/tests/test_xero_branding_themes.pyapps/workflow/tests/test_xero_setup_command.pyapps/workflow/urls.pyapps/workflow/views/xero/__init__.pyapps/workflow/views/xero/xero_base_manager.pyapps/workflow/views/xero/xero_helpers.pyapps/workflow/views/xero/xero_invoice_manager.pyapps/workflow/views/xero/xero_quote_manager.pyapps/workflow/views/xero/xero_view.pydocs/client_onboarding.mddocs/instance-setup-demo.mddocs/instance-setup-production.mddocs/restore-prod-to-nonprod.mddocs/server_setup.mddocs/urls/workflow.mdfrontend/schema.ymlfrontend/src/components/SectionForm.vuefrontend/src/components/__tests__/SectionForm.test.tsfrontend/tests/company-defaults.spec.tsmypy-baseline.txt
💤 Files with no reviewable changes (1)
- mypy-baseline.txt
Address CodeRabbit review on #464. The managers are service objects, not the HTTP boundary: the view is, and it already converts AlreadyLoggedException into a 500 carrying error_id. Their generic arms persisted then returned an error dict, so that path was unreachable. Re-raise instead (ADR 0001), which also fixes real bugs: - Unexpected server errors were reported to the frontend as HTTP 400: the manager set "status": 500 but the view ignores it and hardcodes 400. - The dict path dropped error_id, which ADR 0013 requires. Expected business failures still return success: False dicts -> 4xx. Only unexpected exceptions raise. Also fixes, found while verifying: - delete_document on both managers had no AlreadyLoggedException arm at all, so its bare except re-persisted an already-persisted failure. That is the double-persistence ADR 0001 exists to prevent. CodeRabbit missed this. - xero_quote_manager.delete_document returned "messages" as a list of dicts, but XeroDocumentSuccessResponseSerializer declares ListField(CharField()); DRF rejects a dict, so that path raised on serialization. Fixed the producer to match the contract (ADR 0015). - Four tests never configured xero_sales_branding_theme_id and so failed against the mandatory theme guard this PR added. - xero_view: chain the fallback AssertionError with `from exc` (Ruff B904). ADR 0001 gains an explicit chain-termination clause: the silence about where the chain ends is what made this review ambiguous. Views now pass message= to _build_xero_error_payload so the underlying exception text survives the move to the raise path (ADR 0013). Typing: create_document/delete_document now return a named XeroDocumentResponse TypedDict rather than being unannotated (ADR 0028). It is what surfaced the messages contract bug. mypy-baseline shrinks by 8. Frontend: the branding-theme E2E asserted the selector was enabled before the no-themes skip could run, but an empty theme list is exactly when the control stays disabled by design, so the test timed out instead of skipping. Wait for the load to settle, fail on the error marker, skip on the empty marker, then assert enabled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/test_xero_document_error_handling.py`:
- Around line 29-34: Add self.addCleanup(CompanyDefaults.clear_cache) in setUp
immediately after the existing CompanyDefaults.clear_cache() call, ensuring the
singleton cache is cleared when the test completes while preserving the current
setup behavior.
🪄 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: 54c6f162-393b-4f62-874b-cd57354e34f4
📒 Files selected for processing (12)
apps/workflow/tests/test_xero_branding_themes.pyapps/workflow/tests/test_xero_document_error_handling.pyapps/workflow/tests/test_xero_document_raw_json.pyapps/workflow/tests/test_xero_readonly_provider.pyapps/workflow/views/xero/__init__.pyapps/workflow/views/xero/xero_base_manager.pyapps/workflow/views/xero/xero_invoice_manager.pyapps/workflow/views/xero/xero_quote_manager.pyapps/workflow/views/xero/xero_view.pydocs/adr/0001-exception-already-logged-dedup.mdfrontend/tests/company-defaults.spec.tsmypy-baseline.txt
💤 Files with no reviewable changes (1)
- mypy-baseline.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/workflow/views/xero/xero_base_manager.py
- frontend/tests/company-defaults.spec.ts
- apps/workflow/views/xero/init.py
- apps/workflow/tests/test_xero_branding_themes.py
Comment on lines
+29
to
+34
| def setUp(self) -> None: | ||
| defaults = CompanyDefaults.get_solo() | ||
| CompanyDefaults.objects.filter(pk=defaults.pk).update( | ||
| xero_sales_branding_theme_id=uuid.UUID(THEME_ID) | ||
| ) | ||
| CompanyDefaults.clear_cache() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Register cache cleanup to prevent test pollution.
Since CompanyDefaults is a singleton, modifying and clearing its cache without registering a cleanup can cause the modified cached instance to leak into subsequent tests, leading to test flakiness. Add self.addCleanup(CompanyDefaults.clear_cache) to ensure test isolation, identical to the pattern used in the other test files.
♻️ Proposed fix
def setUp(self) -> None:
defaults = CompanyDefaults.get_solo()
CompanyDefaults.objects.filter(pk=defaults.pk).update(
xero_sales_branding_theme_id=uuid.UUID(THEME_ID)
)
CompanyDefaults.clear_cache()
+ self.addCleanup(CompanyDefaults.clear_cache)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def setUp(self) -> None: | |
| defaults = CompanyDefaults.get_solo() | |
| CompanyDefaults.objects.filter(pk=defaults.pk).update( | |
| xero_sales_branding_theme_id=uuid.UUID(THEME_ID) | |
| ) | |
| CompanyDefaults.clear_cache() | |
| def setUp(self) -> None: | |
| defaults = CompanyDefaults.get_solo() | |
| CompanyDefaults.objects.filter(pk=defaults.pk).update( | |
| xero_sales_branding_theme_id=uuid.UUID(THEME_ID) | |
| ) | |
| CompanyDefaults.clear_cache() | |
| self.addCleanup(CompanyDefaults.clear_cache) |
🤖 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/workflow/tests/test_xero_document_error_handling.py` around lines 29 -
34, Add self.addCleanup(CompanyDefaults.clear_cache) in setUp immediately after
the existing CompanyDefaults.clear_cache() call, ensuring the singleton cache is
cleared when the test completes while preserving the current setup behavior.
SOLO_CACHE is None under tests (settings.py:892, settings_test.py:25), so django-solo's get_solo() reads straight through to the DB and clear_cache() short-circuits on `if cache_name:`. There is no cache to leak between tests, so registering clear_cache as a cleanup guards nothing and implies a hazard that cannot occur. Matches the existing convention in test_xero_branding_themes.py and test_xero_quota_floor.py: clear_cache() in setUp, no cleanup registered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
15 tasks
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.
Summary
Fixes KAN-286 by applying one configured Xero sales branding theme to every quote and sales invoice created by DocketWorks.
Root cause
DocketWorks omitted
BrandingThemeIDfrom quote and invoice payloads, so Xero did not consistently use the terms-bearing template selected for direct Xero documents.Changes
BrandingThemeIDon quote and invoice creation.SortOrdertheme.workflow.0011, using a narrow update of the singleton branding-theme field.xero --setuponboarding flow.Rollout
Existing connected installations need no command. During deployment, migration
workflow.0011detects the tenant plus active durable OAuth credentials, reads the tenant's ordered branding themes, and stores the first theme before services restart. If the Xero request fails or returns no themes, the migration fails transactionally and deployment must be retried rather than starting with incomplete configuration.Fresh installs and credential-scrubbed restores are deliberately skipped by the migration because they cannot make an authenticated tenant request yet. Their mandatory onboarding command fills the same field:
For every installation, confirm the selected theme in Admin > Settings contains the required terms. Verify both a DocketWorks-created quote PDF and invoice PDF in Xero before considering rollout complete.
Validation
new: 0), 57 frontend test files / 331 tests, ESLint, Vue type-check, typed-router checks, production build, and workflow formatting.Deferred
Controlled repair of existing draft documents remains a separate, auditable nice-to-have operation because it would require external Xero writes.
Summary by CodeRabbit