refactor: cleanup accounting workspace and charts #51286
Conversation
github-actions
bot
added
skip-release-notes
📝 WalkthroughWalkthroughThis PR restructures ERPNext's workspace and desktop navigation hierarchy. It consolidates the Payables and Receivables workspaces and their desktop icons by removing them, adds a Profit and Loss line chart to the Financial Reports workspace, reorganizes desktop icons under a unified "ERPNext" parent hierarchy, introduces new number cards (Sales Orders Count, Total Sales Amount, Average Order Value) to the Selling workspace, updates workspace sidebars with new sections (Opening & Closing in Accounting, Setup in Banking and Subscription), and modifies chart label rendering to strip currency suffixes. Multiple timestamps and index fields are updated across configuration files. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
erpnext/workspace_sidebar/accounting.json (1)
180-189: Fix the typo in the existing label.The existing label "Payment Reconciliaition" at line 184 also contains a typo. It should be "Payment Reconciliation".
🔎 Proposed fix
{ "child": 1, "collapsible": 1, "indent": 0, "keep_closed": 0, - "label": "Payment Reconciliaition", + "label": "Payment Reconciliation", "link_to": "Payment Reconciliation", "link_type": "DocType", "show_arrow": 0, "type": "Link" },
🧹 Nitpick comments (3)
erpnext/selling/number_card/average_order_values_1/average_order_values_1.json (1)
14-18: Consider using a cleaner naming convention.The
namefield uses"Average Order Values-1"with a suffix that appears auto-generated. If this is the only such card, consider using"Average Order Values"for cleaner identification.erpnext/workspace_sidebar/budget.json (1)
69-78: Section Break has unnecessarylink_typefield.The Reports Section Break includes
"link_type": "DocType"on line 76, which appears unnecessary for a Section Break element. This might be a copy-paste artifact from the previous item.🔎 Proposed fix
{ "child": 0, "collapsible": 1, "icon": "notepad-text", "indent": 1, "keep_closed": 1, "label": "Reports", - "link_type": "DocType", "show_arrow": 0, "type": "Section Break" },erpnext/workspace_sidebar/banking.json (1)
45-55: Section Break has unnecessarylink_typefield.Similar to the budget.json file, this Section Break includes
"link_type": "DocType"which is unnecessary for a Section Break element.🔎 Proposed fix
{ "child": 0, "collapsible": 1, "icon": "database", "indent": 1, "keep_closed": 1, "label": "Setup", - "link_type": "DocType", "show_arrow": 0, "type": "Section Break" },
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.jsonerpnext/accounts/workspace/financial_reports/financial_reports.jsonerpnext/accounts/workspace/payables/payables.jsonerpnext/accounts/workspace/receivables/receivables.jsonerpnext/desktop_icon/accounting.jsonerpnext/desktop_icon/assets.jsonerpnext/desktop_icon/banking.jsonerpnext/desktop_icon/buying.jsonerpnext/desktop_icon/crm.jsonerpnext/desktop_icon/financial_reports.jsonerpnext/desktop_icon/manufacturing.jsonerpnext/desktop_icon/opening_&_closing.jsonerpnext/desktop_icon/payables.jsonerpnext/desktop_icon/projects.jsonerpnext/desktop_icon/quality.jsonerpnext/desktop_icon/receivables.jsonerpnext/desktop_icon/selling.jsonerpnext/desktop_icon/stock.jsonerpnext/desktop_icon/subscription.jsonerpnext/desktop_icon/support.jsonerpnext/selling/number_card/average_order_values_1/average_order_values_1.jsonerpnext/selling/report/sales_order_trends/sales_order_trends.pyerpnext/selling/workspace/selling/selling.jsonerpnext/workspace_sidebar/accounting.jsonerpnext/workspace_sidebar/banking.jsonerpnext/workspace_sidebar/budget.jsonerpnext/workspace_sidebar/payables.jsonerpnext/workspace_sidebar/receivables.jsonerpnext/workspace_sidebar/subscription.json
💤 Files with no reviewable changes (7)
- erpnext/accounts/workspace/payables/payables.json
- erpnext/accounts/workspace/receivables/receivables.json
- erpnext/workspace_sidebar/receivables.json
- erpnext/desktop_icon/payables.json
- erpnext/workspace_sidebar/payables.json
- erpnext/desktop_icon/receivables.json
- erpnext/desktop_icon/opening_&_closing.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-17T14:11:06.959Z
Learnt from: sagarvora
Repo: frappe/erpnext PR: 50155
File: erpnext/controllers/accounts_controller.py:2992-3005
Timestamp: 2025-10-17T14:11:06.959Z
Learning: In ERPNext, item child doctypes (like "Sales Invoice Item", "Delivery Note Item", etc.) have exactly one non-custom Link field that references their parent transaction doctype (like "Sales Order", "Purchase Order", etc.). This is a schema design pattern that can be relied upon when determining reference fields for mapping logic.
Applied to files:
erpnext/workspace_sidebar/accounting.json
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/selling/report/sales_order_trends/sales_order_trends.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (25)
erpnext/desktop_icon/accounting.json (1)
3-17: LGTM: Metadata alignment with ERPNext parent.The parent_icon change from "Accounts" to "ERPNext" aligns with the broader desktop icon restructuring in this PR.
erpnext/desktop_icon/manufacturing.json (1)
3-17: LGTM: Consistent metadata restructuring.The metadata updates follow the same pattern as other desktop icons in this PR, standardizing under the "ERPNext" parent.
erpnext/desktop_icon/selling.json (1)
3-17: LGTM: Metadata standardization.The changes standardize this desktop icon's metadata with others in the refactoring.
erpnext/desktop_icon/support.json (1)
3-17: LGTM: Metadata alignment.The metadata updates are consistent with the PR's desktop icon restructuring objectives.
erpnext/desktop_icon/quality.json (1)
3-17: LGTM: Metadata cleanup.The metadata changes align with the systematic desktop icon restructuring across the PR.
erpnext/selling/report/sales_order_trends/sales_order_trends.py (1)
39-39: LGTM: Cleaner chart labels.Stripping the " (Amt)" suffix improves label readability on charts. The logic is safe since
replace()handles cases where the substring isn't present.erpnext/desktop_icon/banking.json (1)
11-11: Bank Clearance DocType exists and is a valid navigation target.Verification confirms the "Bank Clearance" DocType is properly defined with existing implementations. The workspace sidebar has been updated to include both "Bank Clearance" and "Bank Reconciliation Tool" as separate navigation options, making the desktop icon change to "Bank Clearance" a valid and intentional design decision.
erpnext/desktop_icon/assets.json (1)
3-17: LGTM! Desktop icon metadata updated correctly.The changes reorganize the Assets icon under the ERPNext parent and adjust the index, consistent with the broader desktop icon restructuring in this PR.
erpnext/desktop_icon/projects.json (1)
3-17: LGTM! Desktop icon metadata updated correctly.The Projects icon is being reorganized under the ERPNext parent with index normalization, consistent with other desktop icons in this refactor.
erpnext/desktop_icon/subscription.json (1)
9-17: Verify the parent_icon and idx values.The Subscription desktop icon has
idxset to 99 andparent_iconremains as "Accounts", which differs from the pattern in other desktop icons being moved toparent_icon: "ERPNext"withidx: 1. Please confirm this is intentional.erpnext/desktop_icon/buying.json (1)
3-17: LGTM! Desktop icon metadata updated correctly.The Buying icon metadata is updated consistently with the desktop icon reorganization pattern.
erpnext/workspace_sidebar/accounting.json (1)
245-315: LGTM! New Opening & Closing section added.The new section consolidates opening and closing operations with appropriate tools and links. The structure is consistent with other sidebar sections.
erpnext/workspace_sidebar/subscription.json (1)
45-88: LGTM! Setup section added appropriately.The new Setup section provides convenient access to commonly needed master data (Customer, Supplier, Item) when managing subscriptions. This enhances the workspace usability.
erpnext/desktop_icon/stock.json (1)
3-17: LGTM! Desktop icon metadata updated correctly.The Stock icon metadata is updated consistently with the desktop icon reorganization pattern.
erpnext/desktop_icon/financial_reports.json (1)
3-17: LGTM! Desktop icon metadata updated correctly.The Financial Reports icon is reorganized under the ERPNext parent. The
idx: 0(vs.idx: 1for most other icons) likely positions this icon with higher priority in the navigation, which seems appropriate for financial reporting.erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.json (1)
20-22: Chart enhancements look good.The addition of
show_values_over_chartand changing the chart type fromBartoLineare valid enhancements for the Profit and Loss visualization.erpnext/workspace_sidebar/budget.json (1)
80-91: Budget Variance Report link configuration looks correct.The report link properly uses
link_type: "Report"and points toBudget Variance Report.erpnext/selling/workspace/selling/selling.json (3)
629-634: Number card integration looks correct.The new
number_cardsarray properly references theAverage Order Values-1card defined in the companion JSON file.
676-676: Report reference doctype addition is appropriate.Adding
report_ref_doctype: "Sales Order"correctly associates the Sales Analytics shortcut with the Sales Order doctype for context.
648-648: Frappe School URL is accessible and valid.The new URL
https://frappe.school/lms/courses/sales-management-coursereturns a 301 redirect, confirming the domain is valid and the course path is correctly routed. The domain migration fromschool.frappe.iotofrappe.schoolis working as expected.Note: Other workspace files in the codebase (stock, projects, manufacturing, buying) still reference the old domain and may need similar updates for consistency.
erpnext/workspace_sidebar/banking.json (2)
12-43: Banking operations reorganization looks well-structured.The primary banking operations (Bank Clearance, Bank Reconciliation, Reconciliation Report) are now prominently placed at the top, with appropriate icons and link types. The Reconciliation Report correctly uses
link_type: "Report"for the Bank Reconciliation Statement.
56-91: Setup section items are properly configured.Bank, Bank Account, and Plaid Settings are correctly marked with
child: 1to indicate they belong under the Setup section.erpnext/accounts/workspace/financial_reports/financial_reports.json (2)
2-8: Workspace configuration enhancements are well-structured.The additions of
app: "erpnext"and thechartsarray properly integrate the Profit and Loss chart into the Financial Reports workspace. The chart reference aligns with the updated dashboard chart configuration.
282-284: Explicit type declaration is a good practice.Adding
"type": "Workspace"makes the doctype explicit, improving clarity and consistency across workspace configurations.erpnext/selling/number_card/average_order_values_1/average_order_values_1.json (1)
10-21: No issues found. Thefunction: "Average"andreport_function: "Sum"combination is a standard and valid pattern in ERPNext Number Cards. The Average function calculates the average ofbase_rounded_totalacross filtered documents, while the Sum report function aggregates comparison statistics over the specified time interval. This same configuration is used identically in the buying module'saverage_order_valuescard.
erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.json
Outdated
Show resolved
Hide resolved
ruthra-kumar
force-pushed
the
refactor_workspace_charts
branch
from
f8ff3c3 to
a06adea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
erpnext/workspace_sidebar/accounting.json (1)
179-189: Fix the typo in the label.The label "Payment Reconciliaition" contains a typo. It should be "Payment Reconciliation" to match the
link_tofield.🔎 Proposed fix
{ "child": 1, "collapsible": 1, "indent": 0, "keep_closed": 0, - "label": "Payment Reconciliaition", + "label": "Payment Reconciliation", "link_to": "Payment Reconciliation", "link_type": "DocType", "show_arrow": 0, "type": "Link" },
♻️ Duplicate comments (2)
erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.json (1)
17-17: Personal email in owner field.The
ownerfield contains a personal email (john@example.in) instead ofAdministrator. Standard ERPNext fixtures should useAdministratorto maintain consistency and avoid exposing personal information.🔎 Proposed fix
- "owner": "john@example.in", + "owner": "Administrator",erpnext/workspace_sidebar/accounting.json (1)
190-200: Fix the typo in the label.The label "Process Payment Reconciliaition" contains a typo. It should be "Process Payment Reconciliation" (note the correct spelling in the
link_tofield).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.jsonerpnext/accounts/workspace/financial_reports/financial_reports.jsonerpnext/accounts/workspace/payables/payables.jsonerpnext/accounts/workspace/receivables/receivables.jsonerpnext/desktop_icon/accounting.jsonerpnext/desktop_icon/assets.jsonerpnext/desktop_icon/banking.jsonerpnext/desktop_icon/buying.jsonerpnext/desktop_icon/crm.jsonerpnext/desktop_icon/financial_reports.jsonerpnext/desktop_icon/manufacturing.jsonerpnext/desktop_icon/opening_&_closing.jsonerpnext/desktop_icon/payables.jsonerpnext/desktop_icon/projects.jsonerpnext/desktop_icon/quality.jsonerpnext/desktop_icon/receivables.jsonerpnext/desktop_icon/selling.jsonerpnext/desktop_icon/stock.jsonerpnext/desktop_icon/subscription.jsonerpnext/desktop_icon/support.jsonerpnext/selling/number_card/average_order_values_1/average_order_values_1.jsonerpnext/selling/report/sales_order_trends/sales_order_trends.pyerpnext/selling/workspace/selling/selling.jsonerpnext/workspace_sidebar/accounting.jsonerpnext/workspace_sidebar/banking.jsonerpnext/workspace_sidebar/budget.jsonerpnext/workspace_sidebar/payables.jsonerpnext/workspace_sidebar/receivables.jsonerpnext/workspace_sidebar/subscription.json
💤 Files with no reviewable changes (7)
- erpnext/desktop_icon/opening_&_closing.json
- erpnext/desktop_icon/receivables.json
- erpnext/accounts/workspace/receivables/receivables.json
- erpnext/accounts/workspace/payables/payables.json
- erpnext/workspace_sidebar/payables.json
- erpnext/desktop_icon/payables.json
- erpnext/workspace_sidebar/receivables.json
🚧 Files skipped from review as they are similar to previous changes (9)
- erpnext/desktop_icon/subscription.json
- erpnext/selling/report/sales_order_trends/sales_order_trends.py
- erpnext/desktop_icon/selling.json
- erpnext/desktop_icon/stock.json
- erpnext/desktop_icon/banking.json
- erpnext/desktop_icon/financial_reports.json
- erpnext/selling/number_card/average_order_values_1/average_order_values_1.json
- erpnext/selling/workspace/selling/selling.json
- erpnext/desktop_icon/projects.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-17T14:11:06.959Z
Learnt from: sagarvora
Repo: frappe/erpnext PR: 50155
File: erpnext/controllers/accounts_controller.py:2992-3005
Timestamp: 2025-10-17T14:11:06.959Z
Learning: In ERPNext, item child doctypes (like "Sales Invoice Item", "Delivery Note Item", etc.) have exactly one non-custom Link field that references their parent transaction doctype (like "Sales Order", "Purchase Order", etc.). This is a schema design pattern that can be relied upon when determining reference fields for mapping logic.
Applied to files:
erpnext/workspace_sidebar/accounting.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (15)
erpnext/desktop_icon/accounting.json (1)
3-17: Routine metadata update – looks good.The timestamp and parent icon updates align with the broader desktop icon reorganization under ERPNext branding.
erpnext/desktop_icon/crm.json (1)
3-17: Routine metadata update – looks good.The timestamp, idx, and parent icon changes are consistent with the desktop icon reorganization.
erpnext/desktop_icon/assets.json (1)
3-17: Routine metadata update – looks good.The changes align with the broader ERPNext branding consolidation across desktop icons.
erpnext/desktop_icon/quality.json (1)
3-17: Routine metadata update – looks good.The metadata changes are consistent with the ERPNext desktop icon reorganization.
erpnext/desktop_icon/support.json (1)
3-17: Routine metadata update – looks good.The timestamp and parent icon updates follow the established pattern for desktop icon reorganization.
erpnext/desktop_icon/manufacturing.json (1)
3-17: Routine metadata update – looks good.The changes are consistent with the desktop icon reorganization under ERPNext branding.
erpnext/desktop_icon/buying.json (1)
3-17: Routine metadata update – looks good.The metadata changes align with the broader desktop icon consolidation effort.
erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.json (1)
20-22: Chart visualization changes look intentional.The chart type change from Bar to Line and the addition of
show_values_over_chartenhance the Profit and Loss chart display as intended by the PR objectives.erpnext/accounts/workspace/financial_reports/financial_reports.json (2)
282-283: LGTM! Standard workspace type metadata added.The
titleandtype: "Workspace"fields are standard Frappe doctype metadata that help identify and categorize this configuration correctly.
2-8: LGTM! Workspace metadata and chart reference are correctly structured.The addition of the
appfield and the embedded "Profit and Loss" chart follows standard Frappe workspace conventions. The chart object structure withchart_nameandlabelis correct. The referenced "Profit and Loss" dashboard chart exists in the codebase and is properly configured as a standard Dashboard Chart with report-based visualization.erpnext/workspace_sidebar/subscription.json (1)
45-88: LGTM! Logical addition of Setup section.The new Setup section with Customer, Supplier, and Item links is a logical enhancement to the Subscription workspace, providing quick access to frequently needed master data.
erpnext/workspace_sidebar/budget.json (1)
60-91: LGTM! Improved organization of Budget workspace.The restructuring logically separates the Monthly Distribution DocType from the Budget Variance Report by introducing a dedicated Reports section. This improves navigation clarity.
erpnext/workspace_sidebar/banking.json (2)
45-91: LGTM! Well-organized Setup section.The new Setup section logically groups Bank, Bank Account, and Plaid Settings, improving the sidebar navigation structure. This is consistent with the organizational pattern applied across other workspace sidebars in this PR.
12-43: All DocType and Report references in the updated banking.json are valid. Verification confirms:
Bank Clearanceis a valid DocTypeBank Reconciliation Toolis a valid DocTypeBank Reconciliation Statementis a valid Report (making the link_type change from DocType to Report correct)erpnext/workspace_sidebar/accounting.json (1)
245-315: All DocType references in the "Opening & Closing" section are valid and exist in the codebase. Each of the 5 referenced DocTypes has proper definitions:
- Chart of Accounts Importer
- Opening Invoice Creation Tool
- Accounting Period
- Exchange Rate Revaluation
- Period Closing Voucher
The section structure, icons, labels, and mappings are correct.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #51286 +/- ##
========================================
Coverage 79.06% 79.06%
========================================
Files 1179 1179
Lines 121203 121203
========================================
+ Hits 95828 95833 +5
+ Misses 25375 25370 -5
🚀 New features to boost your workflow:
|
ruthra-kumar
force-pushed
the
refactor_workspace_charts
branch
from
a06adea to
9e9eb47
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
erpnext/workspace_sidebar/accounting.json (1)
190-200: Fix the typo in the label (duplicate issue).The label "Process Payment Reconciliaition" contains a typo that was already flagged in a previous review. It should be "Process Payment Reconciliation" to match the
link_tofield.🔎 Proposed fix
{ "child": 1, "collapsible": 1, "indent": 0, "keep_closed": 0, - "label": "Process Payment Reconciliaition", + "label": "Process Payment Reconciliation", "link_to": "Process Payment Reconciliation", "link_type": "DocType", "show_arrow": 0, "type": "Link" },
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.jsonerpnext/accounts/workspace/accounting/accounting.jsonerpnext/accounts/workspace/financial_reports/financial_reports.jsonerpnext/accounts/workspace/payables/payables.jsonerpnext/accounts/workspace/receivables/receivables.jsonerpnext/desktop_icon/accounting.jsonerpnext/desktop_icon/assets.jsonerpnext/desktop_icon/banking.jsonerpnext/desktop_icon/buying.jsonerpnext/desktop_icon/crm.jsonerpnext/desktop_icon/financial_reports.jsonerpnext/desktop_icon/manufacturing.jsonerpnext/desktop_icon/opening_&_closing.jsonerpnext/desktop_icon/payables.jsonerpnext/desktop_icon/projects.jsonerpnext/desktop_icon/quality.jsonerpnext/desktop_icon/receivables.jsonerpnext/desktop_icon/selling.jsonerpnext/desktop_icon/stock.jsonerpnext/desktop_icon/subscription.jsonerpnext/desktop_icon/support.jsonerpnext/selling/number_card/average_order_values_1/average_order_values_1.jsonerpnext/selling/report/sales_order_trends/sales_order_trends.pyerpnext/selling/workspace/selling/selling.jsonerpnext/workspace_sidebar/accounting.jsonerpnext/workspace_sidebar/banking.jsonerpnext/workspace_sidebar/budget.jsonerpnext/workspace_sidebar/payables.jsonerpnext/workspace_sidebar/receivables.jsonerpnext/workspace_sidebar/subscription.json
💤 Files with no reviewable changes (7)
- erpnext/accounts/workspace/receivables/receivables.json
- erpnext/workspace_sidebar/receivables.json
- erpnext/workspace_sidebar/payables.json
- erpnext/desktop_icon/opening_&_closing.json
- erpnext/desktop_icon/receivables.json
- erpnext/accounts/workspace/payables/payables.json
- erpnext/desktop_icon/payables.json
🚧 Files skipped from review as they are similar to previous changes (11)
- erpnext/selling/report/sales_order_trends/sales_order_trends.py
- erpnext/desktop_icon/selling.json
- erpnext/desktop_icon/accounting.json
- erpnext/selling/number_card/average_order_values_1/average_order_values_1.json
- erpnext/desktop_icon/support.json
- erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.json
- erpnext/desktop_icon/manufacturing.json
- erpnext/desktop_icon/buying.json
- erpnext/desktop_icon/assets.json
- erpnext/desktop_icon/crm.json
- erpnext/desktop_icon/projects.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-17T14:11:06.959Z
Learnt from: sagarvora
Repo: frappe/erpnext PR: 50155
File: erpnext/controllers/accounts_controller.py:2992-3005
Timestamp: 2025-10-17T14:11:06.959Z
Learning: In ERPNext, item child doctypes (like "Sales Invoice Item", "Delivery Note Item", etc.) have exactly one non-custom Link field that references their parent transaction doctype (like "Sales Order", "Purchase Order", etc.). This is a schema design pattern that can be relied upon when determining reference fields for mapping logic.
Applied to files:
erpnext/workspace_sidebar/accounting.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Patch Test
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Summary
🔇 Additional comments (19)
erpnext/workspace_sidebar/budget.json (2)
57-68: LGTM!The Monthly Distribution link is properly configured with consistent properties.
69-91: Verify indent/child consistency for the Reports section.The Section Break has
indent: 1while preceding items haveindent: 0, and the Budget Variance item haschild: 1butindent: 0. If Budget Variance is intended to be a child of the Reports section, itsindentshould likely be1or higher to maintain visual hierarchy.Additionally,
link_type: "DocType"on a Section Break (line 76) appears unnecessary since section breaks don't link anywhere.🔎 Suggested fix for indent consistency
{ "child": 1, "collapsible": 1, "icon": "file-text", - "indent": 0, + "indent": 1, "keep_closed": 0, "label": "Budget Variance", "link_to": "Budget Variance Report", "link_type": "Report", "show_arrow": 0, "type": "Link" }erpnext/desktop_icon/financial_reports.json (1)
3-17: LGTM! Consistent refactoring of desktop icon metadata.The changes align with the PR's objective to cleanup and standardize desktop icon metadata across ERPNext. The
parent_iconupdate to "ERPNext" andidxreset are consistent with other desktop icon updates in this PR.erpnext/desktop_icon/quality.json (1)
3-17: LGTM! Consistent desktop icon cleanup.These metadata updates (timestamps,
idx,logo_urlremoval,parent_iconstandardization) are part of the systematic refactoring across desktop icons.erpnext/workspace_sidebar/subscription.json (1)
44-88: LGTM! Logical addition of setup entities to Subscription workspace.Adding a "Setup" section with links to Customer, Supplier, and Item provides convenient access to related master data from the Subscription workspace. The structure and nesting are appropriate.
erpnext/accounts/workspace/accounting/accounting.json (2)
594-610: LGTM! Cleaner number card labels.Simplifying the labels by removing the "Total" prefix improves readability while maintaining the correct
number_card_namereferences for functionality.
17-17: Note: Workspace ordering changed.The
idxvalue changed from 0 to 3, which will affect the display order of this workspace in the UI. Ensure this reordering aligns with the intended user experience.erpnext/desktop_icon/subscription.json (1)
9-9: Unusual idx value - verify intentional de-prioritization.Setting
idxto 99 will move the Subscription icon to the end of the desktop navigation. This is a significant UX change. Please confirm this de-prioritization is intentional.erpnext/desktop_icon/stock.json (1)
3-17: LGTM! Consistent desktop icon refactoring.These changes follow the same pattern as other desktop icon updates in this PR, standardizing metadata and namespace organization.
erpnext/desktop_icon/banking.json (1)
11-11: No action needed. The "Bank Clearance" DocType is properly defined in the codebase (erpnext/accounts/doctype/bank_clearance/) with complete implementation and tests. The change is correct.erpnext/workspace_sidebar/accounting.json (1)
245-315: All referenced DocTypes in the "Opening & Closing" section exist and are correctly linked.erpnext/workspace_sidebar/banking.json (2)
45-55: LGTM! Setup section structure is well-organized.The new Setup section break properly groups configuration items with:
- Appropriate database icon
- Collapsed by default (
keep_closed: 1) to reduce clutter- Proper indentation for hierarchical organization
56-91: No action required. Child items under section breaks consistently useindent: 0across all workspace sidebar files in ERPNext. This is the intentional pattern and not an issue—thechild: 1marker withindent: 0is the standard structure for items grouped under a section break, regardless of the parent section's indent value.Likely an incorrect or invalid review comment.
erpnext/accounts/workspace/financial_reports/financial_reports.json (4)
9-9: LGTM! Content layout is well-structured.The content field properly defines a workspace layout with:
- Full-width Profit and Loss chart
- Spacer for visual separation
- Four cards (Financial Statements, Ledgers, Profitability, Other Reports) that match the Card Break labels in the links array
16-16: LGTM! Icon change is appropriate.Changing the icon from "file" to "table" better represents the nature of financial reports, which are typically presented in tabular format.
282-283: LGTM! Type field addition standardizes the schema.Adding the explicit
"type": "Workspace"field improves schema clarity and consistency with other workspace definitions.
2-8: The "Profit and Loss" chart exists and is properly configured. The workspace reference is valid—no action required.erpnext/selling/workspace/selling/selling.json (2)
648-648: LGTM! URL update improves security and tracking.The Learn Sales Management shortcut URL has been properly updated to:
- Use HTTPS for secure connections
- Link directly to the specific course page
- Include UTM tracking (
utm_source=in_app) for analytics
676-676: Thereport_ref_doctypefield is a valid and widely-established field in ERPNext workspace configuration. It is used consistently across multiple workspace modules (selling, buying, stock, manufacturing) to associate report shortcuts with their primary DocType. The value "Sales Order" is appropriate for the Sales Analytics report.
ruthra-kumar
force-pushed
the
refactor_workspace_charts
branch
from
9e9eb47 to
666c77d
Compare
|
@CodeRabbit resolve |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
erpnext/selling/workspace/selling/selling.json (1)
9-9: Critical: Number card name mismatches between content and definitions.The
contentfield references number card names that don't match thenumber_cardsarray definitions:
- Content references
"Sales Orders"but Line 636 defines"Sales Orders Count"- Content references
"Average Order Value"but Line 640 defines"Average Sales Order Value"Only
"Total Sales Amount"matches correctly. This will prevent the number cards from displaying in the workspace.🔎 Proposed fix - Update content field to match number_cards definitions
The content field is a minified JSON string. The three number card entries should be:
-{"id":"43fzlS1qZg","type":"number_card","data":{"number_card_name":"Sales Orders","col":4}} +{"id":"43fzlS1qZg","type":"number_card","data":{"number_card_name":"Sales Orders Count","col":4}} -{"id":"0Ioq-P11FP","type":"number_card","data":{"number_card_name":"Average Order Value","col":4}} +{"id":"0Ioq-P11FP","type":"number_card","data":{"number_card_name":"Average Sales Order Value","col":4}}Run this script to confirm the actual number card names:
#!/bin/bash # Verify the actual number card names defined in the Selling module echo "=== Sales Orders number card ===" fd --type f "sales_orders" erpnext/selling/number_card/ --exec cat {} | grep -E '"name"|"label"' echo -e "\n=== Average Sales Order Value number card ===" fd --type f "average_sales_order_value" erpnext/selling/number_card/ --exec cat {} | grep -E '"name"|"label"' echo -e "\n=== Total Sales Amount number card ===" fd --type f "total_sales_amount" erpnext/selling/number_card/ --exec cat {} | grep -E '"name"|"label"'
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.jsonerpnext/accounts/workspace/accounting/accounting.jsonerpnext/accounts/workspace/financial_reports/financial_reports.jsonerpnext/accounts/workspace/payables/payables.jsonerpnext/accounts/workspace/receivables/receivables.jsonerpnext/desktop_icon/accounting.jsonerpnext/desktop_icon/assets.jsonerpnext/desktop_icon/banking.jsonerpnext/desktop_icon/buying.jsonerpnext/desktop_icon/crm.jsonerpnext/desktop_icon/financial_reports.jsonerpnext/desktop_icon/manufacturing.jsonerpnext/desktop_icon/opening_&_closing.jsonerpnext/desktop_icon/payables.jsonerpnext/desktop_icon/projects.jsonerpnext/desktop_icon/quality.jsonerpnext/desktop_icon/receivables.jsonerpnext/desktop_icon/selling.jsonerpnext/desktop_icon/stock.jsonerpnext/desktop_icon/subscription.jsonerpnext/desktop_icon/support.jsonerpnext/selling/number_card/average_sales_order_value/average_sales_order_value.jsonerpnext/selling/number_card/sales_orders_count/sales_orders_count.jsonerpnext/selling/number_card/total_sales_amount/total_sales_amount.jsonerpnext/selling/report/sales_order_trends/sales_order_trends.pyerpnext/selling/workspace/selling/selling.jsonerpnext/workspace_sidebar/accounting.jsonerpnext/workspace_sidebar/banking.jsonerpnext/workspace_sidebar/budget.jsonerpnext/workspace_sidebar/payables.jsonerpnext/workspace_sidebar/receivables.jsonerpnext/workspace_sidebar/subscription.json
💤 Files with no reviewable changes (7)
- erpnext/desktop_icon/payables.json
- erpnext/accounts/workspace/payables/payables.json
- erpnext/desktop_icon/opening_&_closing.json
- erpnext/workspace_sidebar/receivables.json
- erpnext/desktop_icon/receivables.json
- erpnext/workspace_sidebar/payables.json
- erpnext/accounts/workspace/receivables/receivables.json
✅ Files skipped from review due to trivial changes (1)
- erpnext/selling/number_card/sales_orders_count/sales_orders_count.json
🚧 Files skipped from review as they are similar to previous changes (11)
- erpnext/selling/report/sales_order_trends/sales_order_trends.py
- erpnext/desktop_icon/support.json
- erpnext/workspace_sidebar/accounting.json
- erpnext/desktop_icon/manufacturing.json
- erpnext/desktop_icon/projects.json
- erpnext/accounts/workspace/accounting/accounting.json
- erpnext/desktop_icon/banking.json
- erpnext/desktop_icon/stock.json
- erpnext/workspace_sidebar/subscription.json
- erpnext/desktop_icon/subscription.json
- erpnext/desktop_icon/crm.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T04:52:35.361Z
Learnt from: vorasmit
Repo: frappe/erpnext PR: 49098
File: erpnext/accounts/doctype/financial_report_row/financial_report_row.json:118-124
Timestamp: 2025-11-14T04:52:35.361Z
Learning: In ERPNext Financial Report Row (erpnext/accounts/doctype/financial_report_row), when data_source is "Account Data" and advanced_filtering is false, the filters_editor field provides a UI that automatically populates the calculation_formula field behind the scenes. The calculation_formula field can be hidden but mandatory in this case because it gets set automatically before validation occurs.
Applied to files:
erpnext/selling/number_card/average_sales_order_value/average_sales_order_value.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (18)
erpnext/workspace_sidebar/banking.json (5)
12-20: LGTM! Clear and consistent labeling.The icon, label, and link target are all aligned correctly for Bank Clearance functionality.
45-55: LGTM! Setup section follows established pattern.The new Setup section break structure is consistent with the existing Dunning section pattern in this file. The icon and collapsible configuration are appropriate.
56-91: LGTM! Setup items are well-structured.The Bank, Bank Account, and Plaid Settings entries are properly configured as children of the Setup section. Labels match link targets, and icons are appropriate.
36-44: The "Bank Reconciliation Statement" report exists and is correctly configured. The link_to reference and link_type are valid.
24-32: No action needed. The Bank Reconciliation Tool DocType exists and is properly configured. The label-to-link discrepancy is intentional and follows Frappe conventions (user-friendly labels paired with system DocType names).erpnext/desktop_icon/financial_reports.json (1)
3-3: LGTM: Consistent with desktop icon reorganization.The changes align with the broader desktop icon hierarchy consolidation. The
idxchange from 7 to 0 may affect icon ordering in the UI.Also applies to: 9-9, 13-13, 17-17
erpnext/workspace_sidebar/budget.json (2)
57-68: LGTM: Item reordered in sidebar.The change from "Budget Variance" report to "Monthly Distribution" DocType at this position reflects the sidebar reorganization to move reports into a dedicated section.
69-91: Structure is correct and consistent with other workspace sidebars.The new Reports section follows the established pattern across workspace sidebars. The Section Break correctly uses
child: 0withkeep_closed: 1, and the nested Budget Variance item useschild: 1withindent: 0. This structure matches the pattern used in accounting.json, banking.json, and subscription.json.erpnext/desktop_icon/assets.json (1)
3-3: LGTM: Consistent desktop icon updates.The changes follow the same pattern as other desktop icons: updated timestamps,
idxreset to 1,logo_urlremoval, andparent_iconset to "ERPNext".Also applies to: 9-9, 13-13, 17-17
erpnext/desktop_icon/selling.json (1)
3-3: LGTM: Consistent with desktop icon reorganization.The updates mirror the pattern across all desktop icons in this PR, consolidating the hierarchy under ERPNext.
Also applies to: 9-9, 13-13, 17-17
erpnext/desktop_icon/quality.json (1)
3-3: LGTM: Desktop icon metadata standardized.The changes are consistent with the desktop icon hierarchy reorganization applied across all icons in this PR.
Also applies to: 9-9, 13-13, 17-17
erpnext/desktop_icon/buying.json (1)
3-3: LGTM: Completes the desktop icon hierarchy consolidation.This final desktop icon update maintains consistency with all other icons in the PR, successfully consolidating the navigation hierarchy under ERPNext.
Also applies to: 9-9, 13-13, 17-17
erpnext/selling/number_card/total_sales_amount/total_sales_amount.json (1)
1-26: The field configuration is correct.The
base_rounded_totalfield exists in the Sales Order doctype as a Currency field with label "Rounded Total (Company Currency)". This is the appropriate field for aggregating total sales amount.erpnext/desktop_icon/accounting.json (1)
3-3: LGTM: Accounting icon correctly consolidated to ERPNext hierarchy.The parent_icon change consolidates accounting to point directly to "ERPNext" as the top-level parent, aligning with the main module reorganization. Related accounting sub-features (banking, subscription, taxes, budget) appropriately retain "Accounts" as their parent, creating a consistent two-tier hierarchy. No navigation or workflow issues detected.
erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.json (1)
20-22: LGTM! Chart type conversion aligns with PR objectives.The conversion from Bar to Line chart and the addition of
show_values_over_chartimprove the visualization of Profit & Loss trends over time.erpnext/accounts/workspace/financial_reports/financial_reports.json (1)
3-8: LGTM! P&L chart integration complete.The Profit and Loss chart is properly added to the Financial Reports workspace, aligning with the updated chart configuration that converts it to a Line chart.
erpnext/selling/workspace/selling/selling.json (2)
656-656: LGTM! Updated training URL.The Learn Sales Management URL has been updated to use the current
frappe.schooldomain.
684-684: Good addition of report reference doctype.Adding
report_ref_doctypeto the Sales Analytics shortcut provides better context for filtering and navigation.
erpnext/selling/number_card/average_sales_order_value/average_sales_order_value.json
Show resolved
Hide resolved
✅ Actions performedComments resolved. |
1 participant
Uh oh!
There was an error while loading. Please reload this page.