-
Notifications
You must be signed in to change notification settings - Fork 71
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
Notify new user if their email domain matches existing Perma partner(s) #3596
Notify new user if their email domain matches existing Perma partner(s) #3596
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3596 +/- ##
===========================================
+ Coverage 68.81% 68.85% +0.03%
===========================================
Files 48 48
Lines 7014 7023 +9
===========================================
+ Hits 4827 4836 +9
Misses 2187 2187 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Jazzed about this feature.
One thought, and one question.
The thought: I don't think we have an appropriate index for the query that looks for matching registrars. I have a production-like database locally, which would make it easy for me to test and see an index in use; if you'd like, I'm happy to pull this down, add one, and push up a commit. I think something like:
indexes = [
models.Index(fields=['status', 'website'])
]
might do the job.
Are you familiar with Django debug toolbar? We have it installed: if you run with invoke run --debug-toolbar
, you get access to a sidebar that includes an SQL panel that lets you inspect all the SQL that runs when loading a page. When investigating things like this, I like to spuriously add the ORM queries I'm digging into to, like, the About page, so that they show up in the sidebar when you visit /about/, without any further work required... but I know other people prefer other strategies. (For instance, finding the SQL generated by the ORM query and running EXPLAIN in the dbshell themselves, etc.)
The question is, double checking that registrar.email
is where we want to direct people. As far as I am aware, when we email registrars, we usually email the registrar users associated with a registrar. I don't have an opinion, just wanted to make sure this is the intent.
@rebeccacremona Good call re: adding an index, since we'll be calling this over and over. Yes, if you have capacity to test drive that locally on your production-like DB and push up any commits, I'd welcome it. That's a good question re: registrar emails. The proposed UX here comes from this discussion with Clare and Jack, which I interpreted to mean "share the list of |
@cmsetzer Righto, will do the index shortly! Hmmm, reading that conversation, I think it's a little ambiguous... especially because sometimes when chatting, we have a tendency to refer to registrar users as "registrars". But! I think this is fine. Scrolling through a couple pages of the registrar list in production, it does seem like these emails are generally populated with meaningful values. We could always readdress if we hear from users that it turns out that a large percentage of these addresses turn out to bounce, or whatever! |
This branch adds some logic to notify new users if we think they might belong to one or more existing Perma registrars.
How this works: upon new user registration,
suggest_registrars
queries the DB forRegistrar.website
values whose base domain matches the base domain from the new user's email address. If there are matches, they are included in the confirmation email:Validation
website
values. For example,https://lil.law.harvard.edu
andhttps://library.harvard.edu
./sign-up
and create a new user account using an email address with the base domain from step 2. For example,new_user@law.harvard.edu
.