Skip to content
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

Fix #527 -- HijackUserAdminMixin: handle cases in which ForeignKey to User is not set #528

Merged
merged 3 commits into from Mar 10, 2023

Conversation

simonkern
Copy link
Contributor

No description provided.

Copy link
Collaborator

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you already provided a patch. Excellent. I only had a small comment. Great that you added tests as well 👍

hijack/permissions.py Show resolved Hide resolved
@simonkern
Copy link
Contributor Author

@codingjoe is that what you had in mind?

Copy link
Collaborator

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there 👍
One last change and I believe we can release this.

hijack/permissions.py Outdated Show resolved Hide resolved
handle hijacked==None and prevent error 500

resolves django-hijack#527

Revert "condition replaced by exception handling in permission checks"

This reverts commit 922b498.
@simonkern
Copy link
Contributor Author

@codingjoe any further changes you want me to do?

@codingjoe
Copy link
Collaborator

@codingjoe any further changes you want me to do?

Nothing except sponsoring my maybe, so I find more time for OSS 🤣

In all seriousness, I will try to review it today.

Copy link
Collaborator

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @simonkern! Let's get this out of the door!

@codingjoe codingjoe merged commit e096bf9 into django-hijack:master Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using HijackUserAdminMixin on Classes with optional ForeignKeys to User may lead to error 500
2 participants