feat: Letter Management feature standardize official correspondence. #50918
Conversation
- Letter Type master for categorizing letter formats - Letter Template (Jinja) with subject and content placeholders - Letter (Submittable) with dynamic recipient (Customer / Supplier / Employee / Shareholder / Contact) - Auto-fetch recipient name and address - Company and recipient address display formatting - Letter template rendering into letter content - Letterhead selection from company defaults - Minimal professional print format (letterhead + footer) This makes creating, formatting and printing official letters fast and consistent, without manual formatting each time.
📝 WalkthroughWalkthroughAdds a new Letter feature to ERPNext: three doctypes (Letter, Letter Template, Letter Type), client scripts for Letter and templates, a standard print format, backend logic for recipient resolution and template rendering, and test coverage (comprehensive tests for Letter; scaffolds for Letter Template and Letter Type). Several init.py files received license headers. JSON fixtures define doctypes, print format, and a test record for Letter Type. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
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: 6
🧹 Nitpick comments (6)
erpnext/crm/doctype/letter/test_letter.py (1)
16-22: Placeholder test class with no test methods.The Letter DocType includes significant logic for recipient validation, address resolution, and template rendering. Consider adding at least basic integration tests for core functionality before merging.
erpnext/crm/doctype/letter_type/letter_type.json (1)
31-44: Consider expanding permissions for Letter Type.Currently, only "System Manager" has access to Letter Type. Depending on the use case, other roles like "HR Manager", "Sales Manager", or "CRM Manager" may need to create or manage letter types for their departments.
erpnext/crm/doctype/letter_template/test_letter_template.py (1)
16-22: Empty test class – consider adding tests forget_letter_template.The
get_letter_templatefunction inletter_template.pyhas rendering logic that would benefit from test coverage, particularly:
- Template rendering with valid Jinja placeholders
- Handling of missing subject/content fields
- JSON string parsing for the
docparameterWould you like me to generate sample test cases for these scenarios?
erpnext/crm/doctype/letter/letter.json (1)
174-179: Consider using a Link field to Language doctype.The
languagefield is defined asDatatype, but ERPNext has aLanguagedoctype. Using aLinkfield would provide validation and a dropdown selection of available languages.{ "fieldname": "language", - "fieldtype": "Data", + "fieldtype": "Link", "label": "Print Language", + "options": "Language", "print_hide": 1 }erpnext/crm/doctype/letter_template/letter_template.py (1)
34-36: Consider handling JSON parse errors gracefully.If
docis an invalid JSON string,json.loads()will raise aJSONDecodeError. This could result in an unclear error message for API consumers.@frappe.whitelist() def get_letter_template(template_name, doc): if isinstance(doc, str): - doc = json.loads(doc) + try: + doc = json.loads(doc) + except json.JSONDecodeError: + frappe.throw(_("Invalid JSON format for doc parameter"))This requires adding the import:
from frappe import _erpnext/crm/doctype/letter/letter.py (1)
36-53: Recipient name resolution logic is sound; consider centralizing it to avoid duplication.
set_recipient_name()andget_recipient_name_field()give you a clean mapping fromrecipient_type→ name field, and a safe fallback to"name". The same mapping logic is reimplemented inget_recipient_details()below, which means adding a new recipient type or changing the mapping requires edits in two places.Extracting this mapping/name‑resolution into a small shared helper (e.g.
def resolve_recipient_name(recipient_type, recipient): ...) and using it both inLetter.set_recipient_name()andget_recipient_details()would keep behavior consistent and reduce maintenance overhead.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
erpnext/crm/doctype/letter/letter.js(1 hunks)erpnext/crm/doctype/letter/letter.json(1 hunks)erpnext/crm/doctype/letter/letter.py(1 hunks)erpnext/crm/doctype/letter/test_letter.py(1 hunks)erpnext/crm/doctype/letter_template/letter_template.js(1 hunks)erpnext/crm/doctype/letter_template/letter_template.json(1 hunks)erpnext/crm/doctype/letter_template/letter_template.py(1 hunks)erpnext/crm/doctype/letter_template/test_letter_template.py(1 hunks)erpnext/crm/doctype/letter_type/letter_type.js(1 hunks)erpnext/crm/doctype/letter_type/letter_type.json(1 hunks)erpnext/crm/doctype/letter_type/letter_type.py(1 hunks)erpnext/crm/doctype/letter_type/test_letter_type.py(1 hunks)erpnext/crm/print_format/letter_standard/letter_standard.json(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
erpnext/crm/doctype/letter/letter.js
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
erpnext/crm/doctype/letter/letter.json
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
🪛 Ruff (0.14.7)
erpnext/crm/doctype/letter/letter.py
28-28: Syntax error in forward annotation: Expected an expression
(F722)
32-32: Undefined name Customer
(F821)
32-32: Undefined name Supplier
(F821)
32-32: Undefined name Employee
(F821)
32-32: Undefined name Shareholder
(F821)
32-32: Undefined name Contact
(F821)
⏰ 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). (7)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Check Commit Titles
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (9)
erpnext/crm/doctype/letter_type/letter_type.js (1)
1-8: LGTM!Standard ERPNext DocType client-side scaffold. The commented-out placeholder is appropriate for a new DocType that doesn't require custom client-side logic yet.
erpnext/crm/doctype/letter_type/letter_type.py (1)
1-20: LGTM!Standard ERPNext DocType class scaffold with auto-generated type hints. The
passstatement is conventional for DocTypes that don't require custom server-side logic.erpnext/crm/doctype/letter_type/test_letter_type.py (1)
1-22: LGTM!Standard test scaffold for a simple master DocType. Given Letter Type's minimal logic (single unique field), the placeholder is acceptable.
erpnext/crm/doctype/letter/letter.js (1)
37-133: LGTM!The event handlers for
recipient,recipient_type,recipient_address,company,company_address, andletter_templatefollow consistent patterns for field dependencies, data fetching, and field clearing. The integration with server-side methods and ERPNext utilities is appropriate.erpnext/crm/doctype/letter_template/letter_template.js (1)
1-8: Scaffold file with commented boilerplate is acceptable.The file serves as a placeholder for future form event handling. If no immediate UI logic is planned, consider removing the commented code to keep the codebase clean, or leave it if you plan to add functionality soon.
erpnext/crm/doctype/letter/letter.json (1)
191-205: Permissions are limited to System Manager only.Currently, only System Manager can access Letters. Consider whether additional roles (e.g., HR Manager for employee letters, Sales Manager for customer letters) should have access based on your use case requirements.
erpnext/crm/print_format/letter_standard/letter_standard.json (1)
1-33: Well-structured print format with proper status handling.The print format template is comprehensive with:
- Proper status badges for CANCELLED and DRAFT states
- Good use of
_()for internationalization- Defensive fallback with
doc.content or ""- Clean CSS styling appropriate for official letters
erpnext/crm/doctype/letter_template/letter_template.py (2)
26-30: Good template validation on save.Validating subject and content templates during document save prevents invalid Jinja syntax from being stored, which would cause runtime errors during rendering.
43-47: Template rendering context is appropriately scoped.The rendering uses user-provided
docdata as context for a trusted template from the database. Sincevalidate_templateis called on save (lines 26-30), the templates themselves are validated for safe Jinja syntax.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #50918 +/- ##
===========================================
- Coverage 78.91% 78.89% -0.03%
===========================================
Files 1174 1180 +6
Lines 119373 119592 +219
===========================================
+ Hits 94200 94348 +148
- Misses 25173 25244 +71
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
erpnext/crm/doctype/letter/letter.py (1)
39-53: Recipient name resolution is sound; consider de-duplicating name-field logic
set_recipient_namedefensively picks the appropriate display field perrecipient_typeand falls back to"name"if that column doesn’t exist, which should work across the supported doctypes and customizations.The same mapping logic for
"Shareholder","Contact", and the<type>_namepattern is reimplemented inline inget_recipient_details. To keep behavior in sync long term, consider extracting this mapping into a small shared helper (or reusingget_recipient_name_fieldfrom there) and calling it from both places.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/crm/doctype/letter/letter.py(1 hunks)erpnext/crm/doctype/letter_template/letter_template.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
erpnext/crm/doctype/letter/letter.py (1)
erpnext/crm/doctype/letter_template/letter_template.py (1)
validate(26-30)
🪛 Ruff (0.14.7)
erpnext/crm/doctype/letter/letter.py
28-28: Syntax error in forward annotation: Expected an expression
(F722)
32-32: Undefined name Customer
(F821)
32-32: Undefined name Supplier
(F821)
32-32: Undefined name Employee
(F821)
32-32: Undefined name Shareholder
(F821)
32-32: Undefined name Contact
(F821)
⏰ 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 (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (1)
erpnext/crm/doctype/letter_template/letter_template.py (1)
26-30: Jinja validation hook invalidateis appropriateConditionally validating
subjectandcontentkeeps the hook simple and avoids unnecessary work when fields are empty; this matches how other templated doctypes typically validate Jinja.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
erpnext/crm/doctype/letter/letter.py (1)
64-64: Critical: Invalidthrowparameter will causeTypeErrorat runtime.This issue was previously flagged:
frappe.has_permission()does not accept athrowparameter. The call will fail withTypeError: has_permission() got an unexpected keyword argument 'throw'when invoked.The correct approach is to check the return value and raise explicitly:
- frappe.has_permission(recipient_type, doc=recipient, throw=True) + if not frappe.has_permission(recipient_type, doc=recipient): + frappe.throw(_("Not permitted to access {0} {1}").format(recipient_type, recipient), frappe.PermissionError)Ensure
from frappe import _is imported at the top if not already present.
🧹 Nitpick comments (2)
erpnext/crm/doctype/letter/letter.py (2)
66-75: Consider extracting name field resolution to eliminate duplication.The logic for determining
name_field(lines 66-70) and retrieving the recipient name (lines 72-75) duplicates the logic inget_recipient_name_field()(lines 47-53) andset_recipient_name()(lines 42-45).To improve maintainability, extract this into a shared module-level helper function:
def _get_recipient_name(recipient_type, recipient): """Helper to resolve recipient name field and retrieve value.""" name_field = ( "title" if recipient_type == "Shareholder" else ("full_name" if recipient_type == "Contact" else recipient_type.lower() + "_name") ) if frappe.db.has_column(recipient_type, name_field): return frappe.db.get_value(recipient_type, recipient, name_field) return frappe.db.get_value(recipient_type, recipient, "name")Then use it in both
set_recipient_name()andget_recipient_details().
56-57: Consider adding type hints for input validation.Adding type annotations will enable Frappe's request validation to reject invalid inputs:
@frappe.whitelist() -def get_recipient_details(recipient_type, recipient): +def get_recipient_details(recipient_type: str, recipient: str):This provides an additional layer of defense against malformed requests.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
erpnext/crm/doctype/letter/letter.py(1 hunks)erpnext/crm/doctype/letter/test_letter.py(1 hunks)erpnext/crm/doctype/letter_template/letter_template.py(1 hunks)erpnext/crm/doctype/letter_template/test_letter_template.py(1 hunks)erpnext/crm/doctype/letter_type/test_letter_type.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- erpnext/crm/doctype/letter_type/test_letter_type.py
- erpnext/crm/doctype/letter_template/letter_template.py
- erpnext/crm/doctype/letter_template/test_letter_template.py
- erpnext/crm/doctype/letter/test_letter.py
🧰 Additional context used
🪛 Ruff (0.14.7)
erpnext/crm/doctype/letter/letter.py
28-28: Syntax error in forward annotation: Expected an expression
(F722)
32-32: Undefined name Customer
(F821)
32-32: Undefined name Supplier
(F821)
32-32: Undefined name Employee
(F821)
32-32: Undefined name Shareholder
(F821)
32-32: Undefined name Contact
(F821)
⏰ 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 (2)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Patch Test
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
erpnext/crm/doctype/letter/letter.py (1)
71-80: Consider extracting the recipient name field logic to reduce duplication.The logic for determining
name_fieldand retrievingrecipient_name(lines 71-80) duplicates the implementation in theLetterclass methods (get_recipient_name_fieldandset_recipient_name, lines 48-54 and 40-46). This creates a maintenance burden—any changes to the name field mapping would need to be updated in both places.Consider extracting this logic into a module-level helper function that both the class and the whitelisted function can call:
+def _get_recipient_name_field(recipient_type: str) -> str: + """Get the appropriate name field for a given recipient type.""" + if recipient_type == "Shareholder": + return "title" + elif recipient_type == "Contact": + return "full_name" + else: + return recipient_type.lower() + "_name" + + class Letter(Document): # ... def get_recipient_name_field(self): - if self.recipient_type == "Shareholder": - return "title" - elif self.recipient_type == "Contact": - return "full_name" - else: - return self.recipient_type.lower() + "_name" + return _get_recipient_name_field(self.recipient_type)Then update
get_recipient_detailsto use the helper:- name_field = ( - "title" - if recipient_type == "Shareholder" - else ("full_name" if recipient_type == "Contact" else recipient_type.lower() + "_name") - ) + name_field = _get_recipient_name_field(recipient_type)This improves maintainability and readability by eliminating the nested ternary expression.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/crm/doctype/letter/letter.py(1 hunks)erpnext/crm/doctype/letter_template/letter_template.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
erpnext/crm/doctype/letter/letter.py (1)
erpnext/crm/doctype/letter_template/letter_template.py (1)
validate(26-30)
🪛 Ruff (0.14.7)
erpnext/crm/doctype/letter/letter.py
29-29: Syntax error in forward annotation: Expected an expression
(F722)
33-33: Undefined name Customer
(F821)
33-33: Undefined name Supplier
(F821)
33-33: Undefined name Employee
(F821)
33-33: Undefined name Shareholder
(F821)
33-33: Undefined name Contact
(F821)
⏰ 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 (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/crm/doctype/letter_template/letter_template.py (1)
26-53: LGTM! Security and best practices properly implemented.The implementation correctly addresses all previous security concerns:
- Type hints enable automatic request validation (line 34)
- Permission check enforced before template access (line 42)
- Jinja template validation prevents injection (lines 27-30)
- Proper error handling for invalid JSON input (lines 35-39)
erpnext/crm/doctype/letter/letter.py (1)
37-54: LGTM! Validation logic is clean and correct.The recipient name resolution is properly implemented with appropriate fallback logic when the expected name field doesn't exist.
I followed the same approach used in Payment Entry. In Payment Entry, the party_name is fetched using this logic, and I’ve applied a similar concept to fetch recipient_name. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
erpnext/crm/doctype/letter/letter.js (1)
67-109: Consider extracting address display logic (optional).The recipient_address and company_address handlers share identical logic for fetching and displaying addresses. If this pattern appears elsewhere in the codebase, a small helper function could reduce duplication:
function update_address_display(frm, address_field, display_field) { if (frm.doc[address_field]) { frappe.call({ method: "frappe.contacts.doctype.address.address.get_address_display", args: { address_dict: frm.doc[address_field] }, callback: function (r) { if (r.message) { frm.set_value(display_field, r.message); } }, }); } else { frm.set_value(display_field, ""); } }However, given the simplicity and clarity of the inline approach, this refactor is entirely optional.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/crm/doctype/letter/letter.js(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
erpnext/crm/doctype/letter/letter.js
[error] 1-1: pre-commit: trailing-whitespace hook modified files and exited with code 1. Changes were applied to erpnext/crm/doctype/letter/letter.js.
⏰ 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 (2)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (1)
erpnext/crm/doctype/letter/letter.js (1)
111-131: Well-structured template handler.The letter_template handler correctly fetches and populates template data with appropriate null checks. The approach follows ERPNext conventions and integrates cleanly with the backend API.
|
@aadhilpm Currently, we are not adding any new features to ERPNext's CRM Module. Check out Frappe CRM. Also, I recommend opening a GitHub issue for a feature request first and then working on it. |
|
Thanks for the update. Can you please suggest which module this feature would be suitable for? I added it under this module because DocTypes like Contract, Appointment, etc. already belong here. If there is a better place, I will update the code accordingly. |
|
@aadhilpm you can add a feature request on Frappe CRM. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
erpnext/utilities/doctype/letter_template/letter_template.json (1)
42-42: Update the Jinja documentation link.The help HTML contains an outdated Jinja documentation link (
http://jinja.pocoo.org/docs/dev/templates/). Update it to the current official URL:https://jinja.palletsprojects.com/en/stable/.
🧹 Nitpick comments (4)
erpnext/utilities/doctype/letter_type/test_letter_type.py (1)
14-20: Consider adding basic CRUD tests.While LetterType is a simple master doctype, adding basic tests for creation and validation would improve coverage.
erpnext/utilities/doctype/letter/letter.py (1)
48-54: Consider extracting recipient name field logic (already discussed).The logic for determining the recipient name field is duplicated in lines 71-75 of
get_recipient_details(). While the PR author noted this follows the Payment Entry pattern, extracting it to a module-level helper would reduce duplication and simplify maintenance.However, given this was already discussed and defended as following established patterns, this is optional.
erpnext/utilities/doctype/letter/letter.js (2)
37-51: Add error handling to server calls.None of the
frappe.callcallbacks handle errors. If server methods fail, users won't receive feedback. Consider adding error handlers to improve UX:frappe.call({ method: "...", args: { ... }, callback: function(r) { // success handling }, error: function(r) { frappe.msgprint(__("Failed to load data. Please try again.")); } });Also applies to: 69-79, 95-105, 113-129
67-83: Consider extracting address display logic.The
recipient_addressandcompany_addresshandlers have nearly identical logic. You could extract a helper function to reduce duplication:function update_address_display(frm, address_field, display_field) { if (frm.doc[address_field]) { frappe.call({ method: "frappe.contacts.doctype.address.address.get_address_display", args: { address_dict: frm.doc[address_field] }, callback: function(r) { if (r.message) { frm.set_value(display_field, r.message); } } }); } else { frm.set_value(display_field, ""); } }However, this pattern is common in ERPNext, so the current approach is acceptable.
Also applies to: 93-109
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
erpnext/utilities/doctype/letter/__init__.py(1 hunks)erpnext/utilities/doctype/letter/letter.js(1 hunks)erpnext/utilities/doctype/letter/letter.json(1 hunks)erpnext/utilities/doctype/letter/letter.py(1 hunks)erpnext/utilities/doctype/letter/test_letter.py(1 hunks)erpnext/utilities/doctype/letter_template/__init__.py(1 hunks)erpnext/utilities/doctype/letter_template/letter_template.js(1 hunks)erpnext/utilities/doctype/letter_template/letter_template.json(1 hunks)erpnext/utilities/doctype/letter_template/letter_template.py(1 hunks)erpnext/utilities/doctype/letter_template/test_letter_template.py(1 hunks)erpnext/utilities/doctype/letter_type/__init__.py(1 hunks)erpnext/utilities/doctype/letter_type/letter_type.js(1 hunks)erpnext/utilities/doctype/letter_type/letter_type.json(1 hunks)erpnext/utilities/doctype/letter_type/letter_type.py(1 hunks)erpnext/utilities/doctype/letter_type/test_letter_type.py(1 hunks)erpnext/utilities/print_format/__init__.py(1 hunks)erpnext/utilities/print_format/letter_standard/__init__.py(1 hunks)erpnext/utilities/print_format/letter_standard/letter_standard.json(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- erpnext/utilities/doctype/letter_template/letter_template.js
- erpnext/utilities/doctype/letter_type/init.py
- erpnext/utilities/doctype/letter_template/init.py
- erpnext/utilities/doctype/letter_type/letter_type.js
- erpnext/utilities/print_format/init.py
- erpnext/utilities/print_format/letter_standard/init.py
- erpnext/utilities/doctype/letter/init.py
🧰 Additional context used
🧬 Code graph analysis (1)
erpnext/utilities/doctype/letter/letter.py (1)
erpnext/utilities/doctype/letter_template/letter_template.py (1)
validate(26-30)
🪛 Ruff (0.14.7)
erpnext/utilities/doctype/letter/letter.py
29-29: Syntax error in forward annotation: Expected an expression
(F722)
33-33: Undefined name Customer
(F821)
33-33: Undefined name Supplier
(F821)
33-33: Undefined name Employee
(F821)
33-33: Undefined name Shareholder
(F821)
33-33: Undefined name Contact
(F821)
⏰ 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 (3)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Summary
🔇 Additional comments (11)
erpnext/utilities/print_format/letter_standard/letter_standard.json (1)
1-33: LGTM! Print format configuration is well-structured.The print format configuration correctly defines the Letter Standard template with appropriate settings for margins, PDF generation, and Jinja rendering. The embedded HTML template properly handles letterhead, status indicators (CANCELLED/DRAFT), and document field rendering.
erpnext/utilities/doctype/letter_type/letter_type.py (1)
8-20: LGTM! Appropriate placeholder for simple master doctype.The LetterType class implementation is appropriate for a simple master doctype that requires no custom validation or business logic. The TYPE_CHECKING block is properly structured.
erpnext/utilities/doctype/letter/letter.json (1)
1-211: LGTM! DocType configuration is well-structured.The Letter DocType configuration properly defines:
- Dynamic recipient linking based on recipient_type
- Appropriate field relationships (addresses, templates, company)
- Submittable document workflow
- Link filters for letter_template based on letter_type (line 139)
- Proper permissions and naming series
erpnext/utilities/doctype/letter_template/letter_template.py (2)
26-30: LGTM! Template validation is properly implemented.The validate method correctly validates both subject and content Jinja templates before saving, preventing invalid templates from being stored.
33-53: LGTM! Template rendering with proper security checks.The function correctly:
- Handles JSON parsing with appropriate error handling
- Enforces read permissions on the template
- Conditionally renders subject and content fields
- Returns a well-structured dictionary
The permission check (line 42) ensures users can only render templates they have access to.
erpnext/utilities/doctype/letter/letter.py (3)
37-46: LGTM! Recipient name resolution with defensive field checking.The validate and set_recipient_name methods correctly resolve the recipient name with appropriate defensive checks using
frappe.db.has_column()to avoid errors when expected fields don't exist.
57-87: LGTM! Robust recipient details resolution with proper security.The function correctly implements:
- Input validation with early return (lines 59-60)
- Existence verification (lines 62-63)
- Permission enforcement using
frappe.has_permission()(lines 65-69)- Defensive field access with
has_column()checks (lines 77, 84)- Appropriate error messages with translation support
The security checks ensure users can only access recipient data they have permission to view.
29-33: Static analysis false positives can be safely ignored.The Ruff warnings about undefined names and syntax errors in the TYPE_CHECKING block are false positives. The
DF.Literaltype annotations with string values are valid syntax for type checking purposes and don't execute at runtime.erpnext/utilities/doctype/letter_type/letter_type.json (1)
1-51: LGTM!The Letter Type DocType is well-structured. The
quick_entryflag is appropriate for a simple lookup, and the configuration follows ERPNext conventions.erpnext/utilities/doctype/letter/letter.js (2)
5-33: LGTM!The query filters correctly handle missing prerequisites by returning empty result sets, and the address query integration follows ERPNext patterns.
35-65: LGTM!The recipient handlers correctly manage field dependencies and maintain consistent state when selections change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
erpnext/utilities/doctype/letter/test_letter.py (2)
10-15: IncludeContactinEXTRA_TEST_RECORD_DEPENDENCIESfor more reliable coverage
test_recipient_name_for_contact(Line 66 onward) depends on at least oneContactrecord butEXTRA_TEST_RECORD_DEPENDENCIES(Line 13) does not list"Contact". In environments where no contact fixtures are loaded by default, this test will be skipped rather than executed, silently reducing coverage.Consider explicitly adding
"Contact"to ensure the fixture is always available:-EXTRA_TEST_RECORD_DEPENDENCIES = ["Customer", "Supplier", "Employee", "Shareholder", "Letter Type"] +EXTRA_TEST_RECORD_DEPENDENCIES = [ + "Customer", + "Supplier", + "Employee", + "Shareholder", + "Letter Type", + "Contact", +]
23-25: Callsuper().setUp()to preserve IntegrationTestCase initializationOverriding
setUp(Line 23) without callingsuper().setUp()can bypass important initialization logic inIntegrationTestCase(e.g., environment setup, fixture loading hooks). To avoid subtle issues and keep this test aligned with framework patterns, call the parent implementation before clearingLetterrecords:- def setUp(self): - frappe.db.sql("delete from `tabLetter`") + def setUp(self): + super().setUp() + frappe.db.sql("delete from `tabLetter`")
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/utilities/doctype/letter/test_letter.py(1 hunks)erpnext/utilities/doctype/letter_type/test_records.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
erpnext/utilities/doctype/letter/test_letter.py (1)
erpnext/utilities/doctype/letter/letter.py (3)
get_recipient_details(58-87)get_recipient_name_field(48-54)set_recipient_name(40-46)
⏰ 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 (3)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/utilities/doctype/letter_type/test_records.json (1)
1-5: Fixture structure and naming are appropriateThe fixture provides a minimal, clearly named
_Test Letter Typerecord that aligns with howcreate_letter()references it in tests. No issues found.erpnext/utilities/doctype/letter/test_letter.py (1)
26-146: Good, focused coverage of recipient resolution and helper behaviorThe tests around Lines 26–96 and 97–134 exercise the key behaviors of
Letterandget_recipient_details:
recipient_nameresolution for all supported recipient types.- Correct mapping from
recipient_typeto name fields.- Handling of empty parameters and non-existent recipients.
- Guard behavior when
recipientorrecipient_typeis missing.
Thecreate_letterhelper (Lines 136–146) keeps scenarios concise and readable. Overall, this is solid, maintainable coverage for the feature.
|
@diptanilsaha Since Letters are needed across the company, I moved the feature from CRM to Utilities and updated all the code. Let me know if anything else needs to be changed. |
2 participants
This PR introduces a Letter Management, allowing users to create consistent, professional, and reusable official letters directly from ERPNext.
Since the company issues many letters regularly, it is important to maintain a proper record. This ensures that all letters released from the company can be tracked, referenced, and retrieved when required.
Screen.Recording.2025-12-04.at.12.24.14.AM.mov
Features included
Benefits
Standardized and consistent letter formats across all departments
Quick creation and printing without manual formatting
Centralized tracking of all letters released by the company
Usage
Docs: https://docs.frappe.io/erpnext/letter