[OSDEV-1411] Django admin: Fix editing of contributor list#418
[OSDEV-1411] Django admin: Fix editing of contributor list#418VadimKovalenkoSNF merged 4 commits intomainfrom
Conversation
React App | Jest test suite - Code coverage reportTotal: 26.93%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/django/api/migrations/0160_allow_null_parsing_errors_in_facilitylist.py (2)
3-3: Remove unused importThe
django.contrib.postgres.indexesimport is not used in this migration.-import django.contrib.postgres.indexes🧰 Tools
🪛 Ruff
3-3:
django.contrib.postgres.indexesimported but unusedRemove unused import:
django.contrib.postgres.indexes(F401)
3-3: Remove unused importThe
django.contrib.postgres.indexesimport is not used in this migration.-import django.contrib.postgres.indexes🧰 Tools
🪛 Ruff
3-3:
django.contrib.postgres.indexesimported but unusedRemove unused import:
django.contrib.postgres.indexes(F401)
src/django/api/models/facility/facility_list.py (1)
89-90: Consider documenting the error structure schema.Since
parsing_errorsis a JSONField that can now be null or contain an empty list, it would be helpful to document the expected structure of error entries when they exist. This could help maintain consistency in error reporting across the application.Consider adding a docstring or documentation that outlines:
- The expected JSON schema for error entries
- When null vs. empty list should be used
- Example error structures for common scenarios
doc/release/RELEASE-NOTES.md (1)
30-30: Consider enhancing the bug fix descriptionThe current description could be more specific about the actual fix implemented. Consider updating it to:
-* [OSDEV-1411](https://opensupplyhub.atlassian.net/browse/OSDEV-1411) - Django admin: fixed issue when update facility list with empty array `Parsing errors` field. +* [OSDEV-1411](https://opensupplyhub.atlassian.net/browse/OSDEV-1411) - Django admin: fixed validation issue by allowing empty arrays (`[]`) and null values in the `Parsing errors` field by adding `blank=True` and `null=True` attributes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/migrations/0160_allow_null_parsing_errors_in_facilitylist.py(1 hunks)src/django/api/models/facility/facility_list.py(1 hunks)
🧰 Additional context used
🪛 Ruff
src/django/api/migrations/0160_allow_null_parsing_errors_in_facilitylist.py
3-3: django.contrib.postgres.indexes imported but unused
Remove unused import: django.contrib.postgres.indexes
(F401)
🔇 Additional comments (4)
src/django/api/migrations/0160_allow_null_parsing_errors_in_facilitylist.py (3)
14-18: LGTM! The field modifications address the validation issues.
The changes correctly implement the requirements by:
- Adding
blank=Trueto allow empty input in forms - Adding
null=Trueto allow NULL values in database - Including clear help text describing the field's purpose
13-19: Consider data migration needs
Since this migration modifies the parsing_errors field to allow null values, consider whether existing records need data migration (e.g., converting empty lists [] to NULL).
7-11: LGTM!
The migration class is properly defined with correct dependency chain.
src/django/api/models/facility/facility_list.py (1)
89-90: LGTM! The changes correctly address the validation issue.
The addition of null=True and blank=True to the parsing_errors JSONField appropriately resolves the Django Admin validation problem while maintaining backward compatibility.
Let's verify the usage of parsing_errors field in views and serializers:
doc/release/RELEASE-NOTES.md (1)
Line range hint 1-1411: LGTM! Well-structured release notes
The release notes follow a consistent format and provide comprehensive documentation of changes across releases. The organization into clear sections (database changes, code changes, architecture changes, bug fixes, new features) makes it easy to track and understand the evolution of the project.
src/django/api/migrations/0160_allow_null_parsing_errors_in_facilitylist.py
Show resolved
Hide resolved
60b63cb to
809c2f4
Compare
|



Fix OSDEV-1411.
Previously, when a user enters
[](or leaves the field empty), Django Admin interpreted it as "empty input" and failed validation because the field is marked as required.Update
parsing_errorsfield:blank=True: Updated the model field to allow blank input in forms. This tells Django Admin to treat the field as optional.null=True: This allows the field to have aNULLvalue in the database if explicitly set to null in the request.