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 response Ids - POST voyages/teams/teamId/techs #138

Merged
merged 15 commits into from
Apr 27, 2024

Conversation

timDeHof
Copy link
Contributor

@timDeHof timDeHof commented Apr 19, 2024

Description

This PR will rename the id property in the TechItemResponse class to teamTechStackItemVoteId to provide better clarity and improve semantics. The example value for the teamMemberId property has been updated to 8 to maintain consistency with other example values. The create method in the TechsService class now returns an object that includes the teamTechStackItemVoteId property for consistency. Similarly, the delete method now returns an object that includes the teamTechStackItemVoteId property for consistency.

Issue link

Fixes # https://app.clickup.com/t/86azw0fjm

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?

In swagger:

  1. run POST /api/v1/auth/login

  2. After a successful login, run POST /api/v1/voyages/teams/{teamId}/techs/{teamTechId} with 'teamId: 2' and 'teamTechId: 6'. I found these two parameters worked, as the example parameters gave some errors.
    The response body should be a success like the picture below:
    Screenshot 2024-04-19 at 5 33 20 PM

  3. After voting on a tech, run DELETE /api/v1/voyages/teams/{teamId}/techs/{teamTechId} with the same parameters previous used.
    The response body should be a success like the picture below:
    Screenshot 2024-04-19 at 5 33 46 PM

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

…otedId for clarity

fix(techs.response.ts): update example value for teamMemberId property to 8 for consistency
fix(techs.service.ts): update return value of create method to include teamTechStackItemVoteId property for consistency
fix(techs.service.ts): update return value of delete method to include teamTechStackItemVotedId property for consistency
The id property in the TechItemResponse class has been renamed to teamTechStackItemVotedId to provide better clarity and improve semantics. The example value for the teamMemberId property has been updated to 8 to maintain consistency with other example values. The create method in the TechsService class now returns an object that includes the teamTechStackItemVoteId property for consistency. Similarly, the delete method now returns an object that includes the teamTechStackItemVotedId property for consistency.
fix(techs.e2e-spec.ts): fix property name in expected response object
The property name "teamTechStackItemVotedId" in the returned object of the `createTechVote` method in `TechsService` has been corrected to "teamTechStackItemVoteId" to match the actual property name. Similarly, in the `techs.e2e-spec.ts` file, the property name "id" in the expected response object has been corrected to "teamTechStackItemVoteId" to match the actual property name.
…he voteTechStackItem method

The comment clarifies that if the voteTechStackItem method is successful, it will return an object containing the details of the vote. This comment improves the code readability and provides additional information for developers working with this method.
@timDeHof timDeHof self-assigned this Apr 19, 2024
timDeHof and others added 2 commits April 19, 2024 17:41
The changelog has been updated to include the following changes: - Modified response for POST /api/v1/voyages/teams/{teamId}/techs/{teamTechId} & DELETE /api/v1/voyages/teams/{teamId}/techs/{teamTechId}, refactor id as teamTechStackItemVoteId value [#138](#138)
cherylli
cherylli previously approved these changes Apr 20, 2024
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.

Everything works fine - I think we should add a more descriptive error message when we have an invalid Id, since there's no teamTechId = 11, and maybe also update the sample input? (you can add to this PR or I can write another task for this)

image

@timDeHof
Copy link
Contributor Author

Yes, I will do that. I was getting same the error when I started this task and was a little confusing which was making me think it was another issue.

…ter in addExistingTechVote and removeVote methods

The example values for the teamTechId parameter in the addExistingTechVote and removeVote methods have been updated to reflect the correct values. This ensures that the examples provided are accurate and align with the expected input for the teamTechId parameter.
@timDeHof
Copy link
Contributor Author

I have updated the example parameters in POST /api/v1/voyages/teams/{teamId}/techs/{teamTechId} and DELETE /api/v1/voyages/teams/{teamId}/techs/{teamTechId} to provide an accurate output.

… item in addExistingTechVote method

The addExistingTechVote method in the TechsService class now includes error handling to check if the team tech item exists before proceeding with the vote. If the team tech item is not found, a BadRequestException is thrown with the message "Team Tech Item not found". This ensures that only valid team tech items can be voted on.
@timDeHof
Copy link
Contributor Author

I have added error handling that would check if the team tech stack id is present since the message that is shown from bad request response isn't very descriptive.

@timDeHof timDeHof requested a review from cherylli April 23, 2024 13:28
@cherylli cherylli changed the title Update response Ids - POST voyages/teams/teamId/techs Update response Ids - POST voyages/teams/teamId/techs/{teamTechId} Apr 24, 2024
@cherylli cherylli changed the title Update response Ids - POST voyages/teams/teamId/techs/{teamTechId} Update response Ids - POST voyages/teams/teamId/techs Apr 24, 2024
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.

I just realize the original task is to update POST voyages/teams/teamId/techs but it seems `POST voyages/teams/teamId/techs/{teamTechId} is updated here

Although I think all techs vote related endpoints would require update anyway

In POST voyages/teams/teamId/techs it automatically adds a vote as well, you can verify the ID in the database

…deleting a vote or tech stack item

fix(techs): update ApiResponse description for removing a vote to include case when tech stack item is removed
fix(techs): update TechsService to return TechItemDeleteResponse when a vote is removed and tech stack item is deleted
The TechItemDeleteResponse class is added to handle the response when a vote or tech stack item is deleted. The ApiResponse description for removing a vote is updated to include the case when the tech stack item is removed. The TechsService is updated to return the TechItemDeleteResponse when a vote is removed and the tech stack item is deleted. This provides a more informative response to the client.
…elete method

The variable 'deletedVote' was not being used in the delete method of the TechsService class. Removing this unused variable improves code readability and eliminates unnecessary clutter.
@timDeHof
Copy link
Contributor Author

I just realize the original task is to update POST voyages/teams/teamId/techs but it seems `POST voyages/teams/teamId/techs/{teamTechId} is updated here

Although I think all techs vote related endpoints would require update anyway

In POST voyages/teams/teamId/techs it automatically adds a vote as well, you can verify the ID in the database

Thank you for catching that. I managed to setup the return to be similar to the POST voyages/teams/teamId/techs/{teamTechId} one. In addition, I made changes to the removeVote service to return just a message and statusCode of 200 when it removes a vote for a tech stack item or removes the tech stack item when its votes come down to zero.

@cherylli cherylli self-requested a review April 25, 2024 03: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 was working with manual test on swagger but e2e tests are failing now due to the response updates

image

… Controller

The assertions in the e2e tests for the Techs Controller have been updated to reflect changes in the response structure. The "id" property has been renamed to "teamTechStackItemVoteId" and some other properties have been removed. The updated assertions now check for the presence of the "message" and "statusCode" properties in the response. This change improves the accuracy of the tests and ensures they are aligned with the current implementation.
… is deleted from the database

feat(techs.e2e-spec.ts): add test case to verify that tech stack item and its associated vote are deleted
The new test case verifies that the tech stack item is deleted from the database after it is deleted from the team's tech stack. This ensures that the deletion is properly reflected in the database. Additionally, a new test case is added to verify that both the tech stack item and its associated vote are deleted when the tech stack item is deleted from the team's tech stack. This ensures that the deletion is properly handled and all related data is removed.
@timDeHof
Copy link
Contributor Author

I fixed the tests that were failing and add a new test that checks if a tech stack item is deleted from the database if its last vote was deleted.

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.

everything work fine, all tests passed

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.

Looks good - it worked as described

@timDeHof timDeHof merged commit 7ea1b86 into dev Apr 27, 2024
1 check passed
@timDeHof timDeHof deleted the fix/tech-response-ids branch April 27, 2024 11:53
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.

3 participants