amitaibu / og

A fork to work on OG8
https://github.com/Gizra/og
29 stars 16 forks source link

Remove audience fields from users #265

Closed amitaibu closed 8 years ago

amitaibu commented 8 years ago

I'm still chewing on this idea, but here are some initial thoughts:

OG7

We already make a better distinction, and a user is a special case and OgMembership applies only to it. This already makes the API more predictable. Developers will still use good old entity reference for associating group content (i.e. non-user entities) to groups.

However, we currently still have this mix in users, where you could use OgMembership directly or work via the user's audience field a la Entity-reference.

However this duplication adds a lot of code to OG. And I'm not sure that having field API dictate the logic is still the right way. Furthermore, controlling the business logic of how many/ which type memberships can be created for a user, can be achieved without having the logic inside the field API.

Summary

Completely remove the Audience fields for users. It will remain only for a group content, as a special case of an Entity reference field -- just so we could easily find content that is related to group.

For users, our API will only deal directly with the OgMembership entity. We could fairly easily add a UI that will allow group admins to add more groups to a user, without opting to field API.

I believe this change will result with:

amitaibu commented 8 years ago

Update:

wip__remove_audience_field_from_users_by_amitaibu_ _pull_request__265_ _amitaibu_og

amitaibu commented 8 years ago

Tests are passing. Now it's time to think If there are strong points not to go ahead with the proposed changes.

amitaibu commented 8 years ago

@pfrenssen @RoySegall ^^

amitaibu commented 8 years ago

@damiankloip some of your awesome code will be removed by this one. But it is for a greater cause πŸ˜„

damiankloip commented 8 years ago

@amitaibu Fine by me, you can martyr my code :) However, wont we need to start adding lots more code back to allow people to administer these memberships via the UI? Or all we really lost is viewing the memberships easily on actual users?

alesrebec commented 8 years ago

In 95% of all the cases I'm in for cleaner code and predictable API.. this is one of those cases... so.. +1

RoySegall commented 8 years ago

So let me get this straight - there is no special use case for users or node. We only have a normal entity reference field and the membership still exists to see the user status in the group?

amitaibu commented 8 years ago

We only have a normal entity reference field and the membership still exists to see the user status in the group

Exactly.

However, wont we need to start adding lots more code back to allow people to administer these memberships via the UI?

Not more then we currently have, and defiantly less complicated. To show the existing memberships, we can use Views. To allow admin to add users to different groups they could:

RoySegall commented 8 years ago

I think it's an AWESOME idea. This will also make OG more understandable by having a field as a field and not some weird astral projection of field.

damiankloip commented 8 years ago

To me it sounds like something the field already gives you.. Isn't it way more customisable when people can just move a field widget around (especially with field group etc..). The field is just a wrapper for administering a users memberships. A new form will have to re implement something similar (autocomplete of groups etc..)? People will then probably end up making their own widget again in some cases maybe.

I guess this is going to happen anyway, but I am edging to -1. I don't care that I did it originally. I just don't see the gains from removing this. I don't currently use this with the OG stuff we are doing but it doesn't affect my work in any way. I just create my own memberships. I can then use the widget if I like.

amitaibu commented 8 years ago

@damiankloip Let me try to pull your finger away from the -1 πŸ˜‰ :

The inital idea of having this OgMembership as field storage was coming from the idea of lets devs work always with the field API, without needing to worry which entity they are on. But ever since the change we did that OgMembership is only for users, I've started realizing that it's ok to special case users Vs group content.

I used to think that we got the UI "for free" by implementing this field storage, however we can see in this PR "for free" does come with a certain price.

Also, worth noting that the audience widget on the user page was hidden from regular users, so we are in fact "just" talking about admins. In D7 they could manage the user's group it in two places:

  1. The group's people page, where they have added members and were able to give the full meta data about them (state, role, or any other field attached to the OgMembership like the "Request message" a user normally would fill when requesting membership ). people_in_group_a1___site-install
  2. Directly via the user's field. However, when that user was added through the field, they didn't control all the meta data.

So we the proposed change, we will have to write a form (which is actually an OgMembership create form) similar to the above image, but that also allows selecting a group.

amitaibu commented 8 years ago

For posterity, here's an example of a case where having the field cardinality would have been nice - when we want to limit the amount of groups a user can be subscribed to.

However, I think in most cases there will not be such a restriction, and even if there was it should be in contrib land. For example, looking at a similar scenario, to limit the amount of nodes a user can do, can be done with custom code.

It will be up to us to build in this case the formatter in an extendable way, so implementors can add their buisness logic with ease.

amitaibu commented 8 years ago

In a two days perspective of this idea, I think it's the right way forward.

Adding a few more people with OG experience to get some more feedback, or points that I might be missing.

@pfrenssen @mike-potter @rbran100

Also, going to re-roll πŸ˜‰

mike-potter commented 8 years ago

I think this makes sense. I always felt that the group_audience field was just "odd" because it had that different OgMembership storage. Never saw much benefit coming from the Field API and it just caused more confusion.

From a UI perspective, rather than just having a multivalue entity-reference on the User's edit page for all their groups, it would be much nicer to have a separate page for users to un-subscribe from their groups, or to subscribe to new groups. We've always added users directly from the group itself, but I could see a benefit from a user-focused UI showing all their groups to manage.

But that idea still doesn't need the Field API, so getting rid of it is fine.

amitaibu commented 8 years ago

it would be much nicer to have a separate page for users to un-subscribe from their groups, or to subscribe to new groups

Indeed, that's what I'm thinking of. I think that OG should come with a "serving suggestion" on how such a page might look, and it can be extended by other implementing modules.

Be warned, though, @mike-potter that I'm going to ping you for help and ideas for those page 😜

amitaibu commented 8 years ago

Alright, not too much feedback (3 positives and one negative(ish)). After 4 more days of thinking of it, I still think we should go ahead with this change. Cross your fingers :)