-
Notifications
You must be signed in to change notification settings - Fork 0
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: patch "Report Title" option to "Report Name" in skip export #1034
fix: patch "Report Title" option to "Report Name" in skip export #1034
Conversation
Affects advanced settings of intacct, netsuite, business central, qbo and sage 300
WalkthroughThe changes involve modifications to the HTML templates of two components: Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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 (5)
src/app/shared/components/si/helper/skip-export/skip-export.component.html (2)
26-26
: LGTM! Consider extracting the condition to a variable for improved readability.The changes correctly implement the requirement to update "Report Title" to "Report Name" in the dropdown options. The conditional check ensures that only 'report_title' is replaced while maintaining other field names.
To improve code readability, consider extracting the condition to a variable:
{{ let updatedFieldName = option.field_name === 'report_title' ? 'report_name' : option.field_name; }} {{ updatedFieldName | snakeCaseToSpaceCase | titlecase }}This approach would make the template easier to read and maintain, especially if this condition needs to be reused elsewhere in the component.
Also applies to: 30-30
Line range hint
91-97
: Consider applying the "Report Name" change to the second condition dropdown.The changes to replace "Report Title" with "Report Name" have been applied to the first condition dropdown, but not to the second one. This inconsistency might lead to confusion for users.
To maintain consistency throughout the form, consider applying the same changes to the second condition dropdown:
<ng-template let-option pTemplate="item"> - {{ option.field_name | snakeCaseToSpaceCase | titlecase }} + {{ (option.field_name === 'report_title' ? 'report_name' : option.field_name) | snakeCaseToSpaceCase | titlecase }} </ng-template> <!-- For selected option --> <ng-template let-selectedOption pTemplate="selectedItem"> - {{ selectedOption.field_name | snakeCaseToSpaceCase | titlecase }} + {{ (selectedOption.field_name === 'report_title' ? 'report_name' : selectedOption.field_name) | snakeCaseToSpaceCase | titlecase }} </ng-template>This change will ensure that the terminology is consistent across both condition dropdowns.
src/app/shared/components/configuration/configuration-skip-export/configuration-skip-export.component.html (3)
27-27
: LGTM! Consider applying the change consistently.The change aligns well with the PR objective of updating "Report Title" to "Report Name" in the user interface. This modification enhances clarity for users without affecting the underlying data structure.
For consistency, consider applying this change to other occurrences of 'report_title' in the file, if any exist.
36-36
: LGTM! Consider refactoring for DRY principle.This change correctly updates the selected item display, maintaining consistency with the dropdown options. It ensures that "Report Name" is displayed instead of "Report Title" when selected.
To adhere to the DRY (Don't Repeat Yourself) principle, consider extracting the conditional expression into a custom pipe or a component method. This would centralize the logic and make future updates easier. For example:
@Pipe({ name: 'fieldNameDisplay' }) export class FieldNameDisplayPipe implements PipeTransform { transform(value: string): string { return value === 'report_title' ? 'report_name' : value; } }Then, in the template:
{{ selectedOption.field_name | fieldNameDisplay | snakeCaseToSpaceCase | titlecase }}This approach would reduce duplication and make the code more maintainable.
Incomplete Terminology Update Detected
The following occurrences of "report_title" were found and still need to be updated:
src/app/shared/components/configuration/configuration-skip-export/configuration-skip-export.component.html
:
- Line 27
- Line 36
Additionally, "report_title" was found in the following files and may require similar updates:
src/stories/ExportLogChildTableDialog.stories.ts
src/stories/ExportLogTable.stories.ts
src/stories/DashboardExportLogDialog.stories.ts
src/stories/DashboardAccountingErrorDialog.stories.ts
src/app/shared/components/configuration/configuration-skip-export/configuration-skip-export.component.ts
src/app/shared/components/si/helper/skip-export/skip-export.component.html
src/app/shared/components/si/helper/skip-export/skip-export.component.ts
src/app/core/models/intacct/misc/skip-export.model.ts
src/app/core/models/intacct/db/expense.model.ts
src/app/core/models/common/advanced-settings.model.ts
src/app/integrations/sage300/sage300-shared/fixture.ts
src/app/integrations/qbo/qbo.fixture.ts
src/app/integrations/business-central/business-central-shared/business-central.fixture.ts
Please ensure all instances are updated to "report_name" for consistency.
🔗 Analysis chain
Line range hint
1-180
: Overall, the changes look good. Ensure consistency throughout the file.The modifications successfully update the terminology from "Report Title" to "Report Name" in the dropdown for both options and selected items. This aligns well with the PR objectives and improves clarity for users.
To ensure complete implementation of the change:
- Verify that all occurrences of 'report_title' in this file have been addressed.
- Check if similar changes are needed in related components or services.
Run the following script to verify:
Review the results to ensure comprehensive implementation of the terminology update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining occurrences of 'report_title' in the file grep -n "report_title" src/app/shared/components/configuration/configuration-skip-export/configuration-skip-export.component.html # Search for related files that might need similar updates fd -e ts -e html | xargs grep -l "report_title"Length of output: 1365
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/app/shared/components/configuration/configuration-skip-export/configuration-skip-export.component.html (2 hunks)
- src/app/shared/components/si/helper/skip-export/skip-export.component.html (1 hunks)
Description
Affects advanced settings of intacct, netsuite, business central, qbo and sage 300
Clickup
https://app.clickup.com/
Summary by CodeRabbit
New Features
Bug Fixes