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

feat: poc for date grouping #1174

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

feat: poc for date grouping #1174

wants to merge 10 commits into from

Conversation

DhaaraniCIT
Copy link
Contributor

@DhaaraniCIT DhaaraniCIT commented Feb 5, 2025

Description

feat: poc for date grouping

Clickup

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

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Enhanced expense export settings with additional date grouping options, including 'Export date', 'Verification date', 'Spend date', and others, providing more flexible selection choices.
    • Introduced new mock data for testing, improving coverage for expense grouping date options.
  • Refactor
    • Streamlined the logic for handling expense grouping date options, ensuring a consistent and simplified configuration experience for export settings.

Copy link
Contributor

coderabbitai bot commented Feb 5, 2025

Walkthrough

This pull request introduces new static methods in the ExportSettingModel to enhance functionality related to expense grouping date options. The methods getExpenseGroupingDateOptions and constructExportDateOptions are added to provide and filter date options based on various parameters. Additionally, the QBO export settings component has been updated to utilize these new methods, simplifying the handling of expense grouping date options and improving code maintainability. Mock data for testing has also been introduced to ensure proper validation of these changes.

Changes

File Changes Summary
src/app/core/models/common/export-settings.model.ts Added static methods: getExpenseGroupingDateOptions and constructExportDateOptions for managing expense grouping date options.
src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts Modified updateCCCExpenseGroupingDateOptions and setupCustomDateOptionWatchers to use ExportSettingModel.constructExportDateOptions for date options.
src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.spec.ts Added mock data: mockCCCExpenseDateGrouping and mockReimbursableExpenseDateGrouping; updated tests to validate new date options.
src/app/integrations/qbo/qbo.fixture.ts Introduced constants: mockCCCExpenseDateGrouping and mockReimbursableExpenseDateGrouping as arrays of SelectFormOption objects.

Suggested labels

size/XS

Suggested reviewers

  • ashwin1111
  • ruuushhh

Poem

I'm a little rabbit, hopping with delight,
New methods sprout in our code, oh what a sight!
Centralized logic in a rhythmic dance,
Date options now given a fair chance.
Cheers to clean code and a hoppin' good night! 🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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.

@github-actions github-actions bot added the size/M Medium PR label Feb 5, 2025
Copy link

github-actions bot commented Feb 5, 2025

PR description must contain a link to a ClickUp (case-insensitive)

1 similar comment
Copy link

github-actions bot commented Feb 5, 2025

PR description must contain a link to a ClickUp (case-insensitive)

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

🧹 Nitpick comments (3)
src/app/core/models/common/export-settings.model.ts (2)

112-135: Fix typo in function name.

The function name should be plural to match its return type.

Apply this diff to fix the typo:

-static getReimbursableExpenseGroupingDateOption(): SelectFormOption[] {
+static getReimbursableExpenseGroupingDateOptions(): SelectFormOption[] {

137-161: Clean up code style and improve type safety.

The function has the following issues:

  1. Inconsistent spacing
  2. Use of 'this' in static context
  3. Unnecessary type assertion in filter callback

Apply this diff to improve the code:

-static dateGrouping(exportType:string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
-      
-      const excludedDate = expenseGrouping === 'expense' 
-      ? ExportDateType.LAST_SPENT_AT 
-      : ExportDateType.SPENT_AT;
-      
-      if (exportType === 'CCC') {
-        return this.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
-      }
-
-      let dateOptions = this.getReimbursableExpenseGroupingDateOption();
-      let filterOptions: ExportDateType[] = [];
-
-      if (showApprovedDate) {
-        filterOptions.push(ExportDateType.VERIFIED_AT);
-      } else if (showVerificationDate) {
-        filterOptions.push(ExportDateType.APPROVED_AT);
-      }
-
-      filterOptions.push(excludedDate);
-      
-      return dateOptions.filter((item): item is { label: string; value: ExportDateType } =>
-        item.value !== null && !filterOptions.includes(item.value as ExportDateType))
+static dateGrouping(exportType: string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
+  const excludedDate = expenseGrouping === 'expense'
+    ? ExportDateType.LAST_SPENT_AT
+    : ExportDateType.SPENT_AT;
+
+  if (exportType === 'CCC') {
+    return ExportSettingModel.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
+  }
+
+  const dateOptions = ExportSettingModel.getReimbursableExpenseGroupingDateOptions();
+  const filterOptions: ExportDateType[] = [];
+
+  if (showApprovedDate) {
+    filterOptions.push(ExportDateType.VERIFIED_AT);
+  } else if (showVerificationDate) {
+    filterOptions.push(ExportDateType.APPROVED_AT);
+  }
+
+  filterOptions.push(excludedDate);
+
+  return dateOptions.filter(item => item.value !== null && !filterOptions.includes(item.value));
🧰 Tools
🪛 Biome (1.9.4)

[error] 144-144: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 147-147: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🪛 ESLint

[error] 138-138: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 139-139: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 140-140: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 142-142: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 147-147: 'dateOptions' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 148-148: 'filterOptions' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 157-157: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 159-160: Missing semicolon.

(semi)

🪛 GitHub Check: lint

[failure] 138-138:
Trailing spaces not allowed


[failure] 139-139:
Trailing spaces not allowed


[failure] 140-140:
Trailing spaces not allowed


[failure] 142-142:
Trailing spaces not allowed


[failure] 147-147:
'dateOptions' is never reassigned. Use 'const' instead


[failure] 148-148:
'filterOptions' is never reassigned. Use 'const' instead


[failure] 157-157:
Trailing spaces not allowed


[failure] 159-159:
Missing semicolon

🪛 GitHub Actions: TypeScript Lint Check

[error] 138-138: Trailing spaces not allowed

src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts (1)

344-359: Clean up commented code and fix semicolons.

  1. Remove commented code as it's tracked in version control.
  2. Add missing semicolons.

Apply this diff to clean up the code:

-      // this.reimbursableExpenseGroupingDateOptions = QBOExportSettingModel.getReimbursableExpenseGroupingDateOptions();
-      // this.reimbursableExpenseGroupingDateOptions = ExportSettingModel.constructGroupingDateOptions(reimbursableExportGroup, this.reimbursableExpenseGroupingDateOptions);
-      this.reimbursableExpenseGroupingDateOptions = ExportSettingModel.dateGrouping('rem', this.exportSettingForm.controls.reimbursableExportGroup.value, true, false)
+      this.reimbursableExpenseGroupingDateOptions = ExportSettingModel.dateGrouping('rem', this.exportSettingForm.controls.reimbursableExportGroup.value, true, false);

-    //     // this.cccExpenseGroupingDateOptions = QBOExportSettingModel.getReimbursableExpenseGroupingDateOptions();
-    //     // this.cccExpenseGroupingDateOptions = ExportSettingModel.constructGroupingDateOptions(creditCardExportGroup, this.cccExpenseGroupingDateOptions);
-        this.cccExpenseGroupingDateOptions = ExportSettingModel.dateGrouping('rem', creditCardExportGroup, true, false)
+        this.cccExpenseGroupingDateOptions = ExportSettingModel.dateGrouping('rem', creditCardExportGroup, true, false);
🧰 Tools
🪛 ESLint

[error] 344-344: Comments should not begin with a lowercase character.

(capitalized-comments)


[error] 345-345: Comments should not begin with a lowercase character.

(capitalized-comments)


[error] 346-347: Missing semicolon.

(semi)


[error] 358-359: Missing semicolon.

(semi)

🪛 GitHub Check: lint

[failure] 344-344:
Comments should not begin with a lowercase character


[failure] 345-345:
Comments should not begin with a lowercase character

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7688c10 and 7110f3a.

📒 Files selected for processing (2)
  • src/app/core/models/common/export-settings.model.ts (2 hunks)
  • src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts (2 hunks)
🧰 Additional context used
🪛 ESLint
src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts

[error] 344-344: Comments should not begin with a lowercase character.

(capitalized-comments)


[error] 345-345: Comments should not begin with a lowercase character.

(capitalized-comments)


[error] 346-347: Missing semicolon.

(semi)

src/app/core/models/common/export-settings.model.ts

[error] 138-138: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 139-139: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 140-140: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 142-142: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 147-147: 'dateOptions' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 148-148: 'filterOptions' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 157-157: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 159-160: Missing semicolon.

(semi)

🪛 GitHub Check: lint
src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts

[failure] 344-344:
Comments should not begin with a lowercase character


[failure] 345-345:
Comments should not begin with a lowercase character

src/app/core/models/common/export-settings.model.ts

[failure] 138-138:
Trailing spaces not allowed


[failure] 139-139:
Trailing spaces not allowed


[failure] 140-140:
Trailing spaces not allowed


[failure] 142-142:
Trailing spaces not allowed


[failure] 147-147:
'dateOptions' is never reassigned. Use 'const' instead


[failure] 148-148:
'filterOptions' is never reassigned. Use 'const' instead


[failure] 157-157:
Trailing spaces not allowed


[failure] 159-159:
Missing semicolon

🪛 Biome (1.9.4)
src/app/core/models/common/export-settings.model.ts

[error] 144-144: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 147-147: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🪛 GitHub Actions: TypeScript Lint Check
src/app/core/models/common/export-settings.model.ts

[error] 138-138: Trailing spaces not allowed

🔇 Additional comments (2)
src/app/core/models/common/export-settings.model.ts (1)

91-110: LGTM! Well-structured date options for credit card expenses.

The function provides a comprehensive set of date options for credit card expense grouping, with proper labels and values.

src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts (1)

318-320: LGTM! Consistent usage of the new date grouping method.

The method correctly utilizes ExportSettingModel.dateGrouping with appropriate parameters based on the export type.

Copy link

github-actions bot commented Feb 5, 2025

PR description must contain a link to a ClickUp (case-insensitive)

@DhaaraniCIT
Copy link
Contributor Author

Screen.Recording.2025-02-05.at.3.28.06.PM.mov

Copy link

github-actions bot commented Feb 5, 2025

PR description must contain a link to a ClickUp (case-insensitive)

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: 1

🧹 Nitpick comments (2)
src/app/core/models/common/export-settings.model.ts (2)

112-135: Rename method for consistency.

The method name should be plural to match other similar methods in the class.

-    static getReimbursableExpenseGroupingDateOption(): SelectFormOption[] {
+    static getReimbursableExpenseGroupingDateOptions(): SelectFormOption[] {

137-137: Improve type safety of parameters.

Consider using more specific types for the parameters:

-    static dateGrouping(exportType:string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
+    static dateGrouping(
+      exportType: 'CCC' | 'REIMBURSABLE',
+      expenseGrouping: ExpenseGroupingFieldOption,
+      showApprovedDate: boolean,
+      showVerificationDate: boolean
+    ): SelectFormOption[] {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7110f3a and 0642d91.

📒 Files selected for processing (1)
  • src/app/core/models/common/export-settings.model.ts (2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/models/common/export-settings.model.ts

[error] 144-144: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 147-147: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🪛 ESLint
src/app/core/models/common/export-settings.model.ts

[error] 138-138: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 140-140: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 142-142: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 147-147: 'dateOptions' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 148-148: 'filterOptions' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 158-158: Unexpected console statement.

(no-console)


[error] 158-159: Missing semicolon.

(semi)


[error] 161-162: Missing semicolon.

(semi)


[error] 163-163: Unexpected console statement.

(no-console)


[error] 163-164: Missing semicolon.

(semi)


[error] 164-164: Trailing spaces not allowed.

(no-trailing-spaces)

🪛 GitHub Check: lint
src/app/core/models/common/export-settings.model.ts

[failure] 138-138:
Trailing spaces not allowed


[failure] 140-140:
Trailing spaces not allowed


[failure] 142-142:
Trailing spaces not allowed


[failure] 147-147:
'dateOptions' is never reassigned. Use 'const' instead


[failure] 148-148:
'filterOptions' is never reassigned. Use 'const' instead


[failure] 158-158:
Unexpected console statement


[failure] 158-158:
Missing semicolon


[failure] 161-161:
Missing semicolon


[failure] 163-163:
Unexpected console statement


[failure] 163-163:
Missing semicolon

🪛 GitHub Actions: TypeScript Lint Check
src/app/core/models/common/export-settings.model.ts

[error] 138-138: Trailing spaces not allowed

🔇 Additional comments (1)
src/app/core/models/common/export-settings.model.ts (1)

91-110: LGTM! Well-structured implementation of credit card date options.

The method provides a clear and consistent set of date options for credit card expenses.

Comment on lines 137 to 167
static dateGrouping(exportType:string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {

const excludedDate = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
? ExportDateType.LAST_SPENT_AT
: ExportDateType.SPENT_AT;

if (exportType === 'CCC') {
return this.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
}

let dateOptions = this.getReimbursableExpenseGroupingDateOption();
let filterOptions: ExportDateType[] = [];

if (showApprovedDate) {
filterOptions.push(ExportDateType.VERIFIED_AT);
} else if (showVerificationDate) {
filterOptions.push(ExportDateType.APPROVED_AT);
}

filterOptions.push(excludedDate);

console.log(filterOptions, excludedDate)

const d = dateOptions.filter((item): item is { label: string; value: ExportDateType } =>
item.value !== null && !filterOptions.includes(item.value as ExportDateType))

console.log(d)

return d;

}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Clean up implementation and improve type safety.

The method has several issues that need to be addressed:

     static dateGrouping(exportType:string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
-      
       const excludedDate = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
-      ? ExportDateType.LAST_SPENT_AT 
+      ? ExportDateType.LAST_SPENT_AT
       : ExportDateType.SPENT_AT;
-      
+
       if (exportType === 'CCC') {
-        return this.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
+        return ExportSettingModel.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
       }

-      let dateOptions = this.getReimbursableExpenseGroupingDateOption();
-      let filterOptions: ExportDateType[] = [];
+      const dateOptions = ExportSettingModel.getReimbursableExpenseGroupingDateOptions();
+      const filterOptions: ExportDateType[] = [];

       if (showApprovedDate) {
         filterOptions.push(ExportDateType.VERIFIED_AT);
       } else if (showVerificationDate) {
         filterOptions.push(ExportDateType.APPROVED_AT);
       }

       filterOptions.push(excludedDate);

-      console.log(filterOptions, excludedDate)
-
-      const d = dateOptions.filter((item): item is { label: string; value: ExportDateType } =>
-        item.value !== null && !filterOptions.includes(item.value as ExportDateType))
-
-      console.log(d)
-      
-      return d;
+      return dateOptions.filter((item): item is { label: string; value: ExportDateType } =>
+        item.value !== null && !filterOptions.includes(item.value as ExportDateType));
     }

Changes made:

  1. Replaced this with class name in static context
  2. Removed debug console.log statements
  3. Fixed code style issues (trailing spaces, semicolons)
  4. Used const for variables that aren't reassigned
  5. Improved variable naming
📝 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.

Suggested change
static dateGrouping(exportType:string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
const excludedDate = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
? ExportDateType.LAST_SPENT_AT
: ExportDateType.SPENT_AT;
if (exportType === 'CCC') {
return this.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
}
let dateOptions = this.getReimbursableExpenseGroupingDateOption();
let filterOptions: ExportDateType[] = [];
if (showApprovedDate) {
filterOptions.push(ExportDateType.VERIFIED_AT);
} else if (showVerificationDate) {
filterOptions.push(ExportDateType.APPROVED_AT);
}
filterOptions.push(excludedDate);
console.log(filterOptions, excludedDate)
const d = dateOptions.filter((item): item is { label: string; value: ExportDateType } =>
item.value !== null && !filterOptions.includes(item.value as ExportDateType))
console.log(d)
return d;
}
static dateGrouping(exportType:string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
const excludedDate = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
? ExportDateType.LAST_SPENT_AT
: ExportDateType.SPENT_AT;
if (exportType === 'CCC') {
return ExportSettingModel.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
}
const dateOptions = ExportSettingModel.getReimbursableExpenseGroupingDateOptions();
const filterOptions: ExportDateType[] = [];
if (showApprovedDate) {
filterOptions.push(ExportDateType.VERIFIED_AT);
} else if (showVerificationDate) {
filterOptions.push(ExportDateType.APPROVED_AT);
}
filterOptions.push(excludedDate);
return dateOptions.filter((item): item is { label: string; value: ExportDateType } =>
item.value !== null && !filterOptions.includes(item.value as ExportDateType));
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 144-144: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 147-147: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🪛 ESLint

[error] 138-138: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 140-140: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 142-142: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 147-147: 'dateOptions' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 148-148: 'filterOptions' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 158-158: Unexpected console statement.

(no-console)


[error] 158-159: Missing semicolon.

(semi)


[error] 161-162: Missing semicolon.

(semi)


[error] 163-163: Unexpected console statement.

(no-console)


[error] 163-164: Missing semicolon.

(semi)


[error] 164-164: Trailing spaces not allowed.

(no-trailing-spaces)

🪛 GitHub Check: lint

[failure] 138-138:
Trailing spaces not allowed


[failure] 140-140:
Trailing spaces not allowed


[failure] 142-142:
Trailing spaces not allowed


[failure] 147-147:
'dateOptions' is never reassigned. Use 'const' instead


[failure] 148-148:
'filterOptions' is never reassigned. Use 'const' instead


[failure] 158-158:
Unexpected console statement


[failure] 158-158:
Missing semicolon


[failure] 161-161:
Missing semicolon


[failure] 163-163:
Unexpected console statement


[failure] 163-163:
Missing semicolon

🪛 GitHub Actions: TypeScript Lint Check

[error] 138-138: Trailing spaces not allowed

static getCreditCardExpenseGroupingDateOptions(): SelectFormOption[] {
return [
{
label: 'Card Transaction Post Date',
Copy link
Contributor

Choose a reason for hiding this comment

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

Card transaction post date

value: ExportDateType.SPENT_AT
},
{
label: 'Last Spend date',
Copy link
Contributor

Choose a reason for hiding this comment

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

Last spend date

value: ExportDateType.LAST_SPENT_AT
},
{
label: brandingContent.common.currentDate,
Copy link
Contributor

Choose a reason for hiding this comment

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

Export date

static getReimbursableExpenseGroupingDateOption(): SelectFormOption[] {
return [
{
label: brandingContent.common.currentDate,
Copy link
Contributor

Choose a reason for hiding this comment

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

Export date

value: ExportDateType.APPROVED_AT
},
{
label: 'Last Spend date',
Copy link
Contributor

Choose a reason for hiding this comment

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

Last spend date

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: 1

♻️ Duplicate comments (1)
src/app/core/models/common/export-settings.model.ts (1)

102-102: ⚠️ Potential issue

Fix typo in label text.

There's a typo in the label text: "Last dpend date" should be "Last spend date".

-          label: 'Last dpend date',
+          label: 'Last spend date',
🧹 Nitpick comments (1)
src/app/core/models/common/export-settings.model.ts (1)

112-112: Maintain consistent method naming.

The method name should be plural "Options" to maintain consistency with other similar methods in the class (e.g., getCreditCardExpenseGroupingDateOptions).

-    static getReimbursableExpenseGroupingDateOption(): SelectFormOption[] {
+    static getReimbursableExpenseGroupingDateOptions(): SelectFormOption[] {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0642d91 and 69cd298.

📒 Files selected for processing (1)
  • src/app/core/models/common/export-settings.model.ts (2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/models/common/export-settings.model.ts

[error] 145-145: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 149-149: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🪛 ESLint
src/app/core/models/common/export-settings.model.ts

[error] 139-139: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 140-140: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 142-142: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 147-147: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 150-150: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 157-157: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 159-159: Trailing spaces not allowed.

(no-trailing-spaces)

🪛 GitHub Check: lint
src/app/core/models/common/export-settings.model.ts

[failure] 139-139:
Trailing spaces not allowed


[failure] 140-140:
Trailing spaces not allowed


[failure] 142-142:
Trailing spaces not allowed


[failure] 147-147:
Trailing spaces not allowed


[failure] 150-150:
Trailing spaces not allowed


[failure] 157-157:
Trailing spaces not allowed


[failure] 159-159:
Trailing spaces not allowed

🪛 GitHub Actions: TypeScript Lint Check
src/app/core/models/common/export-settings.model.ts

[error] 139-139: Trailing spaces not allowed.

Comment on lines 137 to 162
static dateGrouping(exportType: string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
// Determine the excluded date based on expenseGrouping
const excludedDate = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
? ExportDateType.LAST_SPENT_AT
: ExportDateType.SPENT_AT;

// Handle CCC export type
if (exportType === 'CCC') {
return this.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
}

// Get base date options
const dateOptions = this.getReimbursableExpenseGroupingDateOption();

// Determine filter options based on showApprovedDate and showVerificationDate
const filterOptions = [
...(showApprovedDate ? [ExportDateType.VERIFIED_AT] : []),
...(showVerificationDate ? [ExportDateType.APPROVED_AT] : []),
excludedDate
];

// Filter out excluded and unwanted dates
return dateOptions.filter(option =>
option.value !== null && !filterOptions.includes(option.value as ExportDateType)
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve type safety and code style.

The method has several issues that need to be addressed:

  1. Using this in static context
  2. Code formatting issues
  3. Type safety for exportType parameter

Apply these changes:

-    static dateGrouping(exportType: string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
+    static dateGrouping(
+      exportType: 'CCC' | 'REIMBURSABLE',
+      expenseGrouping: string,
+      showApprovedDate: boolean,
+      showVerificationDate: boolean
+    ): SelectFormOption[] {
       const excludedDate = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
-        ? ExportDateType.LAST_SPENT_AT 
+        ? ExportDateType.LAST_SPENT_AT
         : ExportDateType.SPENT_AT;

       if (exportType === 'CCC') {
-        return this.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
+        return ExportSettingModel.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
       }

       const dateOptions = this.getReimbursableExpenseGroupingDateOption();
+      // Filter out excluded and unwanted dates based on conditions
       const filterOptions = [
         ...(showApprovedDate ? [ExportDateType.VERIFIED_AT] : []),
         ...(showVerificationDate ? [ExportDateType.APPROVED_AT] : []),
         excludedDate
       ];

-      return dateOptions.filter(option => 
-        option.value !== null && !filterOptions.includes(option.value as ExportDateType)
-      );
+      return dateOptions.filter(option =>
+        option.value !== null && !filterOptions.includes(option.value as ExportDateType));
     }

Changes made:

  1. Added union type for exportType parameter
  2. Replaced this with class name in static context
  3. Fixed code formatting issues
  4. Added explanatory comment for filter logic
📝 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.

Suggested change
static dateGrouping(exportType: string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
// Determine the excluded date based on expenseGrouping
const excludedDate = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
? ExportDateType.LAST_SPENT_AT
: ExportDateType.SPENT_AT;
// Handle CCC export type
if (exportType === 'CCC') {
return this.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
}
// Get base date options
const dateOptions = this.getReimbursableExpenseGroupingDateOption();
// Determine filter options based on showApprovedDate and showVerificationDate
const filterOptions = [
...(showApprovedDate ? [ExportDateType.VERIFIED_AT] : []),
...(showVerificationDate ? [ExportDateType.APPROVED_AT] : []),
excludedDate
];
// Filter out excluded and unwanted dates
return dateOptions.filter(option =>
option.value !== null && !filterOptions.includes(option.value as ExportDateType)
);
}
static dateGrouping(
exportType: 'CCC' | 'REIMBURSABLE',
expenseGrouping: string,
showApprovedDate: boolean,
showVerificationDate: boolean
): SelectFormOption[] {
// Determine the excluded date based on expenseGrouping
const excludedDate = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
? ExportDateType.LAST_SPENT_AT
: ExportDateType.SPENT_AT;
// Handle CCC export type
if (exportType === 'CCC') {
return ExportSettingModel.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
}
// Get base date options
const dateOptions = this.getReimbursableExpenseGroupingDateOption();
// Filter out excluded and unwanted dates based on conditions
const filterOptions = [
...(showApprovedDate ? [ExportDateType.VERIFIED_AT] : []),
...(showVerificationDate ? [ExportDateType.APPROVED_AT] : []),
excludedDate
];
return dateOptions.filter(option =>
option.value !== null && !filterOptions.includes(option.value as ExportDateType));
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 145-145: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 149-149: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🪛 ESLint

[error] 139-139: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 140-140: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 142-142: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 147-147: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 150-150: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 157-157: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 159-159: Trailing spaces not allowed.

(no-trailing-spaces)

🪛 GitHub Check: lint

[failure] 139-139:
Trailing spaces not allowed


[failure] 140-140:
Trailing spaces not allowed


[failure] 142-142:
Trailing spaces not allowed


[failure] 147-147:
Trailing spaces not allowed


[failure] 150-150:
Trailing spaces not allowed


[failure] 157-157:
Trailing spaces not allowed


[failure] 159-159:
Trailing spaces not allowed

🪛 GitHub Actions: TypeScript Lint Check

[error] 139-139: Trailing spaces not allowed.

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: 1

♻️ Duplicate comments (1)
src/app/core/models/common/export-settings.model.ts (1)

102-102: ⚠️ Potential issue

Fix typo in label text.

The label "Last dpend date" contains a typo and should be "Last spend date".

-          label: 'Last dpend date',
+          label: 'Last spend date',
🧹 Nitpick comments (1)
src/app/core/models/common/export-settings.model.ts (1)

112-112: Fix method naming inconsistency.

The method name should be plural (Options) to maintain consistency with getCreditCardExpenseGroupingDateOptions().

-    static getReimbursableExpenseGroupingDateOption(): SelectFormOption[] {
+    static getReimbursableExpenseGroupingDateOptions(): SelectFormOption[] {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 69cd298 and abbb7e6.

📒 Files selected for processing (2)
  • src/app/core/models/common/export-settings.model.ts (2 hunks)
  • src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/models/common/export-settings.model.ts

[error] 145-145: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 149-149: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

Comment on lines 137 to 162
static dateGrouping(exportType: string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
// Determine the excluded date based on expenseGrouping
const excludedDate = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
? ExportDateType.LAST_SPENT_AT
: ExportDateType.SPENT_AT;

// Handle CCC export type
if (exportType === 'CCC') {
return this.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
}

// Get base date options
const dateOptions = this.getReimbursableExpenseGroupingDateOption();

// Determine filter options based on showApprovedDate and showVerificationDate
const filterOptions = [
...(showApprovedDate ? [ExportDateType.VERIFIED_AT] : []),
...(showVerificationDate ? [ExportDateType.APPROVED_AT] : []),
excludedDate
];

// Filter out excluded and unwanted dates
return dateOptions.filter(option =>
option.value !== null && !filterOptions.includes(option.value as ExportDateType)
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve type safety and method naming.

The method has several areas for improvement:

  1. Using this in static context should be avoided
  2. exportType parameter should be strictly typed
  3. Method name could be more descriptive
-    static dateGrouping(exportType: string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
+    static getFilteredDateGroupingOptions(
+      exportType: 'CCC' | 'REIMBURSABLE',
+      expenseGrouping: string,
+      showApprovedDate: boolean,
+      showVerificationDate: boolean
+    ): SelectFormOption[] {
       const excludedDate = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
         ? ExportDateType.LAST_SPENT_AT
         : ExportDateType.SPENT_AT;

       if (exportType === 'CCC') {
-        return this.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
+        return ExportSettingModel.getCreditCardExpenseGroupingDateOptions().filter(option => option.value !== excludedDate);
       }

       const dateOptions = this.getReimbursableExpenseGroupingDateOption();
       const filterOptions = [
         ...(showApprovedDate ? [ExportDateType.VERIFIED_AT] : []),
         ...(showVerificationDate ? [ExportDateType.APPROVED_AT] : []),
         excludedDate
       ];

       return dateOptions.filter(option =>
         option.value !== null && !filterOptions.includes(option.value as ExportDateType)
       );
     }

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Biome (1.9.4)

[error] 145-145: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 149-149: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

Copy link
Contributor

@ashwin1111 ashwin1111 left a comment

Choose a reason for hiding this comment

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

we can do one more revision over a call tomo

value: ExportDateType.POSTED_AT
},
{
label: 'Spend date',
value: ExportDateType.SPENT_AT
},
{
label: 'Last Spend date',
label: 'Last dpend date',
Copy link
Contributor

Choose a reason for hiding this comment

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

spend


// Determine filter options based on showApprovedDate and showVerificationDate
const filterOptions = [
...(showApprovedDate ? [ExportDateType.VERIFIED_AT] : []),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isApprovedAtSelected same for verified at

const dateOptions = this.getReimbursableExpenseGroupingDateOption();

// Determine filter options based on showApprovedDate and showVerificationDate
const filterOptions = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

better rename

}

// Get base date options
const dateOptions = this.getReimbursableExpenseGroupingDateOption();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reimbursableDateOptions

];
}

static dateGrouping(exportType: string, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

have two different function for ccc and reim

: ExportDateType.SPENT_AT;

// Handle CCC export type
if (exportType === 'CCC') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isCoreCCCModule main credicard module (ccp, bank transaction, debit card)

];
}

static getReimbursableExpenseGroupingDateOption(): SelectFormOption[] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add all the options to single array and keep them poping out based on conditions

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: 2

🧹 Nitpick comments (2)
src/app/core/models/common/export-settings.model.ts (2)

112-139: Rename method for clarity.

The method name could be more descriptive to indicate it returns all available date options.

-    static expenseGroupingDateOption(): SelectFormOption[] {
+    static getAllExpenseGroupingDateOptions(): SelectFormOption[] {

141-164: Improve method organization and naming.

The method handles complex filtering logic and could benefit from the following improvements:

  1. Rename the method to better describe its purpose
  2. Extract the filtering logic into smaller helper methods for better maintainability

Consider refactoring like this:

-    static dateGrouping(
+    static getFilteredDateGroupingOptions(
       exportType: FundSource,
       expenseGrouping: string,
       showApprovedDate: boolean,
       showVerificationDate: boolean
     ): SelectFormOption[] {
+      const excludedDates = ExportSettingModel.getExcludedDates(
+        exportType,
+        expenseGrouping,
+        showApprovedDate,
+        showVerificationDate
+      );
+
+      const dateOptions = ExportSettingModel.expenseGroupingDateOption();
+      return ExportSettingModel.filterDateOptionsByExclusions(dateOptions, excludedDates);
+    }
+
+    private static getExcludedDates(
+      exportType: FundSource,
+      expenseGrouping: string,
+      showApprovedDate: boolean,
+      showVerificationDate: boolean
+    ): ExportDateType[] {
+      const excludedDate = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
+        ? ExportDateType.LAST_SPENT_AT
+        : ExportDateType.SPENT_AT;
+
+      const excludeApprovedAtOrVerified = showApprovedDate
+        ? ExportDateType.VERIFIED_AT
+        : (showVerificationDate ? ExportDateType.APPROVED_AT : null);
+
+      const excludedPostedAt = exportType !== FundSource.CORPORATE_CARD
+        ? ExportDateType.POSTED_AT
+        : null;
+
+      return [excludedDate, excludeApprovedAtOrVerified, excludedPostedAt].filter((date): date is ExportDateType => date !== null);
+    }
+
+    private static filterDateOptionsByExclusions(
+      dateOptions: SelectFormOption[],
+      excludedDates: ExportDateType[]
+    ): SelectFormOption[] {
+      return dateOptions.filter(option =>
+        option.value !== null && !excludedDates.includes(option.value as ExportDateType)
+      );
+    }
🧰 Tools
🪛 Biome (1.9.4)

[error] 158-158: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between abbb7e6 and 72ffac1.

📒 Files selected for processing (2)
  • src/app/core/models/common/export-settings.model.ts (2 hunks)
  • src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/models/common/export-settings.model.ts

[error] 158-158: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: unit-test

value: ExportDateType.SPENT_AT
},
{
label: 'Last dpend date',
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix typo in label text.

The label "Last dpend date" contains a typo and should be "Last spend date".

-          label: 'Last dpend date',
+          label: 'Last spend date',
📝 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.

Suggested change
label: 'Last dpend date',
label: 'Last spend date',

const dateOptionsToBeExcluded = [excludedDate, excludeApprovedAtOrVerified, excludedPostedAt];

// Get base date options
const dateOptions = this.expenseGroupingDateOption();
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Replace this with class name in static context.

Using this in a static context can be confusing. Use the class name instead.

-      const dateOptions = this.expenseGroupingDateOption();
+      const dateOptions = ExportSettingModel.expenseGroupingDateOption();
📝 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.

Suggested change
const dateOptions = this.expenseGroupingDateOption();
const dateOptions = ExportSettingModel.expenseGroupingDateOption();
🧰 Tools
🪛 Biome (1.9.4)

[error] 158-158: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

@@ -87,6 +88,81 @@ export class ExportSettingModel {

}

static getCreditCardExpenseGroupingDateOptions(): SelectFormOption[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this unused method

];
}

static expenseGroupingDateOption(): SelectFormOption[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

getExpenseGroupingDateOptions()

];
}

static dateGrouping(exportType: FundSource, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

constructExportDateOptions()

Copy link
Contributor

Choose a reason for hiding this comment

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

expenseGrouping can be of ExpenseGroupingFieldOption type?

Copy link
Contributor

Choose a reason for hiding this comment

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

isApprovedAtSelected, isVerifiedAtSelected

static dateGrouping(exportType: FundSource, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {

// Determine the excluded date based on expenseGrouping
const excludedDate = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
Copy link
Contributor

Choose a reason for hiding this comment

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

excludedSpendDateOption

: ExportDateType.SPENT_AT;

// Determine the excluded date based on customer choose
const excludeApprovedAtOrVerified = showApprovedDate ? ExportDateType.VERIFIED_AT : (showVerificationDate ? ExportDateType.APPROVED_AT : null);
Copy link
Contributor

Choose a reason for hiding this comment

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

excludedApprovedOrVerifiedOption

this.exportSettingForm.controls.creditCardExportGroup.setValue(ExpenseGroupingFieldOption.EXPENSE_ID);
this.exportSettingForm.controls.creditCardExportGroup.disable();

this.cccExpenseGroupingDateOptions = ExportSettingModel.dateGrouping(FundSource.CCC, this.exportSettingForm.controls.creditCardExportGroup.value, false, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

replace false

} else {
this.cccExpenseGroupingDateOptions = this.reimbursableExpenseGroupingDateOptions.concat();
this.cccExpenseGroupingDateOptions = ExportSettingModel.dateGrouping(FundSource.REIMBURSABLE, this.exportSettingForm.controls.creditCardExportGroup.value, false, false);
this.helperService.enableFormField(this.exportSettingForm, 'creditCardExportGroup');
}
const allowedValues = this.cccExpenseGroupingDateOptions.map(option => option.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not required

this.reimbursableExpenseGroupingDateOptions = ExportSettingModel.constructGroupingDateOptions(reimbursableExportGroup, this.reimbursableExpenseGroupingDateOptions);
// This.reimbursableExpenseGroupingDateOptions = QBOExportSettingModel.getReimbursableExpenseGroupingDateOptions();
// This.reimbursableExpenseGroupingDateOptions = ExportSettingModel.constructGroupingDateOptions(reimbursableExportGroup, this.reimbursableExpenseGroupingDateOptions);
this.reimbursableExpenseGroupingDateOptions = ExportSettingModel.dateGrouping(FundSource.REIMBURSABLE, this.exportSettingForm.controls.reimbursableExportGroup.value, true, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

true false should be sent based on the workspace setting

});

this.exportSettingForm.controls.creditCardExportType?.valueChanges.subscribe(creditCardExportType => {
this.updateCCCExpenseGroupingDateOptions(this.exportSettingForm.get('creditCardExportType')?.value);
this.updateCCCExpenseGroupingDateOptions(creditCardExportType);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not required anymore no? we can ignore cc export type watcher since we're dependent on grouping

if (brandingConfig.brandId==='fyle' && this.exportSettingForm.get('creditCardExportType')?.value && this.exportSettingForm.get('creditCardExportType')?.value !== QBOCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE && this.exportSettingForm.get('creditCardExportType')?.value !== QBOCorporateCreditCardExpensesObject.DEBIT_CARD_EXPENSE) {
this.cccExpenseGroupingDateOptions = QBOExportSettingModel.getReimbursableExpenseGroupingDateOptions();
this.cccExpenseGroupingDateOptions = ExportSettingModel.constructGroupingDateOptions(creditCardExportGroup, this.cccExpenseGroupingDateOptions);
if (this.exportSettingForm.get('creditCardExportType')?.value && this.exportSettingForm.get('creditCardExportType')?.value !== QBOCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE && this.exportSettingForm.get('creditCardExportType')?.value !== QBOCorporateCreditCardExpensesObject.DEBIT_CARD_EXPENSE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs an else case as well

if (ccc export type)
if (core ccc module) {}
else {}

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: 1

🧹 Nitpick comments (3)
src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts (3)

313-323: Improve code maintainability by extracting common logic.

The method could be more concise by extracting the common call to constructExportDateOptions.

   private updateCCCExpenseGroupingDateOptions(selectedValue: QBOCorporateCreditCardExpensesObject): void {
+    const fundSource = [QBOCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE, QBOCorporateCreditCardExpensesObject.DEBIT_CARD_EXPENSE].includes(selectedValue)
+      ? FundSource.CCC
+      : FundSource.REIMBURSABLE;
+
     if ([QBOCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE, QBOCorporateCreditCardExpensesObject.DEBIT_CARD_EXPENSE].includes(selectedValue)) {
       this.exportSettingForm.controls.creditCardExportGroup.setValue(ExpenseGroupingFieldOption.EXPENSE_ID);
       this.exportSettingForm.controls.creditCardExportGroup.disable();
-
-      this.cccExpenseGroupingDateOptions = ExportSettingModel.constructExportDateOptions(FundSource.CCC, this.exportSettingForm.controls.creditCardExportGroup.value, false, false);
-    } else {
-      this.cccExpenseGroupingDateOptions = ExportSettingModel.constructExportDateOptions(FundSource.REIMBURSABLE, this.exportSettingForm.controls.creditCardExportGroup.value, false, false);
-      this.helperService.enableFormField(this.exportSettingForm, 'creditCardExportGroup');
     }
+
+    this.cccExpenseGroupingDateOptions = ExportSettingModel.constructExportDateOptions(
+      fundSource,
+      this.exportSettingForm.controls.creditCardExportGroup.value,
+      false,
+      false
+    );
+
+    if (!fundSource.includes(selectedValue)) {
+      this.helperService.enableFormField(this.exportSettingForm, 'creditCardExportGroup');
+    }
   }

340-341: Simplify boolean assignments.

The ternary operators for boolean assignments can be simplified using direct comparison.

-      const isApprovedAtSelected = this.exportSettingForm.controls.reimbursableExportDate.value === ExportDateType.APPROVED_AT ? true : false;
-      const isVerifiedAtSelected = this.exportSettingForm.controls.reimbursableExportDate.value === ExportDateType.VERIFIED_AT ? true : false;
+      const isApprovedAtSelected = this.exportSettingForm.controls.reimbursableExportDate.value === ExportDateType.APPROVED_AT;
+      const isVerifiedAtSelected = this.exportSettingForm.controls.reimbursableExportDate.value === ExportDateType.VERIFIED_AT;

-      const isApprovedAtSelected = this.exportSettingForm.controls.creditCardExportDate.value === ExportDateType.APPROVED_AT ? true : false;
-      const isVerifiedAtSelected = this.exportSettingForm.controls.creditCardExportDate.value === ExportDateType.VERIFIED_AT ? true : false;
+      const isApprovedAtSelected = this.exportSettingForm.controls.creditCardExportDate.value === ExportDateType.APPROVED_AT;
+      const isVerifiedAtSelected = this.exportSettingForm.controls.creditCardExportDate.value === ExportDateType.VERIFIED_AT;

Also applies to: 346-347

🧰 Tools
🪛 Biome (1.9.4)

[error] 340-340: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)


[error] 341-341: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)


348-352: Improve readability of conditional logic.

The conditional logic for handling different export types could be more readable by extracting the conditions into meaningful variables.

-      if (this.exportSettingForm.get('creditCardExportType')?.value && this.exportSettingForm.get('creditCardExportType')?.value !== QBOCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE && this.exportSettingForm.get('creditCardExportType')?.value !== QBOCorporateCreditCardExpensesObject.DEBIT_CARD_EXPENSE) {
+      const creditCardExportType = this.exportSettingForm.get('creditCardExportType')?.value;
+      const isCoreCCCModule = creditCardExportType === QBOCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE || 
+                             creditCardExportType === QBOCorporateCreditCardExpensesObject.DEBIT_CARD_EXPENSE;
+
+      if (creditCardExportType && !isCoreCCCModule) {
         this.cccExpenseGroupingDateOptions = ExportSettingModel.constructExportDateOptions(FundSource.REIMBURSABLE, creditCardExportGroup, isApprovedAtSelected, isVerifiedAtSelected);
       } else {
         this.cccExpenseGroupingDateOptions = ExportSettingModel.constructExportDateOptions(FundSource.CORPORATE_CARD, creditCardExportGroup, isApprovedAtSelected, isVerifiedAtSelected);
       }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 72ffac1 and fa4be68.

📒 Files selected for processing (2)
  • src/app/core/models/common/export-settings.model.ts (2 hunks)
  • src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts (3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/models/common/export-settings.model.ts

[error] 137-137: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts

[error] 340-340: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)


[error] 341-341: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)


[error] 346-346: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)


[error] 347-347: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

🔇 Additional comments (2)
src/app/core/models/common/export-settings.model.ts (2)

91-118: LGTM! Well-structured date options.

The method provides a clear and comprehensive list of date options with user-friendly labels mapped to appropriate export date types.


120-143: LGTM! Well-organized filtering logic.

The method has:

  • Clear comments explaining each step
  • Meaningful variable names
  • Well-structured filtering logic
🧰 Tools
🪛 Biome (1.9.4)

[error] 137-137: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

const dateOptionsToBeExcluded = [excludedSpendDateOption, excludedApprovedOrVerifiedOption, excludedPostedAtOption];

// Get base date options
const unfilteredDateOptions = this.getExpenseGroupingDateOptions();
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Replace this with class name in static context.

Using this in a static context can be confusing. Use the class name instead.

-      const unfilteredDateOptions = this.getExpenseGroupingDateOptions();
+      const unfilteredDateOptions = ExportSettingModel.getExpenseGroupingDateOptions();
📝 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.

Suggested change
const unfilteredDateOptions = this.getExpenseGroupingDateOptions();
const unfilteredDateOptions = ExportSettingModel.getExpenseGroupingDateOptions();
🧰 Tools
🪛 Biome (1.9.4)

[error] 137-137: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

@@ -138,27 +117,27 @@ export class ExportSettingModel {
];
}

static dateGrouping(exportType: FundSource, expenseGrouping: string, showApprovedDate: boolean, showVerificationDate: boolean): SelectFormOption[] {
static constructExportDateOptions(exportType: FundSource, expenseGrouping: ExpenseGroupingFieldOption, isApprovedAtSelected: boolean, isVerifiedAtSelected: boolean): SelectFormOption[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

exportType can be isCoreCCCModule: boolean

Comment on lines 346 to 347
const isApprovedAtSelected = this.exportSettingForm.controls.creditCardExportDate.value === ExportDateType.APPROVED_AT ? true : false;
const isVerifiedAtSelected = this.exportSettingForm.controls.creditCardExportDate.value === ExportDateType.VERIFIED_AT ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

same logic being repeated, move out to model so that all apps can use it

Copy link
Contributor

Choose a reason for hiding this comment

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

also for ccc we've never used this, so safe to make it false false for ccc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we do this in model we don't need to send isApprovedAtSelected, isVerifiedAtSelected or any parameter we can directly send the form and check. can we do like this

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: 1

♻️ Duplicate comments (1)
src/app/core/models/common/export-settings.model.ts (1)

137-137: ⚠️ Potential issue

Replace this with class name in static context.

Using this in a static context can be confusing. Use the class name instead.

-      const unfilteredDateOptions = this.getExpenseGroupingDateOptions();
+      const unfilteredDateOptions = ExportSettingModel.getExpenseGroupingDateOptions();
🧰 Tools
🪛 Biome (1.9.4)

[error] 137-137: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🧹 Nitpick comments (3)
src/app/integrations/qbo/qbo.fixture.ts (1)

245-254: LGTM! New mock data added for date grouping options.

The new mock data follows a consistent structure and uses the ExportDateType enum correctly.

Consider adding JSDoc comments to describe the purpose and structure of these mock constants:

+/**
+ * Mock data for CCC expense date grouping options.
+ * Used in export settings component tests.
+ */
 export const mockCCCExpenseDateGrouping: SelectFormOption[] = [
   {label: 'Export date', value: ExportDateType.CURRENT_DATE},
   {label: 'Verification date', value: ExportDateType.VERIFIED_AT},
   {label: 'Spend date', value: ExportDateType.SPENT_AT}
 ];

+/**
+ * Mock data for reimbursable expense date grouping options.
+ * Used in export settings component tests.
+ */
 export const mockReimbursableExpenseDateGrouping: SelectFormOption[] = [
   {label: 'Export date', value: ExportDateType.CURRENT_DATE},
   {label: 'Spend date', value: ExportDateType.SPENT_AT}
 ];
src/app/core/models/common/export-settings.model.ts (2)

120-120: Improve type safety for parameters.

The expenseGrouping parameter should be explicitly typed as ExpenseGroupingFieldOption.

-    static constructExportDateOptions(isCoreCCCModule: boolean, expenseGrouping: ExpenseGroupingFieldOption, exportDateGroupingValue: ExportDateType): SelectFormOption[] {
+    static constructExportDateOptions(isCoreCCCModule: boolean, expenseGrouping: ExpenseGroupingFieldOption, exportDateGroupingValue: ExportDateType): SelectFormOption[] {

128-128: Simplify complex conditional logic.

The long conditional expression could be simplified for better readability.

-      const excludedApprovedOrVerifiedOption = exportDateGroupingValue === ExportDateType.APPROVED_AT ? [ExportDateType.VERIFIED_AT] : (exportDateGroupingValue === ExportDateType.VERIFIED_AT ? [ExportDateType.APPROVED_AT] : [ExportDateType.APPROVED_AT, ExportDateType.VERIFIED_AT]);
+      let excludedApprovedOrVerifiedOption: ExportDateType[] = [];
+      if (exportDateGroupingValue === ExportDateType.APPROVED_AT) {
+        excludedApprovedOrVerifiedOption = [ExportDateType.VERIFIED_AT];
+      } else if (exportDateGroupingValue === ExportDateType.VERIFIED_AT) {
+        excludedApprovedOrVerifiedOption = [ExportDateType.APPROVED_AT];
+      } else {
+        excludedApprovedOrVerifiedOption = [ExportDateType.APPROVED_AT, ExportDateType.VERIFIED_AT];
+      }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fa4be68 and bb17b7f.

📒 Files selected for processing (4)
  • src/app/core/models/common/export-settings.model.ts (2 hunks)
  • src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.spec.ts (3 hunks)
  • src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts (3 hunks)
  • src/app/integrations/qbo/qbo.fixture.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/models/common/export-settings.model.ts

[error] 137-137: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: unit-test
🔇 Additional comments (2)
src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.spec.ts (1)

709-709: LGTM! Test assertions updated to use more specific mock data.

The test assertions have been updated to use dedicated mock data (mockReimbursableExpenseDateGrouping and mockCCCExpenseDateGrouping) for date grouping options, which improves test clarity and maintainability.

Also applies to: 730-730

src/app/core/models/common/export-settings.model.ts (1)

91-118: LGTM! Well-structured implementation of date options.

The method provides a clear and consistent set of date options with descriptive labels and proper enum values.

@@ -117,21 +117,21 @@ export class ExportSettingModel {
];
}

static constructExportDateOptions(exportType: FundSource, expenseGrouping: ExpenseGroupingFieldOption, isApprovedAtSelected: boolean, isVerifiedAtSelected: boolean): SelectFormOption[] {
static constructExportDateOptions(isCoreCCCModule: boolean, expenseGrouping: ExpenseGroupingFieldOption, exportDateGroupingValue: ExportDateType): SelectFormOption[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

exportDateType: ExportDateType


// Array of unwanted dates
const dateOptionsToBeExcluded = [excludedSpendDateOption, excludedApprovedOrVerifiedOption, excludedPostedAtOption];
const dateOptionsToBeExcluded = [excludedSpendDateOption, excludedPostedAtOption].concat(excludedApprovedOrVerifiedOption);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not [excludedSpendDateOption, excludedApprovedOrVerifiedOption, excludedPostedAtOption]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is because is now an array, if we put array inside array it wooun't work as we expected.
it will be like [laste_spend_date, [approved_at, verified_at], posted_at]. with this if we use filter it will not work as expected

@DhaaraniCIT DhaaraniCIT added deploy Triggers deployment of active branch to Staging qa_deploy Deploys to QA env labels Feb 20, 2025
Copy link

Unit Test Coverage % values
Statements 33.07% ( 4171 / 12611 )
Branches 26.5% ( 1182 / 4460 )
Functions 25.63% ( 895 / 3491 )
Lines 33.25% ( 4105 / 12343 )

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: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d8b4576 and f408a04.

📒 Files selected for processing (1)
  • src/app/core/models/common/export-settings.model.ts (2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/models/common/export-settings.model.ts

[error] 137-137: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: staging_deploy
🔇 Additional comments (1)
src/app/core/models/common/export-settings.model.ts (1)

91-118: LGTM! Well-structured date options.

The method provides a comprehensive list of date options with clear labels and corresponding enum values.

import { DefaultDestinationAttribute, DestinationAttribute } from "../db/destination-attribute.model";
import { DestinationOptionKey, ExpenseGroupingFieldOption, ExportDateType, IntacctCorporateCreditCardExpensesObject, IntacctExportSettingDestinationOptionKey, IntacctReimbursableExpensesObject, NetsuiteExportSettingDestinationOptionKey, QboExportSettingDestinationOptionKey, SplitExpenseGrouping } from "../enum/enum.model";
import { DestinationOptionKey, ExpenseGroupingFieldOption, ExportDateType, FundSource, IntacctCorporateCreditCardExpensesObject, IntacctExportSettingDestinationOptionKey, IntacctReimbursableExpensesObject, NetsuiteExportSettingDestinationOptionKey, QboExportSettingDestinationOptionKey, SplitExpenseGrouping } from "../enum/enum.model";
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove unused import.

The FundSource enum is imported but not used in the file. Remove it to keep the imports clean.

-import { DestinationOptionKey, ExpenseGroupingFieldOption, ExportDateType, FundSource, IntacctCorporateCreditCardExpensesObject, IntacctExportSettingDestinationOptionKey, IntacctReimbursableExpensesObject, NetsuiteExportSettingDestinationOptionKey, QboExportSettingDestinationOptionKey, SplitExpenseGrouping } from "../enum/enum.model";
+import { DestinationOptionKey, ExpenseGroupingFieldOption, ExportDateType, IntacctCorporateCreditCardExpensesObject, IntacctExportSettingDestinationOptionKey, IntacctReimbursableExpensesObject, NetsuiteExportSettingDestinationOptionKey, QboExportSettingDestinationOptionKey, SplitExpenseGrouping } from "../enum/enum.model";
📝 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.

Suggested change
import { DestinationOptionKey, ExpenseGroupingFieldOption, ExportDateType, FundSource, IntacctCorporateCreditCardExpensesObject, IntacctExportSettingDestinationOptionKey, IntacctReimbursableExpensesObject, NetsuiteExportSettingDestinationOptionKey, QboExportSettingDestinationOptionKey, SplitExpenseGrouping } from "../enum/enum.model";
import { DestinationOptionKey, ExpenseGroupingFieldOption, ExportDateType, IntacctCorporateCreditCardExpensesObject, IntacctExportSettingDestinationOptionKey, IntacctReimbursableExpensesObject, NetsuiteExportSettingDestinationOptionKey, QboExportSettingDestinationOptionKey, SplitExpenseGrouping } from "../enum/enum.model";

Comment on lines +120 to +143
static constructExportDateOptions(isCoreCCCModule: boolean, expenseGrouping: ExpenseGroupingFieldOption, exportDateType: ExportDateType): SelectFormOption[] {

// Determine the excluded date based on expenseGrouping
const excludedSpendDateOption = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
? ExportDateType.LAST_SPENT_AT
: ExportDateType.SPENT_AT;

// Determine the excluded date based on customer choose
const excludedApprovedOrVerifiedOption = exportDateType === ExportDateType.APPROVED_AT ? [ExportDateType.VERIFIED_AT] : (exportDateType === ExportDateType.VERIFIED_AT ? [ExportDateType.APPROVED_AT] : [ExportDateType.APPROVED_AT, ExportDateType.VERIFIED_AT]);

// Determine the excluded date based on export Type
const excludedPostedAtOption = !isCoreCCCModule ? ExportDateType.POSTED_AT : null;

// Array of unwanted dates
const dateOptionsToBeExcluded = [excludedSpendDateOption, excludedPostedAtOption].concat(excludedApprovedOrVerifiedOption);

// Get base date options
const unfilteredDateOptions = this.getExpenseGroupingDateOptions();

// Filter out excluded and unwanted dates
return unfilteredDateOptions.filter(option =>
option.value !== null && !dateOptionsToBeExcluded.includes(option.value as ExportDateType)
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Fix static context and improve type safety.

The method has a well-documented logic flow with clear comments. However, there are a few improvements needed:

  1. Using this in a static context should be avoided
  2. The expenseGrouping parameter should be strictly typed
-    static constructExportDateOptions(isCoreCCCModule: boolean, expenseGrouping: ExpenseGroupingFieldOption, exportDateType: ExportDateType): SelectFormOption[] {
+    static constructExportDateOptions(
+      isCoreCCCModule: boolean,
+      expenseGrouping: ExpenseGroupingFieldOption,
+      exportDateType: ExportDateType
+    ): SelectFormOption[] {
       // Determine the excluded date based on expenseGrouping
       const excludedSpendDateOption = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
         ? ExportDateType.LAST_SPENT_AT
         : ExportDateType.SPENT_AT;

       // Determine the excluded date based on customer choose
       const excludedApprovedOrVerifiedOption = exportDateType === ExportDateType.APPROVED_AT ? [ExportDateType.VERIFIED_AT] : (exportDateType === ExportDateType.VERIFIED_AT ? [ExportDateType.APPROVED_AT] : [ExportDateType.APPROVED_AT, ExportDateType.VERIFIED_AT]);

       // Determine the excluded date based on export Type
       const excludedPostedAtOption = !isCoreCCCModule ? ExportDateType.POSTED_AT : null;

       // Array of unwanted dates
       const dateOptionsToBeExcluded = [excludedSpendDateOption, excludedPostedAtOption].concat(excludedApprovedOrVerifiedOption);

       // Get base date options
-      const unfilteredDateOptions = this.getExpenseGroupingDateOptions();
+      const unfilteredDateOptions = ExportSettingModel.getExpenseGroupingDateOptions();

       // Filter out excluded and unwanted dates
       return unfilteredDateOptions.filter(option =>
         option.value !== null && !dateOptionsToBeExcluded.includes(option.value as ExportDateType)
       );
     }
📝 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.

Suggested change
static constructExportDateOptions(isCoreCCCModule: boolean, expenseGrouping: ExpenseGroupingFieldOption, exportDateType: ExportDateType): SelectFormOption[] {
// Determine the excluded date based on expenseGrouping
const excludedSpendDateOption = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
? ExportDateType.LAST_SPENT_AT
: ExportDateType.SPENT_AT;
// Determine the excluded date based on customer choose
const excludedApprovedOrVerifiedOption = exportDateType === ExportDateType.APPROVED_AT ? [ExportDateType.VERIFIED_AT] : (exportDateType === ExportDateType.VERIFIED_AT ? [ExportDateType.APPROVED_AT] : [ExportDateType.APPROVED_AT, ExportDateType.VERIFIED_AT]);
// Determine the excluded date based on export Type
const excludedPostedAtOption = !isCoreCCCModule ? ExportDateType.POSTED_AT : null;
// Array of unwanted dates
const dateOptionsToBeExcluded = [excludedSpendDateOption, excludedPostedAtOption].concat(excludedApprovedOrVerifiedOption);
// Get base date options
const unfilteredDateOptions = this.getExpenseGroupingDateOptions();
// Filter out excluded and unwanted dates
return unfilteredDateOptions.filter(option =>
option.value !== null && !dateOptionsToBeExcluded.includes(option.value as ExportDateType)
);
}
static constructExportDateOptions(
isCoreCCCModule: boolean,
expenseGrouping: ExpenseGroupingFieldOption,
exportDateType: ExportDateType
): SelectFormOption[] {
// Determine the excluded date based on expenseGrouping
const excludedSpendDateOption = expenseGrouping === ExpenseGroupingFieldOption.EXPENSE_ID
? ExportDateType.LAST_SPENT_AT
: ExportDateType.SPENT_AT;
// Determine the excluded date based on customer choose
const excludedApprovedOrVerifiedOption = exportDateType === ExportDateType.APPROVED_AT
? [ExportDateType.VERIFIED_AT]
: (exportDateType === ExportDateType.VERIFIED_AT
? [ExportDateType.APPROVED_AT]
: [ExportDateType.APPROVED_AT, ExportDateType.VERIFIED_AT]);
// Determine the excluded date based on export Type
const excludedPostedAtOption = !isCoreCCCModule ? ExportDateType.POSTED_AT : null;
// Array of unwanted dates
const dateOptionsToBeExcluded = [excludedSpendDateOption, excludedPostedAtOption].concat(excludedApprovedOrVerifiedOption);
// Get base date options
const unfilteredDateOptions = ExportSettingModel.getExpenseGroupingDateOptions();
// Filter out excluded and unwanted dates
return unfilteredDateOptions.filter(option =>
option.value !== null && !dateOptionsToBeExcluded.includes(option.value as ExportDateType)
);
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 137-137: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

@DhaaraniCIT DhaaraniCIT added deploy Triggers deployment of active branch to Staging qa_deploy Deploys to QA env and removed deploy Triggers deployment of active branch to Staging qa_deploy Deploys to QA env labels Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy Triggers deployment of active branch to Staging qa_deploy Deploys to QA env size/M Medium PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants