-
Notifications
You must be signed in to change notification settings - Fork 10
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
[#336] Removed Hijack #343
Conversation
Codecov Report
@@ Coverage Diff @@
## master #343 +/- ##
==========================================
- Coverage 94.78% 94.77% -0.01%
==========================================
Files 131 131
Lines 4543 4538 -5
==========================================
- Hits 4306 4301 -5
Misses 237 237
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@alextreme deze kan ook nagekeken worden |
src/objects/accounts/admin.py
Outdated
from .models import User | ||
|
||
|
||
@admin.register(User) | ||
class _UserAdmin(UserAdmin, HijackUserAdminMixin): | ||
list_display = UserAdmin.list_display + ("hijack_field",) | ||
class UserAdmin(UserAdmin): |
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.
dit kan dan weg volgens mij
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.
dit kan dan weg volgens mij
Ik kan de @admin.register(User)
niet gebruiken zonder de UserAdmin class
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.
3918e19
to
37672ca
Compare
@alextreme rebase gelukt |
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.
Fine by me, but I suspect that @annashamray would like to see the hijack-removal and the blacking of the migrations in two separate commits
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.
@alextreme I think like this PR and #341 contradict each other. In PR #341 hijack is even updated
I think one of them should be closed
@@ -36,3 +34,4 @@ elastic-apm # Elastic APM integration | |||
# Common ground libraries | |||
vng_api_common[markdown_docs]>=1.6.4 | |||
zgw-consumers # external api auths | |||
markdown==3.4.3 |
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.
Please remove pinned version from base.in, we pin versions in base.txt
And why is this package needed?
Django needs to be upgraded (that is what #341 was for) and django-hijack needs to be removed. That Erhan also decided to upgrade django-hijack in #341 is indeed redundant, but not a contridiction. If this PR is approved & merged before #341 then #341 needs to be updated accordingly. |
@alextreme Ok, then I'll wait for #341 to be merged and then review this PR |
No description provided.