-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#1885] Changed document admin to be readonly #887
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #887 +/- ##
===========================================
+ Coverage 94.68% 94.79% +0.10%
===========================================
Files 831 857 +26
Lines 29275 30085 +810
===========================================
+ Hits 27720 28519 +799
- Misses 1555 1566 +11 ☔ View full report in Codecov by Sentry. |
Note my remark: "This is to be corrected, the default document-file extensions from the Open Zaak configuration can be used as the default whitelist. This validation is to be added on a model-level, and other filefields in Open Inwoner can be checked and if necessary the same validation can be added to those filefields as well." As Users can upload documents (via a plan) this will need validation of some kind. And the other FileFields will likely also require attention. |
b8eeb2b
to
d1bd1bd
Compare
2de7a07
to
4138e9a
Compare
363492c
to
ebfe38e
Compare
@alextreme in the PR description I made a list of the file fields that I could find.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenbal See note in the diff.
src/open_inwoner/accounts/admin.py
Outdated
fields = [ | ||
"uuid", | ||
"name", | ||
"description", | ||
"status", | ||
"type", | ||
"end_date", | ||
"display_file_url", | ||
"is_for", | ||
"created_on", | ||
"updated_on", | ||
"created_by", | ||
"plan", | ||
"is_deleted", | ||
] | ||
readonly_fields = ( | ||
"uuid", | ||
"name", | ||
"description", | ||
"status", | ||
"type", | ||
"end_date", | ||
"display_file_url", | ||
"is_for", | ||
"created_on", | ||
"updated_on", | ||
"created_by", | ||
"plan", | ||
"is_deleted", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not very DRY to repeat the same list? You could assign then same:
readonly_fields = fields
Also even more robust and less manual configuration to use has_change_permission()
instead of explicitly listing everything. For example, if a new field gets added we need to remember it has to be added here (twice!) instead of being readonly by default. It is also more correct to make the model as a whole readonly instead of manually marking all fields.
This goes for all the changes that do similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated PR looks fine to me, retriggered the tests
task: https://taiga.maykinmedia.nl/project/open-inwoner/task/1885
In the pentest you could upload here, but this is user content so no need to create/edit it through the admin.
Fields:
settings.UPLOAD_FILE_TYPES
settings.UPLOAD_FILE_TYPES
settings.UPLOAD_FILE_TYPES
siteconfig.theme_stylesheet-> .css extension validationCaseUploadForm.files-> configurable extension validation in admin