-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Orgs own projects #865
Conversation
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. |
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.
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.
backend/LexBoxApi/GraphQL/CustomTypes/OrgProjectsGqlConfiguration.cs
Outdated
Show resolved
Hide resolved
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.
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/CustomTypes/OrgProjectsGqlConfiguration.cs
Outdated
Show resolved
Hide resolved
a6dd490
to
f47d4df
Compare
Rebased on top of current |
Rebasing on top of |
374eb9b
to
634ae02
Compare
@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. |
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.
2 small requests 🙂
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.
Rebasing on top of |
e00415a
to
c716b2e
Compare
They need to have Org in the name.
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. |
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 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. |
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.
A permission change and I think you intended to push a small UI change?
Removing projects follows same permissions as adding them.
@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). |
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.
Cool beans 😎
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.
I'm fine with Tim's approval
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: