feat(spp_hazard): promote to Beta with UX improvements #32
Conversation
…iner - Set development_status to Beta - Add emjay0921 as maintainer - Make impact records readonly when verification_status is closed - Make incident forms readonly when status is closed - Remove OCA banner from README
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 enhances the user experience and stability of 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 successfully promotes the spp_hazard module to Beta status and enhances the user experience by making closed impact records and incidents read-only, as described. The implementation is solid. My review includes a few suggestions to improve the consistency of XML formatting for better code maintainability.
| <field name="impact_type_id" options="{'no_create': True}" | ||
| readonly="verification_status == 'closed'"/> |
There was a problem hiding this comment.
The formatting of attributes for this field tag is inconsistent with other fields in this file (e.g., registrant_id on line 54). For better readability and maintainability, please adopt a consistent style where attributes other than name and placeholder are on new lines.
| <field name="impact_type_id" options="{'no_create': True}" | |
| readonly="verification_status == 'closed'"/> | |
| <field name="impact_type_id" | |
| options="{'no_create': True}" | |
| readonly="verification_status == 'closed'"/> |
| <field name="category_id" options="{'no_create': True}" | ||
| readonly="status == 'closed'"/> |
There was a problem hiding this comment.
The formatting of attributes for this field tag is inconsistent with other fields in this file (e.g., name on line 66). For better readability and maintainability, please adopt a consistent style where attributes other than name and placeholder are on new lines.
| <field name="category_id" options="{'no_create': True}" | |
| readonly="status == 'closed'"/> | |
| <field name="category_id" | |
| options="{'no_create': True}" | |
| readonly="status == 'closed'"/> |
| <p class="text-muted">Add areas affected by this incident. Specify severity and population estimates for each area.</p> | ||
| <field name="incident_area_ids"> | ||
| <p class="text-muted" invisible="status == 'closed'">Add areas affected by this incident. Specify severity and population estimates for each area.</p> | ||
| <field name="incident_area_ids" readonly="status == 'closed'"> |
There was a problem hiding this comment.
For fields that contain child elements, it's better to place attributes on new lines for readability. This also makes it consistent with how other fields are formatted in this file.
<field name="incident_area_ids"
readonly="status == 'closed'">
| </page> | ||
| <page string="Impacts" name="impacts"> | ||
| <field name="impact_ids"> | ||
| <field name="impact_ids" readonly="status == 'closed'"> |
There was a problem hiding this comment.
For fields that contain child elements, it's better to place attributes on new lines for readability. This also makes it consistent with how other fields are formatted in this file.
<field name="impact_ids"
readonly="status == 'closed'">
Add custom tools/readme_template.rst.jinja to avoid OCA banner in generated READMEs. Add --template-filename to pre-commit config. Regenerate spp_hazard README with correct template.
1 participant
Why is this change needed?
How was the change implemented?
readonly="verification_status == 'closed'"to all editable fields in impact form.readonly="status == 'closed'"to all editable fields in incident form.development_statusto Beta, addedemjay0921as maintainer.New unit tests
Unit tests executed by the author
How to test manually
Related links