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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,31 @@ on:
branches:
- master
jobs:
unit:
name: Unit Tests
runs-on: ubuntu-latest
timeout-minutes: 10
env:
TWILIO_ACCOUNT_SID: AC_dummy_value_so_tests_dont_error_out
TWILIO_AUTH_TOKEN: 123
elasticURL: "http://localhost:9200"
NODE_COVERALLS_DEBUG: 1
steps:
- uses: actions/checkout@v2
- name: install node
uses: actions/setup-node@v2
with:
node-version: "16"
- uses: bahmutov/npm-install@v1

- run: yarn unittest --coverage
- name: Coveralls
uses: coverallsapp/github-action@master
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
flag-name: Unit tests
parallel: true

tests:
name: Tests
runs-on: ubuntu-latest
Expand Down Expand Up @@ -37,6 +62,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.


- run: yarn db:refresh

- run: yarn test --coverage --detectOpenHandles
- name: Coveralls
uses: coverallsapp/github-action@master
Expand Down Expand Up @@ -149,7 +178,7 @@ jobs:
# - run: 'curl ''http://localhost:4000'' -X POST -H ''content-type: application/json'' --data ''{ "query": "query { search(termId: \"202250\", query: \"fundies\") { nodes { ... on ClassOccurrence { name subject classId } } } }" }'''

coverage:
needs: [end_to_end, tests]
needs: [end_to_end, tests, unit]
name: Sends Coveralls coverage
runs-on: ubuntu-latest
env:
Expand Down
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,11 @@
"about:tsc": "//// Runs TSC (because of our config, this will NOT compile the files, just typecheck them)",
"tsc": "tsc",
"about:test": "//// Runs a basic suite of unit and integration tests",
"test": "yarn jest --verbose --testPathIgnorePatterns='(seq|reg|git).[jt]s'",
"test": "yarn jest --verbose --testPathIgnorePatterns='(seq|reg|git|unit).[jt]s'",
"about:dbtest": "//// Runs tests interacting with our database. Must have the Docker containers running. This won't touch live data - it will create and teardown a temporary database.",
"dbtest": "jest -i --projects tests/database --verbose",
"about:unittest": "//// Runs unit tests. Does not need db, elasticsearch, spun up. Does not need the Docker containers to be running.",
"unittest": "jest -i --projects tests/unit --verbose",
"about:build_backend": "//// Compiles this project",
"build_backend": "rm -rf dist && mkdir -p dist && babel --extensions '.js,.ts' . -d dist/ --copy-files --ignore node_modules --ignore .git --include-dotfiles && rm -rf dist/.git",
"about:build": "//// Compiles this project, surpressing output",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
-- AlterTable
ALTER TABLE "followed_courses" ADD COLUMN "notif_count" INTEGER NOT NULL DEFAULT 0;

-- AlterTable
ALTER TABLE "followed_sections" ADD COLUMN "notif_count" INTEGER NOT NULL DEFAULT 0;
2 changes: 2 additions & 0 deletions prisma/schema.prisma
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ model Subject {
model FollowedCourse {
courseHash String @map("course_hash")
userId Int @map("user_id")
notifCount Int @default(0) @map("notif_count")
course Course @relation(fields: [courseHash], references: [id], onDelete: Cascade)
user User @relation(fields: [userId], references: [id], onDelete: Cascade)

Expand All @@ -101,6 +102,7 @@ model FollowedCourse {
model FollowedSection {
sectionHash String @map("section_hash")
userId Int @map("user_id")
notifCount Int @default(0) @map("notif_count")
section Section @relation(fields: [sectionHash], references: [id], onDelete: Cascade)
user User @relation(fields: [userId], references: [id], onDelete: Cascade)

Expand Down
48 changes: 43 additions & 5 deletions services/notifyer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
* See the license file in the root folder for details.
*/

import { User } from "@prisma/client";
import { User, FollowedSection } from "@prisma/client";

Check warning on line 6 in services/notifyer.ts

View workflow job for this annotation

GitHub Actions / Lint & Type checks

'FollowedSection' is defined but never used. Allowed unused vars must match /^_/u
import prisma from "./prisma";
import twilioNotifyer from "../twilio/notifs";
import macros from "../utils/macros";
import {
Expand Down Expand Up @@ -45,9 +46,20 @@
return;
} else {
const courseNotifPromises: Promise<void>[] = notificationInfo.updatedCourses
.map((course) => {
const courseMessage = generateCourseMessage(course);
.map(async (course) => {
const users = courseHashToUsers[course.courseHash] ?? [];

await prisma.followedCourse.updateMany({
where: {
courseHash: course.courseHash,
userId: { in: users.map((u) => u.id) },
},
data: {
notifCount: { increment: 1 },
},
});

const courseMessage = generateCourseMessage(course);
return users.map((user) => {
return twilioNotifyer.sendNotificationText(
user.phoneNumber,
Expand All @@ -59,9 +71,21 @@

const sectionNotifPromises: Promise<void>[] =
notificationInfo.updatedSections
.map((section) => {
const sectionMessage = generateSectionMessage(section);
.map(async (section) => {
const users = sectionHashToUsers[section.sectionHash] ?? [];

//increment notifCount of this section's entries in followedSection
await prisma.followedSection.updateMany({
where: {
sectionHash: section.sectionHash,
userId: { in: users.map((u) => u.id) },
},
data: {
notifCount: { increment: 1 },
},
});

const sectionMessage = generateSectionMessage(section);
return users.map((user) => {
return twilioNotifyer.sendNotificationText(
user.phoneNumber,
Expand All @@ -71,6 +95,20 @@
})
.reduce((acc, val) => acc.concat(val), []);

//delete any entries in followedCourse w/ notifCount >= 3
await prisma.followedCourse.deleteMany({
where: {
notifCount: { gt: 2 },
},
});

//delete any entries in followedSection w/ notifCount >= 3
await prisma.followedSection.deleteMany({
where: {
notifCount: { gt: 2 },
},
});

await Promise.all([...courseNotifPromises, ...sectionNotifPromises]).then(
() => {
macros.log("Notifications sent from notifyer!");
Expand Down
5 changes: 4 additions & 1 deletion services/updater.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ class Updater {
this.SECTION_MODEL
);

//Filter out courseHash & sectionHash if they have too high notifsSent

await sendNotifications(
notificationInfo,
courseHashToUsers,
Expand Down Expand Up @@ -518,7 +520,8 @@ class Updater {
const columnName = `${modelName}_hash`;
const pluralName = `${modelName}s`;
const dbResults = (await prisma.$queryRawUnsafe(
`SELECT ${columnName}, JSON_AGG(JSON_BUILD_OBJECT('id', id, 'phoneNumber', phone_number)) FROM followed_${pluralName} JOIN users on users.id = followed_${pluralName}.user_id GROUP BY ${columnName}`
//test edit: edited this select cmd to filter out any followed_modelName w/ notifsSent greater than 2
`SELECT ${columnName}, JSON_AGG(JSON_BUILD_OBJECT('id', id, 'phoneNumber', phone_number)) FROM followed_${pluralName} JOIN users on users.id = followed_${pluralName}.user_id WHERE notif_count < 3 GROUP BY ${columnName}`
)) as Record<string, any>[];

return Object.assign(
Expand Down
62 changes: 0 additions & 62 deletions tests/database/search.test.seq.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import searcher from "../../services/searcher";
import prisma from "../../services/prisma";
import { LeafQuery, ParsedQuery } from "../../types/searchTypes";

beforeAll(async () => {
searcher.subjects = {};
Expand All @@ -23,67 +22,6 @@ describe("searcher", () => {
});
});

//Unit tests for the parseQuery function
describe("parseQuery", () => {
it("query with no phrases", () => {
const retQueries: ParsedQuery = searcher.parseQuery(
"this is a query with no phrases"
);
expect(retQueries.phraseQ.length).toEqual(0); //no phrase queries
expect(retQueries.fieldQ).not.toEqual(null);
const fieldQuery: LeafQuery = retQueries.fieldQ;

expect(fieldQuery).toEqual({
multi_match: {
query: "this is a query with no phrases",
type: "most_fields",
fields: searcher.getFields(),
},
});
});

it("query with just a phrase", () => {
const retQueries: ParsedQuery = searcher.parseQuery('"this is a phrase"');

expect(retQueries.phraseQ.length).toEqual(1);
expect(retQueries.fieldQ).toEqual(null);

const phraseQuery: LeafQuery = retQueries.phraseQ[0];

expect(phraseQuery).toEqual({
multi_match: {
query: "this is a phrase",
type: "phrase",
fields: searcher.getFields(),
},
});
});

it("query with a phrase and other text", () => {
const retQueries: ParsedQuery = searcher.parseQuery('text "phrase" text');
expect(retQueries.phraseQ.length).toEqual(1);
expect(retQueries.fieldQ).not.toEqual(null);

const phraseQuery: LeafQuery = retQueries.phraseQ[0];
const fieldQuery: LeafQuery = retQueries.fieldQ;

expect(phraseQuery).toEqual({
multi_match: {
query: "phrase",
type: "phrase",
fields: searcher.getFields(),
},
});

expect(fieldQuery).toEqual({
multi_match: {
query: "text text",
type: "most_fields",
fields: searcher.getFields(),
},
});
});
});
// TODO: create an association between cols in elasticCourseSerializer and here
describe("generateQuery", () => {
it("generates match_all when no query", () => {
Expand Down
Loading
Loading