feat!: document naming rule will now use posting date of the document #51433
Conversation
github-actions
bot
added
accounts
needs-tests
📝 WalkthroughWalkthroughAdds TFY support and truncation for fiscal-year naming: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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: 1
🧹 Nitpick comments (1)
erpnext/hooks.py (1)
405-409: LGTM: Clean refactor of naming series variables.The refactor from a hardcoded dictionary to a list combined with dict comprehension makes it easier to maintain and extend the supported naming series variables. All variables now consistently map to
erpnext.accounts.utils.parse_naming_series_variable, which handles each variable type appropriately.If you anticipate frequently adding new variables, consider adding a docstring or comment documenting what each variable represents:
# Variables for document naming series: # FY: Full fiscal year (e.g., "2025-2026") # TFY: Truncated fiscal year (e.g., "25-26") # ABBR: Company abbreviation # YY, YYYY, MM, DD, JJJ: Date format codes # WW: Week number naming_series_variables_list = ["FY", "TFY", "ABBR", "MM", "DD", "YY", "YYYY", "JJJ", "WW"]
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/utils.pyerpnext/hooks.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/accounts/utils.pyerpnext/hooks.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 (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/accounts/utils.py (2)
1513-1520: LGTM: TFY variable support added correctly.The function now handles both "FY" and "TFY" variables, passing the appropriate
truncateparameter toget_fiscal_year. The truncation logic will format fiscal years like "2025-2026" as "25-26".
86-92: No changes needed. The function behavior is safe and maintains backward compatibility.The return value of
Falseon line 92 only occurs whenraise_on_missing=Falseis explicitly passed. All three callers in the codebase that use this flag already have defensive checks (if not fiscal_year:orif fiscal_year and ...) that handle both empty lists andFalseidentically. The default behavior (raise_on_missing=True) continues to raiseFiscalYearErroras before, so the 40+ callers using direct indexing are unaffected.
mihir-kandoi
changed the title
mihir-kandoi
added
needs-tests
mihir-kandoi
force-pushed
the
naming-rule-based-on-posting-date
branch
from
f272066 to
e9b6145
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
erpnext/accounts/utils.py (1)
1530-1536: Address fallback behavior and variable naming issues (previously flagged).This segment has multiple issues that were identified in a previous review:
- Line 1536: Falls back to
determine_consecutive_week_numberfor ANY unknown variable, which masks errors for misspelled/unsupported variables.- Lines 1532-1534: Inconsistent fallback - uses
now_datetime()here but the FY/TFY branch usesgetdate().- Line 1534: Variable named
datebut assigned a datetime object, should be renamed todtordatetime_obj.The previous review suggested:
- Use consistent fallback approach (prefer
getdate())- Rename the variable for clarity
- Raise an explicit error for unknown variables instead of silently calling
determine_consecutive_week_number- Only call
determine_consecutive_week_numberfor the "WW" variable explicitly
🧹 Nitpick comments (1)
erpnext/accounts/utils.py (1)
85-92: Add validation for fiscal year format when truncating.The truncation logic assumes the fiscal year name follows a "YYYY-YYYY" format with a hyphen separator. If the format differs, the code could fail silently or produce incorrect results.
Consider adding validation:
🔎 Proposed validation
if fiscal_years: fiscal_year = fiscal_years[0] if truncate: - return ("-".join(y[-2:] for y in fiscal_year[0].split("-")), fiscal_year[1], fiscal_year[2]) + year_parts = fiscal_year[0].split("-") + if len(year_parts) != 2: + frappe.throw(_("Fiscal year name must be in 'YYYY-YYYY' format for truncation")) + truncated_year = "-".join(part[-2:] for part in year_parts) + return (truncated_year, fiscal_year[1], fiscal_year[2]) return fiscal_year return False
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/utils.pyerpnext/hooks.py
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/hooks.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/accounts/utils.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 (3)
- GitHub Check: Patch Test
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/accounts/utils.py (2)
14-14: LGTM! Imports support new naming functionality.The imports for
determine_consecutive_week_numberandnow_datetimeare necessary for the date-based naming series variables implementation.Also applies to: 29-29
1512-1520: LGTM! TFY variable support is correctly implemented.The logic properly handles both FY and TFY variables, using posting_date as the primary date source (aligning with PR objectives), and correctly passes the truncate parameter to
get_fiscal_year.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #51433 +/- ##
===========================================
- Coverage 79.05% 79.04% -0.01%
===========================================
Files 1179 1179
Lines 121306 121327 +21
===========================================
+ Hits 95897 95908 +11
- Misses 25409 25419 +10
🚀 New features to boost your workflow:
|
|
This can be made non-breakinh easily by making it configurable. |
2 participants
This PR does two things:
TFY(truncated fiscal year). This will enable users to use truncated fiscal year names (25-26 instead of the full 2025-2026).Limitations:
When the posting date/time is edited manually, the name will not update.
frappe/frappe#35586 has to be merged before this is merged
Note: This is a breaking change, merge carefully
no-docs