feat(spp_change_request_v2): improve CR creation wizard UX#29
feat(spp_change_request_v2): improve CR creation wizard UX#29
Conversation
- Add debounced search field with custom OWL widget for live registrant search - Search by name and registrant IDs (reg_ids.value) - Render search results as clickable HTML table rows - Show max 2 IDs per row with +N badge and tooltip for overflow - Add pagination (10 per page) with Previous/Next navigation - Display selected registrant info with all IDs as badges - Preserve search state when changing registrant selection - Add display_name to detail base using CR reference - Replace Submit for Approval with Proceed button on detail forms - Proceed validates proposed changes before navigating to main CR form - Remove New Request menu item (use list view button instead) - Remove View Registrant stat button, rename Open Form to View Registrant - Show Preview Changes stat button only in developer mode
Summary of ChangesHello @emjay0921, 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 overhauls the user experience for creating Change Requests, particularly focusing on the initial registrant selection process. The changes introduce a more dynamic and user-friendly search mechanism, providing live feedback and better information display. Additionally, the navigation between the wizard, detail forms, and the main Change Request form has been refined to ensure a smoother and more intuitive user journey, reducing friction and improving overall efficiency for users initiating change requests. 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 significantly improves the user experience of the Change Request creation wizard by implementing a debounced, paginated search for registrants. The changes are well-executed, introducing new OWL components for the search functionality and updating the backend logic accordingly. The overall implementation is robust and enhances usability. My review includes a few suggestions to further improve performance, maintainability, and conciseness of the code.
| for rec in self: | ||
| if rec.reg_ids: | ||
| parts = [] | ||
| for rid in rec.reg_ids: | ||
| if rid.value: | ||
| label = rid.id_type_as_str or "ID" | ||
| parts.append(f"{label} ({rid.value})") | ||
| rec.reg_id_display = ", ".join(parts) if parts else "" | ||
| else: | ||
| rec.reg_id_display = "" |
There was a problem hiding this comment.
The logic in this method can be made more concise and Pythonic by using a list comprehension. This would eliminate the need for the outer if/else block and the conditional expression within the join call, making the code easier to read and maintain.
parts = [
f"{(rid.id_type_as_str or 'ID')} ({rid.value})"
for rid in rec.reg_ids
if rid.value
]
rec.reg_id_display = ", ".join(parts)| setup() { | ||
| this.containerRef = useRef("container"); | ||
| onMounted(() => this._attachClickHandler()); | ||
| onPatched(() => this._attachClickHandler()); | ||
| } | ||
|
|
||
| get htmlContent() { | ||
| return this.props.record.data[this.props.name] || ""; | ||
| } | ||
|
|
||
| _attachClickHandler() { | ||
| const el = this.containerRef.el; | ||
| if (!el) return; | ||
| // Row selection | ||
| el.querySelectorAll(".o_cr_search_result").forEach((row) => { | ||
| row.onclick = (ev) => { | ||
| ev.preventDefault(); | ||
| ev.stopPropagation(); | ||
| const partnerId = parseInt(row.dataset.partnerId); | ||
| if (partnerId) { | ||
| this.props.record.update({_selected_partner_id: partnerId}); | ||
| } | ||
| }; | ||
| }); | ||
| // Pagination | ||
| el.querySelectorAll(".o_cr_page_prev, .o_cr_page_next").forEach((link) => { | ||
| link.onclick = (ev) => { | ||
| ev.preventDefault(); | ||
| ev.stopPropagation(); | ||
| const page = parseInt(link.dataset.page); | ||
| if (!isNaN(page) && page >= 0) { | ||
| this.props.record.update({_search_page: page}); | ||
| } | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
For better performance and maintainability, consider using event delegation for handling clicks instead of attaching individual onclick handlers to each result row and pagination link. By adding a single event listener to the container, you can avoid re-attaching handlers on every render (onPatched) and reduce the total number of event listeners, which is more efficient, especially with many search results.
Also, it's a good practice to always provide the radix parameter to parseInt to avoid unexpected behavior.
setup() {
this.containerRef = useRef("container");
onMounted(() => {
this.containerRef.el.addEventListener("click", this._onClick.bind(this));
});
}
get htmlContent() {
return this.props.record.data[this.props.name] || "";
}
_onClick(ev) {
// Row selection
const row = ev.target.closest(".o_cr_search_result");
if (row) {
ev.preventDefault();
ev.stopPropagation();
const partnerId = parseInt(row.dataset.partnerId, 10);
if (partnerId) {
this.props.record.update({_selected_partner_id: partnerId});
}
return;
}
// Pagination
const pageLink = ev.target.closest(".o_cr_page_prev, .o_cr_page_next");
if (pageLink) {
ev.preventDefault();
ev.stopPropagation();
const page = parseInt(pageLink.dataset.page, 10);
if (!isNaN(page) && page >= 0) {
this.props.record.update({_search_page: page});
}
}
}| <templates xml:space="preserve"> | ||
|
|
||
| <t t-name="spp_change_request_v2.CrSearchResultsField"> | ||
| <div t-ref="container" class="o_field_cr_search_results" style="width:100%" t-out="htmlContent"/> |
There was a problem hiding this comment.
The inline style style="width:100%" is redundant, as the o_field_cr_search_results class already has its width set to 100% !important in the associated CSS file (cr_search_results.css). It's best practice to rely on the stylesheet for styling to improve maintainability and separation of concerns. Please remove the inline style.
| <div t-ref="container" class="o_field_cr_search_results" style="width:100%" t-out="htmlContent"/> | |
| <div t-ref="container" class="o_field_cr_search_results" t-out="htmlContent"/> |
| def _render_search_results(self): | ||
| """Search and render paginated HTML results.""" | ||
| search_domain = self._get_search_domain() | ||
| total = self.env["res.partner"].search_count(search_domain) | ||
|
|
||
| if not total: | ||
| self.search_results_html = Markup( | ||
| "<p class='text-muted'>No registrants found.</p>" | ||
| ) | ||
| return | ||
|
|
||
| page = self._search_page or 0 | ||
| page_size = self._SEARCH_PAGE_SIZE | ||
| max_page = (total - 1) // page_size | ||
| page = min(page, max_page) | ||
|
|
||
| offset = page * page_size | ||
| partners = self.env["res.partner"].search( | ||
| search_domain, limit=page_size, offset=offset | ||
| ) | ||
|
|
||
| rows = [] | ||
| for p in partners: | ||
| # Build ALL IDs in "TypeName (value)" format, show max 2 | ||
| id_parts = [] | ||
| if p.reg_ids: | ||
| for rid in p.reg_ids: | ||
| if rid.value: | ||
| label = rid.id_type_as_str or "ID" | ||
| id_parts.append(f"{label} ({rid.value})") | ||
| if not id_parts: | ||
| id_html = Markup("") | ||
| id_title = "" | ||
| elif len(id_parts) <= 2: | ||
| id_html = escape(", ".join(id_parts)) | ||
| id_title = "" | ||
| else: | ||
| visible = escape(", ".join(id_parts[:2])) | ||
| extra = len(id_parts) - 2 | ||
| id_html = Markup( | ||
| '{} <span class="badge text-bg-secondary ms-1">' | ||
| "+{} <i class='fa fa-info-circle'></i></span>" | ||
| ).format(visible, extra) | ||
| id_title = ", ".join(id_parts) | ||
| ptype = ( | ||
| '<i class="fa fa-users"></i> Group' | ||
| if p.is_group | ||
| else '<i class="fa fa-user"></i> Individual' | ||
| ) | ||
| rows.append( | ||
| Markup( | ||
| '<tr class="o_cr_search_result" style="cursor:pointer"' | ||
| ' data-partner-id="{}" data-partner-name="{}">' | ||
| "<td>{}</td>" | ||
| '<td title="{}">{}</td>' | ||
| "<td>{}</td></tr>" | ||
| ).format( | ||
| p.id, | ||
| escape(p.name or ""), | ||
| escape(p.name or ""), | ||
| escape(id_title), | ||
| id_html, | ||
| Markup(ptype), | ||
| ) | ||
| ) | ||
|
|
||
| table = Markup( | ||
| '<table class="table table-hover table-sm mb-0 w-100">' | ||
| "<thead><tr><th>Name</th><th>ID</th><th>Type</th></tr></thead>" | ||
| "<tbody>{}</tbody></table>" | ||
| ).format(Markup("").join(rows)) | ||
|
|
||
| # Pagination header | ||
| start = offset + 1 | ||
| end = min(offset + page_size, total) | ||
| prev_cls = "text-muted" if page == 0 else "o_cr_page_prev" | ||
| next_cls = "text-muted" if page >= max_page else "o_cr_page_next" | ||
| pagination = Markup( | ||
| '<div class="d-flex justify-content-between align-items-center mb-2 px-1">' | ||
| '<small class="text-muted">{}-{} of {}</small>' | ||
| "<div>" | ||
| '<a class="{} me-3" style="cursor:pointer" data-page="{}">← Previous</a>' | ||
| '<a class="{}" style="cursor:pointer" data-page="{}">Next →</a>' | ||
| "</div></div>" | ||
| ).format(start, end, total, prev_cls, page - 1, next_cls, page + 1) | ||
|
|
||
| self.search_results_html = pagination + table |
There was a problem hiding this comment.
This method manually constructs a significant amount of HTML using string formatting. While it's done securely with Markup and escape, this approach can be difficult to read and maintain.
For better separation of concerns, consider refactoring this to use a server-side QWeb template. You could define the template in an XML file and render it using self.env['ir.qweb']._render(). This would make the Python code cleaner and the HTML structure more manageable.
…xact match for ID search - Remove ID column from search results table (show Name and Type only) - Remove ID badges from selected registrant info card - Change reg_ids.value search from ilike (partial) to = (exact match)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #29 +/- ##
=======================================
Coverage ? 62.98%
=======================================
Files ? 239
Lines ? 19213
Branches ? 0
=======================================
Hits ? 12101
Misses ? 7112
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on March 17
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| parts.append(f"{label} ({rid.value})") | ||
| rec.reg_id_display = ", ".join(parts) if parts else "" | ||
| else: | ||
| rec.reg_id_display = "" |
There was a problem hiding this comment.
Unused computed field reg_id_display never referenced
Low Severity
The reg_id_display computed field and its _compute_reg_id_display method are added to res.partner but never referenced anywhere else — not in any XML view, template, or other Python code. The PR description mentions "ID display: Shows max 2 IDs per row with +N badge" and "Selected registrant info: Shows name, type, and all IDs as badges," but neither the search results table in _render_search_results nor the _compute_registrant_info method uses this field. This is dead code that appears to be leftover from an incomplete feature.
| def _onchange_request_type(self): | ||
| """Clear registrant if it doesn't match the new target type.""" | ||
| """Clear registrant and search if type changes.""" | ||
| self.search_text = False |
There was a problem hiding this comment.
Redundant type-matching conditional after unconditional search clear
Low Severity
The newly added self.search_text = False at the top of _onchange_request_type triggers a cascading _onchange_search_text call, which unconditionally clears registrant_id. This makes the subsequent conditional check (lines 380–384 that only clear registrant_id when the target type mismatches) dead code — registrant_id will always be cleared by the cascading onchange regardless of whether the type matches. The conditional misleadingly suggests selective clearing still happens.
1 participant
Why is this change needed?
The CR creation wizard needed a better registrant search experience — searching by ID, live results, pagination, and improved UX flow from wizard → detail form → main CR form.
How was the change implemented?
SearchDelayFieldtriggers search after 500ms typing delaynameandreg_ids.valuefieldsCrSearchResultsFieldOWL widget renders results as a table with click-to-select via_selected_partner_idbridge field+Nbadge and native tooltip for overflow_compute_display_nameon detail base shows CR reference instead ofmodel,idMirror of: https://github.com/OpenSPP/openspp-modules-v2/pull/294
New unit tests
Unit tests executed by the author
How to test manually
Related links
https://github.com/OpenSPP/openspp-modules-v2/pull/294
Note
Medium Risk
Touches core CR creation flow and introduces unsanitized HTML rendering (
sanitize=False) plus new JS widgets/onchange bridges, which could cause UI regressions or injection risks if inputs aren’t strictly controlled.Overview
Improves the Change Request creation wizard by replacing the
registrant_iddropdown with a debounced, paginated live search (newsearch_delayandcr_search_resultsOWL field widgets) that searches by name and exact ID number, renders clickable HTML results, and supports switching the selected registrant.Tightens the wizard→detail→CR workflow by adding
action_proceed_to_cr(blocks proceeding if no proposed changes), setting detail records’display_nameto the CR reference, and updating detail form headers to use Proceed instead of submit actions.Includes minor UX/access changes: hides the Preview smart button to dev-only, removes the redundant “New Request” menu item, renames the registrant link button, and gates “Check for Updates” in conflicts UI behind a new
is_conflict_detection_enabledrelated flag.Written by Cursor Bugbot for commit 6700624. This will update automatically on new commits. Configure here.