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

Pass through redirect query arg #233

Merged
merged 2 commits into from
Dec 7, 2023
Merged

Conversation

blag
Copy link
Contributor

@blag blag commented Oct 17, 2023

Fixes #231.

Pass through a next=... query parameter to the signup and login views.

The docs for SIGNUP_REDIRECT and LOGIN_REDIRECT indicate they should be a URL name, the default values for these settings are URL names, and the tests for the allauth backend do not override those options and so expect a URL name.

But the tests for the basic backend override that option with a direct URL:

        settings.INVITATIONS_LOGIN_REDIRECT = "/login-url/"
        ...
        settings.INVITATIONS_LOGIN_REDIRECT = "/login-url/"
        ...
        settings.INVITATIONS_SIGNUP_REDIRECT = "/signup-url/"
        ...
        settings.INVITATIONS_SIGNUP_REDIRECT = "/non-existent-url/"
        ...
        settings.INVITATIONS_SIGNUP_REDIRECT = "/non-existent-url/"
        ...
    @override_settings(INVITATIONS_SIGNUP_REDIRECT="/non-existent-url/")
    ...
        settings.INVITATIONS_LOGIN_REDIRECT = "/login-url/"

Which is why I do this in this PR:

        try:
            signup_redirect = reverse(app_settings.SIGNUP_REDIRECT)
        except NoReverseMatch:
            signup_redirect = app_settings.SIGNUP_REDIRECT
        ...
        try:
            login_redirect = reverse(app_settings.LOGIN_REDIRECT)
        except NoReverseMatch:
            login_redirect = app_settings.LOGIN_REDIRECT

Fixing this may not be backwards compatible with existing users. It is also best left for a separate discussion, so for this PR I allowed for both options.

I also removed the walrus operator in a separate commit, so once we drop support for Python 3.7 we can revert that single commit to use the walrus operator.

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (dde3307) 64.34% compared to head (51e481e) 65.71%.

Files Patch % Lines
invitations/views.py 82.60% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
+ Coverage   64.34%   65.71%   +1.36%     
==========================================
  Files          18       18              
  Lines         474      490      +16     
==========================================
+ Hits          305      322      +17     
+ Misses        169      168       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blag
Copy link
Contributor Author

blag commented Oct 17, 2023

pre-commit.ci autofix

@valberg
Copy link
Contributor

valberg commented Dec 1, 2023

Hi @blag

Nice work! 👏

Python 3.7 has been dropped in 2.1.0 - so we can revert the mentioned commit 😊

Also there is a conflict.

I'll approve and merge when those issues are resolved 👍

Copy link
Contributor

@valberg valberg left a comment

Choose a reason for hiding this comment

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

Reintroduce the walrus operator 🦭

Fix conflict.

🚀

@blag blag force-pushed the pass-through-redirect-query-arg branch from 39d4262 to 51e481e Compare December 7, 2023 08:07
@blag
Copy link
Contributor Author

blag commented Dec 7, 2023

Removed commit and fixed conflict.

Copy link
Contributor

@valberg valberg left a comment

Choose a reason for hiding this comment

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

Awesome! 👏

@valberg valberg merged commit c3162d2 into master Dec 7, 2023
54 checks passed
@valberg valberg deleted the pass-through-redirect-query-arg branch December 7, 2023 08:53
@blag
Copy link
Contributor Author

blag commented Dec 15, 2023

Hey @valberg, thanks for the merge.

Do you mind tagging a release? I have a project that needs my fix and I'd like to specify a version instead of a commit hash for it. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: pass query params like ?next=/some/page through the flow
2 participants