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

Feature: ideation/projectIdea selection #136

Merged
merged 10 commits into from
Apr 26, 2024
Merged

Conversation

JoshuaHinman
Copy link
Contributor

Description

This PR allows a voyage team member to choose one of their team's ideations as the final selection for their voyage.
Two new routes are added:
/voyages/teams/{teamId}/ideations/{ideationId}/select
/voyages/teams/{teamId}/ideations/reset-selection
Note: The name "ideations" refers to the projectIdeas model

There is a database migration, which adds the value isSelected to the projectIdeas schema. Your local DB will need to be updated for this change

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:
npx prisma migrate dev
npx prisma db push
Test:
yarn test:e2e ideations
yarn 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

timDeHof
timDeHof previously approved these changes Apr 19, 2024
Copy link
Collaborator

@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.

Looks great! All the tests pass!

Copy link
Collaborator

@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.

the actual responses and sample responses do not match in both endpoints, maybe you need to make a new response structure instead of using the ideationVote one
image
image

Another thing is the TeamId param doesn't seem to be doing anything, it works even if I put teamId = 55.

I think we need it for OWN_TEAM permission but we probably need another OWN_RESOURCE permission for future (https://app.clickup.com/t/86azr0q42)

Should we remove it for now to avoid possible confusion from the frontend team?

Everything else work fine for me

@JoshuaHinman
Copy link
Contributor Author

I think we need it for OWN_TEAM permission but we probably need another OWN_RESOURCE permission for future

Yes I did notice this issue - teamId is used to verify OWN_TEAM, but there is no check that the ideation is for that team. Also, the projectIdea schema only references the teamMember that created it, not the team. Should I add a function that checks this?

@cherylli
Copy link
Collaborator

I think we need it for OWN_TEAM permission but we probably need another OWN_RESOURCE permission for future

Yes I did notice this issue - teamId is used to verify OWN_TEAM, but there is no check that the ideation is for that team. Also, the projectIdea schema only references the teamMember that created it, not the team. Should I add a function that checks this?

yeah because teamMember is linked to a voyage team, so there's no need to have a "voyageTeam" linked to ideation. If it's not too much work, sure. Otherwise, we can just put a note in the code, and leave it till we add something like OWN_RESOURCE permission. I think all the other endpoints in ideation have the same issue anyway

@JoshuaHinman JoshuaHinman marked this pull request as draft April 23, 2024 01:23
@JoshuaHinman JoshuaHinman marked this pull request as ready for review April 23, 2024 01:28
Copy link
Collaborator

@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.

Perfect, thanks! I should have the CASL setup ready soon, then we can add all the permissions properly

@cherylli
Copy link
Collaborator

@timDeHof @curtwl this is ready for review again 🙏

Copy link
Collaborator

@curtwl curtwl left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@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.

After running yarn migrate:test:docker and yarn push:test:docker, I ran into couple of errors. First, when I run yarn seed, I get the following error:
Screenshot 2024-04-24 at 7 15 47 PM
Its seems that isSelected column in VoyageTeamMember model

The second error I encountered was when I ran yarn test:e2e:docker and I got the following error:
Screenshot 2024-04-24 at 7 16 06 PM
its seems the tests are missing login step

@cherylli
Copy link
Collaborator

I also get the tests failed (forgot to run the tests again after the updates) but seeding is fine for me

@JoshuaHinman
Copy link
Contributor Author

I also get the tests failed (forgot to run the tests again after the updates) but seeding is fine for me

I ran the tests again and they work for me... the two routes that aren't working each call a loginAdmin() function that logs in as {
email: "jessica.williamson@gmail.com",
password: "password",
}
Not sure why it would only work for me

@JoshuaHinman
Copy link
Contributor Author

After running yarn migrate:test:docker and yarn push:test:docker, I ran into couple of errors. First, when I run yarn seed, I get the following error:
Its seems that isSelected column in VoyageTeamMember model

The second error I encountered was when I ran yarn test:e2e:docker and I got the following error: its seems the tests are missing login step

Ok I'm trying to figure out what's going on here-
I checked on my end, and there is no column isSelected in voyageTeamMember. I've seen seeding errors like this when the DB has not migrated as intended. It is working for me, using yarn migrate and yarn push.

The two tests, shown failing in the image, are the only ones that login as admin Jessica Williamson. It looks like the login request is returning unauthorized for some reason. It is working for me, so I'm not sure what the difference is.

Thank you for posting the screenshots - they are helpful!

@curtwl
Copy link
Collaborator

curtwl commented Apr 25, 2024

It's still working fine for me too
image

@timDeHof
Copy link
Collaborator

Ok, I ran yarn migrate and yarn push in Docker then ran yarn:e2e:docker and everything passed. I even ran their counterparts and everything passed.

So I'm going to approve these changes...

@JoshuaHinman JoshuaHinman merged commit e11b42c into dev Apr 26, 2024
1 check passed
@JoshuaHinman JoshuaHinman deleted the feature/ideations-select branch April 26, 2024 00:32
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