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

Disable email confirmation. #14406

Merged
merged 1 commit into from Apr 18, 2017
Merged

Disable email confirmation. #14406

merged 1 commit into from Apr 18, 2017

Conversation

ashercodeorg
Copy link
Contributor

In particular, this PR removes the sending of confirmation emails and the partial alerting the user that their email has not been confirmed. Further PRs will dismantle associated code (deleting the partial view itself, deleting the confirmation email, updating our devise usage, and removing the associated DB columns).

@ashercodeorg
Copy link
Contributor Author

@poorvasingal and/or @ryansloan to confirm we do still want to deprecate our usage of confirmable.

Copy link

@jeremydstone jeremydstone left a comment

Choose a reason for hiding this comment

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

LGTM, pending your being sure (per your comments on the PR) that this is a change we want to make.

@poorvasingal
Copy link
Contributor

I can't think of a reason right now for why we'd want to keep this feature. But I wouldn't be surprised if we wanted to add it back in when we start thinking about parent accounts. I think that feature will be different enough though (e.g. it'll be an email asking the parent to confirm that they are okay with their child having their own account).

@ashercodeorg
Copy link
Contributor Author

@poorvasingal: Since our (past) usage of email confirmation is tied to the Devise::Models::Confirmable module, this feature is fairly restricted in flexibility. If we were to want to store a parent email, that would be a very different feature for which we should be using another solution.

@joshlory
Copy link
Contributor

Does this mean we no longer confirm teacher account email addresses?

@aoby
Copy link
Contributor

aoby commented Apr 17, 2017

@joshlory yes, this is the email address confirmation email that goes out automatically when a new teacher account is created. We have a different concept of validated / confirmed teachers, that enables parts of the UI (I don't know too much about this part), which needs an upgrade but is currently handled by the cohort model.

@ashercodeorg
Copy link
Contributor Author

@joshlory: Yes, as @aoby notes. This feature was a frequent source of zendesk tickets, a frequent source of confusion, a PII leak (for some time, we accidentally stored a student email if they started as a teacher, modified the email without confirming it, and switched to a student account), and never used (in that nothing changed before or after confirmation).

@aoby: My guess as that you are thinking of authorized teachers which (for example) solution access for some levels is gated on.

@ashercodeorg
Copy link
Contributor Author

FYI @caleybrock, this is expected to change any eyes tests that happened to capture the alert. My intent is to accept any new baselines that arise from the robo-DTT this AM.

@ashercodeorg ashercodeorg merged commit a25bb09 into staging Apr 18, 2017
@ashercodeorg ashercodeorg deleted the disableConfirmable branch April 18, 2017 12:08
@aoby
Copy link
Contributor

aoby commented Apr 18, 2017

Yes @ashercodeorg that's it: authorized teachers. Thanks! Long term we need to remove the cohort dependency, but that's another story.

@ashercodeorg
Copy link
Contributor Author

Do we have any idea what will eventually replace the cohort dependency?

@aoby
Copy link
Contributor

aoby commented Apr 18, 2017

No. We have talked vaguely about making a new model (or property on user) for this purpose, but nothing concrete.

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.

None yet

5 participants