"Group creator must complete their membership" Wizard duplicates membership

Confirming this issue.

Note: this behaviour does not occur when you untick "Group creator must complete their membership".

It happens, because we call addMember method, which adds user, when we want to add a user as an admin of the group.
It works well, when we don't use wizard, but with it we save membership twice on the group level and then on the group content form
I still can't think of solution, but I will try to find it. If you have any ideas how to avoid it let me know, I will present a patch

1) I have added on Group entity, if the wizard setting and don't create a member, if it is active, because it would be created through the form
2) On GroupContentForm I add roles to membership.

Set creator_membership to false in the Group Content form. Like this member will not be added in a Group entity and the tests will work the same way

The patch in #8 causes a fatal error:
Error: Call to undefined method Drupal\node\Entity\Node::getGroupType() in Drupal\group\Entity\Form\GroupContentForm->complete() (line 190 of /var/www/html/drupal/docroot/modules/contrib/group/src/Entity/Form/GroupContentForm.php

We need a check that it is the group creation wizzard. It should be fixed

What happens if you adjust Group::postSave()?

  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    parent::postSave($storage, $update);

    // If a new group is created and the group type is configured to grant group
    // creators a membership by default, add the creator as a member.
    // @todo Deprecate in 8.x-2.x in favor of a form-only approach. API-created
    //   groups should not get this functionality because it may create
    //   incomplete group memberships.
    $group_type = $this->getGroupType();
    if ($update === FALSE && $group_type->creatorGetsMembership()) {
      $values = ['group_roles' => $group_type->getCreatorRoleIds()];
      $this->addMember($this->getOwner(), $values);
    }
  }

to:

    if ($update === FALSE && $group_type->creatorGetsMembership() && !$group_type->creatorMustCompleteMembership()) {

That should also prevent the initial membership from being added, leading to only the one from the form being added, right? And it would also fix the @todo in one go. But we might want to leave that note regardless as a reminder for 8.2.x.

Nice! I have checked it works! The patch is provided

The patch worked 8.x-1.0-rc4. Thanks!

I can confirm that patch in #12 fixes the issue.

An update on this: While using patch #12 I found if a group is created via Api then it does not add any member at all to the group. It solves the duplicate issue while adding the member via 2 step wizard.
So I tried the patch in #10 , which seemed to work fine in both scenarios and not creating any duplicates

@u_tiwari Yes, you are right. It is a regression. So, in this case I would stick with my previous patch (#12). I couldn't find the better way to pass information from form to postSave

@lobsterr , you mean sticking to number #10 right, as #12 does not add any members at all while using an api to create the group lets say json api or rest

@u_tiwari I will check this. It should work theoretically, but maybe I miss something

Ok, I have checked from rest API , from ui. We will stick with #10 (https://www.drupal.org/project/group/issues/3016197#comment-13175648)

Also It has been tested by the community and it takes into account different ways of creating group

I've used the patch in #12 and am using the latest rc2 version and it still creates a duplicate for me.

#10 works like magic. Thank you.

+1 for patch #10, test with 1.0-rc5. Set as RTBC.

#10 work for me.

Tests are failing, probably because the default state of group types is:

  /**
   * The group creator automatically receives a membership.
   *
   * @var bool
   */
  protected $creator_membership = TRUE;

  /**
   * The group creator must immediately complete their membership.
   *
   * @var bool
   */
  protected $creator_wizard = TRUE;

And unless you set the 2nd to FALSE in those tests, they expect a membership to be created but now have none.

I'll drop here an issue we found when using patch #10.
Not sure if the problem is with the patch, or if the patch just raised another different problem in group_flex module. What I'm sure is that the error just happens when using this patch, so I leave here the information. If the patch gets merged as is, I hope a follow up can be created here or (probably) in group_flex.

Steps:

1. Enable group_flex module
2. Create a group type named 'Flexible group type' checking the options:
   - The group creator automatically becomes a member
   - Group creator must complete their membership
   - Enable group flex
   - Group type visibility: Let owner decide
   - Group joining methods: Select both 'Call to action (join button)' and 'Request to join' and check 'Group owner can select the joining method(s) for their group'
3. Add 'Content editor' role the permissions: 'Flexible group type: Create new group' + 'Access the Group overview page' + 'Administer group settings'
4. Create a user that has only 'Content editor' role and log in
5. Add a group using 'Flexible group type', set visibility to 'Private'

I see this error:

Drupal\Core\Entity\EntityStorageException: Group permissions are not saved correctly. in Drupal\group_flex\GroupFlexGroupSaver->saveGroupJoiningMethods() (line 165 of group_flex/src/GroupFlexGroupSaver.php).

Again, I'm just leaving this here as a to do (somewhere) if the patch #10 gets merged. It's not a blocker.
Sorry that I don't have the time to further investigate and clarify this.
Anyway, I think it's worth to share.

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 I also added the check that a user is added or not, depending on the settings of a 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

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: One membership gets created, none 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!