Email Notifications #324

edwardneo · 2022-10-01T02:35:39Z

Implemented email notifications for the following views

When a student adds themselves to a section: notification --> mentor and confirmation --> student
When a student swaps sections: notification --> mentor and confirmation --> student
When a student drops a section: notification --> mentor and confirmation --> student
When a student is forcibly dropped by a coordinator: notification --> mentor and notification --> student

Added mock classes for testing email functions and patching for tests that run the email functions
Added custom errors and error handling
Added new "subscribed" field to all users
Added profile tab and page containing subscribed toggle button

smartspot2 · 2023-01-15T11:32:13Z

FYI a868dc6 is a merge commit and should be amended with a rebase (either squash or amend the commit); try doing this before many other changes are added as well, otherwise it can be particularly frustrating to handle later on. This would also make rebasing onto master to get the latest updates a lot easier in the future as well. Additionally, db800d1 and 53bca4c are also merge commits, and should also be amended before marking the PR as ready for review.

For future reference, use git pull -r to pull with a rebase (git automatically pulls with a merge commit if there are conflicts or divergent branches), as it seems like the merge commit was made when you pulled changes after making a local commit.

While you're doing the rebase, also look into 20311d7 as well, as this commit introduces a package-lock.json version change from v2 to v1, which is what causes the huge diff in this PR; this change should be reverted. To prevent this from happening again in the future when you do an npm install, ensure you're using the latest LTS version of node (18.13.0, though anything 18.x should do); I recommend using nvm to manage your node versions.

edwardneo · 2023-01-20T10:03:26Z

Things to review

Are all the emails necessary/written well?
Do we need to send an email to the mentor of the section a student swaps out of?
Is batching necessary?

smartspot2

The csm_web/scheduler/tests/test_emails.py is an empty file, and should probably be deleted.

Also, how does one test the API locally? There should probably be some kind of documentation written for this too—perhaps in the README.md in the repo, or perhaps in a wiki page in GitHub, etc.

Additionally, a lot of CI tests are failing, and it looks like it's because of missing dependencies; make sure you've added all of your new dependencies to the requirements files.

Otherwise, this looks pretty good overall; I haven't tested the actual functionality locally though, as I'm unsure how the credentials and access token are supposed to be set up.

.gitignore

csm_web/frontend/src/components/App.tsx

csm_web/frontend/src/components/ProfilePage.tsx

csm_web/scheduler/email/email_utils.py

csm_web/scheduler/email/quickstart.py

csm_web/scheduler/migrations/0033_rename_subscribe_user_subscribed.py

csm_web/scheduler/models.py

csm_web/scheduler/tests/models/test_student.py

csm_web/scheduler/tests/utils.py

edwardneo · 2023-02-11T09:57:18Z

In general, I thought I fixed the branching issues with rebasing, but I suppose there are a lot of random loose ends. I'll take a more in depth look and a lot of these requests should be fixed after that.

cypress · 2023-02-28T08:16:34Z

Passing run #96 ↗︎

0	78	0	0	0

Details:

Email Notifications
Project: csm_web	Commit: `2ab7cb3f3d`
Status: Passed	Duration: 02:31 💡
Started: Mar 14, 2023 3:22 AM	Ended: Mar 14, 2023 3:24 AM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

csm_web/scheduler/email/email_content/signature.html

csm_web/scheduler/views/section.py

edwardneo · 2023-04-07T16:24:35Z

Not sure what is going on with Cypress, but all the Django tests work.

smartspot2 · 2023-08-03T04:51:06Z

csm_web/scheduler/tests/models/test_word_of_the_day.py

+def test_submit_attendance_failure(client, setup_section, word, mocker):
    """
    Check that submitting an invalid word fails to update attendance.
    """
+    mocker.patch('scheduler.email.email_utils._email_send_message')


With these tests, there should also be an assertion that the _email_send_message function is actually called to send the email, if it's expected, and an assertion that it is not called if an email is not expected.

The tests could also be parameterized to include the different subscription settings from the user.

csm_web/scheduler/views/student.py

smartspot2 · 2023-08-03T04:56:22Z

csm_web/scheduler/views/user.py

+    @action(detail=True, methods=["PUT"])
+    def email(self, request, pk=None):
+        user = get_object_or_error(self.queryset, pk=pk)
+        user.subscribed = not user.subscribed
+        user.save()
+        return Response(status=status.HTTP_200_OK)


Generally, PUT requests should be idempotent; that is, repeated calls should have the exact same behavior as just one call. Here, flipping the user.subscribed variable each time the PUT request is sent goes against REST principles. Adding some field in the request body would be more conventional.

smartspot2 · 2023-08-03T04:57:13Z

requirements.txt

When this branch is rebased onto master, these requirements should be moved to the pyproject.toml file, and the requirements should be re-exported from poetry.

csm_web/scheduler/email/email_utils.py

smartspot2 · 2023-08-03T05:27:27Z

csm_web/frontend/src/components/Profile.tsx

+  const loadUser = () => {
+    fetchJSON("/userinfo").then(userInfo => {
+      let priorityEnrollment = null;
+      if (userInfo.priorityEnrollment) {
+        priorityEnrollment = new Date(Date.parse(userInfo.priorityEnrollment));
+      }
+
+      const convertedUserInfo: UserInfo = {
+        ...userInfo,
+        priorityEnrollment
+      };
+      setUserInfo(convertedUserInfo);
+    });
+  };


This should be put into a react-query hook instead, which will handle caching (and cache invalidation) for you.

smartspot2 · 2023-08-03T05:28:11Z

csm_web/frontend/src/components/Profile.tsx

