feat(spp_change_request_v2): improve Update ID Document CR type#104
feat(spp_change_request_v2): improve Update ID Document CR type#104
Conversation
- Redesign detail form: add title header, use registrant_id field, move operation below registrant, hide remarks section - Rename "Update Existing ID" to "Edit ID" in operation selection - Add operation-specific review layouts (comparison table for Edit, summary table for Add/Remove) with header support - Add configurable allowed operations (allow_id_add/edit/remove) on CR type with new "ID Operations" tab in type config form - Create reusable filterable_radio widget in spp_base_common that supports per-option disabling via disabled_map option - Fix id_type_id to use spp.vocabulary.code (matching spp.registry.id) - Fix spp.registry.id display_name to show "ID Type - Value" - Fix _name_search to search by ID type, value, or partner name - Hide ID Information section in Edit mode until existing ID selected - Wrap cr_types.xml in noupdate=1 to prevent upgrade overwrites - Update preview tests for new structure
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the 'Update ID Document' change request type by modernizing its user interface and providing greater flexibility for administrators. It introduces the ability to configure which ID operations (add, edit, remove) are permitted for a given change request type, improving control and user experience. The changes also refine how ID information is displayed and searched, ensuring a more intuitive and robust system for managing registrant identities. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant UI/UX improvements to the 'Update ID Document' change request type, making it more flexible and user-friendly. The introduction of configurable operations and a reusable filterable_radio widget are great additions. The code is well-structured, but I've identified a few areas for improvement:
- A potential security vulnerability (XSS) in the review page rendering.
- Some code duplication in a Python
onchangemethod and an XML view that could be refactored for better maintainability.
My detailed comments are attached to the relevant files. Overall, this is a solid feature enhancement.
| return self._render_comparison_table(changes, header=header) | ||
| return self._render_action_summary(action, changes, header=header) |
There was a problem hiding this comment.
The functions _render_comparison_table and _render_action_summary render HTML for the review page. They use the internal helper _format_review_value to format values. This helper does not escape string values, which could lead to a Cross-Site Scripting (XSS) vulnerability if a user-controlled value (like an ID value) contains malicious HTML or JavaScript. Since review_comparison_html is rendered with sanitize=False, it's crucial to escape all user-provided data.
I recommend updating _format_review_value to use odoo.tools.html_escape. For example:
# In _format_review_value method:
from odoo.tools import html_escape
# ... at the end of the function, instead of just str(value):
return html_escape(str(value))There was a problem hiding this comment.
Fixed in 9d94eef — added html_escape via markupsafe.escape to _format_review_value.
| if self.operation == "add": | ||
| self.existing_id_record_id = False | ||
| self.id_type_id = False | ||
| self.id_value = False | ||
| self.expiry_date = False | ||
| elif self.operation in ("update", "remove"): | ||
| self.id_type_id = False | ||
| self.id_value = False | ||
| self.expiry_date = False |
There was a problem hiding this comment.
There's some code duplication in how fields are cleared for different operations. The fields id_type_id, id_value, and expiry_date are reset in both the if and elif blocks. You can simplify this by clearing these fields once before the conditional logic to improve readability and maintainability.
| if self.operation == "add": | |
| self.existing_id_record_id = False | |
| self.id_type_id = False | |
| self.id_value = False | |
| self.expiry_date = False | |
| elif self.operation in ("update", "remove"): | |
| self.id_type_id = False | |
| self.id_value = False | |
| self.expiry_date = False | |
| self.id_type_id = False | |
| self.id_value = False | |
| self.expiry_date = False | |
| if self.operation == 'add': | |
| self.existing_id_record_id = False | |
There was a problem hiding this comment.
Fixed in 9d94eef — deduplicated the field clearing.
| <group invisible="operation != 'update'"> | ||
| <group> | ||
| <field | ||
| name="existing_id_record_id" | ||
| options="{'no_create': True}" | ||
| required="operation in ['update', 'remove']" | ||
| string="Select ID to Edit" | ||
| options="{'no_create': True, 'no_open': True}" | ||
| required="operation == 'update'" | ||
| readonly="not is_cr_manager or approval_state not in ('draft', 'revision')" | ||
| /> | ||
| </group> | ||
| </group> | ||
|
|
||
| <!-- Select existing ID for Remove operation --> | ||
| <group invisible="operation != 'remove'"> | ||
| <group> | ||
| <field | ||
| name="current_id_value" | ||
| invisible="not existing_id_record_id" | ||
| name="existing_id_record_id" | ||
| string="Select ID to Remove" | ||
| options="{'no_create': True, 'no_open': True}" | ||
| required="operation == 'remove'" | ||
| readonly="not is_cr_manager or approval_state not in ('draft', 'revision')" | ||
| /> | ||
| </group> | ||
| </group> |
There was a problem hiding this comment.
The field existing_id_record_id is defined twice, once for the 'update' operation and once for the 'remove' operation. This creates code duplication and makes the view harder to maintain. You can combine these into a single group and field definition that is visible for both operations.
| <group invisible="operation != 'update'"> | |
| <group> | |
| <field | |
| name="existing_id_record_id" | |
| options="{'no_create': True}" | |
| required="operation in ['update', 'remove']" | |
| string="Select ID to Edit" | |
| options="{'no_create': True, 'no_open': True}" | |
| required="operation == 'update'" | |
| readonly="not is_cr_manager or approval_state not in ('draft', 'revision')" | |
| /> | |
| </group> | |
| </group> | |
| <!-- Select existing ID for Remove operation --> | |
| <group invisible="operation != 'remove'"> | |
| <group> | |
| <field | |
| name="current_id_value" | |
| invisible="not existing_id_record_id" | |
| name="existing_id_record_id" | |
| string="Select ID to Remove" | |
| options="{'no_create': True, 'no_open': True}" | |
| required="operation == 'remove'" | |
| readonly="not is_cr_manager or approval_state not in ('draft', 'revision')" | |
| /> | |
| </group> | |
| </group> | |
| <!-- Select existing ID for Edit/Remove operations --> | |
| <group invisible="operation not in ('update', 'remove')"> | |
| <group> | |
| <field | |
| name="existing_id_record_id" | |
| string="Select Existing ID" | |
| options="{'no_create': True, 'no_open': True}" | |
| required="operation in ('update', 'remove')" | |
| readonly="not is_cr_manager or approval_state not in ('draft', 'revision')" | |
| /> | |
| </group> | |
| </group> |
There was a problem hiding this comment.
Intentionally kept as two separate groups. They show different field labels per operation — "Select ID to Edit" for update and "Select ID to Remove" for remove — so the user gets context-appropriate labeling. Merging them into one group would lose that UX distinction.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #104 +/- ##
==========================================
+ Coverage 71.17% 71.19% +0.02%
==========================================
Files 704 731 +27
Lines 38554 40229 +1675
==========================================
+ Hits 27439 28641 +1202
- Misses 11115 11588 +473
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ions - Add is_operation_locked boolean field set when proceeding to Documents/Review - Show computed operation_display char field when locked instead of radio - Change filterable_radio widget to hide disabled options (d-none) instead of graying them out - Reset lock when operation is changed via onchange
- Add html_escape to _format_review_value to prevent XSS in review page - Deduplicate field clearing in _onchange_operation
2 participants
Why is this change needed?
The "Update ID Document" change request type needed UI/UX improvements to match the quality of the "Edit Individual Information" CR type, plus configurable allowed operations.
How was the change implemented?
allow_id_add/edit/remove) on CR type model with "ID Operations" tabfilterable_radiowidget inspp_base_commonsupporting per-option disabling viadisabled_mapid_type_idto usespp.vocabulary.code(matchingspp.registry.id)spp.registry.iddisplay_name to show "ID Type - Value" instead of partner name_name_searchto search by ID type, value, or partner namecr_types.xmlinnoupdate="1"to prevent upgrade overwritesNew unit tests
Updated
test_update_id_previewfor new preview structure with_headerkey.Unit tests executed by the author
spp_cr_types_base: 7/7 passedspp_change_request_v2: 279/279 passed (286 total with cr_types_base)How to test manually
Related links