fix: address review findings and add tests for spp_cel_registry_search#105
fix: address review findings and add tests for spp_cel_registry_search#105gonzalesedwin1123 wants to merge 3 commits into19.0from
Conversation
- Change auto_install to False (feature, not bridge module) - Change category from OpenSPP/Core to OpenSPP (consistent with siblings) - Remove duplicate static class attribute on result buttons - Add icon to Disabled badge for accessibility (colorblind users) - Remove incorrect role="status" from static empty state - Replace non-universal example expression (r.monthly_income) - Fix error state showing misleading "0 results" instead of resetting - Improve catch error message to not blame expression for server errors - Fix invisible hover effect (same color as page background) - Update DESCRIPTION.md and regenerate README.rst - Add 12 tests covering security groups, client action, menu, visibility
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 focuses on stabilizing the 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 provides a comprehensive set of fixes and improvements for the spp_cel_registry_search module, including configuration changes, UX and accessibility enhancements, bug fixes, and documentation updates. A significant and valuable addition is the new test suite, which covers security, actions, and menu visibility, greatly improving the module's robustness. The changes are well-implemented and align with the descriptions. I've added a few suggestions to further improve code maintainability and test coverage.
| this.state.hasSearched = false; | ||
| this.state.results = []; | ||
| this.state.totalCount = 0; |
There was a problem hiding this comment.
There's an opportunity to reduce code duplication. The state reset logic on these lines is identical to the one in the if (result.error) block (lines 120-122). Consider extracting this logic into a private helper method to improve maintainability, for example:
_resetErrorState() {
this.state.hasSearched = false;
this.state.results = [];
this.state.totalCount = 0;
}You could then call this._resetErrorState() in both error handling blocks.
| def test_menu_visible_to_cel_search_user(self): | ||
| """A user with group_cel_search_user should see the menu.""" | ||
| user = self.env["res.users"].create( | ||
| { | ||
| "name": "Test CEL Search User", | ||
| "login": "test_cel_search_user", | ||
| "group_ids": [ | ||
| Command.link(self.env.ref("base.group_user").id), | ||
| Command.link(self.group_cel_search_user.id), | ||
| ], | ||
| } | ||
| ) | ||
| visible_menus = self.env["ir.ui.menu"].with_user(user).search([]) | ||
| self.assertIn( | ||
| self.menu, | ||
| visible_menus, | ||
| "Menu should be visible to CEL Search users", | ||
| ) |
There was a problem hiding this comment.
The user creation logic is duplicated across test_menu_visible_to_cel_search_user, test_menu_visible_to_registry_officer, and test_base_user_does_not_have_cel_search_group. To improve maintainability and reduce boilerplate, consider creating a private helper method to create test users with specific groups. For example:
def _create_user(self, name, login, extra_groups=None):
"""Helper to create a test user."""
group_ids = [Command.link(self.env.ref("base.group_user").id)]
if extra_groups:
group_ids.extend([Command.link(g.id) for g in extra_groups])
return self.env["res.users"].create({
"name": name,
"login": login,
"group_ids": group_ids,
})This would simplify your test setup significantly.
| self.assertFalse( | ||
| user.has_group("spp_cel_registry_search.group_cel_search_user"), | ||
| "Base user should not have CEL Search group", | ||
| ) |
There was a problem hiding this comment.
This test correctly verifies that a base user doesn't have the group_cel_search_user group. To make the test more comprehensive and directly test the security of the menu item, consider also asserting that the 'Advanced Search' menu is not visible to this user.
| self.assertFalse( | |
| user.has_group("spp_cel_registry_search.group_cel_search_user"), | |
| "Base user should not have CEL Search group", | |
| ) | |
| self.assertFalse( | |
| user.has_group("spp_cel_registry_search.group_cel_search_user"), | |
| "Base user should not have CEL Search group", | |
| ) | |
| # Also verify that the menu is not visible | |
| visible_menus = self.env["ir.ui.menu"].with_user(user).search([]) | |
| self.assertNotIn( | |
| self.menu, | |
| visible_menus, | |
| "Menu should not be visible to a base user", | |
| ) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #105 +/- ##
==========================================
- Coverage 71.17% 71.16% -0.01%
==========================================
Files 704 705 +1
Lines 38554 38555 +1
==========================================
Hits 27439 27439
- Misses 11115 11116 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The #### headings created level-4 RST titles that skipped level 3, causing oca-gen-addon-readme to fail with "Inconsistent title style". Convert subsection headings to bold text instead.
1 participant
Summary
Comprehensive review and fixes for
spp_cel_registry_searchin preparation for stable release. The module was analyzed by staff engineer and UX expert reviewers, and all findings were addressed.Manifest & Configuration
auto_install: True→False— this is a feature module, not a bridge/glue module. Withauto_install: True, it silently installed in every deployment viaspp_programscategory: "OpenSPP/Core"→"OpenSPP"— consistent with sibling CEL modulesUX & Accessibility Fixes
classattribute on result buttons (dead code alongsidet-attf-class)fa-banicon to "Disabled" badge for colorblind accessibilityrole="status"from static empty state div (was triggering unnecessary screen reader announcements)r.monthly_income < 2500example withr.registration_date > "2024-01-01"—monthly_incomeis not a base fieldBug Fixes
hasSearchedso the user sees the empty state instead of a false "no results" messageDocumentation
DESCRIPTION.mdto remove stale auto-install referenceREADME.rstandstatic/description/index.htmlTests
Test plan
./scripts/test_single_module.sh spp_cel_registry_search— 12/12 passdevelopment_statusupgrade toProduction/Stable