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

BFF Utility for Organization Management #937

Merged
merged 10 commits into from
Dec 12, 2022
Merged

BFF Utility for Organization Management #937

merged 10 commits into from
Dec 12, 2022

Conversation

bbengfort
Copy link
Collaborator

@bbengfort bbengfort commented Dec 6, 2022

Scope of changes

Adds a bffutil CLI program that will help us manually manage organizations in order to get CipherTrace TSP users access to old/previous organizations and for us to manage all organizations in the BFF.

TODO:

  • add TSP collaborator to organization
  • deal with leveldb namespace issue

Fixes SC-11778

Type of change

  • bug fix
  • new feature
  • documentation
  • other (describe)

Acceptance criteria

  1. In the organization report are we returning enough data?
  2. Does the Organization Store interface edits make sense?
  3. How do we want to deal with the leveldb store namespace issue?
  4. Does the add TSP collaborator to organization workflow make sense?
  5. Have we added the bffutil command to the bff Dockerfile?
  6. Do we have a way to manage the bffutil on the pod?

Author checklist

  • I have manually tested the change and/or added automation in the form of unit tests or integration tests
  • I have updated the dependencies list
  • I have recompiled and included new protocol buffers to reflect changes I made
  • I have added new test fixtures as needed to support added tests
  • Check this box if a reviewer can merge this pull request after approval (leave it unchecked if you want to do it yourself)
  • I have moved the associated Shortcut story to "Ready for Review"

Reviewer(s) checklist

  • Any new user-facing content that has been added for this PR has been QA'ed to ensure correct grammar, spelling, and understandability.
  • I've reviewed the data the organization report command returns
  • The edits to the OrganizationStore interface are reviewed
  • We've created stories or resolved the leveldb namespace issue
  • I've reviewed the add TSP collaborator to organization workflow
  • I've reviewed the bffutil deployment

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2022

Codecov Report

Base: 43.99% // Head: 43.93% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (1405a9c) compared to base (e5430b1).
Patch coverage: 33.33% of modified lines in pull request are covered.

❗ Current head 1405a9c differs from pull request most recent head 0d2f0a0. Consider uploading reports for the commit 0d2f0a0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #937      +/-   ##
==========================================
- Coverage   43.99%   43.93%   -0.07%     
==========================================
  Files         688      688              
  Lines       25573    25609      +36     
  Branches     1562     1565       +3     
==========================================
- Hits        11252    11251       -1     
- Misses      12874    12911      +37     
  Partials     1447     1447              
Flag Coverage Δ
unittests 43.93% <33.33%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...i/src/components/AddNewVaspForm/AddNewVaspForm.tsx 100.00% <ø> (ø)
...b/gds-user-ui/src/components/UserProfile/index.tsx 30.00% <ø> (ø)
web/gds-user-ui/src/utils/axios.ts 16.92% <11.11%> (+0.52%) ⬆️
...user-ui/src/components/UserProfile/UserDetails.tsx 100.00% <100.00%> (ø)
...risacrypto/directory/pkg/store/leveldb/iterator.go 41.57% <0.00%> (-7.12%) ⬇️
...m/trisacrypto/directory/pkg/store/trtl/iterator.go 77.31% <0.00%> (-3.75%) ⬇️
...gds-user-ui/src/components/Collaborators/index.tsx 13.88% <0.00%> (-1.91%) ⬇️
web/gds-admin-ui/src/utils/index.js 46.15% <0.00%> (-0.70%) ⬇️
...b.com/trisacrypto/directory/pkg/store/trtl/trtl.go 62.56% <0.00%> (-0.44%) ⬇️
...trisacrypto/directory/pkg/store/leveldb/leveldb.go 45.44% <0.00%> (-0.35%) ⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bbengfort bbengfort requested a review from pdeziel December 8, 2022 18:40
@bbengfort bbengfort marked this pull request as ready for review December 8, 2022 18:40
Copy link
Collaborator

@pdeziel pdeziel left a comment

Choose a reason for hiding this comment

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

This looks good and thanks for doing the extra GDS work to get the command working. I just had a few questions about some edge cases in addCollab before I mark it as approved.

cmd/bffutil/build.sh Show resolved Hide resolved
cmd/bffutil/main.go Show resolved Hide resolved
cmd/bffutil/main.go Show resolved Hide resolved
cmd/bffutil/main.go Show resolved Hide resolved
if err = db.UpdateOrganization(curOrg); err != nil {
return cli.Exit(fmt.Errorf("could not update user's current organization: %w", err), 1)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to make sure the user does not maintain their role if they are a leader in their "default" organization and they are moved to an organization where they shouldn't be a leader?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Managing roles is a little annoying -- in the first pass at this, the manual trigger will be used for users that have the TSP role, so changing the role isn't too important, and if it is we can do it in auth0. I'm hopeful that the collaborators management feature will come online before we have to really use bffutil in earnest -- but we could create a future story for roles if we think we need it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me, just wanted to make sure we thought of that; since we are primarily using this for TSPs I'm 100% okay with not attempting to change roles.

Copy link
Collaborator

@pdeziel pdeziel left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, the logic looks good to me now.

@bbengfort bbengfort merged commit a156e20 into main Dec 12, 2022
@bbengfort bbengfort deleted the sc-11778/bffutil branch December 12, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants