fix(spp_registry): use correct field for id type in validation error#117
Conversation
spp.vocabulary.code has no `name` field — its _rec_name is `display`. The validation constraint crashed with AttributeError instead of showing the intended ValidationError message.
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 addresses a critical bug in the Highlights
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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an AttributeError in the registrant ID validation by using the display field instead of the non-existent name field on the spp.vocabulary.code model. The change is correct and resolves the described bug. I have also added a comment regarding a potential improvement to the validation trigger logic for better robustness.
| _( | ||
| "The provided %(id_type)s ID '%(value)s' is invalid.", | ||
| id_type=rec.id_type_id.name, | ||
| id_type=rec.id_type_id.display, |
There was a problem hiding this comment.
While this change to use rec.id_type_id.display is correct, the validation method it belongs to has a potential weakness. The _onchange_id_validation method is only triggered when the value field changes, but the validation logic also depends on id_type_id. This means that if id_type_id is changed, the validation for the existing value is not re-evaluated in the UI. This could be improved by also triggering the validation on id_type_id changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #117 +/- ##
==========================================
+ Coverage 70.14% 70.32% +0.17%
==========================================
Files 739 756 +17
Lines 43997 44863 +866
==========================================
+ Hits 30863 31549 +686
- Misses 13134 13314 +180
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Summary
AttributeErrorinspp.registry.idvalidation constraint caused by accessing.nameonspp.vocabulary.code, which has nonamefield (_rec_name = "display")ValidationErrorwith the ID type label instead of crashingTest plan
spp.registry.idrecord with an ID type that hasid_validationset and provide a value that doesn't match the regex — should show a clean validation error with the ID type name