-
-
Notifications
You must be signed in to change notification settings - Fork 840
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
Confirmation email code duplication #2193
Comments
I'm not sure that's worth it seeing how little code there's in the Confirmation controller. Though we could probably move some of the methods of the ActivationMailer to a trait and re-use them in both places. |
Tbh, this seems like a litle over-optimization. If we need to use that code again in a third place, then let's abstract it away. But for now, I think that might bring more confusion than justified |
Honestly I don't agree. The code in the two files literally does the same exact thing, it's just arranged slightly differently. I think this brings more confusion because there are multiples places where the confirmation email can be sent out, with the same exact configuration. A change in the confirmation email would require modifying the same (copy-pasted) code in two places, and I don't see how this could be considered good or make sense. EDIT: moreover, in the case of password reset the code is already extracted in a command. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. |
Hello,
I've noticed that the SendConfirmationEmailController and AccountActivationMailer classes do almost exactly the same thing, with the only difference that they are triggered in different situations.
I think that there's space for improvement and that the code duplication could be avoided.
The text was updated successfully, but these errors were encountered: