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

Teams can create own tech categories #208

Merged
merged 28 commits into from
Oct 27, 2024

Conversation

JoshuaHinman
Copy link
Contributor

@JoshuaHinman JoshuaHinman commented Sep 21, 2024

Description

This PR completes the first part of task 86b1ym631
adds 3 routes for team tech stack categories (create, update, delete)
These are located in the techs section of the API
Now, each team will have it's own instances of the 6 main categories, and can also create their own
The schema for tech stack categories has been updated, along with seed data

Issue link

Fixes # 86b1ym631

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?

Migrate for DB updates and seed:
npx prisma migrate dev

Test routes in swagger
Verify in DB: 66 categories for 11 teams
Verify duplicate category names for the same teamId throw an exception, and are case-insensitive - i.e. "TEST" === "Test"

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

@JoshuaHinman JoshuaHinman self-assigned this Sep 21, 2024
@cherylli
Copy link
Contributor

image

haven't looked at the code yet but saw this in the lint and test workflow, we need to make it nullable or set a default value. At this stage of the project I don't think it matters what we do since prod will be wiped after the alpha test or before the next test phase. Probably better to set a default value because we don't want it null.

But we need to remove the default value after the new schema is pushed/deployed to all databases.

thoughts?

@JoshuaHinman
Copy link
Contributor Author

JoshuaHinman commented Sep 22, 2024

I changed the voyageTeamId relation to optional, even though I don't think it should be. It passed the check - It looks like the other relations are done this way too.

@cherylli
Copy link
Contributor

I changed the voyageTeamId relation to optional, even though I don't think it should be. It passed the check - It looks like the other relations are done this way too.

sounds good. I think some of the relations are nullable because I needed to do that to set up "onDelete" referential actions on team deletion, when I don't want to cascade delete, there are probably better ways we can look into that later. It will probably be a lot more relavent when we are working on the admin dashboard

@cherylli
Copy link
Contributor

removed @Ajen07 as a reviewer as he'll be away for 2 weeks

@cherylli cherylli requested review from timDeHof and removed request for Ajen07 September 22, 2024 15:22
@cherylli cherylli added the high Priority label Sep 23, 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.

image

is teamId same as voyageTeamId?

I tested on swagger (POST, PATCH, DELETE) they all work fine, ran all tests, still pass. Left a few comments but it's a good starting point for the change.

prisma/schema/voyage.prisma Outdated Show resolved Hide resolved
src/techs/dto/create-techstack-category.dto.ts Outdated Show resolved Hide resolved
src/techs/dto/update-techstack-category.dto.ts Outdated Show resolved Hide resolved
src/techs/techs.service.ts Outdated Show resolved Hide resolved
src/techs/techs.controller.ts Outdated Show resolved Hide resolved
src/techs/techs.controller.ts Outdated Show resolved Hide resolved
@JoshuaHinman
Copy link
Contributor Author

is teamId same as voyageTeamId?

Yes that's right - I actually tried removing this param from the controller, but the response sent an error that said "URL shouldn't be empty". I couldn't find the source of the message. Should I remove voyageTeamId from the body?

prisma/schema/teamStack.prisma Outdated Show resolved Hide resolved
src/techs/dto/update-techstack-category.dto.ts Outdated Show resolved Hide resolved
@JoshuaHinman JoshuaHinman marked this pull request as draft October 20, 2024 18:08
@JoshuaHinman JoshuaHinman marked this pull request as ready for review October 21, 2024 23:03
timDeHof
timDeHof previously approved these changes Oct 22, 2024
Copy link
Contributor

@timDeHof timDeHof left a comment

Choose a reason for hiding this comment

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

All unit and e2e tests passed. db shows 66 categories for 11 teams. Also the POST techCategory endpoint throw an exception and is case-sensitive for duplicate category names on the same team id.

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! This is good. I have tested on swagger everything listed in the description works, I just have some questions and very minor changes requested

I assume tests would be added/updated in later parts? Maybe new tests for categories, and update old tests if needed

CHANGELOG.md Outdated Show resolved Hide resolved
prisma/schema/voyage.prisma Outdated Show resolved Hide resolved
prisma/schema/teamStack.prisma Show resolved Hide resolved
prisma/schema/teamStack.prisma Outdated Show resolved Hide resolved
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.

Just randomly discovered this bug, but I think this may not be in the scope of this PR?

image

voyageTeam 1 doesn't own category 51, and I logged in as dan who is in voyageTeam1 but not in the team which owns category 51 (team 9)

====

Other things i've manually tested and all worked

  • create a new tech stack category, throw 409 when duplicated (non case sensitive) as expected
  • added the same category to different teams
  • try to add category to someone elses team (403)
  • patch (own team and other teams)
  • delete (own team and other teams)

@cherylli cherylli requested a review from timDeHof October 24, 2024 01:22
@JoshuaHinman
Copy link
Contributor Author

voyageTeam 1 doesn't own category 51, and I logged in as dan who is in voyageTeam1 but not in the team which owns category 51 (team 9)

I don't think it's specific to this PR but it could be related.
I've seen similar issues before , I think it was because the user was an admin. It looks like Dan is an admin - is that causing the problem?

@cherylli
Copy link
Contributor

voyageTeam 1 doesn't own category 51, and I logged in as dan who is in voyageTeam1 but not in the team which owns category 51 (team 9)

I don't think it's specific to this PR but it could be related. I've seen similar issues before , I think it was because the user was an admin. It looks like Dan is an admin - is that causing the problem?

In my database dan is not an admin, his only role is "voyager" but yeah if you want to fix this in another parts its fine, it's not a huge issue which stops the app from working.

@cherylli
Copy link
Contributor

hi @timDeHof are you able to re- review this PR please

Copy link
Contributor

@timDeHof timDeHof left a comment

Choose a reason for hiding this comment

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

All tests passed! migrating db works. Also the preventing the duplicate tech category name works.

@JoshuaHinman JoshuaHinman merged commit aed92f6 into dev Oct 27, 2024
1 check passed
@JoshuaHinman JoshuaHinman deleted the feature/teams-own-tech-categories branch October 27, 2024 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants