Built out shift signup endpoints #90

hujoseph99 · 2022-01-16T20:52:54Z

Ticket link

Closes #54

Implementation description

Defined CreateShiftSignupRequestDTO, UpdateShiftSignupRequestDTO, ShiftSignupResponseDTO types
Implemented the following CRUD endpoints with their corresponding graphql queries and mutations:
- createShiftSignups
- updateShiftSignup
- getShiftSignupsForUser
Prisma schema changes (with corresponding migrations):
- Added note varchar to Signup prisma model
- Added foreign key constraint to userId in Signup model.
- Added a unique key constraint to for shiftId and userId pair in Signup model

Steps to test

Ensure that the docker containers are up and running
Run prisma migration and generate commands
Ensure that you have test data in the following tables: users, shifts, postings, branches.

If you don't have data, these commands might be helpful for getting some in easily:
Through the Postgres CLI:
- insert into branches (id, name) values (1, 'test branch');
- insert into postings (id, branch_id, type, title, description, auto_closing_date, start_date, end_date, status) values (1, 1, 'GROUP', 'a', 'a', '2022-01-09T21:33:51.302Z', '2022-01-09T21:33:51.302Z', '2022-01-09T21:33:51.302Z', 'DRAFT');
Through graphql interface (localhost:5000/graphql):
- mutation { createShifts( shifts: { postingId: "1" times: [ { startTime: "2022-01-25T13:30", endTime: "2022-01-25T15:00" } { startTime: "2022-01-26T12:00", endTime: "2022-01-26T16:00" } ] endDate: "2022-03-15" recurrenceInterval: BIWEEKLY } ) { id postingId startTime endTime } }
Credits to @lambo-liu in Create Shift CRUD Methods and GraphQL Resolvers #55

Go to localhost:5000/graphql and test the following new endpoints: createShiftSignups, updateShiftSignup, and getShiftSignupsForUser. The following are some query/mutations that I wrote in my own testing that you might be able to use:

query {
  getShiftSignupsForUser(userId: "1") {
    shiftId
    userId
    numVolunteers
    note
    status
  }
}

mutation {
  createShiftSignups(
    shifts: [{ shiftId: 2, userId: 1, numVolunteers: 3, note: "test" }]
  ) {
    shiftId
    userId
    numVolunteers
    note
  }
}

mutation {
  updateShiftSignup(
    shiftId: "2"
    userId: "1"
    update: { status: CONFIRMED, note: "test", numVolunteers: 3 }
  ) {
    shiftId
    userId
    numVolunteers
    note
    status
  }
}

What should reviewers focus on?

Ensure that it works for edge cases like duplicating shift signups, signing up for a shift that doesn't exist, a user that doesn't exist signing up for a shift, etc.
General testing for basic features

Checklist

My PR name is descriptive and in imperative tense
My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
I have run the appropriate linter(s)
I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

LenaNguyen

Just wait for this comment to be addressed. Everything looks great!

backend/services/implementations/shiftSignupService.ts

sherryhli

Just jumping in to point out a small issue with the order of migrations 🙂

backend/prisma/migrations/20220121012944_add_volunteer_users/migration.sql

backend/prisma/migrations/20220116032551_add_note_to_signup/migration.sql

...sma/migrations/20220116173256_added_unique_and_forign_key_contraints_to_signup/migration.sql

backend/prisma/schema.prisma

…signups

hujoseph99 · 2022-01-29T22:07:53Z

Sorry for the late response, I've been busy these past two days. I've made the changes now!

LenaNguyen · 2022-01-30T20:33:24Z

backend/graphql/index.ts

@@ -128,6 +132,7 @@ const authorizedByAllRoles = () =>
 const authorizedByAdmin = () => isAuthorizedByRole(new Set(["ADMIN"]));
 const authorizedByAdminAndVolunteer = () =>
  isAuthorizedByRole(new Set(["ADMIN", "VOLUNTEER"]));
+const authorizedByVolunteer = () => isAuthorizedByRole(new Set(["VOLUNTEER"]));


we also want to check isAuthorizedByUserId here to check that the userid in the token is the same as the userid they are signingup the shift as

backend/prisma/migrations/20220116032551_add_note_to_signup/migration.sql

backend/services/implementations/shiftSignupService.ts

sherryhli · 2022-01-30T21:38:38Z

backend/graphql/index.ts

@@ -128,6 +132,11 @@ const authorizedByAllRoles = () =>
 const authorizedByAdmin = () => isAuthorizedByRole(new Set(["ADMIN"]));
 const authorizedByAdminAndVolunteer = () =>
  isAuthorizedByRole(new Set(["ADMIN", "VOLUNTEER"]));
