-
Notifications
You must be signed in to change notification settings - Fork 7
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
[LC-339] Boost Parents/Children and Permissions #546
Conversation
🦋 Changeset detectedLatest commit: f4bcfbb The changes in this PR will be included in the next version bump. This PR includes changesets to release 33 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for learncarddocs canceled.
|
✅ Deploy Preview for learn-card-chapi-example canceled.
|
regarding queries + inheritance for can*Children: For this situation:
test2 has permissions like this (Hmm, okay this is the second time I've typed out a comment then ended up agreeing with the current behavior... I'll finish this thought for posterity...) Initially I thought that this should result in test2 also being able to edit the grandChild because the ability to edit the child boost would have been inherited by the grandChild. But now I see that that query set on the parent should be interpreted as "can edit the children that match this query" which makes much more sense than "can edit the children that match this query as well as the whole family tree that branches off of those children" No change necessary 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wowowow beautiful job on such a gnarly feature. All working flawlessly for me and it all feels like it makes sense and is easy to interact with from the consuming side. And all those tests!
Big kudos 👏👏👏
Update that one error message and we good to go! 🚀
Co-authored-by: Kyle <63376352+smurflo2@users.noreply.github.com>
Overview
https://www.loom.com/share/06222d43e0514fcb8907e1332ea9957d
🎟 Relevant Jira Issues
[LC-339] Back-End: Stackable Boosts & Permissions
📚 What is the context and goal of this PR?
To allow for Scouts Troops, we need to add the ability for Boosts to have parents/children, and we
also then need a permissions overhaul to account for those new relationships
🥴 TL; RL:
Adds parent/child boosts, and updates permissions on boosts to be more granular
💡 Feature Breakdown (screenshots & videos encouraged!)
New node:
Role
BoostPermissions
properties:New relationsip for Boosts:
PARENT_OF
New relationsip between Boosts and Profiles:
HAS_ROLE
roleId
New routes
createChildBoost
: like create boost, but takes in a uri and creates a child under that boostmakeBoostParent
: creates a parent/child relationship between two boostsremoveBoostParent
: removes a parent/child relationship between two boostsgetBoostChildren
: paginated endpoint for getting child boosts with an optional number of generations (i.e. grandchildren)countBoostChildren
: count endpoint for getting child boosts with an optional number of generations (i.e. grandchildren)getBoostParents
: paginated endpoint for getting parent boosts with an optional number of generations (i.e. grandparents)countBoostParents
: count endpoint for getting parent boosts with an optional number of generations (i.e. grandparents)getBoostPermissions
: gets the current user'sBoostPermissions
for a given boostgetOtherBoostPermissions
: gets another user'sBoostPermissions
for a given boostupdateBoostPermissions
: updates the current user'sBoostPermissions
for a given boostupdateOtherBoostPermissions
: updates another user'sBoostPermissions
for a given boost🛠 Important tradeoffs made:
There's like a whole bunch of tradeoffs all over the place! Some big ones are performance related.
Storing roles as a separate node does incur a perf overhead, but after doing some profiling, I found
that it is negliglible compared to the benefit we get from having it. There's also a performance tradeoff
with the way that we query for specific permissions. Right now, those are optimized toward a read pattern
where there are not usually a ton of parents/grandparents per boost (what happens is neo4j returns an object
for each boost/parent that includes the child boost's perms in every object, repeating the child boost's
perms for every parent! This can be defeated using
COLLECT
, but that incurs a perf cost even if thereare no parents, so we instead to prefer to take the hit only if there are lots of parents!). We are storing
child permissions as a MongoDB Query (using
sift
under the hood) string, which has a whole bucket ofimplications/tradeoffs, and we also just go ahead and apply that query using javascript instead of in
neo4j, which could potentially have huge performance negatives if we ever add the ability to say view
all children/grandchildren boosts that match a given boosts
canIssueChildren
permission and there aretons of them that don't match it
🔍 Types of Changes
💳 Does This Create Any New Technical Debt? ( If yes, please describe and add JIRA TODOs )
Testing
🔬 How Can Someone QA This?
So there's a lot of implied behavior going on, especially with permissions, and I wrote some pretty
extensive tests to account for that, but there's still some manual testing you can do to like check
all of that and play around with things!
First, spin up the network, easiest way now is to do this:
cd services/learn-card-network docker-compose up --build
(You might have to remove your other docker containers via
docker container rm
)Once that's up start the CLI:
pnpm exec nx start cli --skip-nx-cache
Then try out all of these guys!
📱 🖥 Which devices would you like help testing on?
🧪 Code Coverage
I wrote pretty extensive tests! I tried really hard to make the test titles legible/the source of truth for how permissions work!
Documentation
📜 Gitbook
📊 Storybook
✅ PR Checklist
🚀 Ready to squash-and-merge?: