feat: quickbooks export-settings generalise validators#1761
Open
vignesh05904 wants to merge 2 commits intomasterfrom
Open
feat: quickbooks export-settings generalise validators#1761vignesh05904 wants to merge 2 commits intomasterfrom
vignesh05904 wants to merge 2 commits intomasterfrom
Conversation
Contributor
WalkthroughThis PR introduces a centralized field visibility and mandatory-field validation system for QBO export settings. It adds a new form model type, validation constants and dependency checkers, then refactors the component and template to use centralized Changes
Sequence Diagram(s)sequenceDiagram
participant Component as QboExportSettingsComponent
participant Service as QboExportSettingsService
participant FormGroup as FormGroup+Validators
participant Template as Template View
Component->>Service: mapAPIResponseToFormGroup(settings, mapping, getEmployeeMapping callback)
Service->>FormGroup: Create FormGroup with custom validators
FormGroup->>FormGroup: Validators iterate QBO_FIELD_DEPENDENCIES
FormGroup->>FormGroup: Check visibility & mandatory rules via QBO_MANDATORY_FIELD_RULES
Service-->>Component: Return FormGroup with validation logic
Component->>Component: Subscribe to employeeMapping.valueChanges
Component->>FormGroup: On employeeMapping change: updateValueAndValidity()
FormGroup->>FormGroup: Re-apply validators with updated mapping
FormGroup-->>Component: Emit validity/validation errors
Component->>Component: showField(field), isFieldMandatory(field) called by template
Component-->>Template: Return field visibility & mandatory status
Template->>Template: Render/hide fields based on showField() result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
🔁 Code Duplication Report - Angular
🎉 This PR reduces duplicated code by 0.05%! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.html (1)
111-131:⚠️ Potential issue | 🔴 Critical
accountsPayablemay lose mandatory enforcement when reimbursable export type is BILL.Line 121 uses
[isFieldMandatory]="isFieldMandatory('accountsPayable')". The component'sisFieldMandatorymethod delegates toQBO_MANDATORY_FIELD_RULES['accountsPayable'], which only returnstrueforJOURNAL_ENTRY + VENDOR. When the reimbursable export type isBILL, the override returnsfalse— makingaccountsPayableappear optional in the UI.Additionally, the form-level validator (service Lines 331-335) uses the same override, so the form will be considered valid without an
accountsPayablevalue whenBILLis selected. Previously,getMandatoryFieldreturnedtrueforBILL, meaning this is a regression.The root cause is in
QBO_MANDATORY_FIELD_RULESin the service file — theaccountsPayableentry should also returntruewhen reimbursable export type isBILL, or it should be removed entirely fromQBO_MANDATORY_FIELD_RULESso visibility implies mandatoriness (the default).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.html` around lines 111 - 131, QBO_MANDATORY_FIELD_RULES currently marks accountsPayable as mandatory only for JOURNAL_ENTRY and VENDOR, causing isFieldMandatory('accountsPayable') and the form validator to permit missing accountsPayable when reimbursableExportType === BILL; update QBO_MANDATORY_FIELD_RULES so the accountsPayable rule also returns true for BILL (or remove the accountsPayable entry to inherit default-visible-is-mandatory behavior), then verify isFieldMandatory, getMandatoryField (or the form-level validator logic that references QBO_MANDATORY_FIELD_RULES) will treat accountsPayable as required when reimbursableExportType is BILL.
🧹 Nitpick comments (3)
src/app/core/services/qbo/qbo-configuration/qbo-export-settings.service.ts (2)
66-72: Clarify the semantics ofQBO_MANDATORY_FIELD_RULESvs. visibility.The
accountsPayablemandatory rule (Lines 67-69) returnstruewhenreimbursableExportType === JOURNAL_ENTRY && employeeMapping === VENDOR. However,accountsPayablevisibility (Lines 37-41) also includesreimbursableExportType === BILL. WhenBILLis selected, visibility istruebut there's no mandatory override → the default in the validator istrue(Line 332), making it mandatory. This works correctly but the dual-table lookup (visibility + mandatory override) can be confusing.Consider adding a brief inline comment documenting the convention: "Fields not listed here default to mandatory when visible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/services/qbo/qbo-configuration/qbo-export-settings.service.ts` around lines 66 - 72, The QBO_MANDATORY_FIELD_RULES map currently only overrides specific fields (e.g., accountsPayable) and relies on the validator default that a visible field is mandatory, which is confusing given the separate visibility rules (see accountsPayable visibility using reimbursableExportType === BILL); add a concise inline comment above the QBO_MANDATORY_FIELD_RULES declaration explaining the convention: that this map contains only exceptions/overrides and that any field not listed here is considered mandatory when visible by the validator (mention QBO_MANDATORY_FIELD_RULES, accountsPayable, reimbursableExportType and the validator default behavior to help locate the logic).
7-14: Duplicate imports from@angular/formsandenum.model.
FormControlis imported on Line 12, andAbstractControl,FormGroup,ValidationErrorson Line 7 — both from@angular/forms. Similarly,SplitExpenseGroupingis imported on Line 11, and more enum members on Line 14 — both fromsrc/app/core/models/enum/enum.model. Consolidate each into a single import statement.♻️ Proposed consolidation
-import { AbstractControl, FormGroup, ValidationErrors } from '@angular/forms'; ... -import { SplitExpenseGrouping } from 'src/app/core/models/enum/enum.model'; -import { FormControl, Validators } from '@angular/forms'; ... -import { CCCExpenseState, ExpenseState, ExportDateType, NameInJournalEntry, ExpenseGroupingFieldOption, EmployeeFieldMapping, QBOReimbursableExpensesObject, QBOCorporateCreditCardExpensesObject } from 'src/app/core/models/enum/enum.model'; +import { AbstractControl, FormControl, FormGroup, ValidationErrors, Validators } from '@angular/forms'; +import { CCCExpenseState, ExpenseState, ExportDateType, NameInJournalEntry, ExpenseGroupingFieldOption, EmployeeFieldMapping, QBOReimbursableExpensesObject, QBOCorporateCreditCardExpensesObject, SplitExpenseGrouping } from 'src/app/core/models/enum/enum.model';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/services/qbo/qbo-configuration/qbo-export-settings.service.ts` around lines 7 - 14, There are duplicate imports from '@angular/forms' and from 'src/app/core/models/enum/enum.model'; consolidate into single import statements to avoid redundancy: merge AbstractControl, FormGroup, ValidationErrors, FormControl, and Validators into one import from '@angular/forms' and merge SplitExpenseGrouping together with CCCExpenseState, ExpenseState, ExportDateType, NameInJournalEntry, ExpenseGroupingFieldOption, EmployeeFieldMapping, QBOReimbursableExpensesObject, and QBOCorporateCreditCardExpensesObject into one import from 'src/app/core/models/enum/enum.model'; update the import list at the top of qbo-export-settings.service.ts so each module is imported only once while preserving all referenced symbols (e.g., FormControl, AbstractControl, SplitExpenseGrouping, CCCExpenseState, etc.).src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts (1)
155-183: Duplicate injection ofQboEmployeeSettingsServiceand duplicate option initialization.
Both
employeeSettingService(Line 164) andqboEmployeeSettingsService(Line 167) are of typeQboEmployeeSettingsService. Use a single instance.Lines 172-182 (constructor) initialize the same option arrays that are re-initialized in
ngOnInit(Lines 604-613). The constructor assignments are immediately overwritten and can be removed.♻️ Remove duplicate injection and constructor initialization
constructor( private translocoService: TranslocoService, private helperService: HelperService, private qboHelperService: QboHelperService, private mappingService: MappingService, private router: Router, private toastService: IntegrationsToastService, private windowService: WindowService, private workspaceService: WorkspaceService, - private employeeSettingService: QboEmployeeSettingsService, private qboExportSettingsService: QboExportSettingsService, private employeeSettingsService: EmployeeSettingsService, private qboEmployeeSettingsService: QboEmployeeSettingsService, public brandingService: BrandingService, private storageService: StorageService ) { this.windowReference = this.windowService.nativeWindow; - this.reimbursableExpenseGroupingDateOptions = this.qboExportSettingsService.getReimbursableExpenseGroupingDateOptions(); - this.creditCardExportTypes = this.qboExportSettingsService.getCreditCardExportTypes(); - this.cccExpenseStateOptions = this.qboExportSettingsService.getCCCExpenseStateOptions(); - this.expenseStateOptions = this.qboExportSettingsService.getReimbursableExpenseStateOptions(); - this.expenseGroupByOptions = this.qboExportSettingsService.getExpenseGroupByOptions(); - this.cccExpenseGroupingDateOptions = this.reimbursableExpenseGroupingDateOptions.concat(); - this.nameInJournalOptions = this.qboExportSettingsService.getNameInJournalOptions(); - this.splitExpenseGroupingOptions = this.qboExportSettingsService.getSplitExpenseGroupingOptions(); - this.employeeMappingOptions = this.employeeSettingsService.getEmployeeFieldMappingOptions(); - this.autoMapEmployeeOptions = this.qboEmployeeSettingsService.getAutoMapEmployeeOptions(); this.showWarningMessageAboutAutoExpenseCreation = this.storageService.get('showWarningMessageAboutAutoExpenseCreationInQBO') ?? true; }Then update the
postEmployeeSettingscall on Line 243 to useqboEmployeeSettingsServiceinstead ofemployeeSettingService.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts` around lines 155 - 183, Remove the duplicate QboEmployeeSettingsService injection (remove the employeeSettingService constructor parameter) and keep qboEmployeeSettingsService; delete the constructor assignments that duplicate ngOnInit (remove initialization of reimbursableExpenseGroupingDateOptions, creditCardExportTypes, cccExpenseStateOptions, expenseStateOptions, expenseGroupByOptions, cccExpenseGroupingDateOptions, nameInJournalOptions, splitExpenseGroupingOptions, employeeMappingOptions, autoMapEmployeeOptions, showWarningMessageAboutAutoExpenseCreation) so they are only initialized in ngOnInit, and update the postEmployeeSettings call to use qboEmployeeSettingsService instead of employeeSettingService (referencing the constructor, ngOnInit, postEmployeeSettings, employeeSettingService, qboEmployeeSettingsService and the listed option properties).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.html`:
- Around line 111-131: QBO_MANDATORY_FIELD_RULES currently marks accountsPayable
as mandatory only for JOURNAL_ENTRY and VENDOR, causing
isFieldMandatory('accountsPayable') and the form validator to permit missing
accountsPayable when reimbursableExportType === BILL; update
QBO_MANDATORY_FIELD_RULES so the accountsPayable rule also returns true for BILL
(or remove the accountsPayable entry to inherit default-visible-is-mandatory
behavior), then verify isFieldMandatory, getMandatoryField (or the form-level
validator logic that references QBO_MANDATORY_FIELD_RULES) will treat
accountsPayable as required when reimbursableExportType is BILL.
---
Nitpick comments:
In `@src/app/core/services/qbo/qbo-configuration/qbo-export-settings.service.ts`:
- Around line 66-72: The QBO_MANDATORY_FIELD_RULES map currently only overrides
specific fields (e.g., accountsPayable) and relies on the validator default that
a visible field is mandatory, which is confusing given the separate visibility
rules (see accountsPayable visibility using reimbursableExportType === BILL);
add a concise inline comment above the QBO_MANDATORY_FIELD_RULES declaration
explaining the convention: that this map contains only exceptions/overrides and
that any field not listed here is considered mandatory when visible by the
validator (mention QBO_MANDATORY_FIELD_RULES, accountsPayable,
reimbursableExportType and the validator default behavior to help locate the
logic).
- Around line 7-14: There are duplicate imports from '@angular/forms' and from
'src/app/core/models/enum/enum.model'; consolidate into single import statements
to avoid redundancy: merge AbstractControl, FormGroup, ValidationErrors,
FormControl, and Validators into one import from '@angular/forms' and merge
SplitExpenseGrouping together with CCCExpenseState, ExpenseState,
ExportDateType, NameInJournalEntry, ExpenseGroupingFieldOption,
EmployeeFieldMapping, QBOReimbursableExpensesObject, and
QBOCorporateCreditCardExpensesObject into one import from
'src/app/core/models/enum/enum.model'; update the import list at the top of
qbo-export-settings.service.ts so each module is imported only once while
preserving all referenced symbols (e.g., FormControl, AbstractControl,
SplitExpenseGrouping, CCCExpenseState, etc.).
In
`@src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts`:
- Around line 155-183: Remove the duplicate QboEmployeeSettingsService injection
(remove the employeeSettingService constructor parameter) and keep
qboEmployeeSettingsService; delete the constructor assignments that duplicate
ngOnInit (remove initialization of reimbursableExpenseGroupingDateOptions,
creditCardExportTypes, cccExpenseStateOptions, expenseStateOptions,
expenseGroupByOptions, cccExpenseGroupingDateOptions, nameInJournalOptions,
splitExpenseGroupingOptions, employeeMappingOptions, autoMapEmployeeOptions,
showWarningMessageAboutAutoExpenseCreation) so they are only initialized in
ngOnInit, and update the postEmployeeSettings call to use
qboEmployeeSettingsService instead of employeeSettingService (referencing the
constructor, ngOnInit, postEmployeeSettings, employeeSettingService,
qboEmployeeSettingsService and the listed option properties).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/core/models/qbo/qbo-configuration/qbo-export-setting.model.tssrc/app/core/services/qbo/qbo-configuration/qbo-export-settings.service.tssrc/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.htmlsrc/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts
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.
Description
quickbooks export-settings generalise validators
Clickup
https://app.clickup.com
Summary by CodeRabbit
Release Notes
New Features
Refactor