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

✏️ Feat/ #22 Application Update methods #80

Merged
merged 29 commits into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
e3f3b78
Service Edit method
demariadaniel Nov 28, 2024
637f637
Move Test Files
demariadaniel Nov 28, 2024
501d6f0
Use .test naming convention
demariadaniel Nov 28, 2024
ed2be28
Move API test file
demariadaniel Nov 28, 2024
420a74f
Tests for Edit Method
demariadaniel Nov 28, 2024
c2ea32e
Remove npx
demariadaniel Nov 28, 2024
7b84efb
DBml / schema updates
demariadaniel Nov 29, 2024
1fb30cd
Use dist folder for config, add first readme update
demariadaniel Nov 29, 2024
a44b23c
Setup Router & API + Move logic out of service
demariadaniel Nov 29, 2024
42afe20
Working API Test w/ some reorg
demariadaniel Nov 29, 2024
eb27584
Edit for clarity
demariadaniel Dec 2, 2024
04f0dc6
Remove Demo endpoint
demariadaniel Dec 2, 2024
ffbe16f
Route + service improvements
demariadaniel Dec 2, 2024
ba949f7
Add todo
demariadaniel Dec 2, 2024
fc3e38a
Move service type, add error condition
demariadaniel Dec 2, 2024
5002a86
First updates with success/failure
demariadaniel Dec 2, 2024
728572e
Move tests
demariadaniel Dec 2, 2024
5d4915f
Use Success/Failure pattern with getById
demariadaniel Dec 2, 2024
060f027
Add TSDoc
demariadaniel Dec 2, 2024
2783fee
Add getDbInstance type
demariadaniel Dec 2, 2024
596affa
Throw err is no db instance
demariadaniel Dec 2, 2024
d3a8b30
Add body parser
demariadaniel Dec 2, 2024
b77bb36
Simplify variable declaration
demariadaniel Dec 2, 2024
c0079dc
Add Comments + Transaction
demariadaniel Dec 3, 2024
cfc1cb6
Merge branch 'main' of https://github.com/Pan-Canadian-Genome-Library…
demariadaniel Dec 3, 2024
f08b6a9
Print error message in response
demariadaniel Dec 3, 2024
fa1202e
Remove tests from tsconfig
demariadaniel Dec 4, 2024
fc7cce3
Add Edit endpoint to Swagger w/ 404
demariadaniel Dec 4, 2024
7ceb7ea
Add Error Todo
demariadaniel Dec 4, 2024
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
22 changes: 10 additions & 12 deletions apps/api/src/api/application-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@
*/

import { ApplicationStates } from '@pcgl-daco/data-model/src/types.js';
import { getDbInstance } from '../db/index.js';
import applicationService from '../service/application-service.js';
import { type ApplicationContentUpdates, type ApplicationService } from '../service/types.js';

import { getDbInstance } from '../db/index.js';
import { failure } from '../utils/results.js';

export const editApplication = async ({ id, update }: { id: number; update: ApplicationContentUpdates }) => {
const database = getDbInstance();
Expand All @@ -30,7 +30,9 @@ export const editApplication = async ({ id, update }: { id: number; update: Appl
const applicationRecord = await service.getApplicationById({ id });

if (!applicationRecord) {
throw new Error('Application Not Found');
const message = `Application Not Found with ${id}`;
console.error(message);
return failure(message);
}

// Validate Application state will allow updates
Expand All @@ -44,15 +46,11 @@ export const editApplication = async ({ id, update }: { id: number; update: Appl
state === ApplicationStates.DAC_REVIEW;

if (isEditState) {
const updatedRecord = service.editApplication({ id, update });
if (updatedRecord) {
return updatedRecord;
} else {
console.error(`Error updating application`);
return null;
}
const result = await service.editApplication({ id, update });
return result;
} else {
console.error(`Cannot update application with state ${state}`);
return null;
const message = `Cannot update application with state ${state}`;
console.error(message);
return failure(message);
}
};
10 changes: 5 additions & 5 deletions apps/api/src/routes/application-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ import { editApplication } from '../api/application-api.js';
const applicationRouter = express.Router();

applicationRouter.post('/application/edit/', async (req, res) => {
// TODO: Add Auth, Zod, & success/failure types
// TODO: Add Auth & Zod validation
const data = req.body;
const { id, update } = data;
const record = await editApplication({ id, update });
const result = await editApplication({ id, update });

if (record) {
res.send(record);
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 });
}
Copy link
Contributor

@JamesTLopez JamesTLopez Dec 2, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@demariadaniel demariadaniel Dec 2, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

});

Expand Down
12 changes: 7 additions & 5 deletions apps/api/src/service/application-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { and, eq, sql } from 'drizzle-orm';
import { type PostgresDb } from '../db/index.js';
import { applicationContents } from '../db/schemas/applicationContents.js';
import { applications } from '../db/schemas/applications.js';
import { failure, success } from '../utils/results.js';
import {
type ApplicationContentUpdates,
type ApplicationsColumnName,
Expand All @@ -30,8 +31,6 @@ import {
} from './types.js';
import { sortQuery } from './utils.js';

export type ApplicationService = ReturnType<typeof applicationService>;

const applicationService = (db: PostgresDb) => ({
createApplication: async ({ user_id }: { user_id: string }) => {
const newApplication: typeof applications.$inferInsert = {
Expand Down Expand Up @@ -95,11 +94,12 @@ const applicationService = (db: PostgresDb) => ({
contents: editedContents[0],
};

return application;
return success(application);
} catch (err) {
console.error(`Error at editApplication with id: ${id}`);
const message = `Error at editApplication with id: ${id}`;
console.error(message);
console.error(err);
return null;
return failure(message, err);
}
},
findOneAndUpdate: async ({ id, update }: { id: number; update: ApplicationUpdates }) => {
Expand All @@ -124,6 +124,7 @@ const applicationService = (db: PostgresDb) => ({
.from(applications)
.where(eq(applications.id, id))
.leftJoin(applicationContents, eq(applications.contents, applicationContents.id));

if (!applicationRecord[0]) throw new Error('Application record is undefined');

const application = {
Expand All @@ -135,6 +136,7 @@ const applicationService = (db: PostgresDb) => ({
} catch (err) {
console.error(`Error at getApplicationById with id: ${id}`);
console.error(err);

return null;
}
},
Expand Down
32 changes: 32 additions & 0 deletions apps/api/src/utils/results.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright (c) 2024 The Ontario Institute for Cancer Research. All rights reserved
*
* This program and the accompanying materials are made available under the terms of
* the GNU Affero General Public License v3.0. You should have received a copy of the
* GNU Affero General Public License along with this program.
* If not, see <http://www.gnu.org/licenses/>.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY
* EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
* OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT
* SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED
* TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
* OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER
* IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
* ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

// types
export type Success<T> = { success: true; data: T };
export type Failure = { success: false; message: string; errors?: any };
export type Result<T> = Success<T> | Failure;
export type AsyncResult<T> = Promise<Result<T>>;

// helpers
export const success = <T>(data: T): Success<T> => ({ success: true, data });
export const failure = (message: string, errors?: any): Failure => ({
success: false,
message,
errors,
});
4 changes: 2 additions & 2 deletions apps/api/tests/api/application-api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ describe('Application API', () => {
await applicationService.findOneAndUpdate({ id, update: stateUpdate });

const contentUpdate = { applicant_title: 'Dr.' };
const editedApplication = await editApplication({ id, update: contentUpdate });
const result = await editApplication({ id, update: contentUpdate });

assert.ok(!editedApplication);
assert.ok(!result.success);
});

after(() => {});
Expand Down
15 changes: 11 additions & 4 deletions apps/api/tests/services/application-service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,10 +251,14 @@ describe('Application Service', () => {

const update = { applicant_first_name: 'Test' };

const editedApplication = await applicationService.editApplication({ id, update });
const result = await applicationService.editApplication({ id, update });

assert.ok(editedApplication && editedApplication.contents);
assert.ok(result.success);

const editedApplication = result.data;
assert.strictEqual(editedApplication.state, ApplicationStates.DRAFT);

assert.ok(editedApplication.contents);
assert.strictEqual(editedApplication.contents.applicant_first_name, update.applicant_first_name);
});

Expand All @@ -274,11 +278,14 @@ describe('Application Service', () => {
assert.strictEqual(reviewRecord[0].state, ApplicationStates.INSTITUTIONAL_REP_REVIEW);

const contentUpdate = { applicant_last_name: 'User' };
const editedApplication = await applicationService.editApplication({ id, update: contentUpdate });
const result = await applicationService.editApplication({ id, update: contentUpdate });
assert.ok(result.success);

assert.ok(editedApplication && editedApplication.contents);
const editedApplication = result.data;
assert.strictEqual(editedApplication.id, id);
assert.strictEqual(editedApplication.state, ApplicationStates.DRAFT);

assert.ok(editedApplication.contents);
assert.strictEqual(editedApplication.contents.applicant_last_name, contentUpdate.applicant_last_name);
});
});
Expand Down