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

Add support for non-integer primary key user models #216

Merged
merged 1 commit into from Feb 18, 2021
Merged

Conversation

codingjoe
Copy link
Collaborator

@codingjoe codingjoe commented Jan 30, 2021

Simplify URL and view structure. Add support for multiple PK types
based on URL pattern as well as natural key support via URL patterns.

Changes:

  • Deprecate HIJACK_URL_ALLOWED_ATTRIBUTES setting favoring HIJACK_USER_URL_PATTERN.
  • Deprecate URL names login_with_id, login_with_username and login_with_email favoring acquire.
  • Deprecate URL name release_hijack favoring release.
  • Deprecate views login_with_id, login_with_username and login_with_email favoring release_user_view.
  • Deprecate view release_hijack favoring release_user_view.

Close #196
Close #183
Close #184
Close #198
Close #147
Close #175

hijack/tests/test_hijack.py Outdated Show resolved Hide resolved
@Mogost
Copy link
Member

Mogost commented Jan 30, 2021

Overall, I like the changes, but it will take me a while to test them. I'll check it out next week.

@Mogost
Copy link
Member

Mogost commented Feb 6, 2021

@codingjoe could you please resolve the conflicts?

@codecov-io
Copy link

codecov-io commented Feb 6, 2021

Codecov Report

Merging #216 (bccbab9) into master (023cf4f) will decrease coverage by 7.11%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
- Coverage   95.63%   88.51%   -7.12%     
==========================================
  Files           7        7              
  Lines         206      209       +3     
==========================================
- Hits          197      185      -12     
- Misses          9       24      +15     
Impacted Files Coverage Δ
hijack/checks.py 68.88% <ø> (-31.12%) ⬇️
hijack/settings.py 90.90% <66.66%> (-9.10%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 023cf4f...bccbab9. Read the comment docs.

@codingjoe
Copy link
Collaborator Author

@codingjoe could you please resolve the conflicts?

@Mogost done :)

hijack/urls.py Outdated Show resolved Hide resolved
hijack/urls.py Outdated Show resolved Hide resolved
hijack/views.py Outdated Show resolved Hide resolved
Copy link
Member

@Mogost Mogost left a comment

Choose a reason for hiding this comment

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

I tested it. All in all, it seems to be working fine. The backward compatibility is there.
There are a couple of things which I have put in my suggestions.
Another thing, when it will be released, it is desirable to reflect the changes in the release notes well.

hijack/views.py Outdated Show resolved Hide resolved
hijack/views.py Outdated Show resolved Hide resolved
hijack/urls.py Show resolved Hide resolved
hijack/urls.py Show resolved Hide resolved
@codingjoe
Copy link
Collaborator Author

@Mogost regarding the release notes. I would prefer to write those only in GitHub in the future. I would actually prefer to remove the current change log file to avoid confusion. It is included in old distributions, that should be enough. I will open a separate PR for that.

hijack/urls.py Outdated Show resolved Hide resolved
Simplify URL and view structure. Add support for multiple PK types
based on URL pattern as well as natural key support via URL patterns.

Changes:

* Deprecate `HIJACK_URL_ALLOWED_ATTRIBUTES` setting favoring `HIJACK_USER_URL_PATTERN`.
* Deprecate URL names `login_with_id`, `login_with_username` and `login_with_email` favoring `acquire`.
* Deprecate URL name `release_hijack` favoring `release`.
* Deprecate views `login_with_id`, `login_with_username` and `login_with_email` favoring `release_user_view`.
* Deprecate view `release_hijack` favoring `release_user_view`.

Close #196
Close #183
Close #184
Close #198
Close #175

Co-Authored-By: Alexandr Artemyev <mogost@gmail.com>
@Mogost
Copy link
Member

Mogost commented Feb 18, 2021

Probably after this merge it is worth making a release

@codingjoe
Copy link
Collaborator Author

this one https://pypi.org/project/django-hijack/2.3.0 😉

@codingjoe
Copy link
Collaborator Author

Actually, once we updated the template integration, I would shoot for v3 (major) and remove all departed code.

@Mogost
Copy link
Member

Mogost commented Feb 19, 2021

@codingjoe We should not remove old code until we have dealt with django-hijack-admin
https://github.com/django-hijack/django-hijack-admin/blob/934b2b94defe2986f66642bd60238cf752db8803/hijack_admin/admin.py#L24-L29

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.

Allow for UUID IDs in User Change requirement for user_id primary key
3 participants