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.

Sign up for GitHub

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

Jump to bottom

Update team invitation email link #26550

Merged
merged 22 commits into from
Aug 31, 2023
Merged

Update team invitation email link #26550

merged 22 commits into from
Aug 31, 2023

Conversation

jackHay22
Copy link
Contributor

@jackHay22 jackHay22 commented Aug 16, 2023 •
edited
Loading

Motivation

The current team invitation email links directly to the invite acceptance page. This requires users that are logged out or don't have an account to click the link twice (they must click a second time once they have successfully logged in).

This PR detects whether the invited email is already associated with an account and directs the user to the login page or sign-up page accordingly. We then use theredirect_to mechanism to navigate the user to accept the invite once they are successfully logged in.

Changes

  • Changes team invite email link to signup/login + redirect to accept invite
  • Team invite email detects whether a user exists with the invited email when rendering
  • Changes signup route to respect redirects

Scenarios

  1. User does not have an account
  • User navigates to signup, creates an account, and is redirected to accept the invitation.
  • Relevant change: signup route respects redirects.
  1. User has an account but is signed out
  • User navigates to login and is redirected to accept the invitation once they have successfully logged in.
  1. User has an account and is signed in
  • User navigates to login and is redirected to accept the invitation.

TODO

  • Add integration test

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 16, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 16, 2023
@lunny lunny added the type/enhancement An improvement of existing functionality label Aug 21, 2023
@lunny lunny added this to the 1.21.0 milestone Aug 21, 2023
kdumontnu
kdumontnu approved these changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@kdumontnu kdumontnu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍 I think this is a great improvement, based on issues we've seen with new users signing up.

Since it looks like this PR makes two changes, so if there is some hesitation for one, we can also split this out into two PRs and merge the other:

  • Add redirect when user clicks the invite link and they are signed out
  • Make the invite link sign-in or register conditional on whether the email is associated with an account.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 21, 2023
@jackHay22 @jtran
Check that user is not nil before ProhibitLogin
4aa72a1 
Co-authored-by: Jonathan Tran <jonnytran@gmail.com>
@jackHay22 jackHay22 requested a review from lunny August 23, 2023 19:24
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 24, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 24, 2023
@jackHay22
Copy link
Contributor Author

jackHay22 commented Aug 24, 2023 •
edited
Loading

I think we should hold off on merging this. The redirect won't work if there is an activation step after the signup form.

@kdumontnu @lunny

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Aug 24, 2023
@jackHay22 jackHay22 requested a review from jtran August 30, 2023 20:22
kdumontnu
kdumontnu approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@kdumontnu kdumontnu left a comment

Choose a reason for hiding this comment

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

💯 Okay, I think we have sufficiently tested the snot out of this thing 😄

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 30, 2023
@puni9869
Copy link
Member

puni9869 commented Aug 31, 2023 •
edited
Loading

@kdumontnu Could you give a final gohead.
Looks good to me.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 31, 2023
@puni9869 puni9869 added the lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged label Aug 31, 2023
@kdumontnu
Copy link
Contributor

@kdumontnu Could you give a final gohead. Looks good to me.

LGTM as well, but I don't have merge abilities either - will post in discord for a merge

@jolheiser jolheiser added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 31, 2023
@GiteaBot GiteaBot removed the lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged label Aug 31, 2023
@jolheiser jolheiser enabled auto-merge (squash) August 31, 2023 15:44
@jolheiser jolheiser merged commit c0ab707 into go-gitea:main Aug 31, 2023
24 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 31, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 1, 2023
@zjjhot
Merge remote-tracking branch 'giteaoffical/main'
92a4a33 
* giteaoffical/main: (22 commits)
  Use case-insensitive regex for all webpack assets (go-gitea#26867)
  restrict certificate type for builtin SSH server (go-gitea#26789)
  feat(API): add secret deletion functionality for repository (go-gitea#26808)
  Avoid double-unescaping of form value (go-gitea#26853)
  Move web/api context related testing function into a separate package (go-gitea#26859)
  Remove some unused CSS styles (go-gitea#26852)
  [skip ci] Updated translations via Crowdin
  Minor dashboard tweaks, fix flex-list margins (go-gitea#26829)
  Update team invitation email link (go-gitea#26550)
  Redirect from `{repo}/issues/new` to `{repo}/issues/new/choose` when blank issues are disabled (go-gitea#26813)
  Remove "TODO" tasks from CSS file (go-gitea#26835)
  User details page (go-gitea#26713)
  Render code blocks in repo description (go-gitea#26830)
  Remove joinPaths function (go-gitea#26833)
  Remove polluted `.ui.right` (go-gitea#26825)
  Sync tags when adopting repos (go-gitea#26816)
  rm comment about hugo (go-gitea#26832)
  Fix filename for .spectral.yaml (go-gitea#26828)
  [skip ci] Updated translations via Crowdin
  Check blocklist for emails when adding them to account (go-gitea#26812)
  ...
This was referenced Sep 6, 2023
Merged
Closed
KN4CK3R pushed a commit that referenced this pull request Sep 7, 2023
@jackHay22
Team invite url fix when registration disabled (#26950)
1a99533 
This is a follow-on to #26550 and
fixes the case where the team invite links to the registration page if
it is disabled in settings.
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kdumontnu kdumontnu kdumontnu approved these changes

@puni9869 puni9869 puni9869 approved these changes

@lunny lunny Awaiting requested review from lunny

@jtran jtran Awaiting requested review from jtran

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jackHay22 @jtran @kdumontnu @puni9869 @lunny @GiteaBot @jolheiser