✏️ Feat/ #22 Application Update methods #80

demariadaniel · 2024-11-28T21:04:13Z

Feat: #22: Application Update Methods

Summary

Adds Service Method to update Application details, with related API route

Related Issues

22

Description of Changes

Adds Application Service editApplication method for updating application contents
Method sets application state to DRAFT
Adds API editApplication function which validates application state allows edits
Adds first Express route for Applications
Updates to test files, and drizzle config / dbml config related to PR discussion on:
https://github.com/Pan-Canadian-Genome-Library/daco/pull/40/files

Special Instructions

Changes to Drizzle config require running pnpm run build to allow drizzle-kit operations
drizzle-kit now uses /dist for schema lookup

Readiness Checklist

Self Review
- I have performed a self review of code
- I have run the application locally and manually tested the feature
PR Format
- The PR title is properly formatted to match the pattern: #{TicketNumber}: Description of Changes
- Links are included to all relevant tickets
Labels Added
- Label is added for each package/app that is modified (api, ui, data-model, etc.)
- Label is added for the type of work done in this PR (feature, fix, chore, documentation)
Local Testing
- Successfully built all packages locally
- Successfully ran all test suites, all unit and integration tests pass
Updated Tests
- Unit and integration tests have been added that describe the bug that was fixed or the features that were added
Documentation
- All new environment variables added to .env.schema file and documented in the README
- All changes to server HTTP endpoints have open-api documentation
- All new functions exported from their module have TSDoc comment documentation

demariadaniel · 2024-11-28T21:05:43Z

apps/api/package.json

@@ -6,7 +6,7 @@
 	"scripts": {
 		"build": "rimraf ./dist && tsc",
 		"dev": "tsx watch --env-file=.env ./src/main.ts",
-		"test": "npx tsx --test --test-reporter=spec --experimental-test-coverage ./src/tests/**/*",
+		"test": "tsx --test --test-reporter=spec --experimental-test-coverage ./tests/**/*.test.ts",


Feedback from #40

demariadaniel · 2024-11-28T21:06:16Z

apps/api/tests/api/index.test.ts

@@ -20,7 +20,7 @@
 import assert from 'node:assert';
 import { after, describe, it } from 'node:test';

-import { port } from '../server.js';
+import { port } from '../../src/server.js';


test folder moved out of src

JamesTLopez

Minor stuff and a suggestion, otherwise lgtm 🔥

JamesTLopez · 2024-12-02T18:42:22Z

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

+	const applicationRecord = result.data;
+
 	// Validate Application state will allow updates
 	// Edits to Applications under review will revert state to 'DRAFT'
 	const { state } = applicationRecord;


Variable applicationRecord doesn't need to be defined here we can deconstruct directly out of result.data

JamesTLopez · 2024-12-02T18:51:00Z

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

+	if (result.success) {
+		res.send(result.data);
 	} else {
-		res.status(500).send({ error: `Error updating application with id ${id}` });
+		res.status(500).send({ message: result.message, errors: result.errors });
 	}


More of a personal suggestion, I'm a fan of this syntax I find it easier to read and easier to add more error checks in the future if needed:

if (!result.success) { res.status(500).send({ message: result.message, errors: result.errors }); return; } res.send(result.data);

But the current solution works if you wish to proceed.

~~Yes fair, the else could be removed. Generally we prefer putting the 'truthy' condition first but your recommendation is still applicable.~~

Also in the code snippet above I think you copied the Git diff, double res.send's would trigger an error.
Just to be sure I understand you right, you're saying else is not needed here.

edit: My editor seems to prefer keeping the else as a type guard to enforce result is failure

Ah yes my bad, there would be a return statement after the correct res. I have updated it to what my initial thoughts were.

demariadaniel · 2024-12-02T19:45:02Z

Endpoint tested with Postman:

…/daco into feat/22-application-update-methods

leoraba · 2024-12-04T14:06:35Z

apps/api/README.md

+
+- Drizzle is configured to read the schema from the build directory (`/dist`) due to a known Drizzle Kit issue with ESM imports. Any Drizzle Kit operations (including migrations) require first building the application with `pnpm run build`.
+
+- A schema dbml (database markup language) file can be generated using the script `pnpm run dbml`. This file is found at `./src/db/schema.dbml`.


leoraba · 2024-12-04T14:26:43Z

apps/api/tsconfig.json

@@ -6,5 +6,5 @@
 		"module": "NodeNext",
 		"moduleResolution": "NodeNext"
 	},
-	"include": ["src", "./drizzle.config.ts"]
+	"include": ["src", "./drizzle.config.ts", "tests"]


is there a reason why we are including test folder in here?

This was an auto update from moving folders around, doesn't need to be included

leoraba · 2024-12-04T14:54:38Z

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

+const applicationRouter = express.Router();
+const jsonParser = bodyParser.json();
+
+applicationRouter.post('/application/edit/', jsonParser, async (req, res) => {


seems this route hasn't been included in the Swagger documentation

Good call -- we just added swagger yesterday. Adding now.

Added /edit Swagger endpoint, tested and working

leoraba · 2024-12-04T15:20:20Z

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

+	if (result.success) {
+		res.send(result.data);
+	} else {
+		res.status(500).send({ message: result.message, errors: String(result.errors) });


I see this will always return a 500 error code when condition is falsy. However, considering there is a TODO to implement auth and Zod validations, just curious is this also includes the case where id doesn't exist in the database? would that return 4xx or 5xx?

Yes -- if id is not found we're returning a 500
I can update to a 4xx

Added basic updates for 404 handling. We don't have a central error handler setup yet so I've added a TODO as well.

leoraba

Great updates in this PR. I’ve left a few comments but so far looks good to me.

leoraba · 2024-12-04T19:39:21Z

apps/api/src/resources/swagger.yaml

+        '400':
+          description: 'Error editing application - Application not found'
+          content:
+            application/json:
+              schema:
+                type: object
+        '200':
+          description: 'Application updated'
+          content:
+            application/json:
+              schema:
+                type: object


responses schemas can also be defined here using the structures defined by the success and failure helpers

Will add in #58

leoraba

LGTM

demariadaniel added 6 commits November 28, 2024 15:23

Service Edit method

e3f3b78

Move Test Files

637f637

Use .test naming convention

501d6f0

Move API test file

ed2be28

Tests for Edit Method

420a74f

Remove npx

c2ea32e

demariadaniel commented Nov 28, 2024

View reviewed changes

demariadaniel added 12 commits November 29, 2024 10:46

DBml / schema updates

7b84efb

Use dist folder for config, add first readme update

1fb30cd

Setup Router & API + Move logic out of service

a44b23c

Working API Test w/ some reorg

42afe20

Edit for clarity

eb27584

Remove Demo endpoint

04f0dc6

Route + service improvements

ffbe16f

Add todo

ba949f7

Move service type, add error condition

fc3e38a

First updates with success/failure

5002a86

Move tests

728572e

Use Success/Failure pattern with getById

5d4915f

demariadaniel linked an issue Dec 2, 2024 that may be closed by this pull request

Add to Application Layer - User Updating Application #22

Closed

Add TSDoc

060f027

demariadaniel added the daco-api label Dec 2, 2024

demariadaniel self-assigned this Dec 2, 2024

demariadaniel marked this pull request as ready for review December 2, 2024 17:25

demariadaniel requested review from justincorrigible, joneubank, leoraba and JamesTLopez December 2, 2024 17:26

demariadaniel changed the title ~~WIP: Feat/22 application update methods~~ ✏️ Feat/ #22 Application Update methods Dec 2, 2024

demariadaniel added 2 commits December 2, 2024 13:53

Add getDbInstance type

2783fee

Throw err is no db instance

596affa

JamesTLopez reviewed Dec 2, 2024

View reviewed changes

Add body parser

d3a8b30

demariadaniel added 4 commits December 2, 2024 15:40

Simplify variable declaration

b77bb36

Add Comments + Transaction

c0079dc

Merge branch 'main' of https://github.com/Pan-Canadian-Genome-Library…

cfc1cb6

…/daco into feat/22-application-update-methods

Print error message in response

f08b6a9

demariadaniel requested a review from JamesTLopez December 3, 2024 18:51

leoraba reviewed Dec 4, 2024

View reviewed changes

demariadaniel added 3 commits December 4, 2024 10:32

Remove tests from tsconfig

fa1202e

Add Edit endpoint to Swagger w/ 404

fc7cce3

Add Error Todo

7ceb7ea

demariadaniel requested a review from leoraba December 4, 2024 17:25

leoraba reviewed Dec 4, 2024

View reviewed changes

leoraba approved these changes Dec 4, 2024

View reviewed changes

demariadaniel merged commit 391846c into main Dec 4, 2024

demariadaniel deleted the feat/22-application-update-methods branch December 4, 2024 20:49

Buwujiu mentioned this pull request Jan 9, 2025

Endpoint to Update and Save Application #112

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✏️ Feat/ #22 Application Update methods #80

✏️ Feat/ #22 Application Update methods #80

demariadaniel commented Nov 28, 2024 •

edited

Loading

demariadaniel Nov 28, 2024

demariadaniel Nov 28, 2024

JamesTLopez left a comment

JamesTLopez Dec 2, 2024

JamesTLopez Dec 2, 2024 •

edited

Loading

demariadaniel Dec 2, 2024 •

edited

Loading

demariadaniel Dec 2, 2024

JamesTLopez Dec 2, 2024

demariadaniel commented Dec 2, 2024

leoraba Dec 4, 2024

leoraba Dec 4, 2024

demariadaniel Dec 4, 2024

leoraba Dec 4, 2024

demariadaniel Dec 4, 2024

demariadaniel Dec 4, 2024

demariadaniel Dec 4, 2024

leoraba Dec 4, 2024

demariadaniel Dec 4, 2024

demariadaniel Dec 4, 2024

leoraba left a comment

leoraba Dec 4, 2024

demariadaniel Dec 4, 2024

leoraba left a comment


		- Drizzle is configured to read the schema from the build directory (`/dist`) due to a known Drizzle Kit issue with ESM imports. Any Drizzle Kit operations (including migrations) require first building the application with `pnpm run build`.

		- A schema dbml (database markup language) file can be generated using the script `pnpm run dbml`. This file is found at `./src/db/schema.dbml`.

✏️ Feat/ #22 Application Update methods #80

✏️ Feat/ #22 Application Update methods #80

Conversation

demariadaniel commented Nov 28, 2024 • edited Loading

Summary

Related Issues

Description of Changes

Special Instructions

Readiness Checklist

Choose a reason for hiding this comment

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

JamesTLopez Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

demariadaniel Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demariadaniel commented Dec 2, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoraba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoraba left a comment

Choose a reason for hiding this comment

demariadaniel commented Nov 28, 2024 •

edited

Loading

JamesTLopez Dec 2, 2024 •

edited

Loading

demariadaniel Dec 2, 2024 •

edited

Loading