fix: update login error flow and fix redirect url #1117
Conversation
WalkthroughThe changes in this pull request primarily involve renaming and updating visibility conditions in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/app/integrations/business-central/business-central-onboarding/business-central-onboarding-landing/business-central-onboarding-landing.component.html (1)
Line range hint
9-15: Consider enhancing the error message clarityWhile the error message is functional, it could be more specific about what constitutes a "different" account and what "same" means in this context.
Consider updating the context text to be more specific:
- [contextText]="'You had previously set up the integration with a different Dynamic 365 Business Central account. Please choose the same to restore the settings'" + [contextText]="'This integration was previously set up with a different Dynamics 365 Business Central organization/company. Please sign in with the same organization/company account to restore your integration settings'"src/app/integrations/business-central/business-central-onboarding/business-central-onboarding-landing/business-central-onboarding-landing.component.ts (3)
48-51: Consider using kebab-case for route pathsThe route path uses underscore ('business_central') which is inconsistent with Angular's convention of using kebab-case for routes.
- this.router.navigate([`/integrations/business_central/onboarding/landing`]); + this.router.navigate(['/integrations/business-central/onboarding/landing']);Also, consider destructuring the
hasAcceptedproperty from thedataparameter for better type safety:- acceptWarning(data: ConfigurationWarningOut): void { - this.isIncorrectBCConnectedDialogVisible = false; - if (data.hasAccepted) { + acceptWarning({ hasAccepted }: ConfigurationWarningOut): void { + this.isIncorrectBCConnectedDialogVisible = false; + if (hasAccepted) {
Line range hint
67-80: Improve error handling type safetyThe error handling could benefit from more specific typing. Consider creating an interface for the error response:
interface BusinessCentralError { message: string; code?: string; // Add other relevant error fields } // Then update the error handling: }, (error: { error: BusinessCentralError }) => { const errorMessage = error.error?.message ?? 'Failed to connect to Dynamic 365 Business Central. Please try again';
Consider using error constants and standardize error handling
The error message 'Please choose the correct Dynamic 365 Business Central account' is used in multiple components but isn't defined as a constant. Additionally, there are inconsistencies in error message handling across the Business Central integration:
business-central-onboarding-landing.component.ts: Direct string comparisonbusiness-central-onboarding-connector.component.ts: Direct string comparison- Other components use different error message formats (e.g., "Failed to connect to Dynamic 365 Business Central...")
Create a shared error constants file for Business Central integration:
// business-central-error.constants.ts export const BC_ERRORS = { INCORRECT_ACCOUNT: 'Please choose the correct Dynamic 365 Business Central account', CONNECTION_FAILED: 'Failed to connect to Dynamic 365 Business Central. Please try again' };🔗 Analysis chain
Line range hint
75-80: Consider using error codes instead of string comparisonUsing hardcoded error message strings for flow control can be fragile. Consider using error codes or constants instead.
+ private readonly ERROR_INCORRECT_BC_ACCOUNT = 'Please choose the correct Dynamic 365 Business Central account'; + // In error handling - if (errorMessage === 'Please choose the correct Dynamic 365 Business Central account') { + if (errorMessage === this.ERROR_INCORRECT_BC_ACCOUNT) {Let's verify the consistency of this error message across the codebase:
Let's also check the service layer where this error might originate:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for variations of the Business Central account error message rg -i "choose.*business.*central.*account" --type ts rg -i "correct.*business.*central.*account" --type tsLength of output: 1102
Script:
#!/bin/bash # Let's check if there's a shared constant or enum for this error message ast-grep --pattern 'const ERROR_INCORRECT_BC_ACCOUNT = "Please choose the correct Dynamic 365 Business Central account"' ast-grep --pattern 'enum $_ { $$$ }' rg -i "business.*central.*error" --type tsLength of output: 57392
Script:
#!/bin/bash # Look for service files related to Business Central fd -e ts "business-central.*service"Length of output: 1780
Script:
#!/bin/bash # Let's check the business central auth service for error handling cat src/app/core/services/business-central/business-central-core/business-central-auth.service.ts # Also check the connector service where this error might originate cat src/app/core/services/business-central/business-central-configuration/business-central-connector.service.tsLength of output: 3001
Script:
#!/bin/bash # Let's check if this error message is defined as a constant anywhere in the codebase rg -i "const.*ERROR_INCORRECT_BC_ACCOUNT" --type ts # Also check if there are any error message constants defined in any business central related files fd -e ts "business-central" -x cat {} | grep -i "const.*error.*="Length of output: 403
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/integrations/business-central/business-central-onboarding/business-central-onboarding-landing/business-central-onboarding-landing.component.html(1 hunks)src/app/integrations/business-central/business-central-onboarding/business-central-onboarding-landing/business-central-onboarding-landing.component.ts(3 hunks)
🔇 Additional comments (3)
src/app/integrations/business-central/business-central-onboarding/business-central-onboarding-landing/business-central-onboarding-landing.component.html (2)
1-1: LGTM! Correctly renamed QBO reference to BC
The visibility condition has been properly updated to use Business Central terminology while maintaining the correct negation logic.
2-8: Verify routing paths consistency
The template contains several routing paths and integration-specific values. Let's verify their consistency across the codebase.
✅ Verification successful
Routing paths and integration assets are consistent
The verification shows that:
- The routing path
business_central/onboarding/connectoris consistently used across components and services - The Business Central logo asset path
assets/logos/BusinessCentral-logo.svgis correctly referenced - The navigation flow is properly maintained in the token guard, onboarding service, and component implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify routing paths and integration names consistency
# Check for routing path references
rg -A 2 "business_central/onboarding/connector"
# Check for Business Central related constants and configurations
rg "BusinessCentral-logo\.svg"
Length of output: 4845
src/app/integrations/business-central/business-central-onboarding/business-central-onboarding-landing/business-central-onboarding-landing.component.ts (1)
36-36: LGTM! Property rename improves code clarity
The rename from isIncorrectQBOConnectedDialogVisible to isIncorrectBCConnectedDialogVisible better reflects its usage with Business Central integration.
|
JustARatherRidiculouslyLongUsername
merged commit 4973a8a
into
bc-beta-bugfixes
* fix: initialize chart of accounts multiselect when there is no api response (#1110) * fix: remove the posted at date option for ccc expenses grouped by report (#1105) * fix: update login error flow and fix redirect url (#1117) * fix: restrict JE modules to group by expense only (#1113) * fix: restrict JE modules to group by expense only * fix: add a default bank account field for CCC expenses (#1114) * fix: remove validation temporarily (#1111) * fix: add a default bank account field for CCC expenses * fix: add missing options to bank accounts on page init * fix: dynamic content for xero customize settings (#1112) * fix: update sublabel key to avoid build fail (#1116) * fix: Prod fixes of QBD direct (#1118) * fix bugs (#1119) * refactor: capitalization * fix: only ccc exports not being saved (#1121) --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]>
Clickup
https://app.clickup.com/t/86cxaatt3
Summary by CodeRabbit
New Features
Bug Fixes