Skip to content

added shift request api #328

Open
yajuusan wants to merge 9 commits into
mainfrom
yajuusan/feature/shift-request-api-v2
Open

added shift request api #328
yajuusan wants to merge 9 commits into
mainfrom
yajuusan/feature/shift-request-api-v2

Conversation

@yajuusan
Copy link
Copy Markdown
Collaborator

image image

@yajuusan
Copy link
Copy Markdown
Collaborator Author

image

@uppalkrish
Copy link
Copy Markdown
Collaborator

please provide me a demo of this
Don't worry about merge conflicts I can fix them

@uppalkrish uppalkrish requested a review from LoopyB May 18, 2026 13:02
Copy link
Copy Markdown
Collaborator

@LoopyB LoopyB left a comment

Choose a reason for hiding this comment

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

Thanks for adding the ShiftRequest API work. The feature direction is useful and aligns with the need for shift swap / leave request workflows.

I think this needs changes before merge.

1. Lint currently fails on this branch

I checked out the PR branch locally and ran:

npm run lint

The branch currently reports 18 ESLint errors. Even if some of these may be pre-existing or outside the new ShiftRequest files, the PR doesn't currently pass the backend lint check locally, so this should be resolved or clearly separated before merge.

2. Endpoint contract does not match the ticket

The ticket specifies:

  • POST /api/v1/shifts/request
  • GET /api/v1/shifts/requests
  • PATCH /api/v1/shifts/request/{id}

However, the implementation appears to mount the new routes under:

  • /api/v1/shiftrequests/requests
  • /api/v1/shiftrequests/requests/{id}

The controller comments also refer to /api/v1/shifts/requests, while the route file documents /api/v1/shiftrequests/requests, so the endpoint contract is inconsistent across the ticket, controller comments, Swagger docs, and actual route mounting.

Could this please be aligned before merge, or confirmed if the ticket/API contract has intentionally changed?

3. No dedicated ShiftRequest tests found

I could not find a dedicated ShiftRequest test file for this new workflow. Since this introduces a new approval process, I think we need targeted regression coverage for at least:

  • guard creates a SWAP request
  • guard creates a LEAVE request
  • non-guard cannot create requests
  • duplicate pending request is blocked
  • target guard accepts/declines a SWAP
  • employer/admin approves or rejects a request
  • employer cannot view or approve requests for shifts they do not own
  • approved SWAP/LEAVE updates the Shift assignment state consistently

4. Employer authorization/scoping needs tightening

Please confirm that employers can only view/approve/reject requests connected to shifts they created or own. In a multi-employer system, role-level checks are not enough; the request needs data-level ownership scoping as well.

5. Shift assignment updates need confirmation

For approved SWAP/LEAVE requests, please confirm the implementation updates all Shift assignment fields used elsewhere in the backend/frontend, not only one field such as acceptedBy. If other parts of the system rely on guardIds, acceptedBy, status, or applicant fields, partial updates could leave shifts in an inconsistent state.

6. Notification/socket changes look out of scope or incomplete

This PR also changes notification/socket-related files. If these are required for the ShiftRequest workflow, they should be fully integrated and tested. If they are future groundwork, I think they should be deferred to a separate PR to keep this feature focused.

Related to that, existing Swagger documentation appears to have been removed from notification.routes.js. Since this ticket is for the ShiftRequest API, could that documentation be restored unless there is a specific reason to remove it?

Summary: The feature direction is good, but I think this will only be merge-ready when lint passes, the endpoint contract is aligned, the notification/swagger changes are clarified, and the new approval workflow has basic tests.

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.

3 participants