-
Notifications
You must be signed in to change notification settings - Fork 26
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
[#3889] Improve timeline logs admin #4086
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4086 +/- ##
=======================================
Coverage 96.10% 96.10%
=======================================
Files 730 730
Lines 23085 23130 +45
Branches 2698 2701 +3
=======================================
+ Hits 22185 22230 +45
Misses 638 638
Partials 262 262 ☔ View full report in Codecov by Sentry. |
src/openforms/logging/templates/logging/events/outgoing_request_log_details_view_admin.txt
Outdated
Show resolved
Hide resolved
self.assertEqual( | ||
[ | ||
option | ||
for option in html_form.fields.get("action")[0].options | ||
if option[0] == "delete_selected" | ||
], | ||
[], | ||
) |
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.
can't we just assert what is present?
options = {value for value, *_ in html_form.fields.get("action")[0].options}
self.assertEqual(options, {"_export"}) # or whatever the relevant value is
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.
This test is originally to assert that you can't delete records. With your assertion suggestion the test would break whenever a new action gets added? Probably not going happen anyway 🤔
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.
yeah and if the value of the action option in django changes, your version will break and be harder to debug, while this proposed change is a lot more readable and obvious
the previous test assertion worked because it checked that the action dropdown as a whole is absent :)
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.
Well same can happen if django-import-export changes the name of the action as well 😄 but this would be a straightforward test update anyway
fb82f6a
to
6164956
Compare
src/openforms/logging/templates/logging/events/outgoing_request_log_details_view_admin.txt
Outdated
Show resolved
Hide resolved
src/openforms/logging/templates/logging/events/outgoing_request_log_details_view_admin.txt
Outdated
Show resolved
Hide resolved
93dcda1
to
dc18dc1
Compare
dc18dc1
to
ae56b09
Compare
Fixes #3889