add routes to handle create, delete and update team #68

heem42 · 2024-12-26T03:33:44Z

Summary

Added routes to handle create, delete and update the teams

Tests

Manually tested using Postman

Additional information

Issue #64

src/controllers/sqlControllers/createTeam.ts

madcampos · 2025-01-08T17:56:22Z

src/controllers/sqlControllers/createTeam.ts

+import { StatusCodes } from 'src/constants/status-codes.ts';
+import type { TeamCreateBody } from 'src/types/data/team';
+
+export const createTeamSql = async (context: Context<EnvironmentBindings>) => {


suggestion: ‏So, even if we are not doing MVC, it would be good to have some separation of concerns here.

That means, interactions with the database like creating the SQL for it should consume the already parsed object.

Think it this way: who's responsibility is to validate if the request is actually a JSON instead of something else?

That makes more sense to be handled by the request handler, as it handles "low level" things, like validating a request is actually in the proper format (i.e. headers are correct, the JSON is a JSON, etc.).

So, here it makes more sense here to split this function in three parts:

Request handler to do all things related to the request.

DB insertion handler to communicate with the database.

Data validator to throw an error in case there is any problem with the data.

I have made the changes for @heem42 utilizing the above structure(Zod for validation) but we will need to move the functions to there own files in another pull requests.

src/controllers/sqlControllers/createTeam.ts

madcampos · 2025-01-08T18:07:57Z

src/controllers/sqlControllers/createTeam.ts

+		const body: TeamCreateBody = await context.req.json();
+
+		if (!body.name) {
+			return context.json({ error: 'name is required to create a team' }, StatusCodes.INTERNAL_SERVER_ERROR);


suggestion: On the same lines about separation of concerns, here would be better to throw an error instead of sending a response.

That way, the only part of the code that returns a response to the network is the request handler, every other function either returns or throws an error.

Also, on this case the status code should be a 415 Unsupported Media Type.

Added suggestion to CONTRIBUTION.MD update and updated error handling.

Issue #71

madcampos · 2025-01-08T18:10:35Z

src/controllers/sqlControllers/createTeam.ts

+		}
+
+		if (!body.happenedAt) {
+			return context.json({ error: 'happenedAt is required to create a team' });


suggestion: ‏Here again on the sense of "separation of concerns", what about we have all of those checks for validation in a validateTeamRequestData function?

Most probably we will want to run other validations, like, if the name is a string and is not empty, etc. So if we move those checks to a function, they will be all in one place and will be easier to manage.

Created issue to address this #70

madcampos · 2025-01-08T18:22:19Z

src/controllers/sqlControllers/getProfiles.ts

@@ -1,6 +1,6 @@
 import type { Context } from 'hono';
 import { StatusCodes } from '../../constants/status-codes.ts';
-import type { Profile } from '../../db/profile';
+import type { Profile } from 'src/types/data/profile';


nitpick: ‏Please keep relative imports

If you're using vscode, you can add /.vscode/settings.json like this below.

{ "typescript.preferences.importModuleSpecifier": "relative" }

madcampos · 2025-01-08T18:23:09Z

src/controllers/sqlControllers/getTeams.ts

@@ -3,28 +3,26 @@ import { StatusCodes } from '../../constants/status-codes.ts';

 export const getAllTeamsSql = async (context: Context<EnvironmentBindings>) => {


suggestion: ‏Again here the same comment: let's split the code into request handler, sql handler, and validator.

Every entity will have a directory container all three files or are we utilizing another file/directory structure?

src/types/data/team.d.ts

madcampos · 2025-01-08T18:27:32Z

src/controllers/sqlControllers/updateTeam.ts

+import { StatusCodes } from 'src/constants/status-codes.ts';
+import type { TeamUpdateBody } from 'src/types/data/team';
+
+export const updateTeamById = async (context: Context<EnvironmentBindings>) => {


suggestion: ‏Same as before, specially here as we need strong validation. Split the function into request handler, sql handler, and validator.

File split in 3 functions which will moved to individual files in future pull requests.

madcampos · 2025-01-08T18:33:21Z

@kbventures what are your thoughts on adding a validation lidrary, like Zod or Superstruct?

madcampos · 2025-01-15T19:32:01Z

src/controllers/sqlControllers/updateTeam.ts

+			updateValues.push(body.happenedAt);
+		}
+
+		if (updateFields.length === 0) {


suggestion: error handling should be first in processing to shortcut the function.

It makse sure our code only executes on reliable data and avoids computing things if it will error out anyways.

madcampos · 2025-01-15T19:36:39Z

src/controllers/sqlControllers/createTeam.ts

+		const insertedAt = new Date().toISOString();
+		const happenedAt = new Date(body.happenedAt).toISOString();
+
+		const createTeam = await context.env.database.prepare(


nitpick:

Suggested change

const createTeam = await context.env.database.prepare(

const teamCreationResult = await context.env.database.prepare(

madcampos · 2025-01-15T19:39:24Z

src/controllers/sqlControllers/createTeam.ts

+			.bind(id, body.name, SCHEMA_VERSION, body.description ?? '', happenedAt, insertedAt)
+			.run();
+
+		return context.json(createTeam);


nitpick: We don't want to return the result straight from the database here, we want to process the response from the database and handle errors and other exceptions!

Please review Cloudflare's docs and return a meaningful and informative message to the client.

Think about it, a message saying "Error 0x234571" doesn't provide a good experience for the client. We should provide a better message, including the correct HTTP status code if the operation was successful or not.

src/types/data/team.d.ts

kbventures

Great work @heem42

Normally we would wait to let you make the changes but this issue is blocking other road map items.

src/controllers/sqlControllers/createTeam.ts

kbventures · 2025-01-15T19:42:23Z

src/controllers/sqlControllers/getTeams.ts

@@ -3,28 +3,26 @@ import { StatusCodes } from '../../constants/status-codes.ts';

 export const getAllTeamsSql = async (context: Context<EnvironmentBindings>) => {


Every entity will have a directory container all three files or are we utilizing another file/directory structure?

kbventures · 2025-01-16T18:47:12Z

src/controllers/sqlControllers/createTeam.ts

+import { StatusCodes } from 'src/constants/status-codes.ts';
+import type { TeamCreateBody } from 'src/types/data/team';
+
+export const createTeamSql = async (context: Context<EnvironmentBindings>) => {


I have made the changes for @heem42 utilizing the above structure(Zod for validation) but we will need to move the functions to there own files in another pull requests.

kbventures · 2025-01-16T19:20:48Z

src/controllers/sqlControllers/updateTeam.ts

+import { StatusCodes } from 'src/constants/status-codes.ts';
+import type { TeamUpdateBody } from 'src/types/data/team';
+
+export const updateTeamById = async (context: Context<EnvironmentBindings>) => {


File split in 3 functions which will moved to individual files in future pull requests.

madcampos · 2025-01-16T20:53:25Z

src/types/data/team.d.ts

nitpick: Use tabs for formatting‏

Co-authored-by: Marco Campos <[email protected]>

…dummy data

heem42 · 2025-01-18T03:41:20Z

Thank you, @madcampos, for your thoughtful comments and to @kbventures for implementing those fixes. Apologies for the delay in responding—I've been tied up with other work. I really appreciate your efforts on this! Please let me know if there are any further changes required.

sounmind · 2025-01-19T19:54:26Z

src/controllers/sqlControllers/profileSqlControllers/getProfiles.ts

@@ -0,0 +1,59 @@
+import { z } from 'zod';


Have you installed zod? I can't find zod dependency added in the package.json in your PR

heem42 self-assigned this Dec 26, 2024

madcampos requested review from kbventures and madcampos January 8, 2025 17:08

madcampos reviewed Jan 8, 2025

View reviewed changes

pmjuu linked an issue Jan 10, 2025 that may be closed by this pull request

Team CRUD #64

Open

madcampos reviewed Jan 15, 2025

View reviewed changes

madcampos requested review from madcampos and pmjuu January 15, 2025 19:47

kbventures reviewed Jan 16, 2025

View reviewed changes

kbventures added this to the MVP for consuming data milestone Jan 16, 2025

madcampos reviewed Jan 16, 2025

View reviewed changes

src/types/data/team.d.ts

Copy link

Collaborator

madcampos Jan 16, 2025

Choose a reason for hiding this comment

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

nitpick: Use tabs for formatting‏

madcampos requested review from kbventures and madcampos January 16, 2025 20:53

madcampos force-pushed the team-crud branch from 7253632 to 4678829 Compare January 16, 2025 22:06

heem42 and others added 15 commits January 16, 2025 17:31

add routes to handle create, delete and update team

50679f1

Update package.json

6d7dc05

Update package.json

a9f36db

Update createTeam.ts

59ebb70

Update src/controllers/sqlControllers/createTeam.ts

ff21b9c

Co-authored-by: Marco Campos <[email protected]>

Update createTeam.ts

b2a8db0

Update getProfiles.ts

c228c76

Update teamsSql.ts

7c811c5

Update createTeam.ts

1150d96

Update deleteTeam.ts

5bae4f8

Update getProfiles.ts

bd3e2f6

Update createTeam.ts

a291b2b

Update createTeam.ts

db996f8

Update deleteTeam.ts

5df2d5d

Update deleteTeam.ts

3e995fa

kbventures and others added 12 commits January 16, 2025 17:31

Update getTeams.ts

435938c

Update updateTeam.ts

7693d0c

Update src/types/data/team.d.ts

27900c2

Co-authored-by: Marco Campos <[email protected]>

Update src/types/data/team.d.ts

b1a0d12

Co-authored-by: Marco Campos <[email protected]>

Update getProfiles.ts

01a4e9d

Update team.d.ts

7d12537

Update createTeam.ts

31d9a1c

Update deleteTeam.ts

847a695

Update deleteTeam.ts

ab1c5a7

Update createTeam.ts

ec25739

Update deleteTeam.ts

c1fdace

Update updateTeam.ts

2478cbd

kbventures force-pushed the team-crud branch from 4678829 to 2478cbd Compare January 16, 2025 22:36

kbventures and others added 2 commits January 17, 2025 15:04

Functions seperated into handler sql validation functions

eeb8df3

merge with parent branch and update db setup script to avoid invalid …

8c9e49d

…dummy data

pmjuu approved these changes Jan 19, 2025

View reviewed changes

sounmind reviewed Jan 19, 2025

View reviewed changes

kbventures added 3 commits January 20, 2025 14:15

Zod

9f55774

Upudated dummy dada for D1 to include UUIDs

8fc3925

Zod OSI Timestamp check and curl create and and get all automated tests

fc9044e

kbventures approved these changes Jan 20, 2025

View reviewed changes

kbventures merged commit 2d50644 into setup-d1-database Jan 20, 2025

kbventures deleted the team-crud branch January 20, 2025 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add routes to handle create, delete and update team #68

add routes to handle create, delete and update team #68

heem42 commented Dec 26, 2024 •

edited

Loading

madcampos Jan 8, 2025

kbventures Jan 16, 2025

madcampos Jan 8, 2025

kbventures Jan 14, 2025

madcampos Jan 8, 2025

kbventures Jan 14, 2025

madcampos Jan 8, 2025

pmjuu Jan 14, 2025 •

edited

Loading

madcampos Jan 8, 2025

kbventures Jan 15, 2025

madcampos Jan 8, 2025

kbventures Jan 16, 2025

madcampos commented Jan 8, 2025

madcampos Jan 15, 2025

madcampos Jan 15, 2025 •

edited

Loading

madcampos Jan 15, 2025 •

edited

Loading

kbventures left a comment

kbventures Jan 15, 2025

kbventures Jan 16, 2025

kbventures Jan 16, 2025

madcampos Jan 16, 2025

heem42 commented Jan 18, 2025

sounmind Jan 19, 2025

		@@ -3,28 +3,26 @@ import { StatusCodes } from '../../constants/status-codes.ts';

		export const getAllTeamsSql = async (context: Context<EnvironmentBindings>) => {

	const createTeam = await context.env.database.prepare(
	const teamCreationResult = await context.env.database.prepare(

add routes to handle create, delete and update team #68

add routes to handle create, delete and update team #68

Conversation

heem42 commented Dec 26, 2024 • edited Loading

Summary

Tests

Additional information

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

pmjuu Jan 14, 2025 • 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

madcampos commented Jan 8, 2025

Choose a reason for hiding this comment

madcampos Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

madcampos Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

kbventures 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

heem42 commented Jan 18, 2025

Choose a reason for hiding this comment

heem42 commented Dec 26, 2024 •

edited

Loading

pmjuu Jan 14, 2025 •

edited

Loading

madcampos Jan 15, 2025 •

edited

Loading

madcampos Jan 15, 2025 •

edited

Loading