+        className="csm-btn subscription-btn"
+        style={{ backgroundColor: `var(--csm-${userInfo?.subscribed ? "danger" : "green"})` }}
+        onClick={() => {
+          fetchWithMethod(`/users/${userInfo?.id}/email`, HTTP_METHODS.PUT).then(loadUser);


This should be put into a react-query mutation hook; in this hook you can invalidate the earlier GET request for the user's info.

csm_web/frontend/src/components/App.tsx

smartspot2 · 2023-08-07T11:21:15Z

csm_web/scheduler/models.py

@@ -36,6 +36,7 @@ def week_bounds(date):

 class User(AbstractUser):
    priority_enrollment = models.DateTimeField(null=True, blank=True)
+    subscribed = models.BooleanField(default=True, help_text="Email subscription status")


I feel like we've had a conversation about the default here, but what was the reasoning for making this default to True?

It's both a good and a bad thing that people will get notifications for everything right off the bat; they'll be informed of all updates, but they'll also be spammed quite a bit (ex. a user may enroll and drop multiple sections and get an email for every single one of these actions, and the mentors will correspondingly be emailed those notifications as well.)

Having this be opt-in could be better for the amount of email traffic we use, and would also prevent unwanted spam from users. We aren't doing any marketing or anything, so we aren't really losing too much by making this be opt-in rather than opt-out.

I presume the concern was that people wouldn't know that this feature exists, but we can still advertise this feature during orientation, in announcements in slack, etc., as this is mostly helpful for mentors, as opposed to students. Maybe when waitlisting and section swapping are implemented, this could be more useful for students, but even then we can have messages displayed to students asking if they want to enable notifications when they waitlist for a section, or when they send out a request for a swap.

Also, there is also an opportunity here to be more fine-grained with the subscription configuration; there could be options for each possible email that is sent out. For example, a mentor that is also a student may not want any notifications about the sections they're enrolling in, but they do want notifications about students enrolling in their section. Or, a student may not want to receive any notifications about enrolling/dropping from sections, but they do want to receive notifications about waitlists or section swaps (when those come out).

Open to thoughts and discussion on this though!

Making False!

…lAPI with unsubscribe feature, error handling, and testing

edwardneo assigned edwardneo and kevinbu233 Oct 1, 2022

edwardneo force-pushed the feat/notifications/main branch from 83e33fe to 4849c3a Compare January 20, 2023 08:32

edwardneo marked this pull request as ready for review January 20, 2023 08:35

edwardneo requested review from smartspot2, ericyche and noortor January 20, 2023 10:03

edwardneo force-pushed the feat/notifications/main branch from 34cd5b5 to 6cff64e Compare February 3, 2023 01:41

smartspot2 requested changes Feb 10, 2023

View reviewed changes

edwardneo force-pushed the feat/notifications/main branch from b4ec3a5 to 2ab7cb3 Compare March 14, 2023 03:19

smartspot2 requested changes Mar 14, 2023

View reviewed changes

csm_web/scheduler/email/email_content/signature.html Outdated Show resolved Hide resolved

csm_web/scheduler/views/section.py Outdated Show resolved Hide resolved

edwardneo requested a review from smartspot2 April 7, 2023 16:24

smartspot2 requested changes Aug 3, 2023

View reviewed changes

smartspot2 reviewed Aug 7, 2023

View reviewed changes

faithdennis force-pushed the feat/notifications/main branch from 9523d1b to b0a5c3d Compare April 1, 2025 01:42

Implemented email notifications for student section changes with Gmai…

6ff0b61

…lAPI with unsubscribe feature, error handling, and testing

faithdennis force-pushed the feat/notifications/main branch from b0a5c3d to 6ff0b61 Compare April 6, 2025 08:35

adds waitlist emails

822e757

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Email Notifications #324

Email Notifications #324

edwardneo commented Oct 1, 2022 •

edited

Loading

smartspot2 commented Jan 15, 2023 •

edited

Loading

edwardneo commented Jan 20, 2023

smartspot2 left a comment •

edited

Loading

edwardneo commented Feb 11, 2023

cypress bot commented Feb 28, 2023 •

edited

Loading

edwardneo commented Apr 7, 2023

smartspot2 Aug 3, 2023

smartspot2 Aug 3, 2023

smartspot2 Aug 3, 2023

smartspot2 Aug 3, 2023

smartspot2 Aug 3, 2023

smartspot2 Aug 7, 2023

edwardneo Aug 28, 2023

Email Notifications #324

Are you sure you want to change the base?

Email Notifications #324

Conversation

edwardneo commented Oct 1, 2022 • edited Loading

smartspot2 commented Jan 15, 2023 • edited Loading

edwardneo commented Jan 20, 2023

smartspot2 left a comment • edited Loading

Choose a reason for hiding this comment

edwardneo commented Feb 11, 2023

cypress bot commented Feb 28, 2023 • edited Loading

Passing run #96 ↗︎

edwardneo commented Apr 7, 2023

smartspot2 Aug 3, 2023

Choose a reason for hiding this comment

smartspot2 Aug 3, 2023

Choose a reason for hiding this comment

smartspot2 Aug 3, 2023

Choose a reason for hiding this comment

smartspot2 Aug 3, 2023

Choose a reason for hiding this comment

smartspot2 Aug 3, 2023

Choose a reason for hiding this comment

smartspot2 Aug 7, 2023

Choose a reason for hiding this comment

edwardneo Aug 28, 2023

Choose a reason for hiding this comment

edwardneo commented Oct 1, 2022 •

edited

Loading

smartspot2 commented Jan 15, 2023 •

edited

Loading

smartspot2 left a comment •

edited

Loading

cypress bot commented Feb 28, 2023 •

edited

Loading