+const authorizedByVolunteer = () => {
+  return (
+    isAuthorizedByRole(new Set(["VOLUNTEER"])) && isAuthorizedByUserId("userid")


I don't think isAuthorizedByUserId can be used as-is because it just checks the top-level argument with name userId, but in this case, we want a field called userId under an top-evel argument called shifts (which is an array). Would likely need a new middleware function to do this check.

@LenaNguyen Thoughts about making this a separate task as it was not included in the original ticket description?

We will likely need another ticket for sorting out access control anyways, we've been restricting nearly everything to admins only right now

yeah we can do it in a separate ticket

Made a ticket: #115

sherryhli · 2022-01-30T22:13:31Z

backend/prisma/schema.prisma

@@ -124,15 +124,16 @@ model Shift {
 }

 model Signup {
-  id            Int          @id @default(autoincrement())
  shift         Shift        @relation(fields: [shiftId], references: [id])
  shiftId       Int          @map("shifts_id")
  user          User         @relation(fields: [userId], references: [id])


Another thought: we can just make the foreign key reference on the volunteers table rather than the base users table to enforce the "only volunteers can sign up" rule

That also works but I think our middleware for checking the role is good enough for now. We would still need to go through each shift to check that the volunteerID is the same as the volunteerID associated with the user in the token.

LenaNguyen

just one small thing and then i'll approve :))

LenaNguyen · 2022-02-05T03:46:03Z

backend/graphql/index.ts

@@ -128,6 +132,11 @@ const authorizedByAllRoles = () =>
 const authorizedByAdmin = () => isAuthorizedByRole(new Set(["ADMIN"]));
 const authorizedByAdminAndVolunteer = () =>
  isAuthorizedByRole(new Set(["ADMIN", "VOLUNTEER"]));
+const authorizedByVolunteer = () => {
+  return (
+    isAuthorizedByRole(new Set(["VOLUNTEER"])) && isAuthorizedByUserId("userid")


since the isAuthorizedByUserId doesn't work in this case, pls remove it

LenaNguyen

Thanks for handling the changes!

hujoseph99 added 10 commits January 15, 2022 20:59

added graphql shift signup types

7a0eca1

added shift signup interface

b1ef918

added note field to signup prisma model

ddbe06b

added shift signup types

2312ea5

built out create shift signups endpoint

718cf5f

added migration for foreign key and unique constraints for signups

3377254

built out update endpoint

42ff948

built out getshiftsignups endpoint

5b3e3d9

reverted middlewares and added some for new endpoints

ab2e68c

fixed linting errors

027949f

hujoseph99 requested review from sherryhli and LenaNguyen January 16, 2022 20:52

hujoseph99 added 3 commits January 16, 2022 16:00

fixed linting error

fc9f0cc

corrected mutation typo

486aeae

Merge branch 'main' into create-signup-service

a720ed4

LenaNguyen previously approved these changes Jan 26, 2022

View reviewed changes

backend/services/implementations/shiftSignupService.ts Show resolved Hide resolved

sherryhli requested changes Jan 26, 2022

View reviewed changes

hujoseph99 added 2 commits January 29, 2022 16:37

update migrations with main branch

cf4d19d

update createshiftsignup middleware and updates for change of pk for …

c5a9874

…signups

hujoseph99 dismissed LenaNguyen’s stale review via c5a9874 January 29, 2022 22:03

linting fixes

9f0e3bf

hujoseph99 requested review from sherryhli, JoshuaYe and LenaNguyen January 29, 2022 22:08

LenaNguyen requested changes Jan 30, 2022

View reviewed changes

hujoseph99 added 2 commits January 30, 2022 16:00

add userid check to authorizedByVolunteer middleware

d57e636

fix linting error

6095a1a

sherryhli requested changes Jan 30, 2022

View reviewed changes

sherryhli reviewed Jan 30, 2022

View reviewed changes

LenaNguyen mentioned this pull request Feb 3, 2022

Middleware to check user id for nested fields #115

Closed

hujoseph99 added 4 commits February 3, 2022 17:56

Merge branch 'main' into create-signup-service

d0da7d9

update error message to use utils

8d3412f

update migrations to be up to date with main

d17593e

added missing type to error param

50429f1

hujoseph99 requested review from sherryhli and LenaNguyen February 3, 2022 23:17

LenaNguyen requested changes Feb 5, 2022

View reviewed changes

remove unnecessary optional chaining + middleware

7fcbc99

hujoseph99 requested a review from LenaNguyen February 5, 2022 15:28

LenaNguyen approved these changes Feb 6, 2022

View reviewed changes

sherryhli approved these changes Feb 6, 2022

View reviewed changes

hujoseph99 merged commit 8ee3d22 into main Feb 6, 2022

hujoseph99 deleted the create-signup-service branch February 6, 2022 21:21

hujoseph99 mentioned this pull request Feb 20, 2022

Add filtering by status to signups #163

Merged

4 tasks

albertlai431 mentioned this pull request Mar 2, 2022

Add Support for Nested Fields in isAuthorizedByUserId for Retrieving UserId #185

Merged

4 tasks

hujoseph99 mentioned this pull request Apr 2, 2022

Schedule Review Table Data Fetch #259

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Built out shift signup endpoints #90

Built out shift signup endpoints #90

hujoseph99 commented Jan 16, 2022 •

edited

LenaNguyen left a comment

sherryhli left a comment

hujoseph99 commented Jan 29, 2022

LenaNguyen Jan 30, 2022

hujoseph99 Jan 30, 2022

sherryhli Jan 30, 2022

sherryhli Jan 30, 2022

LenaNguyen Jan 30, 2022

LenaNguyen Feb 3, 2022

sherryhli Jan 30, 2022

LenaNguyen Feb 3, 2022

LenaNguyen left a comment

LenaNguyen Feb 5, 2022

LenaNguyen left a comment

Built out shift signup endpoints #90

Built out shift signup endpoints #90

Conversation

hujoseph99 commented Jan 16, 2022 • edited

Ticket link

Implementation description

Steps to test

What should reviewers focus on?

Checklist

LenaNguyen left a comment

Choose a reason for hiding this comment

sherryhli left a comment

Choose a reason for hiding this comment

hujoseph99 commented Jan 29, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LenaNguyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LenaNguyen left a comment

Choose a reason for hiding this comment

hujoseph99 commented Jan 16, 2022 •

edited