search

opendatateam / udata

Customizable and skinnable social platform dedicated to open data.
http://udata.readthedocs.org
GNU Affero General Public License v3.0
239 stars 87 forks source link

Add email and since to org members/requests #3038

Closed ThibaudDauce closed 5 months ago

ThibaudDauce commented 5 months ago

Fix https://github.com/datagouv/data.gouv.fr/issues/1356

ThibaudDauce commented 5 months ago

I'm not sure about the approach of having a dedicated endpoint. I think it adds some confusion with members being returned as part of the organization. Could we copy the user_fields approach on email, having it set to None if permissions are missing?

Yes me too, I first try this approach but didn't find a way to get the correct permissions… Because the field lambda is present in the User, but the permissions are linked to the Organization two levels up (Organization has Member, and Member has User). I didn't find a way to get the parent Organization from the email lambda inside User fields…

maudetes commented 5 months ago

the field lambda is present in the User, but the permissions are linked to the Organization two levels up (Organization has Member, and Member has User). I didn't find a way to get the parent Organization from the email lambda inside User fields…

Indeed! I don't know if we can send additional info to Nested fields using kwargs or something? We could also consider showing the email whenever the current_user is admin (or member?) of an organization with user as member. We would not know if we are in the context of this particular organization, but the user would be allowed to see this email nonetheless. What do you think?

ThibaudDauce commented 5 months ago

the field lambda is present in the User, but the permissions are linked to the Organization two levels up (Organization has Member, and Member has User). I didn't find a way to get the parent Organization from the email lambda inside User fields…

Indeed! I don't know if we can send additional info to Nested fields using kwargs or something? We could also consider showing the email whenever the current_user is admin (or member?) of an organization with user as member. We would not know if we are in the context of this particular organization, but the user would be allowed to see this email nonetheless. What do you think?

I thought about it, but I think the query to fetch this information could be complex (and run each time we use user_fields?). I also thought about trying to get the organization from the Flask request but it seems a little bit too magic and complex to put in user_fields. So I decided to create a new endpoint, in my opinion it's simpler and clearer than having too much logic inside fields but If you want I can try to code one of the other solution to see the difference and the complexity?

maudetes commented 5 months ago

Thank you for the explanation. I think exploring the two alternatives would be interesting. Maybe something like Organization.objects(members__user__all=[current_user, target_user]) may work. I agree though about performance issues that may arise -> we may want to create an index on organization > members > users. Fetching the organization may be magic indeed but would be quite efficient if somehow not so complex.

ThibaudDauce commented 5 months ago

Thank you for the explanation. I think exploring the two alternatives would be interesting. Maybe something like Organization.objects(members__user__all=[current_user, target_user]) may work. I agree though about performance issues that may arise -> we may want to create an index on organization > members > users. Fetching the organization may be magic indeed but would be quite efficient if somehow not so complex.

Thanks for persisting, the current solution is much cleaner without the extra endpoint (and it's really easy to check for org permission in field)