-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Fix part of 19858: Catch and Log Mailchimp Signup Errors #20143
Conversation
If registering the user for bulk emails via MailChimp fails, show an error message directing the user to sign up for bulk emails manually. Then, the user can proceed with registration by de-selecting the option to sign up for emails on the registration form.
Hi @U8NWXD, can you complete the following:
|
Hi @vojtechjelinek, @DubeySandeep, @kevintab95, PTAL at this PR, it modifies files in jobs or platform folders. |
Hi @U8NWXD please assign the required reviewer(s) for this PR. Thanks! |
Unassigning @StephenYu2018 since the review is done. |
Hi @U8NWXD, it looks like some changes were requested on this pull request by @StephenYu2018. PTAL. Thanks! |
Assigning @seanlip so they know to take a look at the question from @StephenYu2018 |
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.
LGTM, thanks @U8NWXD! Hopefully this gives us sufficient info to debug errors in the future. Do we need to hotfix this PR once it is merged -- looks like it might unblock some users trying to register?
P.S. Thanks @U8NWXD for assigning me, I did miss the @-mention! |
@kevintab95 yes, it should be hotfixed. I'm planning to submit the hotfix request form once this PR merges. Is that the right procedure, or should I file the form now to give the release team more lead time? |
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.
LGTM!
Unassigning @StephenYu2018 since they have already approved the PR. |
Hi @U8NWXD, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
@kevintab95 I have filed the hotfix request |
* Add URI to schema validation failure logs * Let users register if MailChimp signup fails If registering the user for bulk emails via MailChimp fails, show an error message directing the user to sign up for bulk emails manually. Then, the user can proceed with registration by de-selecting the option to sign up for emails on the registration form. * Update tests * Catch all exceptions from mailchip signup * Document Exception raised by MailChimp utils * Fix backend controller tests for new logging * Fix typo * Fix typo * Remove variable ID from feedback controller test * Backend test fixes * Controller backend test fixes * Fix backend controller tests
Overview
Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
flow.mov
To simulate mailchimp failing and get debugging information, I applied this diff:
The devserver logs confirm that the simulated failure was correctly interpreted by the backend code:
Note that there's a mistake in the debugging code. The value logged for
bulk_signup_failed
is actuallybulk_email_signup_message_should_be_shown
, which you can see from the diff above.I also tried with the browser console open:
flow_console_error.mov
The console errors also happen in develop:
before_flow.mov
Note that since MailChimp does not integrate with our backup server, this change cannot be effectively tested on the backup server.
Proof of changes on desktop with slow/throttled network
Proof of changes on mobile phone
Proof of changes in Arabic language
PR Pointers