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

Orgs own projects #865

Merged
merged 18 commits into from
Jun 20, 2024
Merged

Orgs own projects #865

merged 18 commits into from
Jun 20, 2024

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Jun 6, 2024

Part of #788, but should not close it yet as this PR is backend-only (well, almost, see below).

This PR contains backend changes for having orgs own projects. As suggested in #788, the DB relation is a many-to-many relationship so that a project can be owned by multiple orgs.

Frontend changes are limited to having the org page now show a list of owned projects. Buttons to add projcts to an org, or remove them, will go in a separate PR as planned.

Screenshot of projects now showing on org page:

image

Copy link

github-actions bot commented Jun 6, 2024

UI unit Tests

11 tests  ±0   11 ✅ ±0   0s ⏱️ ±0s
 3 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit c7f2bb3. ± Comparison against base commit f802d74.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jun 6, 2024

C# Unit Tests

52 tests  ±0   52 ✅ ±0   5s ⏱️ ±0s
10 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit c7f2bb3. ± Comparison against base commit f802d74.

♻️ This comment has been updated with latest results.

@rmunn
Copy link
Contributor Author

rmunn commented Jun 6, 2024

See #788 (comment) for discussion of the permissions around who can add projects to an org (or remove them). Should it be org admins, project managers, or both, who are allowed to do that? (By "both" I mean people who are either an org admin or a project manager, not exclusively people who fill both roles at once).

Please discuss in #788 rather than here, so the discussion will have slightly greater visibility.

@rmunn rmunn marked this pull request as ready for review June 6, 2024 09:45
Copy link
Contributor Author

@rmunn rmunn left a comment

Choose a reason for hiding this comment

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

I'd welcome comment on the AcquireProject and ReleaseProject names for org mutations, as well as thoughts on whether those mutations should live in ProjectMutations instead. I tend to think of the organization as performing the action, while the project is the passive object being acted upon, but if you would prefer otherwise, please let me know.

I also have specific comments/questions about several bits of code; see below.

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

great work Robin! I'd actually be fine with leaving this as a backend only PR to keep it from getting too big.

Not sure what's going on with that index situation, I wonder if it's an EF bug?

backend/LexBoxApi/GraphQL/OrgMutations.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/GraphQL/OrgMutations.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/GraphQL/OrgMutations.cs Show resolved Hide resolved
backend/LexData/SeedingData.cs Show resolved Hide resolved
@rmunn
Copy link
Contributor Author

rmunn commented Jun 10, 2024

Rebased on top of current develop to fix merge conflicts.

@rmunn rmunn self-assigned this Jun 10, 2024
@rmunn
Copy link
Contributor Author

rmunn commented Jun 11, 2024

Rebasing on top of develop again to pull in the fix for #871.

@rmunn rmunn force-pushed the feat/orgs-own-projects branch 2 times, most recently from 374eb9b to 634ae02 Compare June 11, 2024 08:38
@rmunn
Copy link
Contributor Author

rmunn commented Jun 11, 2024

@hahn-kev - I've done a self-review which resulted in a couple of minor code-quality tweaks (634ae02) and one bugfix (e00415a, we were requiring project-manager status to add a project to an org but not requiring the same to remove projects from orgs; now the permissions are consistent between both operations). I don't see any other outstanding issues. Ready for re-review.

Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

2 small requests 🙂

backend/LexBoxApi/GraphQL/OrgMutations.cs Show resolved Hide resolved
backend/LexBoxApi/GraphQL/OrgMutations.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/GraphQL/OrgMutations.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/GraphQL/OrgMutations.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/GraphQL/OrgMutations.cs Outdated Show resolved Hide resolved
rmunn and others added 13 commits June 14, 2024 12:51
No DB migrations yet; those will go in their own commit.
Projects can be created with an optional owning org, and already-created
projects can be acquired and released by an org.
This proves that it works, and orgs can acquire and release projects.
Only users who are an org admin *and* a project manager can add that
project to that org.
The elawa project will remain unowned, so that we can test that orgs
with no projects acquiring their first project (e.g., second test org
acquiring elawa) work just as well.
This PR will be backend-only, so we don't need this frontend test code.
This was done by editing the migration by hand, which is only a good
idea if it has not yet been applied. But since running `skaffold delete`
will delete the DB and let it be re-created, this is not a major problem
since this PR is still in progress and has not yet been merged.
If only project managers are allowed to assign projects to orgs, then
only project managers should be allowed to remove projects from orgs.
@rmunn
Copy link
Contributor Author

rmunn commented Jun 14, 2024

Rebasing on top of develop now that org page has been merged, then I'll start addressing review comments.

@rmunn
Copy link
Contributor Author

rmunn commented Jun 14, 2024

All review comments addressed, but this is not ready for a new review yet: I'm going to push a bit more code so that the org page's projects list actually does something.

@rmunn
Copy link
Contributor Author

rmunn commented Jun 14, 2024

Although this PR was intended to be backend-only, I decided that since the org page is merged now, I might as well have it show the list of projects the org owns.

Implementing UI to add projects to an org (and deciding where to put it) can still be put into a separate PR as originally planned.

Ready for final review and merge, unless we decide to change the plan and expand the scope of this PR to include further frontend changes.

@rmunn rmunn requested a review from myieye June 14, 2024 06:45
@myieye
Copy link
Contributor

myieye commented Jun 14, 2024

@rmunn did you maybe forgot to push a commit? It sounds like you've already made the change to display projects on the org page, but the change isn't in this PR.

Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

A permission change and I think you intended to push a small UI change?

backend/LexBoxApi/GraphQL/OrgMutations.cs Outdated Show resolved Hide resolved
@rmunn rmunn requested a review from myieye June 17, 2024 03:23
@rmunn
Copy link
Contributor Author

rmunn commented Jun 18, 2024

@myieye I know you're busy getting ready for your trip, so if you want I can just dismiss your old review and ask Chris to review this. I'd like to get this merged so that I can make progress on #888 (where I need orgs to own projects so I can start implementing the "confidential projects are hidden on the org page" logic).

Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

Cool beans 😎

@rmunn rmunn mentioned this pull request Jun 18, 2024
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

I'm fine with Tim's approval

@rmunn rmunn merged commit 1228906 into develop Jun 20, 2024
14 checks passed
@rmunn rmunn mentioned this pull request Jun 20, 2024
2 tasks
@myieye myieye deleted the feat/orgs-own-projects branch June 21, 2024 06:45
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