Skip to content

LG-12018: Refactor account deletion/message to be variable#9927

Merged
jmdembe merged 62 commits intomainfrom
jd-LG-12018-account-deletion-variable
Feb 2, 2024
Merged

LG-12018: Refactor account deletion/message to be variable#9927
jmdembe merged 62 commits intomainfrom
jd-LG-12018-account-deletion-variable

Conversation

@jmdembe
Copy link
Copy Markdown
Contributor

@jmdembe jmdembe commented Jan 16, 2024

🎫 Ticket

LG-12018: Refactor account deletion/message to be variable

🛠 Summary of changes

This PR supports future efforts where the waiting period of a deletion account can be a variable.

📜 Testing Plan

N/A because this is an internal change

@jmdembe jmdembe marked this pull request as ready for review January 18, 2024 19:18
@jmdembe jmdembe requested a review from a team January 18, 2024 19:18
@jmdembe jmdembe force-pushed the jd-LG-12018-account-deletion-variable branch from 2f80ef3 to af1055b Compare January 25, 2024 18:02
@jmdembe jmdembe force-pushed the jd-LG-12018-account-deletion-variable branch from 82220ff to eaa8372 Compare January 26, 2024 14:45
@jmdembe jmdembe requested a review from a team January 29, 2024 15:52
@jmdembe jmdembe force-pushed the jd-LG-12018-account-deletion-variable branch from 03e4b1b to 002dc06 Compare January 29, 2024 19:39
@jmdembe jmdembe force-pushed the jd-LG-12018-account-deletion-variable branch from aa62842 to d208f5e Compare January 31, 2024 20:56
def show
analytics.account_reset_visit
presenter = ConfirmationEmailPresenter.new(current_user, view_context)
@confirmation_period = presenter.confirmation_period
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Account reset should use the period from IdentityStore.config.account_reset_wait_period_days, not Devise.confirm_within?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The way I understood this, this should use account_reset_wait_period_days because this is for after the account reset is sent which could vary based on what the user sets.

Copy link
Copy Markdown
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

These changes look good to me.

I noticed there is one more instance of "24 hours" that we need to replace for this ticket. I'll leave it to you if you want to handle that here, or merge this as-is and create a follow-on pull request.

confirm: If you cancel now, you must create a new request and wait another 24
hours to delete your account.

</table>

<p>
<%= t('user_mailer.email_confirmation_instructions.footer', confirmation_period: '24 hours') %>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One other nice effect of this that occurs to me is that this would have been untranslated content, which is now fixed.

You can see "24 hours" in the Spanish version of the current template, for example.

@jmdembe
Copy link
Copy Markdown
Contributor Author

jmdembe commented Feb 2, 2024

I noticed there is one more instance of "24 hours" that we need to replace for this ticket. I'll leave it to you if you want to handle that here, or merge this as-is and create a follow-on pull request.

I'll handle that change here. I do not see a reason why it should be a separate ticket.

@aduth
Copy link
Copy Markdown
Contributor

aduth commented Feb 2, 2024

I'll handle that change here. I do not see a reason why it should be a separate ticket.

That's fine. I wasn't suggesting another ticket though, just another pull request if it'd be easier to separate the code changes that way.

@jmdembe
Copy link
Copy Markdown
Contributor Author

jmdembe commented Feb 2, 2024

I'll handle that change here. I do not see a reason why it should be a separate ticket.

That's fine. I wasn't suggesting another ticket though, just another pull request if it'd be easier to separate the code changes that way.

🤦🏾‍♀️ I read "pull request" as "ticket."

@jmdembe jmdembe merged commit 6fc419d into main Feb 2, 2024
@jmdembe jmdembe deleted the jd-LG-12018-account-deletion-variable branch February 2, 2024 19:37
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.

4 participants