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

Fix part of 19858: Catch and Log Mailchimp Signup Errors #20143

Merged
merged 12 commits into from
Apr 20, 2024

Conversation

U8NWXD
Copy link
Member

@U8NWXD U8NWXD commented Apr 8, 2024

Overview

  1. This PR fixes part of [BUG]: Unable to Register on Oppia.org #19858.
  2. This PR does the following: This PR changes the error-handling code to catch and log errors from MailChimp. If an error occurs, users are directed to sign up for the mailing list manually and then allowed to register. It also adds improved logging. Specifically, it logs the URL at which schema validation failures occur. This improved logging would have helped interpret the second server error in [BUG]: Unable to Register on Oppia.org #19858 (comment).
  3. (For bug-fixing PRs only) The original bug occurred because: Oppia's MailChimp configuration was changed, causing email signups to fail. These failures prevented users from registering if they wanted to subscribe to Oppia's mailing list. This PR changes the error-handling code to catch and log errors from MailChimp. If an error occurs, users are directed to sign up for the mailing list manually and then allowed to register.

Essential Checklist

Please follow the instructions for making a code change.

  • I have linked the issue that this PR fixes in the "Development" section of the sidebar.
  • I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
  • I have written tests for my code.
  • The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
  • I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

flow.mov

To simulate mailchimp failing and get debugging information, I applied this diff:

diff --git a/assets/constants.ts b/assets/constants.ts
index 8e3d5e8684..12824d08ff 100644
--- a/assets/constants.ts
    b/assets/constants.ts
