"Group creator must complete their membership" Wizard duplicates membership

Here's my initial attempt to convert this patch to work with Groups 3.x

Please provide a test first that proves this is broken. It can be as simple as adding someone to a group under the circumstances reported here and counting that there are 2 memberships rather than one. Then we can verify that the patch fixes it.

The patch does not work in Group-2.x as it references a non-existing entity. Probably just a small mistake, but "$entity->" should be "$this->entity->" in a couple of places. Updated patch, which fixes the duplicate membership issue attached.

Note that my application has a form-alter function that processes the wizard two-step form, altering what is displayed and adding a custom validation function.

Sorry: I have not created the requested tests :(

Let's already fix this issue.

I have added tests. Previously tests just checked the UI, but also added the check that a user is added or not depending on the settings of group type.
We can easily port these tests to group 2 and group 1.

Given the work done in #32, removing "Needs Tests" tag. For Review.

Since we are now adjusting this work on 3.0.x (from #29-on), changing version tag.

Okay so a colleague and I had a look at this and we found multiple bugs, one of which leads to corrupted data in the database so bumping this issue to critical.

Group creator membership gets created twice, once during Group::postSave() and once at the end of the form wizard.

This is the critical bug because it adds two entries in the database where the codebase enforces and assumes that there is only one. The first entry comes from postSave() and contains the creator roles, the second one contains no roles but has the fields that were added to the membership. Which leads to bug 2:

The creator wizard does not set the creator roles

This flew under the radar until now because postSave() sort of cleaned up the mess that the form wizard made. Once we fix bug 1 to only save in postSave() if the wizard was not used, this bug will rear its ugly head, so we need to fix that in one go.

Views show multiple nodes and other grouped entities twice

This is because Group v2/3 now join the membership data to run query access checks, making the double memberships double the amount of results Views returns (because of the joins). This problem will go away when we fix the first bug.

Approach we will take

It's important that we first plug the hole, so we will adapt the tests here and add fixes to the above bugs. That will stop people from further running into this bug for newly created groups.

For existing groups with corrupted creator memberships, we will provide queries you can run to establish how much damage was done to your installation. We could write an update hook that takes the roles from the first and the fields from the second membership and merge that into one, but the fact is that we cannot safely assume everyone left their duplicate memberships untouched. If people manually tried to rectify the situation, then we don't know which field values we should keep and which we should discard.

It was therefore decided that we will provide steps to remedy the situation via the UI or via code in the release notes, but stop short of writing an update hook to avoid us messing up people's data integrity even more while trying to fix things.

@kristiaanvandeneynde are there any plans to fix that on groups 2.0 as well?

2 and 3 are the same aside from machine names, so yes

kristiaanvandeneynde → closed merge request !103

Hiding patch as we're going to clean this up in a MR.

Just realized the current approach breaks BC when creating groups through code. It breaks it for a bad use case, but it's a BC break nonetheless. So we need to adjust the MR a bit.

Previous behavior for creator_membership / creator_wizard values:

• FALSE / FALSE: Group creation does nothing
• TRUE / FALSE: Group creation creates membership
• FALSE / TRUE: Incompatible
• TRUE / TRUE: Two memberships (bug) get created, one by wizard one by postSave()

New behavior for creator_membership / creator_wizard values:

• FALSE / FALSE: Group creation does nothing
• TRUE / FALSE: Group creation creates membership
• FALSE / TRUE: Incompatible
• TRUE / TRUE: Zero memberships get created, none by wizard none by postSave()

While it makes no sense to enable the wizard because you want complete membership data and then rely on the incomplete membership created by postSave(), people have code that either does exactly that or loads the incomplete membership to complete its data. This code cannot break in a minor or patch release.

Therefore, I suggest that instead of checking for creator_wizard in postSave(), we instead call getMember() and check that that returns FALSE. This also fixes the bug, but in a BC way. We should immediately add a todo pointing to a follow-up issue where we take care of this confusing creator_membership / creator_wizard once and for all and perhaps even make the automatic membership form-only.

If we allow Group::postSave() to create a membership, then that always comes before the one from the wizard and thus we end up with two memberships again. So the BC fix reintroduces the bug.

Therefore the conclusion is that we cannot know if the wizard was being used without adding some unsaveable data to the group entity. This means that we cannot fix this and keep BC. I am therefore inclined to allow the BC break here and document this in the release notes and a change record.

Reverting the last patch.

Thank you, @kristiaanvandeneynde!

Your updates in the fork are working great for me! After applying the patch I no longer see duplicates in lists of entities for groups.

Fixed, will tag a new release. Thanks all!

Automatically closed - issue fixed for 2 weeks with no activity.