-
Notifications
You must be signed in to change notification settings - Fork 59
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
Namerequest UI: email validation #13966
Comments
One option: create a bcrs-shared-component service that will call the MillionVerifier API. Then import and use this service in Namerequest (and any other UIs that need it - separate tickets). Ref: Single API doc See PR below for prototype code that can be used for further discussion. |
@pwei1018 Since there are a lot of emails in multiple UIs, can we target certain emails initially -- which ones are causing the most Sentry errors? |
@pwei1018 @yuisotozaki @forgeuxGH5 Since using the MillionVerifier API costs money (even though it's a small amount and we have a lot of credits at the moment), I'm thinking we should call the API as little as possible -- as little as once per email entered. I'm thinking we could call MillionVerifier from our UI when the user clicks "File and Save", and show them any errors before continuing. Alternately, we could call MillionVerified from the back end when it receives the filing to process, and return any errors to our UI before continuing. Either way, this will impact the user experience and we need to include the UI/UX team for any special designs around this. My concern is that we'd be doing email syntax validation when the user enters an email address (ie, what we do today), and we'd be doing email verification at a later time in the flow. This may confuse users if we don't do it right. I'm looking for feedback here. Another thing to consider is that the business contact email component prompts for the email a second time to confirm it... Do we still want to do this? (If other users are like me then they just cut and paste the first email, thus duplicating any email errors.) Again, what do you think? |
@forgeuxGH5 I believe this is a good topic for PO/UX cross team discussion. |
Next to step here is to investigate the costs associated with this. - This will have to be reviewed by the SRE team @pwei1018 The OCM team already has an account set up for this. |
@severinbeauvais I created an endpoint in Notify api. It will use both email_validator library and MillionVerifier API to verify email address. I will deploy it soon when I finish the CI stuff. |
For people following this discussion. Re: ^^ The point of having the call to MillionVerifier proxied through one of our APIs is so that our UIs don't expose the MillionVerified key in its network traffic (esp since this is a paid service and we don't want other people using it). This way, our UIs can call our APIs (which requires auth) and no key is exposed. It would be an extra hop each way to verify an email from the UI but from my testing, MillionVerifier is pretty fast to respond, so overall this should be fast as well. |
Sample UI code (that calls Million Verified directly, which we don't want to do, but we could call Patrick's new endpoint above instead). Anyway, this sample code shows what's possible in the UI. Branch: https://github.com/severinbeauvais/business-edit-ui/tree/13966 |
@yuisotozaki @forgeuxGH5 Any decision of this? |
We'll review this approach with the wider team via PO/Design meeting Wed Dec 7. |
@pwei1018 - This just came up on the UX PO sync. Would you please create a RAID ticket to track these decisions around billing. We need to know dollar amounts. |
@yuisotozaki Have you guys talk about this change? This change is really impact on our email job. Lot of errors generated due to email address issue. |
@jdyck-fw What do you mean dollar amounts? OCM team manage the MillionVerified subscription. |
@davemck513 to connect with @severinbeauvais on this - See what options we have, including open source |
@jdyck-fw @davemck513 @severinbeauvais Please make the decision ASAP. You can see the sentry log history. Last 30 days, it had 771k error message generated which consumed half of usage in Sentry. Either we disable the error message to Sentry of entity_emailer service or implement the solution. |
On Oct 25, Severin wrote:
Based on the above, I recommend that we perform email validation in the back end (APIs), so that's only done once per flow. The APIs can return an error to the UIs with a suitable error message (extend existing functionality) that the UI can display to the user (ie, on File and Pay). |
@davemck513 @Mihai-QuickSilverDev @jdyck-fw We should make action on this issue. The emailer throw 840k error messages in last 30 days. Also, more than 13,000 emails were not sent. |
I updated the RAID tix with this info. Just x-posting here for visibility. At PO Sync last Friday, we discussed stopped sending after 10 attempts as a solution for now, realizing there is additional work needed to flag the record. This came from GC Notify today: There were 244 total sends that failed from the application -dev/test api call for user [email protected]. For Development purposes, we recommend against using a live key. You could reach out to that person to stop sending to that email address. If that fails, you could ask someone from your team who has "Manage API integration" permissions to revoke the API key. This is done from from the API Integration menu, then select API keys. You can revoke the problematic key to stop the sends. |
The problem is that users are entering invalid emails like
[email protected]
or[email protected]
, which is causing email sending to fail and to generate a lot of Sentry errors.Sentry error:
https://sentry.io/organizations/registries/issues/2572347016/?query=is%3Aunresolved+NR+9936741&statsPeriod=14d
We need validation rule for email address when client input the applicant information.
In the backend, we can use "email-validator" library.
Example code:
https://github.com/bcgov/bcregistry-sre/blob/main/notify-api/src/notify_api/models/notification.py#L51
Also to look at in this ticket:
^^ some of these came from #14955
The text was updated successfully, but these errors were encountered: