Conversation
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| error: | ||
| 'Failed to update profile: ' + (err instanceof Error ? err.message : 'Unknown error'), | ||
| data: { name, phone } | ||
| data: { phone } |
There was a problem hiding this comment.
'object short notation' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
| return fail(400, { | ||
| errors, | ||
| data: { name, phone } | ||
| data: { phone } |
There was a problem hiding this comment.
'object short notation' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
| error: | ||
| 'Failed to update profile: ' + (err instanceof Error ? err.message : 'Unknown error'), | ||
| data: { name, phone } | ||
| data: { phone } |
There was a problem hiding this comment.
'object short notation' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
| return fail(400, { | ||
| errors, | ||
| data: { name, phone } | ||
| data: { phone } |
There was a problem hiding this comment.
'object short notation' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
| return value.flatMap((item) => collectErrorMessages(item)); | ||
| } | ||
| if (typeof value === 'object') { | ||
| return Object.values(value).flatMap((item) => collectErrorMessages(item)); |
There was a problem hiding this comment.
'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
| if (!value) return []; | ||
| if (typeof value === 'string') return [value]; | ||
| if (Array.isArray(value)) { | ||
| return value.flatMap((item) => collectErrorMessages(item)); |
There was a problem hiding this comment.
'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
| error: | ||
| 'Failed to update profile: ' + (err instanceof Error ? err.message : 'Unknown error'), | ||
| data: { name, phone } | ||
| data: { phone } |
There was a problem hiding this comment.
'object short notation' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
| return fail(400, { | ||
| errors, | ||
| data: { name, phone } | ||
| data: { phone } |
There was a problem hiding this comment.
'object short notation' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
| return value.flatMap((item) => collectErrorMessages(item)); | ||
| } | ||
| if (typeof value === 'object') { | ||
| return Object.values(value).flatMap((item) => collectErrorMessages(item)); |
There was a problem hiding this comment.
'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
| if (!value) return []; | ||
| if (typeof value === 'string') return [value]; | ||
| if (Array.isArray(value)) { | ||
| return value.flatMap((item) => collectErrorMessages(item)); |
There was a problem hiding this comment.
'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
| Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe changes introduce nested line item support for invoice creation (estimates and recurring invoices) across backend and frontend, relax phone field requirements in user registration, make user names read-only in profiles, simplify user creation payloads, and modernize form action handling in login and user management flows. Changes
Sequence Diagram(s)sequenceDiagram actor User as Frontend User participant Form as Estimate/Invoice Form participant API as Backend API participant Serializer as Serializer (create/update) participant Model as Model/Database User->>Form: Fill estimate/invoice + line items Form->>Form: Serialize lineItems to JSON Form->>API: POST/PUT with line_items array API->>Serializer: validate(attrs) alt Validation fails Serializer-->>API: ValidationError API-->>Form: 400 response Form-->>User: Display validation errors else Validation passes Serializer->>Serializer: Extract nested line_items Serializer->>Model: Create/update main object (estimate/invoice) Serializer->>Model: Create/update LineItem instances Model-->>Serializer: Persist all records Serializer->>Serializer: _recalculate_totals() Note over Serializer: Compute subtotal, tax, total from line items Serializer->>Model: Save updated totals Model-->>Serializer: Success Serializer-->>API: Return serialized object API-->>Form: 201/200 response with nested line_items Form-->>User: Show success, display created estimate/invoice end Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
There was a problem hiding this comment.
Pull request overview
This PR primarily expands invoice estimate/recurring support (line items + additional fields), improves a few SvelteKit pages’ robustness around $props() defaults and keyed rendering, and updates both backend serializers/tests and frontend dependencies to match the new payload shapes.
Changes:
- Add line-item create/update support for Estimates and Recurring Invoices across frontend drawers, SvelteKit actions, DRF serializers, and backend tests.
- Adjust login magic-link flow to use the default form action and surface client-side errors; make org/user pages more resilient (defaults, keyed
{#each}). - Update user creation to allow minimal payload (email + role) and bump frontend dependencies.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/routes/(no-layout)/org/+page.svelte | Default $props() data + keyed org list rendering |
| frontend/src/routes/(no-layout)/login/+page.svelte | Magic-link UX tweaks + error display |
| frontend/src/routes/(no-layout)/login/+page.server.js | Switch magic-link action to default |
| frontend/src/routes/(app)/users/+page.svelte | Toast handling for add_user action result |
| frontend/src/routes/(app)/users/+page.server.js | Improve API error message extraction; simplify user payload |
| frontend/src/routes/(app)/profile/+page.svelte | Make name read-only / remove name editing |
| frontend/src/routes/(app)/profile/+page.server.js | Remove name validation + payload fields |
| frontend/src/routes/(app)/invoices/recurring/+page.svelte | Add line-item UI and hidden form fields for recurring invoices |
| frontend/src/routes/(app)/invoices/recurring/+page.server.js | Send *_id relationship keys; parse lineItems JSON into line_items |
| frontend/src/routes/(app)/invoices/estimates/+page.svelte | Add line-item UI and hidden form fields for estimates |
| frontend/src/routes/(app)/invoices/estimates/+page.server.js | Send *_id relationship keys; parse lineItems JSON into line_items |
| frontend/src/lib/components/ui/editable-field/EditableMultiSelect.svelte | Normalize option shapes to prevent keyed {#each} crashes |
| frontend/package.json | Dependency bumps (SvelteKit/Svelte tooling, Sentry, etc.) |
| frontend/pnpm-lock.yaml | Lockfile updates for bumped dependencies |
| backend/invoices/tests/test_invoices_api.py | Add API tests for creating estimate/recurring with line items |
| backend/invoices/serializer.py | Add nested line-item serializers + create/update handling |
| backend/common/tests/test_users.py | Add test for minimal user create payload |
| backend/common/serializer.py | Make phone/address fields optional for user creation docs/serializer |
Files not reviewed (1)
- frontend/pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (5)
frontend/src/routes/(app)/invoices/estimates/+page.server.js:279
JSON.parse(lineItemsJson)can throw if the hiddenlineItemsinput is missing/corrupted/tampered with. Consider wrapping the parse in atry/catchand returningfail(400, { error: 'Invalid line items payload' })with a clear message.
// Parse line items JSON const lineItemsJson = form.get('lineItems')?.toString() || '[]'; const lineItems = JSON.parse(lineItemsJson); frontend/src/routes/(app)/invoices/recurring/+page.server.js:204
JSON.parse(lineItemsJson)can throw if the hiddenlineItemsinput is missing/corrupted/tampered with, which will currently be caught by the outercatchand returned as a generic 400 error (or could become a 500 depending on upstream). Consider wrapping the parse in atry/catchand returningfail(400, { error: 'Invalid line items payload' })with a clear message.
// Parse line items JSON const lineItemsJson = form.get('lineItems')?.toString() || '[]'; const lineItems = JSON.parse(lineItemsJson); if (lineItems.length > 0) { frontend/src/routes/(app)/invoices/estimates/+page.server.js:290
discount_typeis defaulted to'fixed'here (and on line-item mapping), but the backend expects'FIXED'/'PERCENTAGE'(uppercase) perDISCOUNT_TYPES. This will cause 400 responses when creating/updating estimates. Update the values sent to the API to match the backend enum.
// Amounts discount_type: form.get('discountType')?.toString() || 'fixed', discount_value: form.get('discountValue')?.toString() || '0', tax_rate: form.get('taxRate')?.toString() || '0', currency: form.get('currency')?.toString() || 'USD', // Notes notes: form.get('notes')?.toString() || '', terms: form.get('terms')?.toString() || '', // Client address client_address_line: form.get('clientAddressLine')?.toString() || '', client_city: form.get('clientCity')?.toString() || '', client_state: form.get('clientState')?.toString() || '', client_postcode: form.get('clientPostcode')?.toString() || '', client_country: form.get('clientCountry')?.toString() || '', // Relationships account_id: form.get('accountId')?.toString() || null, contact_id: form.get('contactId')?.toString() || null, opportunity_id: form.get('opportunityId')?.toString() || null }; // Parse line items JSON const lineItemsJson = form.get('lineItems')?.toString() || '[]'; const lineItems = JSON.parse(lineItemsJson); if (lineItems.length > 0) { estimateData.line_items = lineItems.map((item, idx) => ({ name: item.name || item.description || '', description: item.description || '', quantity: item.quantity || 1, unit_price: item.unitPrice || '0', discount_type: item.discountType || 'fixed', discount_value: item.discountValue || '0', tax_rate: item.taxRate || '0', order: item.order || idx })); frontend/src/routes/(no-layout)/login/+page.server.js:198
- This action returns
{ success: false, error: ... }as a normal 200 response whenemailis missing. Withuse:enhance, this comes back asresult.type === 'success', so the page logic may treat it as a successful send. Consider returningfail(400, { error: 'Email is required' })for validation errors (and optionally a consistent error shape), so the enhanced form getsresult.type === 'failure'and the UI can showmagicLinkErrorreliably.
default: async ({ request }) => { const formData = await request.formData(); const email = formData.get('email'); if (!email) { return { success: false, error: 'Email is required' }; } frontend/src/routes/(app)/invoices/recurring/+page.server.js:213
discount_typeis defaulted to'fixed'here (and on line-item mapping), but the backend expects'FIXED'/'PERCENTAGE'(uppercase) perDISCOUNT_TYPES. This will cause 400 responses when creating/updating recurring invoices. Update the values sent to the API to match the backend enum.
currency: form.get('currency')?.toString() || 'USD', // Amounts discount_type: form.get('discountType')?.toString() || 'fixed', discount_value: form.get('discountValue')?.toString() || '0', tax_rate: form.get('taxRate')?.toString() || '0', // Notes notes: form.get('notes')?.toString() || '', terms: form.get('terms')?.toString() || '', // Relationships account_id: form.get('accountId')?.toString() || null, contact_id: form.get('contactId')?.toString() || null }; // Parse line items JSON const lineItemsJson = form.get('lineItems')?.toString() || '[]'; const lineItems = JSON.parse(lineItemsJson); if (lineItems.length > 0) { recurringData.line_items = lineItems.map((item, idx) => ({ description: item.description || '', quantity: item.quantity || 1, unit_price: item.unitPrice || '0', discount_type: item.discountType || 'fixed', discount_value: item.discountValue || '0', tax_rate: item.taxRate || '0', order: item.order || idx })); 💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines 804 to +820
| def create(self, validated_data): | ||
| line_items_data = validated_data.pop("line_items", []) | ||
| validated_data["org"] = self.org | ||
| return super().create(validated_data) | ||
| | ||
| estimate = Estimate.objects.create(**validated_data) | ||
| | ||
| for idx, item_data in enumerate(line_items_data): | ||
| EstimateLineItem.objects.create( | ||
| estimate=estimate, | ||
| org=self.org, | ||
| order=item_data.get("order", idx), | ||
| **{k: v for k, v in item_data.items() if k != "order"}, | ||
| ) | ||
| | ||
| estimate.recalculate_totals() | ||
| estimate.save() | ||
| return estimate |
Comment on lines +1036 to +1054
| def update(self, instance, validated_data): | ||
| line_items_data = validated_data.pop("line_items", None) | ||
| | ||
| for attr, value in validated_data.items(): | ||
| setattr(instance, attr, value) | ||
| | ||
| if line_items_data is not None: | ||
| instance.line_items.all().delete() | ||
| for idx, item_data in enumerate(line_items_data): | ||
| RecurringInvoiceLineItem.objects.create( | ||
| recurring_invoice=instance, | ||
| org=self.org or instance.org, | ||
| order=item_data.get("order", idx), | ||
| **{k: v for k, v in item_data.items() if k != "order"}, | ||
| ) | ||
| | ||
| self._recalculate_totals(instance) | ||
| instance.save() | ||
| return instance |
Comment on lines 21 to 32
| function handleMagicLink() { | ||
| isSendingLink = true; | ||
| magicLinkError = ''; | ||
| return async ({ result }) => { | ||
| isSendingLink = false; | ||
| if (result.type === 'success') { | ||
| if (result?.type === 'success') { | ||
| magicLinkSent = true; | ||
| } else if (result.type === 'failure') { | ||
| } else if (result?.type === 'failure') { | ||
| magicLinkError = result.data?.error || 'Something went wrong. Please try again.'; | ||
| } else if (!result) { | ||
| magicLinkError = 'Something went wrong. Please try again.'; | ||
| } |
| accountId: '', | ||
| contactId: '', | ||
| currency: 'USD', | ||
| discountType: 'fixed', |
| | ||
| // Parse line items JSON | ||
| const lineItemsJson = form.get('lineItems')?.toString() || '[]'; | ||
| const lineItems = JSON.parse(lineItemsJson); |
| | ||
| // Parse line items JSON | ||
| const lineItemsJson = form.get('lineItems')?.toString() || '[]'; | ||
| const lineItems = JSON.parse(lineItemsJson); |
Comment on lines +822 to +840
| def update(self, instance, validated_data): | ||
| line_items_data = validated_data.pop("line_items", None) | ||
| | ||
| for attr, value in validated_data.items(): | ||
| setattr(instance, attr, value) | ||
| | ||
| if line_items_data is not None: | ||
| instance.line_items.all().delete() | ||
| for idx, item_data in enumerate(line_items_data): | ||
| EstimateLineItem.objects.create( | ||
| estimate=instance, | ||
| org=self.org or instance.org, | ||
| order=item_data.get("order", idx), | ||
| **{k: v for k, v in item_data.items() if k != "order"}, | ||
| ) | ||
| | ||
| instance.recalculate_totals() | ||
| instance.save() | ||
| return instance |
Comment on lines 1018 to +1034
| def create(self, validated_data): | ||
| line_items_data = validated_data.pop("line_items", []) | ||
| validated_data["org"] = self.org | ||
| return super().create(validated_data) | ||
| | ||
| recurring = RecurringInvoice.objects.create(**validated_data) | ||
| | ||
| for idx, item_data in enumerate(line_items_data): | ||
| RecurringInvoiceLineItem.objects.create( | ||
| recurring_invoice=recurring, | ||
| org=self.org, | ||
| order=item_data.get("order", idx), | ||
| **{k: v for k, v in item_data.items() if k != "order"}, | ||
| ) | ||
| | ||
| self._recalculate_totals(recurring) | ||
| recurring.save() | ||
| return recurring |
| contactId: '', | ||
| opportunityId: '', | ||
| currency: 'USD', | ||
| discountType: 'fixed', |
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
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 by CodeRabbit
New Features
Improvements
Chores