fix(spp_change_request_v2): add no_create/no_open to lookup fields in CR detail views#33
Conversation
… CR detail views Prevent users from creating or editing configuration/lookup records (gender, relationship, country, state, group type, role, ID type) and registrant records directly from change request forms. Users should only select from existing values.
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 improves the user experience and maintains data consistency by restricting the ability to create new or open existing lookup and registrant records directly from various change request forms. This ensures that users must select from predefined options, preventing accidental data creation or modification in critical fields. Highlights
Changelog
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 correctly adds no_create and no_open options to lookup fields in various change request detail views, which will prevent users from creating or editing related records directly from these forms. The changes are well-aligned with the stated goal. I've added a few comments suggesting improvements to the XML formatting for consistency across the views. Adopting a consistent multi-line format for fields with multiple attributes will enhance readability and maintainability. Otherwise, the changes look good.
| <group string="Group Information"> | ||
| <field name="group_name" required="1" /> | ||
| <field name="group_type_id" options="{'no_create': True}" /> | ||
| <field name="group_type_id" options="{'no_create': True, 'no_open': True}" /> |
There was a problem hiding this comment.
For consistency with other views in this PR (e.g., detail_add_member_views.xml), it would be better to format this field over multiple lines, with one attribute per line. This improves readability and makes future diffs cleaner.
| <field name="group_type_id" options="{'no_create': True, 'no_open': True}" /> | |
| <field | |
| name="group_type_id" | |
| options="{'no_create': True, 'no_open': True}" | |
| /> |
| <field name="state_id" options="{'no_create': True, 'no_open': True}" /> | ||
| <field name="postal_code" /> | ||
| <field name="country_id" options="{'no_create': True}" /> | ||
| <field name="country_id" options="{'no_create': True, 'no_open': True}" /> |
There was a problem hiding this comment.
For consistency with other views in this PR (e.g., detail_add_member_views.xml), it would be better to format these fields over multiple lines, with one attribute per line. This improves readability and makes future diffs cleaner.
<field
name="state_id"
options="{'no_create': True, 'no_open': True}"
/>
<field name="postal_code" />
<field
name="country_id"
options="{'no_create': True, 'no_open': True}"
/>
| <field name="state_id" options="{'no_create': True, 'no_open': True}" /> | ||
| <field name="postal_code" /> | ||
| <field name="country_id" options="{'no_create': True}" /> | ||
| <field name="country_id" options="{'no_create': True, 'no_open': True}" /> |
There was a problem hiding this comment.
For consistency with other views in this PR (e.g., detail_add_member_views.xml), it would be better to format these fields over multiple lines, with one attribute per line. This improves readability and makes future diffs cleaner.
<field
name="state_id"
options="{'no_create': True, 'no_open': True}"
/>
<field name="postal_code" />
<field
name="country_id"
options="{'no_create': True, 'no_open': True}"
/>
| required="1" | ||
| /> | ||
| <field name="new_role_id" options="{'no_create': True}" /> | ||
| <field name="new_role_id" options="{'no_create': True, 'no_open': True}" /> |
There was a problem hiding this comment.
For consistency with other views in this PR (e.g., detail_add_member_views.xml), it would be better to format this field over multiple lines, with one attribute per line. This improves readability and makes future diffs cleaner.
<field
name="new_role_id"
options="{'no_create': True, 'no_open': True}"
/>
| <group invisible="operation == 'remove'"> | ||
| <group string="ID Information"> | ||
| <field name="id_type_id" options="{'no_create': True}" /> | ||
| <field name="id_type_id" options="{'no_create': True, 'no_open': True}" /> |
There was a problem hiding this comment.
For consistency with other views in this PR (e.g., detail_add_member_views.xml), it would be better to format this field over multiple lines, with one attribute per line. This improves readability and makes future diffs cleaner.
<field
name="id_type_id"
options="{'no_create': True, 'no_open': True}"
/>
2 participants
Why is this change needed?
Users could create or edit configuration/lookup records (gender, relationship, country, state, group type, role, ID type) and registrant records directly from change request forms. These fields should be select-only.
How was the change implemented?
Added
options="{'no_create': True, 'no_open': True}"to all lookup and registrant Many2one fields across 9 CR detail views:gender_id,relationship_idgender_iddestination_country_idprevious_head_new_role_id(addedno_open)group_type_id,head_gender_id,state_id,country_id(addedno_open)source_group_id,new_group_type_id,state_id,country_id(addedno_open)source_group_id,new_role_id(addedno_open)primary_registrant_idid_type_id(addedno_open)New unit tests
N/A — view-only change
Unit tests executed by the author
N/A
How to test manually