-
Notifications
You must be signed in to change notification settings - Fork 1
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
Added publication show logs link in change view #89
Conversation
Codecov ReportAttention: Patch coverage is
|
22c81a8
to
af3c6d6
Compare
This approach is the best of both worlds: * the admin mixin prepares the template context for the change view * the 'change_form_object_tools.html' template override can be global, provided we check for the necessary context from the mixin - that way we only have a very minimal snippet of template code that needs to be overridden * we don't need provide app/model specific template overrides, nor do we need to specify a custom template to use for the change_view
@@ -58,10 +58,15 @@ def log_deletion(self, request, object, object_repr): | |||
|
|||
return super().log_deletion(request, object, object_repr) # type: ignore reportAttributeAccessIssue | |||
|
|||
def change_view(self, request, object_id, form_url="", extra_context=None): | |||
def change_view(self, request, object_id, form_url="", extra_context={}): |
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.
mutable defaults are an anti-pattern! See https://www.pythonmorsels.com/mutable-default-arguments/
TimelineLog.objects.create(extra_data={}) | ||
TimelineLog.objects.create(extra_data=[]) | ||
|
||
with self.subTest("publications has 'show logs' button and filters correctly"): |
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 is again a test that belongs in the publications
app rather than in the logging
app - the giveway is that you're importing factories/models from another django app.
However, we can rephrase this test a little bit to make it more "generic" in nature, I will push a commit for that.
af3c6d6
to
891a828
Compare
Partially Fixes #16