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

Feat: Enable editing collaborating organizations #1116

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

JoshKornfeld
Copy link
Contributor

@JoshKornfeld JoshKornfeld commented Nov 4, 2022

Description

Closes #1112
PR includes formatting. Specific file changes will be mentioned here.

Backend: Modified editing API to include collaborating orgs. project_views.py
Frontend:

  • Modified project_texts.js by adding some extra strings
  • New component responsible for editing collaborating orgs EditCollaboratingOrganizations.js. This component utilizes the AutoCompleteSearchBar to add any existing organization as a collaborator and MiniOrganizationPreviews to display the existing collaborating organization underneath.
  • New component used in EditProjectContent.js alongside supporting functionality responsible for adding/removing orgs.
  • MiniOrganizationPreviews.js props modified to show a gray border around the mini org previews.
  • ProjectMetaData.js makes use of the new prop to show a gray border conditionally.
  • Removed comment in Post.js

New Interface for editing collaborating orgs as follows:
Screenshot from 2022-11-04 17-59-51

Test plan

  1. Sign in
  2. Go to a project for which you have editing rights
  3. Edit the project via the searchbar and save to see results

Before landing

  1. PR has meaningful title
  2. yarn lint passes (frontend)
  3. yarn format passes (frontend)
  4. make format passes (backend)

Copy link
Contributor

@piperchester piperchester left a comment

Choose a reason for hiding this comment

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

Nice work @JoshKornfeld! Leaving some flyby comments.

frontend/src/components/communication/Post.js Outdated Show resolved Hide resolved
@@ -72,15 +73,16 @@ export default function MiniOrganizationPreview({
size={size}
onDelete={onDelete}
doNotShowName={doNotShowName}
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope for this PR, but do you know why we have a mix of the negative like doNotShow and positive like show prefixes? I think from a component API perspective we should align on one pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had defined doNotShowName as a negative because there was only 1 instance of MiniOrganizationPreview where I didn't want the name to be shown and I didn't want to modify the props of the other instances everywhere else.

Copy link
Contributor

@ddhanesha ddhanesha left a comment

Choose a reason for hiding this comment

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

The feature requirements are not clearly identified in the original #1112 . @JoshKornfeld I would recommend that you consult with @positiveimpact and @tobiasrehm about requirements. I do not think I can approve this PR without clear requirements.

backend/organization/views/project_views.py Outdated Show resolved Hide resolved

return (
<>
{/* TODO: Currently allows any org to be added, maybe needs some verification on selected orgs part*/}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is very important feature to have. I don't think we should allow any organization to be added. Lets make sure that feature requirements are clarified before we move forward merging this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should maybe look to create an issue for this as its own PR, with also making this check for project creation?

@ddhanesha
Copy link
Contributor

@JoshKornfeld please resolve merge conflicts

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.

Feat: Add collaborating orgs to editing projects
3 participants