Update email constants for single release candidate #20170

kevintab95 · 2024-04-16T09:50:22Z

Overview

This PR fixes https://github.com/oppia/release-scripts/issues/133
This PR does the following:

CAN_SEND_EMAILS is now a platform parameter. This will be switched on for the prod server and off for the other servers.
CAN_SEND_EDITOR_ROLE_EMAILS, CAN_SEND_FEEDBACK_MESSAGE_EMAILS and CAN_SEND_SUBSCRIPTION_EMAILS are replaced with CAN_SEND_TRANSACTIONAL_EMAILS.
DEFAULT_EMAIL_UPDATES_PREFERENCE is set to True in feconf.py and is not configurable per-server.
REQUIRE_EMAIL_ON_MODERATOR_ACTION is set to True and the constant is removed.

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).

Testing doc (for PRs with Beam jobs that modify production server data)

A testing doc has been written: ... (ADD LINK) ...
(To be confirmed by the server admin) All jobs in this PR have been verified on a live server, and the PR is safe to merge.

Proof that changes are correct

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

Never force push! If you do, your PR will be closed.
To reply to reviewers, follow these instructions: https://github.com/oppia/oppia/wiki/Make-a-pull-request#step-5-address-review-comments-until-all-reviewers-approve
Some e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.
See the Code Owner's wiki page for what code owners will expect.

* CAN_SEND_EMAILS is now a platform parameter. This will be switched on for the prod server and off for the other servers. * CAN_SEND_EDITOR_ROLE_EMAILS, CAN_SEND_FEEDBACK_MESSAGE_EMAILS and CAN_SEND_SUBSCRIPTION_EMAILS are replaced with CAN_SEND_TRANSACTIONAL_EMAILS. * DEFAULT_EMAIL_UPDATES_PREFERENCE is set to True in feconf.py and is not configurable per-server. * REQUIRE_EMAIL_ON_MODERATOR_ACTION is set to True and the constant is removed.

oppiabot · 2024-04-16T09:50:25Z

Hi @kevintab95, can you complete the following:

The body of this PR is missing the proof that changes are correct section, please update it to include the section.
Thanks!

oppiabot · 2024-04-16T09:50:28Z

Hi @vojtechjelinek, @DubeySandeep, @kevintab95, PTAL at this PR, it adds a new cron job.
Also @kevintab95 It seems you have added or edited a CRON job, if so please request a testing of this CRON job with this server jobs form Please refer to this guide for details.
Thanks!

oppiabot · 2024-04-16T09:50:35Z

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

kevintab95 · 2024-04-16T09:54:38Z

@oppia/release-coordinators please note that for the release containing these changes, the RC must ensure that the CAN_SEND_EMAILS platform parameter is set in production and unset on other servers. Thanks.

vojtechjelinek

Thanks! Took a pass.

vojtechjelinek · 2024-04-16T13:16:21Z

core/controllers/email_dashboard_test.py

+                self.swap_to_always_return(
+                    platform_parameter_services,
+                    'get_platform_parameter_value',
+                    True
+                )
+            )


Suggested change

self.swap_to_always_return(

platform_parameter_services,

'get_platform_parameter_value',

True

)

)

self.swap_to_always_return(

platform_parameter_services,

'get_platform_parameter_value',

True

)

)

ditto below

vojtechjelinek · 2024-04-16T13:25:20Z

core/domain/feedback_services.py

+    if (can_send_emails and (
+            feconf.CAN_SEND_TRANSACTIONAL_EMAILS and


Can we also somehow cleanup this if at this point it is almost unreadable. At least something like this:

# TODO(#12079): Figure out a better way to avoid sending feedback # thread emails for contributor dashboard suggestions. message_changed = len(text) > 0 or old_statuses[index] != new_statuses[index] if ( can_send_emails and feconf.CAN_SEND_TRANSACTIONAL_EMAILS and author_id is not None and user_services.is_user_registered(author_id)) and message_changed and should_send_email ):

oppiabot · 2024-04-16T13:28:42Z

Unassigning @vojtechjelinek since the review is done.

kevintab95

PTAL @seanlip, thanks!

kevintab95 · 2024-05-31T03:00:48Z

core/tests/test_utils.py

@@ -453,10 +453,18 @@ def mock_get_platform_parameter_value(
        Returns:
            PlatformDataTypes. The value of the platform parameter if the
            parameter is present in the platform_parameter_names list.
+
+        Raises:
+            Exception. If the parameter_name is not present in the


kevintab95 · 2024-05-31T03:17:07Z

core/tests/test_utils.py

        """
        platform_parameter_name_value_dict = dict(
            (x.value, y) for x, y in platform_parameter_name_value_tuples
        )
+        if parameter_name not in platform_parameter_name_value_dict:
+            raise Exception(
+                'Unknown platform parameter name: %s' % parameter_name


Done.

Regarding the second question, this is true if the test uses the decorator i.e. if a test uses the decorator, it will need to exhaustively define all the platform parameter that it requires or it will throw the error above. There may still be tests that rely on calling the actual procedure (not mocking) and/or there may be other tests that mocks the method a different way (e.g. always return xxx).

vojtechjelinek

LGTM!

oppiabot · 2024-05-31T07:18:11Z

Unassigning @vojtechjelinek since they have already approved the PR.

…intab95/oppia into update-email-constants-for-single-rc

kevintab95

@seanlip PTAL, thanks!

kevintab95 · 2024-06-04T09:14:55Z

core/tests/test_utils.py

        """
        platform_parameter_name_value_dict = dict(
            (x.value, y) for x, y in platform_parameter_name_value_tuples
        )
+        if parameter_name not in platform_parameter_name_value_dict:
+            raise Exception(
+                'Unknown platform parameter name: %s' % parameter_name


seanlip

Thanks for the cleanup @kevintab95! Pity about the pylint disables, but I don't have a quick way to fix it, so let's leave them for now and figure it out later.

LGTM!

oppiabot · 2024-06-04T15:08:30Z

Hi @kevintab95, 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 added 4 commits April 16, 2024 02:07

fix lint errors

38d03d5

fix lint errors

6309452

fix lint errors

e939e3b

kevintab95 requested review from a team as code owners April 16, 2024 09:50

kevintab95 requested review from vojtechjelinek, BenHenning, lkbhitesh07 and chris7716 April 16, 2024 09:50

oppiabot bot assigned kevintab95 Apr 16, 2024

oppiabot bot assigned DubeySandeep and vojtechjelinek Apr 16, 2024

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

kevintab95 changed the title ~~Update email constants for single rc~~ Update email constants for single release candidate Apr 16, 2024

kevintab95 assigned BenHenning, chris7716 and lkbhitesh07 and unassigned kevintab95 Apr 16, 2024

vojtechjelinek requested changes Apr 16, 2024

View reviewed changes

oppiabot bot unassigned vojtechjelinek Apr 16, 2024

seanlip removed their assignment May 29, 2024

address comments

c794307

kevintab95 commented May 31, 2024

View reviewed changes

kevintab95 assigned seanlip and kevintab95 and unassigned kevintab95 and seanlip May 31, 2024

vojtechjelinek approved these changes May 31, 2024

View reviewed changes

oppiabot bot unassigned vojtechjelinek May 31, 2024

kevintab95 added 3 commits June 4, 2024 01:57

address comments

556eade

Merge branch 'update-email-constants-for-single-rc' of github.com:kev…

dadd0d6

…intab95/oppia into update-email-constants-for-single-rc

fix lint errors

c4ad4b9

kevintab95 commented Jun 4, 2024

View reviewed changes

kevintab95 assigned seanlip and unassigned kevintab95 Jun 4, 2024

seanlip approved these changes Jun 4, 2024

View reviewed changes

seanlip enabled auto-merge June 4, 2024 15:05

seanlip assigned kevintab95 and unassigned seanlip Jun 4, 2024

oppiabot bot added the PR: LGTM label Jun 4, 2024

seanlip added this pull request to the merge queue Jun 4, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 4, 2024

seanlip added this pull request to the merge queue Jun 4, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 4, 2024

kevintab95 added this pull request to the merge queue Jun 4, 2024

Merged via the queue into oppia:develop with commit ea72509 Jun 4, 2024
81 checks passed

kevintab95 deleted the update-email-constants-for-single-rc branch June 4, 2024 19:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update email constants for single release candidate #20170

Update email constants for single release candidate #20170

kevintab95 commented Apr 16, 2024

oppiabot bot commented Apr 16, 2024

oppiabot bot commented Apr 16, 2024

oppiabot bot commented Apr 16, 2024

kevintab95 commented Apr 16, 2024

vojtechjelinek left a comment

vojtechjelinek Apr 16, 2024

vojtechjelinek Apr 16, 2024

kevintab95 Apr 30, 2024

vojtechjelinek Apr 16, 2024

kevintab95 Apr 30, 2024

oppiabot bot commented Apr 16, 2024

kevintab95 left a comment

kevintab95 May 31, 2024

kevintab95 May 31, 2024

vojtechjelinek left a comment

oppiabot bot commented May 31, 2024

kevintab95 left a comment

kevintab95 Jun 4, 2024

seanlip left a comment

oppiabot bot commented Jun 4, 2024

		if (can_send_emails and (
		feconf.CAN_SEND_TRANSACTIONAL_EMAILS and

Update email constants for single release candidate #20170

Update email constants for single release candidate #20170

Conversation

kevintab95 commented Apr 16, 2024

Overview

Essential Checklist

Testing doc (for PRs with Beam jobs that modify production server data)

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

oppiabot bot commented Apr 16, 2024

oppiabot bot commented Apr 16, 2024

oppiabot bot commented Apr 16, 2024

kevintab95 commented Apr 16, 2024

vojtechjelinek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Apr 16, 2024

kevintab95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechjelinek left a comment

Choose a reason for hiding this comment

oppiabot bot commented May 31, 2024

kevintab95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

oppiabot bot commented Jun 4, 2024