@@ -6162,7  6162,7 @@ export default {
 
   "MIN_QUESTION_COUNT_FOR_A_DIAGNOSTIC_TEST_SKILL": 3,
 
-  "BULK_EMAIL_SERVICE_SIGNUP_URL": "",
   "BULK_EMAIL_SERVICE_SIGNUP_URL": "emailSignup.example.com",
 
   // The default number of opportunities to show on the contributor dashboard
   // page.
diff --git a/core/controllers/profile.py b/core/controllers/profile.py
index 7800cbc0bb..9ec8a673d6 100644
--- a/core/controllers/profile.py
    b/core/controllers/profile.py
@@ -530,6  530,7 @@ class SignupHandler(
     @acl_decorators.require_user_id_else_redirect_to_homepage
     def post(self) -> None:
         """Handles POST requests."""
         logging.error('[DEBUGGING] CAN_SEND_EMAILS=%s', feconf.CAN_SEND_EMAILS)
         assert self.user_id is not None
         assert self.normalized_payload is not None
         username = self.normalized_payload['username']
@@ -545,10  546,16 @@ class SignupHandler(
             feconf.DEFAULT_FEEDBACK_MESSAGE_EMAIL_PREFERENCE,
             feconf.DEFAULT_SUBSCRIPTION_EMAIL_PREFERENCE,
         )
         logging.error(
             '[DEBUGGING] bulk_signup_failed=%s',
             bulk_email_signup_message_should_be_shown)
         # Only block registration if bulk email configuration failed and the
         # user requested bulk emails.
         bulk_email_signup_message_should_be_shown = (
             can_receive_email_updates and config_bulk_email_failed)
         logging.error(
             '[DEBUGGING] bulk_email_signup_message_should_be_shown=%s',
             bulk_email_signup_message_should_be_shown)
         if bulk_email_signup_message_should_be_shown:
             self.render_json({
                 'bulk_email_signup_message_should_be_shown': (
diff --git a/core/domain/user_services.py b/core/domain/user_services.py
index e1c01b39d5..d50c2b2cbd 100644
--- a/core/domain/user_services.py
    b/core/domain/user_services.py
@@ -1644,6  1644,7 @@ def update_email_preferences(
     Returns:
         bool. Whether updating the user's bulk email preferences failed.
     """
     logging.error('[DEBUGGING] update_email_preferences() called')
     email_preferences_model = user_models.UserEmailPreferencesModel.get(
         user_id, strict=False)
     if email_preferences_model is None:
@@ -1664,6  1665,9 @@ def update_email_preferences(
             bulk_email_services.add_or_update_user_status(
                 email, {}, 'Account',
                 can_receive_email_updates=can_receive_email_updates))
         logging.error(
             '[DEBUGGING] user_creation_successful=%s',
             user_creation_successful)
         if not user_creation_successful:
             email_preferences_model.site_updates = False
             email_preferences_model.update_timestamps()
diff --git a/core/feconf.py b/core/feconf.py
index 87faad4ce1..65a214d198 100644
--- a/core/feconf.py
    b/core/feconf.py
@@ -587,7  587,7 @@ NOREPLY_EMAIL_ADDRESS = '[email protected]'
 # correspond to owners of the app before setting this to True. If
 # SYSTEM_EMAIL_ADDRESS is not that of an app owner, email messages from this
 # address cannot be sent. If True then emails can be sent to any user.
-CAN_SEND_EMAILS = False
 CAN_SEND_EMAILS = True
 # If you want to turn on this facility please check the email templates in the
 # send_role_notification_email() function in email_manager.py and modify them
 # accordingly.
diff --git a/core/platform/bulk_email/dev_mode_bulk_email_services.py b/core/platform/bulk_email/dev_mode_bulk_email_services.py
index 552e3f1e8b..3654a6605b 100644
--- a/core/platform/bulk_email/dev_mode_bulk_email_services.py
    b/core/platform/bulk_email/dev_mode_bulk_email_services.py
@@ -61,4  61,5 @@ def add_or_update_user_status(
         'Updated status of email ID %s\'s bulk email preference in the service '
         'provider\'s db to %s. Cannot access API, since this is a dev '
         'environment.' % (user_email, can_receive_email_updates))
-    return True
     logging.error('[DEBUGGING] Simulating mailchimp failure.')
     return False

The devserver logs confirm that the simulated failure was correctly interpreted by the backend code:

2024-03-31 17:15:14 ERROR:root:[DEBUGGING] CAN_SEND_EMAILS=True
2024-03-31 17:15:14 ERROR:root:[DEBUGGING] update_email_preferences() called
2024-03-31 17:15:14 ERROR:root:[DEBUGGING] Simulating mailchimp failure.
2024-03-31 17:15:14 ERROR:root:[DEBUGGING] user_creation_successful=False
2024-03-31 17:15:14 ERROR:root:[DEBUGGING] bulk_signup_failed=False
2024-03-31 17:15:14 ERROR:root:[DEBUGGING] bulk_email_signup_message_should_be_shown=True
2024-03-31 17:15:14 INFO     2024-03-31 21:15:14,914 module.py:883] default: "POST /signuphandler/data HTTP/1.1" 200 56
2024-03-31 17:15:15 INFO     2024-03-31 21:15:15,310 module.py:883] default: "GET / HTTP/1.1" 200 2310
2024-03-31 17:15:20 ERROR:root:[DEBUGGING] CAN_SEND_EMAILS=True
2024-03-31 17:15:20 ERROR:root:[DEBUGGING] update_email_preferences() called
2024-03-31 17:15:20 ERROR:root:[DEBUGGING] Simulating mailchimp failure.
2024-03-31 17:15:20 ERROR:root:[DEBUGGING] user_creation_successful=False
2024-03-31 17:15:20 ERROR:root:[DEBUGGING] bulk_signup_failed=False
2024-03-31 17:15:20 ERROR:root:[DEBUGGING] bulk_email_signup_message_should_be_shown=False
2024-03-31 17:15:20 ERROR:root:Please ensure that the value for the admin platform property SIGNUP_EMAIL_SUBJECT_CONTENT is set, before allowing post-signup emails to be sent.

Note that there's a mistake in the debugging code. The value logged for bulk_signup_failed is actually bulk_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

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.
Copy link

oppiabot bot commented Apr 8, 2024

Hi @U8NWXD, can you complete the following:

  1. The body of this PR is missing the required description, please update the body with a description of what this PR does.
    Thanks!

@oppiabot oppiabot bot assigned U8NWXD Apr 8, 2024
Copy link

oppiabot bot commented Apr 8, 2024

Hi @vojtechjelinek, @DubeySandeep, @kevintab95, PTAL at this PR, it modifies files in jobs or platform folders.
Also @U8NWXD, please make sure to fill in the server jobs form for the new job or feature to be tested on the backup server. This PR can be merged only after the test run is successful. Please refer to this guide for details.
Thanks!

@oppiabot oppiabot bot added the PR: Affects datastore layer Labels to indicate a PR that changes the datastore layer. label Apr 8, 2024
Copy link

oppiabot bot commented Apr 8, 2024

Hi @U8NWXD please assign the required reviewer(s) for this PR. Thanks!

@DubeySandeep DubeySandeep removed their assignment Apr 9, 2024
@vojtechjelinek vojtechjelinek removed their assignment Apr 13, 2024
@U8NWXD U8NWXD marked this pull request as ready for review April 14, 2024 15:37
@U8NWXD U8NWXD requested review from a team as code owners April 14, 2024 15:37
Copy link

oppiabot bot commented Apr 16, 2024

Unassigning @StephenYu2018 since the review is done.

Copy link

oppiabot bot commented Apr 16, 2024

Hi @U8NWXD, it looks like some changes were requested on this pull request by @StephenYu2018. PTAL. Thanks!

@U8NWXD U8NWXD assigned seanlip and StephenYu2018 and unassigned U8NWXD Apr 17, 2024
@U8NWXD
Copy link
Member Author

U8NWXD commented Apr 17, 2024

Assigning @seanlip so they know to take a look at the question from @StephenYu2018

Copy link
Member

@kevintab95 kevintab95 left a 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?

@kevintab95 kevintab95 removed their assignment Apr 17, 2024
@seanlip seanlip removed their assignment Apr 17, 2024
@seanlip
Copy link
Member

seanlip commented Apr 17, 2024

P.S. Thanks @U8NWXD for assigning me, I did miss the @-mention!

@U8NWXD
Copy link
Member Author

U8NWXD commented Apr 17, 2024

@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?

@seanlip seanlip assigned seanlip and kevintab95 and unassigned seanlip Apr 17, 2024
Copy link
Member

@StephenYu2018 StephenYu2018 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

oppiabot bot commented Apr 20, 2024

Unassigning @StephenYu2018 since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Apr 20, 2024
Copy link

oppiabot bot commented Apr 20, 2024

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!

@seanlip seanlip added this pull request to the merge queue Apr 20, 2024
Merged via the queue into oppia:develop with commit 9120fb3 Apr 20, 2024
77 checks passed
@U8NWXD U8NWXD deleted the fix-19858 branch April 20, 2024 13:37
@U8NWXD
Copy link
Member Author

U8NWXD commented Apr 20, 2024

@kevintab95 I have filed the hotfix request

lkbhitesh07 pushed a commit that referenced this pull request Apr 22, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Affects datastore layer Labels to indicate a PR that changes the datastore layer. PR: LGTM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants