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

Resolves #4773: Reset password and invitation expiry #4796

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

pepetorres1998
Copy link

@pepetorres1998 pepetorres1998 commented Nov 20, 2024

Resolves #4773

Description

  • Changes the devise configuration for invites to expire 2 weeks after being sent.
    • Changes mailer so the user knows the date and time of expiration in the GMT zone.
  • User password reset was already configured to last 6 hours, modified the mail to reflect that.

Please let me know if it is better to squash the commits into 1 or two commits.

Also, let me know if would be good to add a test with time helpers to test that in fact the invitation/password reset instruction does expire within the time frame. I didn't add them because I thought it is an expensive test to run everytime and also I thought this is something we may not change again so Idk if its worth to include it, but please let me know if it is.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

User invite
Logged in as:

Go to My organization -> Users -> "Invite user to this organization"

Password reset
Sent password reset instructions in the Log In screen with org admin email: org_admin1@example.com

Screenshots

User invitation:

image

User password reset:

image

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hi! Thank you for your work on this so far -- I've got a couple of comments from a functional point of view. Could you address those, please?

@@ -21,7 +21,7 @@ en:
hello: "Hello %{email}"
someone_invited_you: "Someone has invited you to %{url}, you can accept it through the link below."
accept: "Accept invitation"
accept_until: "This invitation will be due in %{due_date}."
accept_until: "This invitation will be due in %{due_date} GMT."
ignore: "If you don't want to accept the invitation, please ignore this email.<br />\nYour account won't be created until you access the link above and set your password."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Per the conversation in the issue, this should be "This invitation will expire at [Date and Time] GMT or if a new password reset is triggered. "

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry, let me know if the message is correct now 🙏

@@ -373,9 +373,6 @@
<% if @resource.invitation_due_at %>
<p><%= t("devise.mailer.invitation_instructions.accept_until", due_date: l(@resource.invitation_due_at, format: :'devise.mailer.invitation_instructions.accept_until_format')) %></p>
<% end %>
<p>For security reasons these invitations expire. This invitation will expire in 8 hours or if a new password reset is triggered.</p>
<p>If your invitation has an expired message, go <%= link_to "here", new_user_password_url %> and enter your email address to reset your password.</p>
<p>Feel free to ignore this email if you are not interested or if you feel it was sent by mistake.</p>
</td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We still need to show the text about what to do if your invitation has expired.

Copy link
Author

Choose a reason for hiding this comment

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

For sure 🙌

@pepetorres1998
Copy link
Author

I made the changes needed, let me know if you want me to squash my commits into one or if anything else is missing 🙌

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.

Reset password and invitation expiry
2 participants