Feature: Allow edit and delete for tech stack item

[Ajen07]

Description

In the above PR the below changes are made:

• Added voyageTeamMemberId in the tech stack Item for the user creating it in the endpoint /api/v1/voyages/teams/{teamid}/techs
• Added new Patch and Delete endpoint for making changes in the tech stack item
• Added relevant e2e tests for the patch and delete endpoints

updated endpoints

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?

Tested with all the existing and new tests.

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

[cherylli]

sorry that was an accident - I thought I deleted the local branch as I accidentally commited to it

[cherylli]

items can be edited/deleted even voyageTeamMemberId doesn't match, maybe they can be added as part of "Adding more permissions"? Although it looks like voyageTeamMemberId is not used, if that's the case we can remove it from the request body, since you're using the user Id to check instead of voyageTeamMemberId, which I think it's good enough, I think the chance of the user accidentally updating another one of their tech items from another team (same userId but different teamMemberId) would be low

Another thing is teamId doesn't seem to be used, so we can leave that out if that's the case

I think the endpoints should probably be PATCH/DELETE voyages/techs/{teamTechId} based on API route naming conventions (I know I put something else in the task description, but please let me know if anything doesn't make sense in future)

which makes it really similar to the vote endpoint - maybe the vote endpoint needs to append /vote to it, might discuss it with the team

[JoshuaHinman]

I don't know if you are adding more permissions checks - but currently all tests pass and swagger routes work

[cherylli]

I think @Ajen07 is still working on the changes since I got a DM just yesterday? Please let us know if this is ready to be reviewed again

[timDeHof]

Great job! tested in swagger passed. I ran yarn test:docker and got a message "A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them."

when I ran yarn test:docker --detectOpenHandles all test passed and didn't see the warning.

Ran yarn test:docker a second time and all tests passed with no warnings. So I guess it something on my end.

[cherylli]

I did the same check again and it worked. I only have a suggestion to update the description to a more descriptive one as commented.

All tests passed with all the new updates

Thanks!

[timDeHof]

I noticed some typescript error within the updated code.

[Ajen07]

I noticed some typescript error within the updated code.

What errors are you facing ?

[timDeHof]

I noticed some typescript error within the updated code.

What errors are you facing ?

The errors came from me not using yarn migrate before. They disappeared after I ran the script.

But I was wondering if there should a message returned when the teamTechStackItems array is empty. I notice in swagger if I try the route /api/v1/voyages/teams/{teamId}/techs with the teamId of 1, I will get back an response object with an empty teamTechStackItems array for each category.

[Ajen07]

I noticed some typescript error within the updated code.

What errors are you facing ?

The errors came from me not using yarn migrate before. They disappeared after I ran the script.

But I was wondering if there should a message returned when the teamTechStackItems array is empty. I notice in swagger if I try the route /api/v1/voyages/teams/{teamId}/techs with the teamId of 1, I will get back an response object with an empty teamTechStackItems array for each category.

The tech categories are created by default . So each category will be empty in the start. I think for tech items for particular category we support empty array .

[cherylli]

I noticed some typescript error within the updated code.

What errors are you facing ?

The errors came from me not using yarn migrate before. They disappeared after I ran the script.
But I was wondering if there should a message returned when the teamTechStackItems array is empty. I notice in swagger if I try the route /api/v1/voyages/teams/{teamId}/techs with the teamId of 1, I will get back an response object with an empty teamTechStackItems array for each category.

The tech categories are created by default . So each category will be empty in the start. I think for tech items for particular category we support empty array .

this is correct (the categories are predefined) - it's possible for techstackitems to be empty (e.g. start of the voyage)

[timDeHof]

I noticed some typescript error within the updated code.

What errors are you facing ?

The errors came from me not using yarn migrate before. They disappeared after I ran the script.
But I was wondering if there should a message returned when the teamTechStackItems array is empty. I notice in swagger if I try the route /api/v1/voyages/teams/{teamId}/techs with the teamId of 1, I will get back an response object with an empty teamTechStackItems array for each category.

The tech categories are created by default . So each category will be empty in the start. I think for tech items for particular category we support empty array .

this is correct (the categories are predefined) - it's possible for techstackitems to be empty (e.g. start of the voyage)

ok, I didn't know that was default. I think everything is ok then but I'll check again tomorrow.

[timDeHof]

Checked again. All tests passed.