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

Confirmation email code duplication #2193

Closed
matteocontrini opened this issue Jun 8, 2020 · 4 comments · Fixed by #2493
Closed

Confirmation email code duplication #2193

matteocontrini opened this issue Jun 8, 2020 · 4 comments · Fixed by #2493

Comments

@matteocontrini
Copy link
Contributor

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.

@clarkwinkelmann
Copy link
Member

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.

@askvortsov1
Copy link
Sponsor Member

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

@matteocontrini
Copy link
Contributor Author

matteocontrini commented Jul 12, 2020

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.

@stale
Copy link

stale bot commented Dec 31, 2020

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.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

@stale stale bot added the stale Issues that have had over 90 days of inactivity label Dec 31, 2020
@clarkwinkelmann clarkwinkelmann removed the stale Issues that have had over 90 days of inactivity label Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants