feat: Enhance of UX/UI feat#3
Conversation
📝 WalkthroughWalkthroughThe pull request restructures the ERPNext Localization doctype by renaming the title field to subject, introducing a Tasks child table for milestone tracking, removing the Frappe Partner doctype, and adding notification templates for developer and internal team communications. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
localization_management_system/localization_management_system/workspace/localization/localization.json (1)
3-3: Remove the stale "Frappe Partner" shortcut from thecontentfield.The content JSON contains a reference to
"Frappe Partner"(id:AjtF543aTo), but this shortcut is not defined in theshortcutsarray. This orphaned reference can cause the workspace to render a broken or missing shortcut in the UI.localization_management_system/localization_management_system/doctype/erpnext_localization/templates/erpnext_localization.html (1)
228-228: Bug: Missingdoc.prefix ondocumentation_url.The template uses
documentation_urlinstead ofdoc.documentation_url, which will cause an undefined variable error or render an empty/incorrect URL. All other fields consistently use thedoc.prefix.🐛 Proposed fix
- <a href="{{ documentation_url }}" target="_blank">Read Docs →</a> + <a href="{{ doc.documentation_url }}" target="_blank">Read Docs →</a>
🤖 Fix all issues with AI agents
In
`@localization_management_system/localization_management_system/doctype/erpnext_localization/erpnext_localization.js`:
- Around line 11-19: The add_publish_web_view_button function is adding a custom
button on every refresh causing duplicates; update
add_publish_web_view_button(frm) to clear existing custom buttons first by
calling frm.clear_custom_buttons() (or another appropriate frm.page/btn-clear
API) before calling frm.add_custom_button so only a single Publish/Unpublish
button appears after save/refresh.
In
`@localization_management_system/localization_management_system/doctype/erpnext_localization/templates/erpnext_localization_row.html`:
- Around line 155-159: The closing </div> for .localization-card-header is
misplaced and is on the same line as the `{% if doc.description %}` block, which
breaks nesting; move the `</div>` so that `.localization-card-header` is closed
before starting `.localization-card-content`, ensure the `{% if doc.description
%}` conditional and the `<p class="localization-card-description">` live inside
`.localization-card-content` with correct indentation, and verify tags close in
proper order around the `doc.description` check.
In
`@localization_management_system/localization_management_system/notification/localization_requested_developer/localization_requested_developer.json`:
- Line 28: The JSON subject value contains an extraneous word "Inbox"; update
the "subject" value in localization_requested_developer.json (the "subject" key)
to remove "Inbox" and use a clearer phrase such as "Localization Application
Submitted" or "Localization Application Received" so the subject reads
correctly.
- Line 14: The notification HTML template is duplicated: the actual notification
is loaded from localization_requested_developer.json via the "message" field
(and "message_type": "HTML"), while localization_requested_developer.html is an
orphaned copy; either delete localization_requested_developer.html to remove
unused files, or if you want to keep it for reference, add a brief top-of-file
comment and document its purpose (e.g., "reference copy for editors; not used at
runtime") and add a note in the notifications README explaining that templates
are loaded from the JSON "message" field so maintainers know why the HTML file
exists.
In
`@localization_management_system/localization_management_system/notification/localization_requested_developer/localization_requested_developer.md`:
- Line 1: The placeholder text "Add your message here" in
localization_requested_developer.md is user-visible and must be replaced with
the final notification copy; open localization_requested_developer.md and
replace the placeholder with a concise, user-facing message that includes the
necessary dynamic fields (e.g., requester name, project/locale, required action,
and a CTA link) and removes any dummy text so the notification renders correctly
for developers.
In
`@localization_management_system/localization_management_system/notification/localization_requested_developer/localization_requested_developer.py`:
- Around line 4-6: get_context currently returns None; update the
get_context(context) function to always return a dictionary (the Jinja template
context) — for example merge/validate the incoming context parameter (if
dict-like) and return a dict with the required keys (or an empty dict) so
rendering never receives None; locate get_context in
localization_requested_developer.py and ensure it returns a dict containing
whatever template variables are needed (or an empty {}) instead of leaving the
function as pass.
In
`@localization_management_system/localization_management_system/notification/localization_requested_internal_team/localization_requested_internal_team.html`:
- Around line 49-57: The .info-row CSS uses display:flex which breaks in some
email clients (notably Outlook); replace the flex-based layout with a
table-based structure: change .info-row to use table/table-row/table-cell
semantics (e.g., make .info-row a table element or set display:table and wrap
each row's cells in elements with display:table-cell), preserve padding and
border-bottom styles, and keep the .info-row:last-child rule to remove the
border and padding; update the HTML where .info-row is used (the info-card rows)
to use <table>/<tr>/<td> or equivalent elements matching the new CSS so content
that relied on flex items becomes table cells.
In `@localization_management_system/patches/set_subjects_from_title.py`:
- Around line 4-6: The patch's execute() currently updates all rows via
frappe.qb.update(EL).set(EL.subject, EL.title).run(), which will overwrite
existing subject values; modify the update to add a WHERE clause so it only
updates records where subject is NULL/empty (e.g., using EL.subject IS NULL or
equivalent) before calling .run(), ensuring you reference the same EL DocType
binding and the same set(EL.subject, EL.title) call.
🧹 Nitpick comments (6)
localization_management_system/localization_management_system/notification/localization_requested_internal_team/localization_requested_internal_team.html (1)
125-133: External image dependency may be blocked by email clients.Many email clients block external images by default. The flag from
flagcdn.commay not display until the recipient enables image loading. The conditional check ondoc.country_codeis good, and the country name still renders as fallback text, so this degrades gracefully.localization_management_system/localization_management_system/doctype/erpnext_localization/templates/erpnext_localization.html (1)
5-11: Duplicate.country-flagCSS definition.The
.country-flagclass is defined identically at lines 5-11 and lines 117-123. Remove one of the duplicates to improve maintainability.♻️ Suggested fix: Remove duplicate CSS block
- .country-flag { - display: inline-block; - margin-right: 0.5rem; - vertical-align: middle; - border-radius: 2px; - box-shadow: 0 1px 2px rgba(0, 0, 0, 0.1); - } - .localization-cta-card {Remove lines 117-123 since the same styles are already defined at lines 5-11.
Also applies to: 117-123
localization_management_system/localization_management_system/doctype/erpnext_localization/templates/erpnext_localization_row.html (1)
15-16: Inconsistent indentation: mixing tabs and spaces.Lines 15-16 use 4-space indentation while the rest of the file uses tabs. This can cause rendering inconsistencies and maintainability issues.
♻️ Suggested fix
- border: 1px solid `#7a7c80`; -} + border: 1px solid `#7a7c80`; +}localization_management_system/localization_management_system/notification/localization_requested_developer/localization_requested_developer.html (1)
154-157: Consider adding a fallback for emptyapplicable_for.If
doc.applicable_foris not set, this will render an empty value. Consider adding a fallback similar to the main template's "Not specified" pattern.♻️ Suggested fix
<div class="info-row"> <div class="info-label">Type:</div> - <div class="info-value">{{ doc.applicable_for }}</div> + <div class="info-value">{{ doc.applicable_for or "Not specified" }}</div> </div>localization_management_system/localization_management_system/doctype/localization_task/localization_task.json (2)
68-74: Consider adding a default value for thestatusfield.The
statusfield has no default value, so newly created tasks will have an empty status. Consider adding"default": "Open"to ensure new tasks start with a meaningful status.Suggested fix
{ + "default": "Open", "fieldname": "status", "fieldtype": "Select", "in_list_view": 1, "label": "Status", "options": "Open\nClosed\nCancelled" },
50-63: Date range validation should be handled in the parent DocType.The
start_dateandend_datefields lack validation to ensureend_date >= start_date. Since this is a child table, consider adding validation logic in the parentERPNextLocalization.validate()method to check task date consistency.
| @@ -0,0 +1 @@ | |||
| Add your message here No newline at end of file | |||
There was a problem hiding this comment.
Replace placeholder notification copy.
This will be user-visible as-is.
🤖 Prompt for AI Agents
In
`@localization_management_system/localization_management_system/notification/localization_requested_developer/localization_requested_developer.md`
at line 1, The placeholder text "Add your message here" in
localization_requested_developer.md is user-visible and must be replaced with
the final notification copy; open localization_requested_developer.md and
replace the placeholder with a concise, user-facing message that includes the
necessary dynamic fields (e.g., requester name, project/locale, required action,
and a CTA link) and removes any dummy text so the notification renders correctly
for developers.
| def execute(): | ||
| EL = frappe.qb.DocType("ERPNext Localization") | ||
| frappe.qb.update(EL).set(EL.subject, EL.title).run() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "set_subjects_from_title.py" -type fRepository: resilient-tech/localization-management-system
Length of output: 234
🌐 Web query:
frappe.qb.update() ERPNext localization patch set_subjects_from_title
💡 Result:
You can write a patch that uses Frappe Query Builder's update to set the subjects field from the title (or any computed value) in bulk. Example patch (save as erpnext/.../patches/set_subjects_from_title.py and add to patches.txt):
import frappe
from frappe.qb import DocType
from frappe.qb import Field
def execute():
Doc = DocType("Your Doctype") # e.g. "Article" or "Web Page"
# assume title and subjects are columns in the table
# build an UPDATE ... SET subjects = WHERE subjects IS NULL
# here we copy title → subjects; adjust expression as needed (e.g. substring, split)
frappe.db.sql(
frappe.qb.update(Doc)
.set(Doc.subjects, Doc.title)
.where(Doc.subjects.isnull())
.get_sql()
)
frappe.db.commit()
Notes:
- Replace "Your Doctype" and field names with your actual doctype/fields.
- If you need string manipulation (e.g. split title into comma-separated subjects), use SQL functions in the set expression; you can use Field("...") to inject SQL functions. Example: .set(Doc.subjects, Field("SUBSTRING_INDEX(%s, ' - ', 1)" % Doc.title)) .
- Test on a copy of your site before running on production and run bench migrate to apply the patch.
References:
- Frappe Query Builder usage (get_query / qb docs). [1]
- Example pattern for writing patches that update doctypes. [2]
Sources:
[1] https://docs.frappe.io/framework/get_query
[2] https://sabbirz.com/blog/update-doctypes-after-data-entry-using-frappe-patches
🌐 Web query:
frappe QueryBuilder update without WHERE clause data loss risk
💡 Result:
Short answer: very high risk — an update issued without a WHERE (or filters) will modify every row in the target table and can cause irreversible data loss unless you can restore from backup. See Frappe/SQL docs and general SQL guidance. [1][2][3]
Immediate safeguards and recommendations
- Never run UPDATE without filters in production; always use QueryBuilder filters or frappe.db.set_value/frappe.db.update with an explicit name/filters. [1][2]
- Preview the affected rows first: run an equivalent SELECT with the same filters before UPDATE. (If the SELECT returns all rows, your UPDATE will too.) [3]
- Use transactions on migration scripts: wrap in a transaction and test rollback before commit, or run on a copy of the DB. Frappe bench migrate/actions should be tested on staging first. [2]
- Take a backup before running any mass update (dump or snapshot) so you can restore quickly. (bench backup / DB-level snapshot.)
- Add safety checks in code: assert rowcount > 0 and <= expected, or require an explicit flag to allow full-table updates; fail otherwise.
- Use LIMIT (if supported) or filter by primary key batches and run small, tested batches rather than one huge UPDATE.
- Log and test scripts on staging/dev and use a dry-run mode that shows SQL without executing it.
- Consider row-level locking (FOR UPDATE/for_update) if concurrency matters. [1]
If you already ran an unsafe UPDATE
- Stop further writes, take a DB dump immediately (even in broken state) to preserve ability to investigate.
- Restore from the most recent good backup to recover; if none, you may need to reconstruct via logs or application-level exports.
- Check binary/transaction logs (MySQL binlog, WAL) for point-in-time recovery if your DB has them enabled.
Relevant references
- QueryBuilder / get_query docs (filters, for_update). [1]
- frappe.db API (set_value / update behavior). [2]
- General SQL behavior for UPDATE without WHERE (affects all rows). [3]
References
[1] Frappe Query/get_query docs — filters, for_update.
[2] Frappe DB API (frappe.db.set_value / update).
[3] SQL: UPDATE without WHERE — explanation and consequences.
Add a WHERE clause to prevent overwriting existing subject values.
This update currently runs unconditionally on all rows. Frappe's documentation for similar bulk update patches includes a guard (e.g., .where(subject IS NULL)) to prevent data loss. Without it, any pre-existing subject values will be overwritten.
Suggested fix
def execute():
EL = frappe.qb.DocType("ERPNext Localization")
- frappe.qb.update(EL).set(EL.subject, EL.title).run()
+ frappe.qb.update(EL).set(EL.subject, EL.title).where(
+ (EL.subject.isnull()) | (EL.subject == "")
+ ).run()🤖 Prompt for AI Agents
In `@localization_management_system/patches/set_subjects_from_title.py` around
lines 4 - 6, The patch's execute() currently updates all rows via
frappe.qb.update(EL).set(EL.subject, EL.title).run(), which will overwrite
existing subject values; modify the update to add a WHERE clause so it only
updates records where subject is NULL/empty (e.g., using EL.subject IS NULL or
equivalent) before calling .run(), ensuring you reference the same EL DocType
binding and the same set(EL.subject, EL.title) call.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements
Removals
✏️ Tip: You can customize this high-level summary in your review settings.