feat: new split expense flow across all dimensions #3490
Conversation
* new flow of add-edit expense page * fix: unit test failure --------- Co-authored-by: Snehasish <[email protected]>
* fix: update the ui for the new split_expense_page * minor * minor color change * fix: update class name * minor * minor --------- Co-authored-by: Snehasish <[email protected]>
WalkthroughThe pull request refactors the expense splitting functionality. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant A as AddEditExpensePage
participant H as splitExpenseHandler
participant N as NavigationController
participant S as SplitExpensePage
U->>A: Tap split expense button
A->>A: Build splitConfig object
A->>H: Invoke splitExpenseHandler
H->>N: Request navigation with splitConfig
N->>S: Load SplitExpensePage with configuration
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
PR title must start with "fix:", "feat:", "chore:", "refactor:", or "test:" (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
PR title must start with "fix:", "feat:", "chore:", "refactor:", or "test:" (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
![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
🔭 Outside diff range comments (1)
src/app/fyle/add-edit-expense/add-edit-expense-1.spec.ts (1)
1182-1214: Thalaiva says: Test cases need more power!The test coverage for splitExpenseHandler could be more comprehensive. Consider adding test cases for:
- Edge cases with different splitConfig combinations
- Error scenarios when form validation fails
- Network error scenarios
describe('splitExpenseHandler():', () => { + it('should handle network errors gracefully', () => { + setFormValid(); + spyOn(component, 'openSplitExpenseModal').and.throwError('Network Error'); + spyOn(component, 'showErrorMessage'); + + component.splitExpenseHandler(); + + expect(component.showErrorMessage).toHaveBeenCalled(); + }); + + it('should validate splitConfig fields correctly', () => { + setFormValid(); + component.txnFields$ = of({ + org_category_id: { is_mandatory: true }, + project_id: { is_mandatory: false }, + cost_center_id: { is_mandatory: true } + }); + + component.splitExpenseHandler(); + + expect(component.openSplitExpenseModal).toHaveBeenCalledWith( + jasmine.objectContaining({ + category: { is_mandatory: true }, + project: { is_mandatory: false }, + costCenter: { is_mandatory: true } + }) + ); + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/app/fyle/add-edit-expense/add-edit-expense-1.spec.ts(10 hunks)src/app/fyle/add-edit-expense/add-edit-expense.page.ts(4 hunks)src/app/fyle/split-expense/split-expense.page.html(5 hunks)src/app/fyle/split-expense/split-expense.page.scss(4 hunks)src/theme/colors.scss(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (6)
src/app/fyle/add-edit-expense/add-edit-expense-1.spec.ts (1)
683-699: Mind it! The splitConfig object structure looks solid!The new splitConfig object elegantly encapsulates field visibility and mandatory status. This is a much better approach than passing individual parameters.
src/theme/colors.scss (1)
48-48: Style it like a boss! New color variables are on point!The new shadow and gradient variables enhance the visual hierarchy:
$shadow-xs: Perfect for subtle elevation effects$pink-gradient-light-2: Adds a soft, elegant gradient for backgroundsAlso applies to: 91-91
src/app/fyle/split-expense/split-expense.page.scss (2)
43-47: These styles are blockbuster material!The updated padding and gap properties create better visual rhythm. The new gradient background adds a subtle but effective visual hierarchy.
Also applies to: 247-252
272-283: Error messages now have style and substance!The error message block styling is well-thought-out with:
- Proper border radius for modern look
- Consistent padding and line height
- Clear visual hierarchy with icon
src/app/fyle/split-expense/split-expense.page.html (2)
29-32: Warning message that packs a punch!Clear and concise warning about the permanent nature of split actions. This helps prevent user mistakes.
34-47: The amount block is now a superstar!The restructured amount block provides better information hierarchy:
- Split count display
- Total amount visibility
- Remaining amount tracking
| text: 'Dimiss as Card Payment', | ||
| handler: () => this.markDismissHandler(), | ||
| }); |
There was a problem hiding this comment.
Ayyo! There's a typo in 'Dimiss', boss!
The word 'Dimiss' should be corrected to 'Dismiss' in the action sheet option text.
Apply this diff to fix the typo:
- text: 'Dimiss as Card Payment',
+ text: 'Dismiss as Card Payment',📝 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.
| text: 'Dimiss as Card Payment', | |
| handler: () => this.markDismissHandler(), | |
| }); | |
| text: 'Dismiss as Card Payment', | |
| handler: () => this.markDismissHandler(), | |
| }); |
| category: { | ||
| is_visible: !!res.txnFields.org_category_id, | ||
| value: formValue.category, | ||
| is_mandatory: res.txnFields.org_category_id?.is_mandatory || false, | ||
| }, | ||
| project: { | ||
| is_visible: !!res.txnFields.project_id, | ||
| value: formValue.project, | ||
| is_mandatory: res.txnFields.project_id?.is_mandatory || false, | ||
| }, | ||
| costCenter: { | ||
| is_visible: !!res.txnFields.cost_center_id, | ||
| value: formValue.costCenter, | ||
| is_mandatory: res.txnFields.cost_center_id?.is_mandatory || false, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Let's cut down the repetition like a superstar!
The construction of splitConfig for 'category', 'project', and 'costCenter' is repetitive. Refactor it into a helper function to make your code more powerful and maintainable.
Apply this diff to refactor the code:
+ function createSplitConfigField(fieldName, fieldValue) {
+ return {
+ is_visible: !!res.txnFields[fieldName],
+ value: fieldValue,
+ is_mandatory: res.txnFields[fieldName]?.is_mandatory || false,
+ };
+ }
- const splitConfig = {
- category: {
- is_visible: !!res.txnFields.org_category_id,
- value: formValue.category,
- is_mandatory: res.txnFields.org_category_id?.is_mandatory || false,
- },
- project: {
- is_visible: !!res.txnFields.project_id,
- value: formValue.project,
- is_mandatory: res.txnFields.project_id?.is_mandatory || false,
- },
- costCenter: {
- is_visible: !!res.txnFields.cost_center_id,
- value: formValue.costCenter,
- is_mandatory: res.txnFields.cost_center_id?.is_mandatory || false,
- },
- };
+ const splitConfig = {
+ category: createSplitConfigField('org_category_id', formValue.category),
+ project: createSplitConfigField('project_id', formValue.project),
+ costCenter: createSplitConfigField('cost_center_id', formValue.costCenter),
+ };📝 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.
| category: { | |
| is_visible: !!res.txnFields.org_category_id, | |
| value: formValue.category, | |
| is_mandatory: res.txnFields.org_category_id?.is_mandatory || false, | |
| }, | |
| project: { | |
| is_visible: !!res.txnFields.project_id, | |
| value: formValue.project, | |
| is_mandatory: res.txnFields.project_id?.is_mandatory || false, | |
| }, | |
| costCenter: { | |
| is_visible: !!res.txnFields.cost_center_id, | |
| value: formValue.costCenter, | |
| is_mandatory: res.txnFields.cost_center_id?.is_mandatory || false, | |
| }, | |
| }; | |
| function createSplitConfigField(fieldName, fieldValue) { | |
| return { | |
| is_visible: !!res.txnFields[fieldName], | |
| value: fieldValue, | |
| is_mandatory: res.txnFields[fieldName]?.is_mandatory || false, | |
| }; | |
| } | |
| const splitConfig = { | |
| category: createSplitConfigField('org_category_id', formValue.category), | |
| project: createSplitConfigField('project_id', formValue.project), | |
| costCenter: createSplitConfigField('cost_center_id', formValue.costCenter), | |
| }; |
| <ion-footer> | ||
| <div class="split-expense--more-actions"> | ||
| <button | ||
| class="btn-outline-secondary split-expense--add-more" | ||
| *ngIf="splitExpensesFormArray.controls.length < 30" | ||
| (click)="add()" | ||
| > | ||
| <mat-icon class="split-expense--add-more-icon" svgIcon="plus-square"></mat-icon> | ||
| <div class="split-expense--add-more-label">Add Split</div> | ||
| </button> | ||
|
|
||
| <button class="btn-outline-secondary split-expense--split-evenly" (click)="splitEvenly()"> | ||
| <mat-icon class="split-expense--split-evenly-icon" svgIcon="split"></mat-icon> | ||
| <div class="split-expense--split-evenly-label">Split Evenly</div> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Footer actions that mean business!
The new footer design with split actions is more intuitive and user-friendly. However, consider adding tooltips for better accessibility.
<button class="btn-outline-secondary split-expense--add-more"
*ngIf="splitExpensesFormArray.controls.length < 30"
+ matTooltip="Add a new split to the expense"
(click)="add()">
...
</button>
<button class="btn-outline-secondary split-expense--split-evenly"
+ matTooltip="Divide the amount equally between all splits"
(click)="splitEvenly()">
...
</button>📝 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.
| <ion-footer> | |
| <div class="split-expense--more-actions"> | |
| <button | |
| class="btn-outline-secondary split-expense--add-more" | |
| *ngIf="splitExpensesFormArray.controls.length < 30" | |
| (click)="add()" | |
| > | |
| <mat-icon class="split-expense--add-more-icon" svgIcon="plus-square"></mat-icon> | |
| <div class="split-expense--add-more-label">Add Split</div> | |
| </button> | |
| <button class="btn-outline-secondary split-expense--split-evenly" (click)="splitEvenly()"> | |
| <mat-icon class="split-expense--split-evenly-icon" svgIcon="split"></mat-icon> | |
| <div class="split-expense--split-evenly-label">Split Evenly</div> | |
| </button> | |
| </div> | |
| <ion-footer> | |
| <div class="split-expense--more-actions"> | |
| <button | |
| class="btn-outline-secondary split-expense--add-more" | |
| *ngIf="splitExpensesFormArray.controls.length < 30" | |
| matTooltip="Add a new split to the expense" | |
| (click)="add()" | |
| > | |
| <mat-icon class="split-expense--add-more-icon" svgIcon="plus-square"></mat-icon> | |
| <div class="split-expense--add-more-label">Add Split</div> | |
| </button> | |
| <button | |
| class="btn-outline-secondary split-expense--split-evenly" | |
| matTooltip="Divide the amount equally between all splits" | |
| (click)="splitEvenly()" | |
| > | |
| <mat-icon class="split-expense--split-evenly-icon" svgIcon="split"></mat-icon> | |
| <div class="split-expense--split-evenly-label">Split Evenly</div> | |
| </button> | |
| </div> | |
| </ion-footer> |
|
PR description must contain a link to a ClickUp (case-insensitive) |
Sishhhh
changed the title
|
PR description must contain a link to a ClickUp (case-insensitive) |
Clickup
Please add link here
Code Coverage
Please add code coverage here
UI Preview
Please add screenshots for UI changes
Summary by CodeRabbit
New Features
Style