#119: Add REST endpoint for DAC to reject applications #179

Azher2Ali · 2025-01-29T19:36:48Z

Summary

This PR introduces a new REST API endpoint to allow DAC to reject applications. Once an application is rejected, it cannot be reopened or modified. The application status transitions from DAC Review to Rejected, and a timestamp is recorded in the application history. The API returns a clear success or failure message. (#119)

Description of Changes

API Service

Added a new POST /applications/{applicationId}/reject endpoint for DAC to reject applications.
Ensured that DAC can only reject one application at a time.
Updated the application status transition logic:
1. DAC Review → Rejected
2. Applications that are Rejected cannot be reopened or modified.
Implemented history tracking by appending a timestamp when an application is rejected.
Standardized API response messages for success and failure scenarios.

Validation & Constraints

Ensured that only applications in DAC Review status can be rejected.
Applications already in Rejected status cannot be modified further.
Restricted the action to authorized DAC users only.
Database Changes

Added a timestamp field to track rejection events in the application history table.

Readiness Checklist

Self Review
- I have reviewed the code and ensured it meets the requirements.
- The feature has been manually tested locally.
PR Format
- The PR title is properly formatted to match the pattern: #{119}: feature: Add REST endpoint for DAC to reject applications
Labels Added
- api, feature labels have been applied.
Local Testing
- Successfully built all packages locally

surajgoraya

Overall it looks solid! Great work :)

Just a few small suggestions + discussion points!

(also, don't forget to paste in a screenshot of the tests passing in your PR while we wait on getting automated testing working 🫡)

surajgoraya · 2025-01-30T15:06:56Z

apps/api/src/routes/application-router.ts

+	if (typeof applicationId !== 'number' || !applicationId) {
+		res.status(400).send({
+			message: 'Invalid request. ApplicationId must be a valid number and is required.',
+			errors: 'MissingOrInvalidParameters',


This is likely out of the scope of this PR, so feel free to ignore this, but I wonder if we should add a code object into this return which is specifically for a error code?

We also likely should make these a centralized thing, but again, that's likely out of the scope of this PR

Yeah useful information to communicate as part of: https://github.com/orgs/Pan-Canadian-Genome-Library/projects/1/views/1?pane=issue&itemId=95389716&issue=Pan-Canadian-Genome-Library%7Cdaco%7C176

surajgoraya · 2025-01-30T15:08:53Z

apps/api/src/routes/application-router.ts

+		if (state === ApplicationStates.REJECTED) {
+			res.status(400).json({ message: 'Application is already rejected and cannot be modified.' });
+		}
+
+		if (state !== ApplicationStates.DAC_REVIEW) {
+			res.status(400).json({ message: 'Application must be in DAC Review status to be rejected.' });
+		}


These two if blocks basically do the same thing, I wonder if it's worth it to specifically do a check for a application is already rejected? We could just provide the !== DAC_REVIEW message since it covers all scenarios.

These two if blocks basically do the same thing, I wonder if it's worth it to specifically do a check for a application is already rejected? We could just provide the !== DAC_REVIEW message since it covers all scenarios.

Should we make the message generic in this case?

Yeah, maybe something like this?:

Application can only be rejected if under DAC Review

or

Application may only be rejected if in DAC Review

surajgoraya · 2025-01-30T15:11:20Z

apps/api/src/resources/swagger.yaml

+              schema:
+                type: object
+                properties:
+                  error:
+                    type: string


You likely should just reference the ClientErrors scheme here? See other API defs in the file for a example.

content: application/json: schema: $ref: '#/components/responses/ClientErrors'

surajgoraya · 2025-01-30T15:15:04Z

apps/api/src/resources/swagger.yaml

+              schema:
+                type: object
+                properties:
+                  error:
+                    type: string
+                  details:
+                    type: string


Sorry, similar thing here, it'll be a good idea to just reference the ServerError schema here too. Might be a good idea to just copy and paste from a previously defined API, this helps standardize things :)

JamesTLopez

Just some small changes! But overall looks solid

JamesTLopez · 2025-01-31T14:25:21Z

apps/api/src/routes/application-router.ts

+			let message = rejectedApplication.message || 'An unexpected error occurred.';
+			let errors = rejectedApplication.errors;


You can probably use const here.

JamesTLopez · 2025-01-31T14:26:11Z

apps/api/src/routes/application-router.ts

+		if (state === ApplicationStates.REJECTED) {
+			res.status(400).json({ message: 'Application is already rejected and cannot be modified.' });
+		}
+
+		if (state !== ApplicationStates.DAC_REVIEW) {
+			res.status(400).json({ message: 'Application must be in DAC Review status to be rejected.' });
+		}


These two if blocks basically do the same thing, I wonder if it's worth it to specifically do a check for a application is already rejected? We could just provide the !== DAC_REVIEW message since it covers all scenarios.

Should we make the message generic in this case?

JamesTLopez · 2025-01-31T14:29:52Z

apps/api/src/routes/application-router.ts

+		res.status(400).json({ message: 'Application ID is required.' });
+	}
+
+	if (typeof applicationId !== 'number' || !applicationId) {


You can probably simplify this condition using something like this:

if (isNaN(parseInt(applicationId))) { ... }

JamesTLopez · 2025-02-04T17:17:38Z

apps/api/src/routes/application-router.ts

+		const { applicationId } = req.body;
+		const { repId, reviewData, comment, role } = req.body;
+
+		if (!role && (role !== 'REP' || role !== 'DAC')) {


Wouldn't this be OR || ? Because if the role is undefined, then it should trigger the condition cause its not a valid role?

if (!role || (role !== 'REP' || role !== 'DAC')) { ... }

demariadaniel · 2025-02-04T22:35:37Z

apps/api/src/routes/application-router.ts

+	if (!applicationId) {
+		res.status(400).json({ message: 'Application ID is required.' });
+	}
+


Both of these if blocks send status 400 and concern applicationId so I think it would make sense to combine them:

if (!applicationId || isNaN(parseInt(applicationId))) { res.status(400).json({ message: 'Invalid request. ApplicationId is required and must be a valid number.', errors: 'MissingOrInvalidParameters', }); }

demariadaniel · 2025-02-04T22:36:58Z

apps/api/src/routes/application-router.ts

+	}
+});
+
+// Endpoint for reps to request revisions


Why is this /request-revisions endpoint being added? It's not in the ticket and the requestApplicationRevisions function does not exist in this branch

demariadaniel · 2025-02-04T22:38:19Z

apps/api/src/routes/application-router.ts

@@ -27,8 +27,11 @@ import {
 	getAllApplications,
 	getApplicationById,
 	getApplicationStateTotals,
+	rejectApplication,
+	requestApplicationRevisions,


This requestApplicationRevisions import doesn't exist in this branch, needs to be removed

demariadaniel

Need to clarify what's going on with the rep-revisions code

Add REST endpoint for DAC to reject applications

0a18d6c

Azher2Ali added New Feature API labels Jan 29, 2025

Azher2Ali changed the title ~~Add REST endpoint for DAC to reject applications (#119)~~ #119 Add REST endpoint for DAC to reject applications Jan 29, 2025

Azher2Ali changed the title ~~#119 Add REST endpoint for DAC to reject applications~~ #119: Add REST endpoint for DAC to reject applications Jan 29, 2025

Azher2Ali requested review from surajgoraya, demariadaniel and JamesTLopez January 29, 2025 19:39

Azher2Ali added daco-api and removed API New Feature labels Jan 29, 2025

surajgoraya reviewed Jan 30, 2025

View reviewed changes

JamesTLopez reviewed Jan 31, 2025

View reviewed changes

Azher2Ali added 2 commits February 3, 2025 14:44

Changes related to feedback

3964d96

Simplifying the condition in the codebase

6782569

JamesTLopez reviewed Feb 4, 2025

View reviewed changes

demariadaniel reviewed Feb 4, 2025

View reviewed changes

demariadaniel requested changes Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#119: Add REST endpoint for DAC to reject applications #179

#119: Add REST endpoint for DAC to reject applications #179

Azher2Ali commented Jan 29, 2025 •

edited

Loading

surajgoraya left a comment •

edited

Loading

surajgoraya Jan 30, 2025

demariadaniel Feb 4, 2025

surajgoraya Jan 30, 2025

JamesTLopez Jan 31, 2025

surajgoraya Jan 31, 2025 •

edited

Loading

surajgoraya Jan 30, 2025

surajgoraya Jan 30, 2025 •

edited

Loading

JamesTLopez left a comment

JamesTLopez Jan 31, 2025

JamesTLopez Jan 31, 2025

JamesTLopez Jan 31, 2025

JamesTLopez Feb 4, 2025

demariadaniel Feb 4, 2025

demariadaniel Feb 4, 2025

demariadaniel Feb 4, 2025

demariadaniel left a comment

		let message = rejectedApplication.message \|\| 'An unexpected error occurred.';
		let errors = rejectedApplication.errors;

#119: Add REST endpoint for DAC to reject applications #179

Are you sure you want to change the base?

#119: Add REST endpoint for DAC to reject applications #179

Conversation

Azher2Ali commented Jan 29, 2025 • edited Loading

Summary

Description of Changes

Readiness Checklist

surajgoraya left a comment • edited Loading

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

surajgoraya Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajgoraya Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

JamesTLopez left a 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

Choose a reason for hiding this comment

demariadaniel left a comment

Choose a reason for hiding this comment

Azher2Ali commented Jan 29, 2025 •

edited

Loading

surajgoraya left a comment •

edited

Loading

surajgoraya Jan 31, 2025 •

edited

Loading

surajgoraya Jan 30, 2025 •

edited

Loading