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

Ap/unsubscribe after 3 notifs #218

Merged
merged 21 commits into from
Oct 5, 2024
Merged

Conversation

ananyaspatil
Copy link
Contributor

@ananyaspatil ananyaspatil commented Aug 15, 2024

Purpose

To reduce twilio costs, a user will be automatically unsubscribed from a course/section after they receive 3 text notifications for that course/section.

Tickets

https://trello.com/c/Aknz6Jdf

Contributors

@ananyaspatil @sebwittr @pranavphadke1 @elvincheng3

Feature List

  • Add notif_count attribute to prisma database schema to track of # of notifs sent out per course or section subscription
  • Send user a text notification if notif_count < 3 for a given course / section
  • Delete the course/section subscription after 3 notifs limit reached
  • Create 'unit' testing subdirectory, move unit tests from general to unit

Reviewers

Primary:
@sebwittr @pranavphadke1 @Anzhuo-W @Lucas-Dunker

Secondary reviewers:
@cherman23 @nickpfeiffer05 @mehallhm

@coveralls
Copy link
Collaborator

coveralls commented Sep 9, 2024

Coverage Status

coverage: 80.533% (+0.05%) from 80.481%
when pulling ecc8013 on AP/unsubscribe-after-3-notifs
into 33e48e4 on master.

@ananyaspatil ananyaspatil marked this pull request as ready for review September 21, 2024 11:30
Copy link
Contributor

@elvincheng3 elvincheng3 left a comment

Choose a reason for hiding this comment

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

made a suggestion, could be for a future ticket but i think it would be best addressed now so it doesn't get overlooked down the line.

@@ -37,6 +37,10 @@ jobs:
node-version: "16"
- uses: bahmutov/npm-install@v1

- run: yarn db:migrate
Copy link
Contributor

Choose a reason for hiding this comment

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

this yaml change wasn't intended to be a fix, just a proof of concept. i think it would make sense if we broke down the test directories into unit and integration subdirectories, and had separate testing CI for unit vs integration tests. it's best if we have clear separation between unit and integration, just in case we have tests that were intended to be unit tests but ended up relying on some dependency. would also be best if the unit tests did not have the db, elastic, or graph spun up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense, I kept the general subdirectory to hold integration tests & have made a separate unit subdirectory.

@ananyaspatil ananyaspatil merged commit 6cd914e into master Oct 5, 2024
10 checks passed
@ananyaspatil ananyaspatil deleted the AP/unsubscribe-after-3-notifs branch October 5, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants