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

Update voyage/features endpoints to follow RESTful api naming convention #123

Merged
merged 7 commits into from
Mar 27, 2024

Conversation

timDeHof
Copy link
Contributor

@timDeHof timDeHof commented Mar 24, 2024

Description

refactor(app.module.ts): update route paths for resources, techs, and ideations to include "teams" for better organization and clarity
fix(features.controller.ts): update route path for creating and retrieving features to include "teams" for better organization and clarity
fix(ideations.e2e-spec.ts): update route paths for creating, retrieving, updating, and deleting ideations to include "teams" for better organization and clarity
fix(techs.e2e-spec.ts): update route paths for retrieving, creating, voting, and deleting techs to include "teams" for better organization and clarity

The changes were made to update the voyages route paths in the codebase to include "teams" for better organization and clarity. This improves the readability and maintainability of the code by clearly indicating that these routes are related to teams.

Issue link

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature updates / changes
  • Tests
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

I have tested these changes with unit tests and e2e tests. Also I viewed the changes in swagger too.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have updated the change log

Useful links

API Naming conventions

Voyage – Features

POST /api/v1/voyages/{teamId}/features  => /api/v1/voyages/teams/{teamId}/features
GET /api/v1/voyages/{teamId}/features  => /api/v1/voyages/teams/{teamId}/features
 
Voyage – Ideations
POST /api/v1/voyages/{teamId}/ideations => /api/v1/voyages/teams/{teamId}/ideations
GET /api/v1/voyages/{teamId}/ideations => /api/v1/voyages/teams/{teamId}/ideations
POST /api/v1/voyages/{teamId}/ideations/{ideationsId}/ideation-votes => /api/v1/voyages/teams/{teamId}/ideations/{ideationsId}/ideation-votes
DELETE /api/v1/voyages/{teamId}/ideations/{ideationsId}/ideation-votes => /api/v1/voyages/teams/{teamId}/ideations/{ideationsId}/ideation-votes
PATCH /api/v1/voyages/{teamId}/ideations/{ideationsId} => /api/v1/voyages/teams/{teamId}/ideations/{ideationsId}
DELETE /api/v1/voyages/{teamId}/ideations/{ideationsId} => /api/v1/voyages/teams/{teamId}/ideations/{ideationsId}
 
Voyage – Resources
POST /api/v1/voyages/{teamId}/resources => /api/v1/voyages/teams/{teamId}/resources
GET /api/v1/voyages/{teamId}/resources => /api/v1/voyages/teams/{teamId}/resources
PATCH /api/v1/voyages/{teamId}/resources/{resourceId} => /api/v1/voyages/teams/{teamId}/resources/{resourceId}
DELETE /api/v1/voyages/{teamId}/resources/{resourceId} => /api/v1/voyages/teams/{teamId}/resources/{resourceId}
 
Voyage – Techs
POST /api/v1/voyages/{teamId}/techs => /api/v1/voyages/teams/{teamId}/techs
GET /api/v1/voyages/{teamId}/techs=> /api/v1/voyages/teams/{teamId}/techs
POST /api/v1/voyages/{teamId}/techs/{teamTechId} => /api/v1/voyages/teams/{teamId}/techs/{teamTechId}
DELETE /api/v1/voyages/{teamId}/techs/{teamTechId} => /api/v1/voyages/teams/{teamId}/techs/{teamTechId}

users
GET /api/v1/users/id/{userId} => /api/v1/users/{userId}
GET /api/v1/users/email/{email} => POST /api/v1/users/lookup-by-email (email is in the request body)

here is Chingu voyages endpoints changes.docx

… ideations to include "teams" for better organization and clarity

fix(features.controller.ts): update route path for creating and retrieving features to include "teams" for better organization and clarity
fix(ideations.e2e-spec.ts): update route paths for creating, retrieving, updating, and deleting ideations to include "teams" for better organization and clarity
fix(techs.e2e-spec.ts): update route paths for retrieving, creating, voting, and deleting techs to include "teams" for better organization and clarity

The changes were made to update the route paths in the codebase to include "teams" for better organization and clarity. This improves the readability and maintainability of the code by clearly indicating that these routes are related to teams.
@timDeHof timDeHof self-assigned this Mar 24, 2024
… to follow API naming convention

The voyages endpoint paths have been refactored to follow the API naming convention. This improves consistency and readability of the codebase.
@timDeHof timDeHof marked this pull request as ready for review March 25, 2024 22:51
Copy link
Contributor

@cherylli cherylli left a comment

Choose a reason for hiding this comment

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

THanks. Looks good, all tests passed. I see one more which could be changed

image
-> users/{userId}

@siasktv
Copy link
Contributor

siasktv commented Mar 27, 2024

Hi, Tim! Read the files seems all good. I checkout to this branch to run the tests but I'm getting this error:
test/sprints.e2e-spec.ts:9:27 - error TS2307: Cannot find module 'jest-extended' or its corresponding type declarations.

9 import { toBeOneOf } from "jest-extended";

Are you getting the same?

@timDeHof
Copy link
Contributor Author

timDeHof commented Mar 27, 2024

Hi, Tim! Read the files seems all good. I checkout to this branch to run the tests but I'm getting this error: test/sprints.e2e-spec.ts:9:27 - error TS2307: Cannot find module 'jest-extended' or its corresponding type declarations.

9 import { toBeOneOf } from "jest-extended";

Are you getting the same?

yes, I did get something similar.it seems running yarn install in the terminal of your IDE or in docker fixed it.

…e userId as a route parameter instead of a query parameter

feat(users.controller.ts): add POST route for getUserDetailsByEmail to allow looking up user details by email
The route path for getUserDetailsById has been fixed to use the userId as a route parameter instead of a query parameter. This improves the semantics and makes the route more RESTful. Additionally, a new POST route has been added for getUserDetailsByEmail to allow looking up user details by email. The email is now passed in the request body instead of the route parameter. This change provides a more secure and flexible way of passing sensitive information.
…mprove clarity

The error message in the NotFoundException has been updated to provide a clearer description of the issue. Instead of mentioning "User (email: ${email} not found", it now says "User with ${email} not found". This change improves the readability and understanding of the error message.
src/users/users.controller.ts Outdated Show resolved Hide resolved
src/users/users.controller.ts Outdated Show resolved Hide resolved
feat(users): add HttpCode decorator to getUserDetailsByEmail endpoint to return 200 status code
fix(users): fix error message in getUserDetailsByEmail endpoint
feat(users): update getUserDetailsByEmail service method to accept UserLookupByEmailDto
The UserLookupByEmailDto class is added to handle the request payload for looking up a user by email. The getUserDetailsByEmail endpoint is updated with the HttpCode decorator to return a 200 status code. The error message in the getUserDetailsByEmail endpoint is fixed to include the correct email syntax. The getUserDetailsByEmail service method is updated to accept the UserLookupByEmailDto object instead of just the email string.
… for isEmail from class-validator

The import and function call for isEmail from class-validator have been removed as they are no longer used in the code. This improves code readability and removes unnecessary dependencies.
Copy link
Contributor

@JoshuaHinman JoshuaHinman left a comment

Choose a reason for hiding this comment

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

All changes are clear and well explained. E2e tests passed. Nice work! The changes to techs.e2e-spec will conflict with my PR but it looks easy to resolve

@timDeHof
Copy link
Contributor Author

All changes are clear and well explained. E2e tests passed. Nice work! The changes to techs.e2e-spec will conflict with my PR but it looks easy to resolve

Thank you
Yes, I was just about to make a comment about the potential conflict that would occur in my code review of your PR.

@timDeHof timDeHof requested a review from cherylli March 27, 2024 21:51
Copy link
Contributor

@cherylli cherylli left a comment

Choose a reason for hiding this comment

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

Thanks. everything looks good!

@timDeHof timDeHof removed request for siasktv and curtwl March 27, 2024 23:21
@timDeHof timDeHof merged commit 039aeda into dev Mar 27, 2024
1 check passed
@timDeHof timDeHof deleted the refactor/voyages-endpoints branch March 27, 2024 23:36
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.

4 participants