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

@vojtechjelinek PTAL

kevintab95 · 2024-05-28T09:53:04Z

core/domain/feedback_services.py

+    )
+    if (
+        server_can_send_emails and
+        (


kevintab95 · 2024-05-28T09:54:29Z

core/tests/test_utils.py

+) -> Callable[[Callable[
+        ..., _GenericHandlerFunctionReturnType]],
+        Callable[..., _GenericHandlerFunctionReturnType]
+]:


kevintab95 · 2024-05-28T09:54:54Z

core/tests/test_utils.py

+            with swap_get_platform_parameter_value_function(
+                platform_parameter_name_value_tuples):
+                return func(*args, **kwargs)


…stants-for-single-rc

seanlip

Thanks @kevintab95! Couple small things, but looks pretty good for codeowners :)

Also @kevintab95 I think this needs a testing doc? It's trivial (set the param on) but we need a way to make sure this gets into the release instructions.

seanlip · 2024-05-28T18:12:28Z

core/controllers/classifier_test.py

@@ -186,6 +187,9 @@ def test_trained_classifier_handler(self) -> None:
            classifier_training_job.status,
            feconf.TRAINING_JOB_STATUS_COMPLETE)

+    @test_utils.use_platform_parameters(


I think "set" is better than "use", since we are always using all platform params (just that, sometimes, they have their default values).

seanlip · 2024-05-28T19:01:50Z

core/tests/test_utils.py

+            PlatformDataTypes. The value of the platform parameter if the
+            parameter is present in the platform_parameter_names list.
+        """
+        platform_parameter_name_value_dict = dict(


What if a parameter that's retrieved is not specified in the given list of tuples?

I've added a check for this now. Done.

BenHenning

I have no concerns for the changes (for Android), and am otherwise deferring to @seanlip for Android & core owners approvals since he's already looking at the PR.

@seanlip

Deferring to @seanlip.

kevintab95

PTAL @seanlip. Thanks!

kevintab95 · 2024-05-29T07:24:12Z

core/controllers/classifier_test.py

@@ -186,6 +187,9 @@ def test_trained_classifier_handler(self) -> None:
            classifier_training_job.status,
            feconf.TRAINING_JOB_STATUS_COMPLETE)

+    @test_utils.use_platform_parameters(


kevintab95 · 2024-05-29T07:50:00Z

core/tests/test_utils.py

+            PlatformDataTypes. The value of the platform parameter if the
+            parameter is present in the platform_parameter_names list.
+        """
+        platform_parameter_name_value_dict = dict(


I've added a check for this now. Done.

kevintab95 · 2024-05-29T08:21:49Z

Also @kevintab95 I think this needs a testing doc? It's trivial (set the param on) but we need a way to make sure this gets into the release instructions.

I've submitted a testing request for this.

Nik-09

Thank you @kevintab95, I have no concerns re frontend changes.

oppiabot · 2024-05-29T13:58:12Z

Unassigning @Nik-09 since they have already approved the PR.

seanlip

Thanks Kevin, a couple of follow-up comments!

seanlip · 2024-05-29T17:36:26Z

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


If the --> The

seanlip · 2024-05-29T17:37:33Z

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


Can we make this more explicit -- something like "The value for the platform parameter XYZ was needed in this test, but not specified in the XXX decorator. Please add a XXX decorator with this information."

Also just to confirm -- if a backend test uses a platform parameter (directly or indirectly), then they must specify its value, is that correct?

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

Thanks @kevintab95, sg. One question though -- have we modified all the tests that rely on calling the actual procedure / mock the method a different way to use the new decorator instead? I think that's important because, otherwise, devs will simply copy those old tests if it's the first example they see.

The way I went about these changes were to find out places where the email related platform parameter is used and then modify those tests to use the decorator instead. I do see around 24 places in the test files where platform_parameter_services.get_platform_parameter_value is mocked without the decorator e.g.

oppia/core/controllers/improvements_test.py

Lines 720 to 726 in 60b6d8e

def test_custom_high_bounce_rate_obsoletion_threshold(self) -> None:

swap_get_platform_parameter_value = self.swap_to_always_return(

platform_parameter_services,

'get_platform_parameter_value',

0.05

)

Can this be fixed in a separate PR as it seems out of scope for the current one?

Update: Per offline discussion, I will be handling these cases in the same PR.

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.

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

kevintab95 commented May 28, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into update-email-con…

6eb104b

…stants-for-single-rc

kevintab95 assigned vojtechjelinek and unassigned kevintab95 May 28, 2024

seanlip requested changes May 28, 2024

View reviewed changes

seanlip assigned kevintab95 and unassigned seanlip May 28, 2024

BenHenning reviewed May 28, 2024

View reviewed changes

BenHenning removed their assignment May 28, 2024

address comments

4633c79

kevintab95 commented May 29, 2024

View reviewed changes

kevintab95 assigned seanlip May 29, 2024

kevintab95 removed their assignment May 29, 2024

Nik-09 approved these changes May 29, 2024

View reviewed changes

oppiabot bot unassigned Nik-09 May 29, 2024

seanlip requested changes May 29, 2024

View reviewed changes

seanlip assigned kevintab95 and unassigned seanlip 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

		if (can_send_emails and (
		feconf.CAN_SEND_TRANSACTIONAL_EMAILS and

	def test_custom_high_bounce_rate_obsoletion_threshold(self) -> None:
	swap_get_platform_parameter_value = self.swap_to_always_return(
	platform_parameter_services,
	'get_platform_parameter_value',
	0.05
	)

Update email constants for single release candidate #20170

Are you sure you want to change the base?

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

Choose a reason for hiding this comment

seanlip 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

BenHenning left a comment

Choose a reason for hiding this comment

kevintab95 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevintab95 commented May 29, 2024

Nik-09 left a comment

Choose a reason for hiding this comment

oppiabot bot commented May 29, 2024

seanlip 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

seanlip May 31, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 •

edited

seanlip May 31, 2024 •

edited