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

Refactor AccountActivationMailer and SendConfirmationEmailController #2493

Merged
merged 7 commits into from
Mar 19, 2021

Conversation

slkrx
Copy link
Contributor

@slkrx slkrx commented Dec 10, 2020

Fixes #2193

Changes proposed in this pull request:

Moves the common code between SendConfirmationEmailController and AccountActivationMailer into a trait.

Reviewers should focus on:

I wasn't sure what to name the trait or where to put it, but because most of the code for the trait was taken from the AccountActivationMailer class I decided to name it the AccountActivationMailerTrait and keep it in the same directory. Also, I decoupled the generateToken and getEmailData functions because I think it will make more modular and maintainable even through it may make using the trait more verbose.

Confirmed

  • Backend changes: tests are green (run composer test).

src/Api/Controller/SendConfirmationEmailController.php Outdated Show resolved Hide resolved
src/User/AccountActivationMailerTrait.php Outdated Show resolved Hide resolved
src/User/AccountActivationMailerTrait.php Show resolved Hide resolved
Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Looks good to me! We're currently in QA for beta 15, and there's a holiday break scheduled after that release, but I see no reason why we shouldn't be able to get this in for beta 16. Thanks!

@askvortsov1
Copy link
Member

askvortsov1 commented Jan 4, 2021

A side effect of this would implement #2515, which still needs a bit more discussion before merging this. That being said, I don't think anything should be changed here yet (no need to exclude the prefix conditionally), unless a decision is reached to keep the prefix in one place but not the other, which seems kinda unlikely.

@SychO9 SychO9 merged commit 374189d into flarum:master Mar 19, 2021
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.

Confirmation email code duplication
3 participants