Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: show either import or export merchants in xero #1080

Merged

Conversation

JustARatherRidiculouslyLongUsername
Copy link
Contributor

@JustARatherRidiculouslyLongUsername JustARatherRidiculouslyLongUsername commented Nov 21, 2024

Description

We have two fields in Xero that control the import and export of Merchants:

In import settings
image

In advanced settings
image

With this change, only one of the two fields can be enabled at the same time. If one is enabled, the other will be hidden.

Clickup

https://app.clickup.com/t/86cx49er1

Summary by CodeRabbit

  • New Features
    • Introduced a new setting for auto-creating merchant destination entities in Xero workspace settings.
  • Improvements
    • Updated the conditions for displaying the toggle for corporate credit card expenses in advanced settings.
    • Simplified the display logic for the vendor import toggle in import settings, now considering the new auto-create setting.

If "Auto Create Fyle Merchants as Contacts on Xero" is set in advanced settings, hide the "Import Suppliers from Xero as Merchants" field in import settings
If "Import Suppliers from Xero as Merchants" is set in import settings, hide the "Auto Create Fyle Merchants as Contacts on Xero" field in advanced settings
Copy link
Contributor

coderabbitai bot commented Nov 21, 2024

Walkthrough

The changes in this pull request involve the addition of a new boolean property, auto_create_merchant_destination_entity, to the XeroWorkspaceGeneralSetting type. This modification is reflected in the xero-workspace-general-setting.model.ts file. Additionally, the conditional rendering logic in two HTML component files, xero-advanced-settings.component.html and xero-import-settings.component.html, has been updated to incorporate checks for this new property, affecting the visibility of certain UI elements based on the workspace settings.

Changes

File Change Summary
src/app/core/models/xero/db/xero-workspace-general-setting.model.ts Added property auto_create_merchant_destination_entity: boolean; to XeroWorkspaceGeneralSetting type.
src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.html Updated conditional rendering for <app-configuration-toggle-field> to include check for !workspaceGeneralSettings.import_suppliers_as_merchants.
src/app/integrations/xero/xero-shared/xero-import-settings/xero-import-settings.component.html Modified conditions for displaying vendor import toggle field to include check for !workspaceGeneralSettings.auto_create_merchant_destination_entity.

Possibly related PRs

  • feat: add split expense grouping config to xero export settings #1071: This PR adds a new property split_expense_grouping to the XeroExportSettingPost type, which is directly related to the new property auto_create_merchant_destination_entity added in the main PR, as both pertain to the configuration of settings in the Xero integration.
  • feat: support split expense grouping for Xero #1075: This PR enhances the XeroExportSettingsModel to include the split_expense_grouping property, which is relevant to the changes in the main PR that introduce a new property in the Xero workspace settings, indicating a broader context of managing expense grouping configurations.

Suggested labels

size/S

Suggested reviewers

  • DhaaraniCIT
  • ashwin1111

🐰 In the meadow, changes sprout,
A toggle here, a flag without doubt.
With settings new, we hop along,
To make our Xero workspace strong!
So let’s embrace this little feat,
For every change, our work's complete! 🌼


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
src/app/core/models/xero/db/xero-workspace-general-setting.model.ts (1)

14-14: Add documentation for merchant-related flags.

Consider adding JSDoc comments to clarify:

  1. The purpose of auto_create_merchant_destination_entity
  2. Its relationship with other merchant-related flags (map_merchant_to_contact, auto_create_destination_entity, import_suppliers_as_merchants)
  3. Which combinations of these flags are valid/invalid

Example:

/**
 * Controls automatic creation of merchants in Xero.
 * When enabled:
 * - import_suppliers_as_merchants must be false
 * - Used in advanced settings for merchant creation
 */
auto_create_merchant_destination_entity: boolean;
src/app/integrations/xero/xero-shared/xero-import-settings/xero-import-settings.component.html (1)

52-56: Improve readability of complex conditional logic

The conditional logic correctly implements the PR objective by hiding the supplier import section when merchant auto-creation is enabled. However, the multi-line condition could be more readable.

Consider extracting the condition to a component property:

- *ngIf="brandingFeatureConfig.featureFlags.importSettings.importVendorsAsMerchants 
-     && workspaceGeneralSettings.corporate_credit_card_expenses_object
-     && !workspaceGeneralSettings.auto_create_merchant_destination_entity"
+ *ngIf="showImportSuppliersSection"

Add to your component:

get showImportSuppliersSection(): boolean {
  return this.brandingFeatureConfig.featureFlags.importSettings.importVendorsAsMerchants 
    && this.workspaceGeneralSettings.corporate_credit_card_expenses_object
    && !this.workspaceGeneralSettings.auto_create_merchant_destination_entity;
}

This would:

  1. Make the template cleaner
  2. Move the business logic to the component
  3. Make the condition reusable and easier to test
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 86da20f and a9b9518.

📒 Files selected for processing (3)
  • src/app/core/models/xero/db/xero-workspace-general-setting.model.ts (1 hunks)
  • src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.html (1 hunks)
  • src/app/integrations/xero/xero-shared/xero-import-settings/xero-import-settings.component.html (1 hunks)
🔇 Additional comments (3)
src/app/core/models/xero/db/xero-workspace-general-setting.model.ts (1)

14-14: Verify mutual exclusivity implementation.

The PR objectives state that only one of two fields should be enabled at a time. Let's verify the implementation of this constraint.

✅ Verification successful

Mutual exclusivity is correctly implemented

Based on the code analysis, the mutual exclusivity between auto_create_merchant_destination_entity and import_suppliers_as_merchants is properly enforced through template conditions:

  1. In xero-advanced-settings.component.html, the merchant auto-creation toggle is shown only when:
*ngIf="workspaceGeneralSettings.corporate_credit_card_expenses_object && !workspaceGeneralSettings.import_suppliers_as_merchants"
  1. In xero-import-settings.component.html, the import suppliers toggle is shown only when:
*ngIf="brandingFeatureConfig.featureFlags.importSettings.importVendorsAsMerchants 
    && workspaceGeneralSettings.corporate_credit_card_expenses_object
    && !workspaceGeneralSettings.auto_create_merchant_destination_entity"

These template conditions ensure that:

  • When auto_create_merchant_destination_entity is true, the import suppliers toggle is hidden
  • When import_suppliers_as_merchants is true, the merchant auto-creation toggle is hidden
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find the component/service files that enforce mutual exclusivity between merchant settings

# Look for files handling these settings
echo "Files potentially handling merchant settings:"
rg -l "auto_create_merchant_destination_entity|import_suppliers_as_merchants" --type ts

# Look for validation logic
echo -e "\nValidation logic:"
rg -B2 -A2 "auto_create_merchant_destination_entity.*import_suppliers_as_merchants|import_suppliers_as_merchants.*auto_create_merchant_destination_entity" --type ts

Length of output: 703


Script:

#!/bin/bash
# Let's examine the implementation in these files more closely

# First, let's look at the model definitions
echo "=== Model Definitions ==="
echo -e "\nxero-workspace-general-setting.model.ts:"
cat src/app/core/models/xero/db/xero-workspace-general-setting.model.ts

echo -e "\nxero-advanced-settings.model.ts:"
cat src/app/core/models/xero/xero-configuration/xero-advanced-settings.model.ts

echo -e "\nxero-import-settings.model.ts:"
cat src/app/core/models/xero/xero-configuration/xero-import-settings.model.ts

# Let's also search for any form components or services that might handle these settings
echo -e "\n=== Form Handling ==="
rg -A 5 "FormGroup.*auto_create_merchant_destination_entity|FormGroup.*import_suppliers_as_merchants" --type ts

# Look for any toggle or change handlers
echo -e "\n=== Change Handlers ==="
rg -B 2 -A 5 "onChange.*auto_create_merchant_destination_entity|onChange.*import_suppliers_as_merchants" --type ts

Length of output: 14742


Script:

#!/bin/bash
# Let's look for component files that might handle these settings
echo "=== Component Files ==="
# First, find any component files that might be related to these settings
fd -e component.ts -e component.html | xargs rg -l "auto_create_merchant_destination_entity|import_suppliers_as_merchants"

# Let's also check for any template files with these controls
echo -e "\n=== Template Usage ==="
rg -A 5 -B 2 '\[(ng|form)]Control.*=.*("|\').*auto_create_merchant_destination_entity|import_suppliers_as_merchants' --type html

# Check for any dependencies or watchers in component files
echo -e "\n=== Dependencies and Watchers ==="
rg -A 5 "valueChanges.*subscribe.*auto_create_merchant_destination_entity|valueChanges.*subscribe.*import_suppliers_as_merchants" --type ts

Length of output: 748


Script:

#!/bin/bash
# Let's examine the template files we found
echo "=== Xero Import Settings Template ==="
cat src/app/integrations/xero/xero-shared/xero-import-settings/xero-import-settings.component.html

echo -e "\n=== Xero Advanced Settings Template ==="
cat src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.html

# Let's also check the component files for any logic
echo -e "\n=== Import Settings Component ==="
cat src/app/integrations/xero/xero-shared/xero-import-settings/xero-import-settings.component.ts

echo -e "\n=== Advanced Settings Component ==="
cat src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.ts

Length of output: 32795

src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.html (1)

Line range hint 95-102: LGTM! The conditional rendering aligns with the PR objective.

The additional condition !workspaceGeneralSettings.import_suppliers_as_merchants ensures that the auto-create merchants toggle is hidden when importing suppliers as merchants is enabled, preventing both options from being active simultaneously.

Let's verify the interaction between these two settings:

✅ Verification successful

The mutual exclusivity between merchant settings is properly implemented

The verification confirms that both settings (import_suppliers_as_merchants and auto_create_merchant_destination_entity) are correctly handled:

  • In xero-advanced-settings.component.html: The auto-create merchants toggle is hidden when import_suppliers_as_merchants is enabled
  • In xero-import-settings.component.html: The import suppliers toggle is hidden when auto_create_merchant_destination_entity is enabled
  • Both settings are properly defined in the workspace general settings model and form controls
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the mutual exclusivity of merchant settings
# Look for any other places where these settings might interact

# Search for references to import_suppliers_as_merchants
rg "import_suppliers_as_merchants" -A 5

# Search for references to auto_create_merchant_destination_entity
rg "auto_create_merchant_destination_entity" -A 5

# Look for any component logic handling these settings
ast-grep --pattern 'class $_ {
  $$$
  workspaceGeneralSettings: $_ {
    $$$
    import_suppliers_as_merchants: $_
    $$$
  }
  $$$
}'

Length of output: 10197

src/app/integrations/xero/xero-shared/xero-import-settings/xero-import-settings.component.html (1)

52-56: Verify mutual exclusivity implementation

Let's ensure the mutual exclusivity between merchant import and export settings is properly implemented.

✅ Verification successful

Based on the code examination, I can now provide a final response regarding the mutual exclusivity implementation:

Mutual exclusivity between merchant settings is properly implemented

The code shows proper implementation of mutual exclusivity between merchant import and export settings:

  • In import settings (xero-import-settings.component.html), the merchant import option is only shown when:

    *ngIf="brandingFeatureConfig.featureFlags.importSettings.importVendorsAsMerchants 
        && workspaceGeneralSettings.corporate_credit_card_expenses_object
        && !workspaceGeneralSettings.auto_create_merchant_destination_entity"
  • In advanced settings (xero-advanced-settings.component.html), the merchant export option is only shown when:

    *ngIf="workspaceGeneralSettings.corporate_credit_card_expenses_object && !workspaceGeneralSettings.import_suppliers_as_merchants"

The implementation ensures that:

  • Both settings cannot be enabled simultaneously
  • The options are only available for corporate card expenses
  • The UI properly reflects these constraints through conditional rendering
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of merchant settings mutual exclusivity

# Check for the corresponding export setting in advanced settings
echo "Checking advanced settings component:"
rg -A 10 "auto_create_merchant_destination_entity" src/app/integrations/xero/xero-shared/xero-advanced-settings/

# Check for proper handling in the component
echo "Checking component implementation:"
rg -A 10 "importSuppliersAsMerchants" src/app/integrations/xero/xero-shared/xero-import-settings/xero-import-settings.component.ts

# Verify form handling
echo "Checking form controls:"
rg "FormControl.*importSuppliersAsMerchants" src/app/integrations/xero/xero-shared/xero-import-settings/

Length of output: 571


Script:

#!/bin/bash
# Let's try with different patterns and file paths

# Check for any references to merchant-related settings in advanced settings
echo "Checking advanced settings for merchant-related configurations:"
rg -l "merchant" src/app/integrations/xero/xero-shared/xero-advanced-settings/

# Check the component class and template for merchant-related logic
echo "Checking import settings component for merchant-related logic:"
rg -l "merchant" src/app/integrations/xero/xero-shared/xero-import-settings/

# If files are found, let's examine their content
echo "Examining content of found files:"
for file in $(rg -l -i "merchant" src/app/integrations/xero/xero-shared/); do
    echo "=== Content of $file ==="
    cat "$file"
    echo
done

# Check for any workspace settings interface definitions
echo "Checking workspace settings interface:"
rg -l "workspaceGeneralSettings" src/app/integrations/xero/

# Look for vendor/merchant import configurations
echo "Checking for vendor/merchant import configurations:"
rg -l "importVendorsAsMerchants" src/app/integrations/xero/

Length of output: 16342


Script:

#!/bin/bash
# Let's examine the component TypeScript files and interfaces to understand the implementation

# Check the import settings component implementation
echo "=== Import Settings Component Implementation ==="
cat src/app/integrations/xero/xero-shared/xero-import-settings/xero-import-settings.component.ts

# Check the advanced settings component implementation
echo "=== Advanced Settings Component Implementation ==="
cat src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.ts

# Look for workspace settings interface
echo "=== Searching for Workspace Settings Interface ==="
rg -A 20 "interface.*WorkspaceGeneralSettings" src/app/integrations/xero/

# Check for any service handling these settings
echo "=== Checking Services ==="
rg -l "workspaceGeneralSettings|importSuppliersAsMerchants|auto_create_merchant_destination_entity" src/app/integrations/xero/xero-shared/services/

Length of output: 19014

Copy link

Unit Test Coverage % values
Statements 35.3% ( 4221 / 11956 )
Branches 28.85% ( 1224 / 4242 )
Functions 27.91% ( 934 / 3346 )
Lines 35.43% ( 4147 / 11703 )

@JustARatherRidiculouslyLongUsername JustARatherRidiculouslyLongUsername merged commit 5aacc1a into master Nov 22, 2024
5 checks passed
JustARatherRidiculouslyLongUsername added a commit that referenced this pull request Nov 22, 2024
* fix: hide import merchants if export merchants is set

If "Auto Create Fyle Merchants as Contacts on Xero" is set in advanced settings, hide the "Import Suppliers from Xero as Merchants" field in import settings

* fix: hide export merchants if import merchants is set

If "Import Suppliers from Xero as Merchants" is set in import settings, hide the "Auto Create Fyle Merchants as Contacts on Xero" field in advanced settings

(cherry picked from commit 5aacc1a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Extra Small PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants