Revert "feat: QBD direct main connection page business logic" #1069
Conversation
This reverts commit 5ecf737.
WalkthroughThis pull request includes several modifications across various files, primarily focusing on the renaming of the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@DhaaraniCIT has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 8
🧹 Outside diff range and nitpick comments (14)
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.ts (2)
30-32: Consider improving type safety of getKeys methodThe method uses
anytype which reduces type safety. Consider using a more specific type or making it generic.- getKeys(obj: any): string[] { + private getKeys<T extends object>(obj: T): Array<keyof T> { return Object.keys(obj) as Array<keyof T>; }
35-35: Add null check for qbdFieldsConsider adding a null check to prevent runtime errors if
qbdFieldsis undefined.- this.fieldLength = Object.keys(this.qbdFields).length; + this.fieldLength = this.qbdFields ? Object.keys(this.qbdFields).length : 0;src/app/shared/components/configuration/configuration-confirmation-dialog/configuration-confirmation-dialog.component.ts (2)
Line range hint
7-41: LGTM: Component maintains focused responsibilityThe component structure is clean and follows Angular best practices:
- Clear input/output property declarations
- Maintains single responsibility principle
- Simple and predictable event handling
The removal of
appName,subLable, andredirectLinkproperties helps reduce complexity and improves maintainability.Consider documenting the expected values for the remaining @input properties in JSDoc comments to improve maintainability, especially:
event: ConfigurationWarningEventheaderTextcontextText
Line range hint
33-35: Consider implementing ngOnInitThe empty
ngOnInitimplementation could be removed since it's not being used. If you're implementing theOnInitinterface, you should have initialization logic.- ngOnInit(): void { - }src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.ts (2)
43-43: Consider removing the unused downloadFilePath propertySince
downloadFilePathis no longer emitted in theonDownloadClickmethod, it appears to be unused. Consider removing this property to maintain code cleanliness.- downloadFilePath: string;
Line range hint
28-43: Consider documenting the event handling pattern changeAs this is a revert PR, the changes to the event emission pattern (removing typed events and file path data) represent a significant architectural shift. Consider:
- Documenting the reason for this change in the component
- Updating any relevant integration documentation
- Ensuring all team members are aware of the new event handling pattern
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.html (4)
Line range hint
1-12: Consider making the step number dynamicThe step number "3" is hardcoded in the template. This could lead to maintenance issues if the workflow steps change in the future.
Consider passing the step number as an input property to make it more maintainable:
- <div class="tw-w-20-px tw-h-20-px tw-rounded-sm tw-bg-bg-secondary tw-text-white tw-text-14-px tw-font-500 tw-text-center tw-mr-8-px"> - 3 - </div> + <div class="tw-w-20-px tw-h-20-px tw-rounded-sm tw-bg-bg-secondary tw-text-white tw-text-14-px tw-font-500 tw-text-center tw-mr-8-px"> + {{stepNumber}} + </div>
14-14: Simplify border class logicThe border class condition
i < fieldLength-1could be simplified using CSS selectors.Consider using CSS
:not(:last-child)selector instead:-<div *ngFor="let field of getKeys(qbdFields); let i = index" class="tw-flex tw-items-center tw-justify-between tw-p-14-px" [ngClass]=" i < fieldLength-1 ? 'tw-border-b tw-border-b-divider-border-color' : '' "> +<div *ngFor="let field of getKeys(qbdFields)" class="tw-flex tw-items-center tw-justify-between tw-p-14-px [&:not(:last-child)]:tw-border-b [&:not(:last-child)]:tw-border-b-divider-border-color">
26-29: Move info text to translation fileThe info text is hardcoded in the template. For better maintainability and future internationalization support, consider moving it to a translation file.
Consider using a translation key:
- <app-configuration-info-label [infoText]="'The values displayed here are synced from QuickBooks Desktop to Fyle. If you notice discrepancies in the synced values, you can continue onboarding; the sync will refresh automatically in 5 minutes to capture any missed values.'" [showIcon]="false"></app-configuration-info-label> + <app-configuration-info-label [infoText]="'QBD_DIRECT.DATA_SYNC.INFO_TEXT' | translate" [showIcon]="false"></app-configuration-info-label>
31-31: Improve footer button state handlingThe button state logic could be clearer, and there's no visible loading state handling for the continue action.
Consider these improvements:
- <app-configuration-step-footer [ctaText]="ConfigurationCtaText.CONTINUE" [isButtonDisabled]="!isCTAEnabled" (save)="onContinueClick()"></app-configuration-step-footer> + <app-configuration-step-footer + [ctaText]="ConfigurationCtaText.CONTINUE" + [isButtonEnabled]="isCTAEnabled" + [isLoading]="isSubmitting" + (save)="onContinueClick()"> + </app-configuration-step-footer>src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.html (2)
Line range hint
46-48: Improve iframe accessibility and performanceThe YouTube video iframe is missing important attributes for accessibility and performance.
Add title and loading attributes:
- <iframe width="427" height="259" src="https://www.youtube.com/embed/2oYdc8KcQnk" frameborder="0"></iframe> + <iframe + width="427" + height="259" + src="https://www.youtube.com/embed/2oYdc8KcQnk" + frameborder="0" + title="How to find QuickBooks Desktop company file path" + loading="lazy"> + </iframe>
Line range hint
63-65: Improve manual download link accessibilityThe manual download link needs keyboard accessibility improvements.
Add keyboard accessibility attributes and focus styles:
- <a class="tw-underline tw-underline-offset-1 tw-cursor-pointer" (click)="onManualDownload()">download the integration file manually</a> + <a + href="javascript:void(0)" + class="tw-underline tw-underline-offset-1 tw-cursor-pointer focus:tw-outline-none focus:tw-ring-2 focus:tw-ring-primary" + (click)="onManualDownload()" + (keydown.enter)="onManualDownload()" + role="button" + tabindex="0"> + download the integration file manually + </a>src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.html (2)
Line range hint
18-75: Enhance iframe accessibilityThe YouTube video iframe should include title and loading attributes for better accessibility and performance.
-<iframe width="427" height="259" src="https://www.youtube.com/embed/2oYdc8KcQnk" frameborder="0"></iframe> +<iframe + width="427" + height="259" + src="https://www.youtube.com/embed/2oYdc8KcQnk" + frameborder="0" + title="QuickBooks Web Connector Setup Tutorial" + loading="lazy" + allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" + allowfullscreen> +</iframe>
Line range hint
76-87: Fix typo in component nameThere's a typo in the component name "chechbox-button" (missing 'k').
-<app-chechbox-button +<app-checkbox-button
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
src/assets/fyle/favicon.pngis excluded by!**/*.png
📒 Files selected for processing (19)
src/app/core/models/enum/enum.model.ts(1 hunks)src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-connector.model.ts(1 hunks)src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-onboarding.model.ts(2 hunks)src/app/core/services/qbd-direct/qbd-direct-configuration/qbd-direct-connector.service.ts(0 hunks)src/app/core/services/qbd-direct/qbd-direct-configuration/qbd-direct-connector.service.ts.service.spec.ts(1 hunks)src/app/core/services/qbd-direct/qbd-direct-configuration/qbd-direct-connector.service.ts.service.ts(1 hunks)src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-connector.service.spec.ts(1 hunks)src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-connector.service.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.html(2 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.ts(2 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.html(4 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.ts(2 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.html(5 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.ts(0 hunks)src/app/integrations/qbd-direct/qbd-direct.component.ts(1 hunks)src/app/shared/components/configuration/configuration-confirmation-dialog/configuration-confirmation-dialog.component.html(1 hunks)src/app/shared/components/configuration/configuration-confirmation-dialog/configuration-confirmation-dialog.component.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/app/core/services/qbd-direct/qbd-direct-configuration/qbd-direct-connector.service.ts
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.ts
✅ Files skipped from review due to trivial changes (3)
- src/app/core/services/qbd-direct/qbd-direct-configuration/qbd-direct-connector.service.ts.service.spec.ts
- src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-connector.service.spec.ts
- src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.html
🧰 Additional context used
🪛 Biome
src/app/core/services/qbd-direct/qbd-direct-configuration/qbd-direct-connector.service.ts.service.ts
[error] 8-8: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-connector.service.ts
[error] 8-8: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (14)
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-connector.model.ts (1)
29-29: LGTM! Verify type usage in dependent components.
The change to use an index signature for SyncDataType aligns with the revert objective. However, since this makes the type more flexible, ensure that all consuming components handle the dynamic nature of the keys appropriately.
Let's verify the usage of this type across the codebase:
✅ Verification successful
Let me gather more information about how the qbdFields is used in the component to ensure type safety:
The type change is safe and correctly handled in the dependent component
The component safely handles the dynamic nature of SyncDataType by:
- Using
Object.keys()to iterate over the dynamic keys - Properly checking for
nullvalues withqbdFields[field] !== null - Displaying either the numeric value or a loading icon based on the field's value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of SyncDataType to verify proper handling
# Look for imports and usages of SyncDataType
rg -l "SyncDataType" | while read -r file; do
if [[ "$file" != "src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-connector.model.ts" ]]; then
echo "=== $file ==="
# Show the context of SyncDataType usage
rg -A 5 -B 5 "SyncDataType" "$file"
fi
done
# Look for potential type assertions or type guards that might need updating
ast-grep --pattern 'as SyncDataType'
Length of output: 1805
Script:
#!/bin/bash
# Check how qbdFields is used in the component template and class
echo "=== Component Template Usage ==="
cat src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.html
echo -e "\n=== Component Class Usage ==="
cat src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.ts
Length of output: 3609
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.ts (2)
16-16: Verify all component usages for breaking change
The change of qbdFields from array to single object is a breaking change that requires verification of all component usages.
✅ Verification successful
Breaking change appears safe to proceed
The verification shows that:
- The only usage of the component in templates is commented out (
<!-- <app-qbd-direct-data-sync></app-qbd-direct-data-sync> -->) - The component is only imported and declared in its own module (
qbd-direct-shared.module.ts) - The test file is skipped (
xdescribe) and contains only a basic creation test
Since there are no active usages of this component in the codebase, the breaking change from array to single object type is safe to proceed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of app-qbd-direct-data-sync component
# to verify [qbdFields] property bindings
rg -t html '<app-qbd-direct-data-sync\s+[^>]*\[qbdFields\]' -A 3
Length of output: 66
Script:
#!/bin/bash
# Let's try a broader search for the component usage
rg -t html 'app-qbd-direct-data-sync' -A 3
# Also search for the component class name in TS files
# to find where it might be imported and used programmatically
rg "QbdDirectDataSyncComponent"
# Let's also check the component's test file to understand expected usage
fd "qbd-direct-data-sync.component.spec.ts" --exec cat {}
Length of output: 2568
Line range hint 1-38: Verify revert consistency
Since this PR is intended to revert changes from PR #1066, please verify that these component changes align with the original state before PR #1066.
✅ Verification successful
Let me try a different approach to verify the revert changes.
Revert changes are correctly implemented
The git history shows that the current changes perfectly match the state before PR #1066. Specifically:
- PR feat: QBD direct main connection page business logic #1066 had changed
qbdFieldsfromSyncDataTypetoSyncDataType[]and modified the field length calculation - It had also added two new inputs:
isLoadingandshowSection - The current code correctly reverts these changes, restoring:
- Original
qbdFieldstype asSyncDataType - Original field length calculation using
Object.keys() - Removal of the additional input properties
- Original
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the file state before PR #1066
gh pr view 1066 --json files -q '.files[].path' | grep 'qbd-direct-data-sync.component.ts' | xargs git show HEAD~1:
Length of output: 408
Script:
#!/bin/bash
# Search for the file in PR #1066
gh pr view 1066 --json files -q '.files[].path' | grep -i "qbd-direct-data-sync"
# Get the commit history for the file
git log --full-history -p -- "**/qbd-direct-data-sync.component.ts"
Length of output: 7930
src/app/shared/components/configuration/configuration-confirmation-dialog/configuration-confirmation-dialog.component.ts (1)
1-5: LGTM: Clean import statements
The imports are properly organized and contain only the necessary dependencies for the component's core functionality. The removal of unused imports aligns with the revert objective.
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.ts (1)
28-28: Verify the removal of type safety from downloadClick EventEmitter
The change from EventEmitter<string> to untyped EventEmitter removes type safety and could potentially break parent components that expect to receive the download file path.
Let's check for any parent component usages:
✅ Verification successful
Let's search for any HTML templates that might be using this component and binding to its downloadClick event. This will help us determine if there are any parent components relying on the event data.
Removing type from downloadClick EventEmitter is safe
The removal of type safety from the downloadClick EventEmitter appears to be safe because:
- The component is currently commented out in the only parent template that includes it
- No parent components are actively listening to the
downloadClickevent - The event is emitted without any payload in the component itself
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using app-qbd-direct-download-file selector
rg -l "app-qbd-direct-download-file" | xargs rg -A 5 "downloadClick"
# Search for any references to the removed string parameter
rg -A 3 "\(downloadClick\)=\".*\$event"
Length of output: 1705
Script:
#!/bin/bash
# Search for files containing app-qbd-direct-download-file selector
rg -l "app-qbd-direct-download-file" --type html
# Search for template usage of downloadClick event in HTML files
rg -A 2 '\(downloadClick\)="[^"]*"' --type html
Length of output: 231
Script:
#!/bin/bash
# Let's check the actual usage in the parent component's template
cat src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.html
# Also check the parent component's TypeScript file for any event handlers
rg -A 3 "downloadClick" src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.ts
Length of output: 503
src/app/shared/components/configuration/configuration-confirmation-dialog/configuration-confirmation-dialog.component.html (1)
6-9: LGTM! Clean implementation of the warning icon section.
The simplified implementation without conditional rendering makes the code more maintainable while following proper styling conventions.
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.html (1)
Line range hint 1-33: Verify revert changes alignment
As this is a revert of PR #1066, please verify that these template changes correctly restore the previous functionality and don't introduce new behavioral changes.
✅ Verification successful
Based on the git history and changes, I can now provide a definitive response about the revert changes:
Template changes are correctly aligned with revert intention
The changes in the template are correctly reverting PR #1066 and restoring the previous functionality by:
- Removing the conditional rendering based on
showSectionandisLoading - Reverting back to the original data structure where
qbdFieldsis an object with key-value pairs instead of an array of objects - Restoring the original template structure without the loading states
- Fixing the CTA button's disabled state logic back to
!isCTAEnabled
The revert successfully restores the component to its simpler, pre-PR #1066 state, maintaining the core data sync functionality without the additional loading states and section visibility controls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the original PR #1066 for template changes
gh pr view 1066 --json files -q '.files[] | select(.path == "src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.html") | .patch'
Length of output: 182
Script:
#!/bin/bash
# Let's try a different approach to check the changes in the file
git log --patch -S "qbd-direct-data-sync.component.html" -- "src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.html"
Length of output: 166
Script:
#!/bin/bash
# Let's also check the component's typescript file for any related changes
git log --patch -S "qbdFields" -- "src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.ts"
Length of output: 2088
Script:
#!/bin/bash
# Check current implementation
cat src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.html
Length of output: 2260
Script:
#!/bin/bash
# Let's check the history of changes in this specific file
git log -p src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.html
Length of output: 12242
Script:
#!/bin/bash
# Let's also check for any recent commits mentioning revert
git log --grep="revert" --since="1 month ago" -p
Length of output: 43505
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-onboarding.model.ts (1)
11-11: Verify if this revert is necessary
Since this PR is reverting changes from PR #1066, please verify if reverting to IN_CORRECT_PASSWORD is absolutely necessary. If there are specific technical reasons for this revert, they should be documented.
Also applies to: 72-72
✅ Verification successful
Let me search for all references to understand the impact better.
Revert to IN_CORRECT_PASSWORD is necessary
The revert is necessary as IN_CORRECT_PASSWORD is actively used across multiple files in the codebase:
src/app/integrations/qbd-direct/qbd-direct.component.ts: Used in route mappingsrc/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-onboarding.model.ts: Used in configurationsrc/app/core/models/enum/enum.model.ts: Defined in bothQbdDirectOnboardingStateandQBDConnectionStatusenums
Changing to INCORRECT_PASSWORD would break these existing references and require coordinated changes across all these files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the history and impact of the password state naming
# Check commit history for the original change
git log -p --all -S "INCORRECT_PASSWORD" -- "src/app/core/models/enum/enum.model.ts"
# Find all current references to either naming
echo "Current references to IN_CORRECT_PASSWORD:"
rg "IN_CORRECT_PASSWORD"
echo "Current references to INCORRECT_PASSWORD:"
rg "INCORRECT_PASSWORD"
Length of output: 2051
Script:
#!/bin/bash
# Search for all references to both variations in the codebase
echo "=== Files containing IN_CORRECT_PASSWORD ==="
rg -l "IN_CORRECT_PASSWORD"
echo -e "\n=== Files containing INCORRECT_PASSWORD ==="
rg -l "INCORRECT_PASSWORD"
# Get more context around the usage
echo -e "\n=== Context for IN_CORRECT_PASSWORD usage ==="
rg -C 3 "IN_CORRECT_PASSWORD"
echo -e "\n=== Context for INCORRECT_PASSWORD usage ==="
rg -C 3 "INCORRECT_PASSWORD"
Length of output: 4867
src/app/integrations/qbd-direct/qbd-direct.component.ts (1)
51-51: Verify enum usage across the codebase
Let's verify all occurrences of this enum value to ensure consistent usage and identify any potential issues caused by this revert.
✅ Verification successful
Enum value IN_CORRECT_PASSWORD is consistently used across the codebase
The verification shows that the enum value IN_CORRECT_PASSWORD is consistently used in all relevant files:
- Defined in
src/app/core/models/enum/enum.model.ts - Used in component routing in
qbd-direct.component.ts - Referenced in onboarding model in
qbd-direct-onboarding.model.ts
There are no inconsistencies or potential issues found with this enum usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any usage of either INCORRECT_PASSWORD or IN_CORRECT_PASSWORD in the codebase
echo "Searching for enum usage in TypeScript/JavaScript files..."
rg -t ts -t js "IN_CORRECT_PASSWORD|INCORRECT_PASSWORD"
# Search specifically in enum definition file
echo "Checking enum definition..."
ast-grep --pattern 'enum QbdDirectOnboardingState {
$$$
}'
Length of output: 2110
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.html (1)
Line range hint 68-70: LGTM!
The configuration step footer logic correctly prevents proceeding until the download is complete.
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.html (3)
Line range hint 1-17: LGTM: Header section is well-structured
The header section follows good UI practices with clear step indication and completion status.
Line range hint 88-97: LGTM: Success message section is well-implemented
The success message section has clear visibility conditions and proper text hierarchy.
98-101: LGTM: Footer section is properly implemented
The footer section correctly handles visibility and button state.
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.ts (1)
12-12: Verify if the empty component class is intentional
The QbdDirectOnboardingConnectorComponent class is currently empty. Please confirm if this is intentional. If methods and properties were removed, ensure that they are no longer required and that no other parts of the application depend on them.
Run the following script to check for any references to properties or methods of QbdDirectOnboardingConnectorComponent:
This script searches for any method or property accesses on instances of QbdDirectOnboardingConnectorComponent, which could indicate dependencies on code that has been removed.
| <div class="tw-text-14-px tw-font-400 tw-text-text-secondary" [innerHTML]="contextText"> | ||
| </div> |
There was a problem hiding this comment.
💡 Codebase verification
HTML content sanitization is required
The verification reveals that the contextText string input is directly bound to innerHTML without any sanitization. The component doesn't import or use Angular's DomSanitizer, making it vulnerable to XSS attacks. To fix this:
- Add DomSanitizer to sanitize the HTML content in
src/app/shared/components/configuration/configuration-confirmation-dialog/configuration-confirmation-dialog.component.ts:- Import DomSanitizer from '@angular/platform-browser'
- Inject it in the constructor
- Transform contextText using sanitizer.bypassSecurityTrustHtml() before binding
🔗 Analysis chain
Verify sanitization of HTML content.
The use of [innerHTML] could potentially expose the application to XSS attacks if the content is not properly sanitized.
Let's verify the sanitization implementation:
Let me gather more information about the contextText implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if DomSanitizer is properly implemented for contextText
# Look for sanitization of contextText in the component
ast-grep --pattern 'class ConfigurationConfirmationDialogComponent {
$$$
contextText$$$
$$$
}'
# Check for DomSanitizer usage
rg "DomSanitizer|sanitize" --type ts
Length of output: 1589
Script:
#!/bin/bash
# Find the implementation of ConfigurationConfirmationDialogComponent
rg -A 10 "export class ConfigurationConfirmationDialogComponent" --type ts
# Check if there's any sanitization happening in the component's constructor or methods
ast-grep --pattern 'class ConfigurationConfirmationDialogComponent {
$$$
constructor($$$) {
$$$
}
$$$
}'
# Look for the component's template usage
fd "configuration-confirmation-dialog.component.ts" --exec cat {}
Length of output: 2997
| <span *ngIf="qbdFields[field] !== null" class="tw-text-text-tertiary tw-font-400 tw-text-14-px">{{qbdFields[field]}}</span> | ||
| <div class="spin-icon tw-text-text-tertiary tw-font-400 tw-text-14-px"> | ||
| <app-svg-icon *ngIf="field.count === null" [svgSource]="'arrow-rotate-sync'" [width]="'14px'" [height]="'14px'" [isTextColorAllowed]="true" [styleClasses]="'tw-text-icon-tertiary'"></app-svg-icon> | ||
| <app-svg-icon *ngIf="qbdFields[field] === null" [svgSource]="'arrow-rotate-sync'" [width]="'14px'" [height]="'14px'" [isTextColorAllowed]="true" [styleClasses]="'tw-text-icon-tertiary'"></app-svg-icon> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance loading state accessibility and error handling
The loading state needs proper accessibility attributes, and there's no visible error state handling.
Consider these improvements:
- <span *ngIf="qbdFields[field] !== null" class="tw-text-text-tertiary tw-font-400 tw-text-14-px">{{qbdFields[field]}}</span>
- <div class="spin-icon tw-text-text-tertiary tw-font-400 tw-text-14-px">
- <app-svg-icon *ngIf="qbdFields[field] === null" [svgSource]="'arrow-rotate-sync'" [width]="'14px'" [height]="'14px'" [isTextColorAllowed]="true" [styleClasses]="'tw-text-icon-tertiary'"></app-svg-icon>
- </div>
+ <ng-container [ngSwitch]="true">
+ <span *ngSwitchCase="qbdFields[field] !== null" class="tw-text-text-tertiary tw-font-400 tw-text-14-px">{{qbdFields[field]}}</span>
+ <div *ngSwitchCase="qbdFields[field] === null" class="spin-icon tw-text-text-tertiary tw-font-400 tw-text-14-px" role="status" aria-label="Loading data">
+ <app-svg-icon [svgSource]="'arrow-rotate-sync'" [width]="'14px'" [height]="'14px'" [isTextColorAllowed]="true" [styleClasses]="'tw-text-icon-tertiary'"></app-svg-icon>
+ </div>
+ <span *ngSwitchDefault class="tw-text-error-text tw-font-400 tw-text-14-px">Error loading data</span>
+ </ng-container>📝 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.
| <span *ngIf="qbdFields[field] !== null" class="tw-text-text-tertiary tw-font-400 tw-text-14-px">{{qbdFields[field]}}</span> | |
| <div class="spin-icon tw-text-text-tertiary tw-font-400 tw-text-14-px"> | |
| <app-svg-icon *ngIf="field.count === null" [svgSource]="'arrow-rotate-sync'" [width]="'14px'" [height]="'14px'" [isTextColorAllowed]="true" [styleClasses]="'tw-text-icon-tertiary'"></app-svg-icon> | |
| <app-svg-icon *ngIf="qbdFields[field] === null" [svgSource]="'arrow-rotate-sync'" [width]="'14px'" [height]="'14px'" [isTextColorAllowed]="true" [styleClasses]="'tw-text-icon-tertiary'"></app-svg-icon> | |
| </div> | |
| <ng-container [ngSwitch]="true"> | |
| <span *ngSwitchCase="qbdFields[field] !== null" class="tw-text-text-tertiary tw-font-400 tw-text-14-px">{{qbdFields[field]}}</span> | |
| <div *ngSwitchCase="qbdFields[field] === null" class="spin-icon tw-text-text-tertiary tw-font-400 tw-text-14-px" role="status" aria-label="Loading data"> | |
| <app-svg-icon [svgSource]="'arrow-rotate-sync'" [width]="'14px'" [height]="'14px'" [isTextColorAllowed]="true" [styleClasses]="'tw-text-icon-tertiary'"></app-svg-icon> | |
| </div> | |
| <span *ngSwitchDefault class="tw-text-error-text tw-font-400 tw-text-14-px">Error loading data</span> | |
| </ng-container> |
| @@ -8,7 +8,7 @@ type QbdOnboardingStepperMap = { | |||
| [QbdDirectOnboardingState.CONNECTION]: number; | |||
| [QbdDirectOnboardingState.PENDING_QWC_UPLOAD]: number; | |||
| [QbdDirectOnboardingState.INCORRECT_COMPANY_PATH]: number; | |||
| [QbdDirectOnboardingState.INCORRECT_PASSWORD]: number; | |||
| [QbdDirectOnboardingState.IN_CORRECT_PASSWORD]: number; | |||
There was a problem hiding this comment.
Regression: Incorrect naming convention used for password state
The renaming from INCORRECT_PASSWORD to IN_CORRECT_PASSWORD introduces a grammatically incorrect term. "Incorrect" should be one word, not two words separated by an underscore.
Apply this diff to maintain proper naming conventions:
- [QbdDirectOnboardingState.IN_CORRECT_PASSWORD]: number;
+ [QbdDirectOnboardingState.INCORRECT_PASSWORD]: number;
- [QbdDirectOnboardingState.IN_CORRECT_PASSWORD]: 2,
+ [QbdDirectOnboardingState.INCORRECT_PASSWORD]: 2,Also applies to: 72-72
| @@ -48,7 +48,7 @@ export class QbdDirectComponent implements OnInit { | |||
| [QbdDirectOnboardingState.CONNECTION]: '/integrations/qbd_direct/onboarding/connector', | |||
| [QbdDirectOnboardingState.PENDING_QWC_UPLOAD]: '/integrations/qbd_direct/onboarding/connector', | |||
| [QbdDirectOnboardingState.INCORRECT_COMPANY_PATH]: '/integrations/qbd_direct/onboarding/connector', | |||
| [QbdDirectOnboardingState.INCORRECT_PASSWORD]: '/integrations/qbd_direct/onboarding/connector', | |||
| [QbdDirectOnboardingState.IN_CORRECT_PASSWORD]: '/integrations/qbd_direct/onboarding/connector', | |||
There was a problem hiding this comment.
Grammatically incorrect enum value name
The enum value IN_CORRECT_PASSWORD is grammatically incorrect. It should be INCORRECT_PASSWORD (one word). This appears to be a regression since this PR is reverting changes from PR #1066.
- [QbdDirectOnboardingState.IN_CORRECT_PASSWORD]: '/integrations/qbd_direct/onboarding/connector',
+ [QbdDirectOnboardingState.INCORRECT_PASSWORD]: '/integrations/qbd_direct/onboarding/connector',Committable suggestion skipped: line range outside the PR's diff.
| <button pButton pRipple label="Download" class="downloadBtn"></button> | ||
| </div> |
There was a problem hiding this comment.
Add missing click handler for download button
The download button is missing its click handler, making it non-functional. This appears to be a regression from the previous version.
Add the click handler:
- <button pButton pRipple label="Download" class="downloadBtn"></button>
+ <button pButton pRipple label="Download" class="downloadBtn" (click)="onDownloadClick()" [disabled]="downloadFilePathField.invalid">Committable suggestion skipped: line range outside the PR's diff.
| required pattern="^(.+)\/([^\/]+)$" | ||
| #downloadFilePathField="ngModel" | ||
| [ngClass]="{ | ||
| 'error-box': (downloadFilePathField.invalid && downloadFilePathField.touched) || !isCompanyPathInvalid | ||
| 'error-box': downloadFilePathField.invalid && downloadFilePathField.touched | ||
| }"> | ||
| <div *ngIf="(downloadFilePathField.invalid && downloadFilePathField.touched) || !isCompanyPathInvalid" class="tw-text-alert-toast"> | ||
| <small *ngIf="downloadFilePathField.errors?.required || !isCompanyPathInvalid" class="tw-flex"> | ||
| <div *ngIf="downloadFilePathField.invalid && downloadFilePathField.touched" class="tw-text-alert-toast"> | ||
| <small *ngIf="downloadFilePathField.errors?.required || downloadFilePathField.errors?.pattern" class="tw-flex"> | ||
| <app-svg-icon [svgSource]="'info-circle-fill'" [isTextColorAllowed]="true" [width]="'16px'" [height]="'16px'" [styleClasses]="'tw-text-form-error-help-text-color'"></app-svg-icon> | ||
| Enter a valid company file path.</small> | ||
| </div> |
There was a problem hiding this comment.
Fix incorrect file path validation pattern
The current pattern ^(.+)\/([^\/]+)$ has several issues:
- It enforces forward slashes (
/) but QuickBooks Desktop runs on Windows which uses backslashes (\) - The pattern conflicts with the placeholder example
C:\Users\[Username]\Documents\QuickBooks - The error message doesn't explain the expected format
Apply these changes:
- pattern="^(.+)\/([^\/]+)$"
+ pattern="^[a-zA-Z]:\\(?:[^\\/:*?\"><|]+\\)*[^\\/:*?\"><|]*$"- <small *ngIf="downloadFilePathField.errors?.required || downloadFilePathField.errors?.pattern" class="tw-flex">
- <app-svg-icon [svgSource]="'info-circle-fill'" [isTextColorAllowed]="true" [width]="'16px'" [height]="'16px'" [styleClasses]="'tw-text-form-error-help-text-color'"></app-svg-icon>
- Enter a valid company file path.</small>
+ <small *ngIf="downloadFilePathField.errors?.required" class="tw-flex">
+ <app-svg-icon [svgSource]="'info-circle-fill'" [isTextColorAllowed]="true" [width]="'16px'" [height]="'16px'" [styleClasses]="'tw-text-form-error-help-text-color'"></app-svg-icon>
+ Company file path is required.</small>
+ <small *ngIf="downloadFilePathField.errors?.pattern" class="tw-flex">
+ <app-svg-icon [svgSource]="'info-circle-fill'" [isTextColorAllowed]="true" [width]="'16px'" [height]="'16px'" [styleClasses]="'tw-text-form-error-help-text-color'"></app-svg-icon>
+ Enter a valid Windows path (e.g., C:\Users\[Username]\Documents\QuickBooks)</small>📝 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.
| required pattern="^(.+)\/([^\/]+)$" | |
| #downloadFilePathField="ngModel" | |
| [ngClass]="{ | |
| 'error-box': (downloadFilePathField.invalid && downloadFilePathField.touched) || !isCompanyPathInvalid | |
| 'error-box': downloadFilePathField.invalid && downloadFilePathField.touched | |
| }"> | |
| <div *ngIf="(downloadFilePathField.invalid && downloadFilePathField.touched) || !isCompanyPathInvalid" class="tw-text-alert-toast"> | |
| <small *ngIf="downloadFilePathField.errors?.required || !isCompanyPathInvalid" class="tw-flex"> | |
| <div *ngIf="downloadFilePathField.invalid && downloadFilePathField.touched" class="tw-text-alert-toast"> | |
| <small *ngIf="downloadFilePathField.errors?.required || downloadFilePathField.errors?.pattern" class="tw-flex"> | |
| <app-svg-icon [svgSource]="'info-circle-fill'" [isTextColorAllowed]="true" [width]="'16px'" [height]="'16px'" [styleClasses]="'tw-text-form-error-help-text-color'"></app-svg-icon> | |
| Enter a valid company file path.</small> | |
| </div> | |
| required pattern="^[a-zA-Z]:\\(?:[^\\/:*?\"><|]+\\)*[^\\/:*?\"><|]*$" | |
| #downloadFilePathField="ngModel" | |
| [ngClass]="{ | |
| 'error-box': downloadFilePathField.invalid && downloadFilePathField.touched | |
| }"> | |
| <div *ngIf="downloadFilePathField.invalid && downloadFilePathField.touched" class="tw-text-alert-toast"> | |
| <small *ngIf="downloadFilePathField.errors?.required" class="tw-flex"> | |
| <app-svg-icon [svgSource]="'info-circle-fill'" [isTextColorAllowed]="true" [width]="'16px'" [height]="'16px'" [styleClasses]="'tw-text-form-error-help-text-color'"></app-svg-icon> | |
| Company file path is required.</small> | |
| <small *ngIf="downloadFilePathField.errors?.pattern" class="tw-flex"> | |
| <app-svg-icon [svgSource]="'info-circle-fill'" [isTextColorAllowed]="true" [width]="'16px'" [height]="'16px'" [styleClasses]="'tw-text-form-error-help-text-color'"></app-svg-icon> | |
| Enter a valid Windows path (e.g., C:\Users\[Username]\Documents\QuickBooks)</small> | |
| </div> |
| @@ -337,7 +337,7 @@ export enum QbdDirectOnboardingState { | |||
| CONFIRM_PRE_REQUISITES = 'CONFIRM_PRE_REQUISITES', | |||
| PENDING_QWC_UPLOAD = 'PENDING_QWC_UPLOAD', | |||
| INCORRECT_COMPANY_PATH = 'INCORRECT_COMPANY_PATH', | |||
| INCORRECT_PASSWORD = 'INCORRECT_PASSWORD', | |||
| IN_CORRECT_PASSWORD = 'IN_CORRECT_PASSWORD', | |||
There was a problem hiding this comment.
Revert incorrect enum value name change
The enum value has been changed from INCORRECT_PASSWORD to IN_CORRECT_PASSWORD, which:
- Splits a single word incorrectly
- Introduces inconsistent naming
- Makes the code less readable
- Doesn't follow TypeScript enum naming conventions
Apply this diff to fix the enum value:
- IN_CORRECT_PASSWORD = 'IN_CORRECT_PASSWORD',
+ INCORRECT_PASSWORD = 'INCORRECT_PASSWORD',This change should also be applied to the QBDConnectionStatus enum at the bottom of the file:
- IN_CORRECT_PASSWORD = 'IN_CORRECT_PASSWORD'
+ INCORRECT_PASSWORD = 'INCORRECT_PASSWORD'Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| @Component({ | ||
| selector: 'app-qbd-direct-onboarding-connector', | ||
| standalone: true, | ||
| imports: [QbdDirectSharedModule, SharedModule, CommonModule], | ||
| imports: [QbdDirectSharedModule, SharedModule], | ||
| templateUrl: './qbd-direct-onboarding-connector.component.html', | ||
| styleUrl: './qbd-direct-onboarding-connector.component.scss' | ||
| }) |
There was a problem hiding this comment.
Correct styleUrl to styleUrls
In the @Component decorator, the property styleUrl should be styleUrls (note the plural form) to properly reference the component's styles. Angular expects styleUrls to be an array.
Apply this diff to fix the issue:
templateUrl: './qbd-direct-onboarding-connector.component.html',
- styleUrl: './qbd-direct-onboarding-connector.component.scss'
+ styleUrls: ['./qbd-direct-onboarding-connector.component.scss']Committable suggestion skipped: line range outside the PR's diff.
Reverts #1066
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Chores