-
Notifications
You must be signed in to change notification settings - Fork 12
Organization collective #530
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
Conversation
allanlasser
left a comment
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.
Yep, we decided to not include automatic verification at this point. However, this decision was made in the context of scoping the changes we need to apply. Since you've already added this in code, I don't think it's a problem to keep it, since this meets a requirement we previously established as useful, but optional for a first pass.
| {% load autologin %} | ||
|
|
||
| {% block body %} | ||
| <p>TK</p> |
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 can contribute email copy for this situation, but it'd be helpful to know the context. When is this sent, to whom, and why?
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.
This email will be sent to the organization admins (of the Parent/Group org) when a Child or Member organization requests to join a Parent or Group organization.
| admins = serializers.SerializerMethodField() | ||
| parent = serializers.SlugRelatedField(read_only=True, slug_field="uuid") | ||
| groups = serializers.SlugRelatedField(read_only=True, slug_field="uuid", many=True) | ||
| share_resources = serializers.BooleanField(read_only=True) |
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.
Making sure I understand this correctly:
- If I belong to an org, that org will be included in my user data payload.
- Since my org is a member of a higher org,
share_resourceswill befalseandgroupswill have the UUID of the org it's a member of. - When I fetch that
groupsorg by UUID, it will haveshare_resources = true
So, as a client I'd need to fetch and check all entities in groups to see whether resource sharing is enabled?
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.
This is correct
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.
In that case, what do you think about expanding the parent and groups values to include share_resources? At the very least, this would eliminate the need for clients to make blind network requests for organizations just to know whether or not they have resources available to share.
For example, let's imagine I'm building a small client service and I want to offer some kind of shared group resource. As is, I'd need to make a request for every org in both sets in order to determine the shared resources available—some of those requests would be unnecessary. If I at least know if a parent or group is sharing resources, I can make a more targeted and efficient set of requests for just the organizations that share resources.
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.
Mitch and I talked about taking an approach where we serialize and send all data about parents and group organizations to clients, so that they can update all their records just by receiving the webhook.
b1454d1 to
4ffcab4
Compare
|
It would be nice if I could add an invitation for an org to join the parent org as a child via the backend for testing purposes. Running from the django shell, I sent an invite from MuckRock (parent) to MuckRock Child Organization to join the organization. I received an email that only said "Invitation to join MuckRock" with a body that said "TK". I can see that the invite was successfully created in the django backend, but it also gives me no way to actually approve it the backend. I seemed not able to send an invite from MuckRock Child Org as the sender to the parent to indicate the child <-> parent relationship. I can imagine situations where this may seem useful, similar to a user requesting to join their organization. It may not be in the scope of this PR. Again going from the backend, I accepted the earlier invitation. It correctly changed the parent of MuckRock Child to MuckRock and added MuckRock child to the list of children for MuckRock. Rejecting a request similarly worked. There is a broken relationship however, in that MuckRock can both be the parent and child to MuckRock Child Organization. This should be resolved. We should enforce that a parent of a child org cannot also be the child of said org |
Thanks - the invitations do not have a UI yet. I added them first and then realized resource sharing was the top priority. We will be setting up the relationships manually for now, so we ensuring parent relationships are not circular can be done later. |
|
Another related PR that needs to be merged in: |
The actual resource sharing happens on the client side
Clients need access to organization relationships (parents and groups) along with
share_resourcesto properly implement sharing resources. These fields are added to the APIDid we decide that there would be no automatic verification sharing? If so we can remove that method.