Admin: log_addition/log_change/log_deletion have incorrect parameter names #2413
Labels
bug
Something isn't working
Comments
|
PR is welcome :) |
|
I shall create one later today! |
sergei-maertens
added a commit
to GPP-Woo/GPP-publicatiebank
that referenced
this issue
Oct 21, 2024
Created an upstream issue to django-stubs for the incorrect method signature compared to Django's actual implementation: typeddjango/django-stubs#2413
sergei-maertens
added a commit
to GPP-Woo/GPP-publicatiebank
that referenced
this issue
Oct 21, 2024
* ✨ [#16] added logger function * ✨ [#16] created generic auditlog mixin for admin * ✨ [#16] added audit log event list filter * ✨ [#16] added audit log request headers * ✨ [#16] extended auditlog to create logs for inline admin * ✨ [#16] added system check for missing AdminAuditLogMixin * ✨ [#16] added mixin for api loggin * ✅ [#16] added unit tests for audit logging and api header requirement * ✅ [#16] added inline admin audit log testing * 🎨 [#16] improved type hints * 🎨 [#16] Type annotate + solidify audit headers hook impl The paths key in OpenAPI schema can have keys/values that are not operations, so blindly adding the parameters to each item is too fragile. Additionally, the permission class can re-use the headers defined for the API spec generation, so that we guarantee that implementation and documentation actually remain properly in sync. * ⏪ [#16] Revert audit headers requirement for Catalogi API endpoint This endpoint is implemented using the ZGW API's Catalogi API spec, and the read endpoint does not require any kind of audit headers. * 🏷️ [#16] Fix type checker annotations Created an upstream issue to django-stubs for the incorrect method signature compared to Django's actual implementation: typeddjango/django-stubs#2413 * 🏷️ [#16] Clean up and simplify types used in logging module * Declared the JSON types in general purpose module, they're not logging specific and will get used in a broader context. * Used the new type alias syntax which greatly declutters the code * Removed TYPE_CHECKING guards as the circular imports/AppRegistry not ready errors are resolved earlier * Made the logevent functions kwarg-only, since positional arguments are too confusing at call-sites. * 🎨 [#16] Clean up logging implementation * There was no request to log status codes, so don't do it * When snapshotting the model data, don't dump to JSON and then load it again, instead use a variant of django's built in django.forms.model_to_dict and rely on Django to serialize it at the DB level * Removed most of the # type: ignore comments and replaced with proper type annotations * Made the top-level API for the logging module more explicit, relying on positional arguments is too fragile and makes it unreadable what is semantically being logged. * Moved the public API into woo_publications.logging.service, since doing model imports in the __init__.py layer tries to use django concepts before the app-registry is ready, and that prevents us from having properly typed code. * 🎨 [#16] Enable audit trail logging on user model * 🎨 [#16] Clean up tests There's no need for the audit headers to be an instance variable, instead they can be declared as module constants. The indirection to setUp is there already anyway, and the constant notation makes it more clear that nothing special is going on. * 🚚 [#16] Put API schema generation tests in correct package * 🎨 [#16] Rewrite test to be more semantic The building blocks are already there - all we care about is that the correct amount of log records are shown, and that the correct records are shown. * 🎨 [#16] Apply consistent kwargs-only Python API * 🎨 [#16] Clean up admin logging tests * 🎨 [#16] Properly add dynamic webtest fields This also resolves the type checker errors. * ✅ [#16] Add test for incomplete user information * 🎨 [#16] Implement permission class tests as unit test We don't need to test the entire views, especially in a general-purpose piece of functionality you want to provide unit tests and separately implement the integration tests in the places where it should be used. This cleanup removes the tight coupling with the publications or metadata apps and instead implements the expected behaviour of the permission class itself. * 🚚 [#16] Move tests into right place Tests that operate on a particular model or django app belong in that particular django app - not in the general purpose app providing the implementation. Ideally, the general purpose implementation has agnostic unit tests to ensure the correct behaviour too, but with admin mixins that's a bit harder to achieve, so we'll pick the lesser evil here. * 🚚 [#16] Move publication API logging tests to correct location * ✅ [#16] Add missing tests for metadata audit logs * 🎨 [#16] Implement audit headers as DRF spectacular extension Instead of a post processing hook. The extension approach allows us to add it to the API schema only when it's relevant - e.g. the Catalogi API endpoint does not require this. * 🐛 [#16] Fix inline update logging This was incorrectly being logged as create * 📝 [#16] Ensure Python API is documented properly --------- Co-authored-by: Sergei Maertens <[email protected]>
sergei-maertens
added a commit
to maykinmedia/django-stubs
that referenced
this issue
Oct 21, 2024
Renamed the 'object' parameter to 'obj' to be consistent with the runtime implementation. Verified against Django 4.2 and the main branch.
sergei-maertens
added a commit
to maykinmedia/django-stubs
that referenced
this issue
Oct 21, 2024
Renamed the 'object' parameter to 'obj' to be consistent with the runtime implementation. Verified against Django 4.2 and the main branch.
sobolevn
pushed a commit
that referenced
this issue
Oct 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Bug report
What's wrong
In the
admin/options.pyistubs, thelog_addition,log_changeandlog_deletionuse theobjectparameter name for the object affected, however the Django methods (4.2 and main, see https://github.com/django/django/blob/3fad712a91a8a8f6f6f904aff3d895e3b06b24c7/django/contrib/admin/options.py#L944) useobjfor the paremeter name.This results in Pyright disagreeing with overrides:
MyPy doesn't appear to report any problems related to the parameter names.
How is that should be
Change the
objectvariable name toobj.System information
pythonversion: 3.12.6djangoversion: 4.2.16mypyversion: 1.11.2django-stubsversion: 5.1.0django-stubs-extversion: 5.1.0The text was updated successfully, but these errors were encountered: