Add a status to the group (Publish/Unpublish)

I did a completely new patch, because the previous one was not following the right approach and did not pass the test.

I started to work on this because it is needed for #2829966: Support for Revisions on groups.

The patch looks good to me overall, but I didn't look in depth at the access control handler changes so I'll leave the final review/RTBC for a maintainer of the module.

Looks pretty OK to me as well, I am wondering what exactly the meaning of the published flag for groups means for their related content.

We did implement our own status in our own project, where the group status does not affect really affect itself (they are all internal anyway) but actually makes the nodes of the group accessible or not.

I guess it is fine to limit to just to the group itself for now, but I assume it might cause some confusion/misunderstanding of how this works.

Hi @amateescu and @Berdir,

Added an access test.
Regarding the related content access I think the best way is first add support to make groups publish/unpublish and then maybe we can add a feature to limit group content access based on that.
Can you please help me reviewing the other child issues of #2829966: Support for Revisions on groups.

I based the access test on Taxonomy test and I needed to change some comments I forgot in previous patch.

It looks very good, I have tested all presented cases and couldn't find any issues

It would be great if this got into the Group module. We're looking to apply this patch to the Open Social distribution because we have a real need for it (in courses using the social_course module but also because some users want to be able to "stage" groups before sharing them with the platform).

We have a PR open here https://github.com/goalgorilla/open_social/pull/1319 to start using this code already, so we can remove some of our custom solutions (that replicate a large part of this but aren't able to alter the Group entity's interface).

Just wanted to chime in and say we're heavily depending on this patch to satisfy the need to "stage" groups as well. We will likely be working on some additional support of Workflow transitions and the like as well to satisfy the client's editorial requirements for groups and group_content.

Patch works pretty well, but note that you can still end up showing unpublished groups to end users if you have a view of groups. The easy workaround is to just ensure you filter out unpublished content in your view.

Patch looks good, will try and get a proper review in next week and add it to RC4.

If the change is too drastic, it may need to wait for 8.1.0 as a feature update. But given how it's fully BC, we should be fine. What would ease my mind given the recent RC3 update fiasco and help me commit this faster is an update test proving nothing breaks during the update hook.

a heads up that group_update_8016 in #9 is also declared by https://www.drupal.org/files/issues/2019-07-04/group-3061321-37.patch in https://www.drupal.org/project/group/issues/3061321#37

Rerolled patch against 8.x-1.x

Missed a use statement

How would one published/unpublish a group programmatically? Can we use the same method as nodes?

$group->setPublished(FALSE);
$group->setPublished(TRUE);

@brooke_heaton ther is EntityPublishedTrait used, it provides

setPublsihed 

and

isPublished

methods to check and set status

Patch #17 appears to no longer apply clean. :(

@brooke_heaton I've just applied the patch to the latest dev version everything is applied correctly. No issues. Can you try one more time on the clean version. Maybe you have additional patches ?

I think this patch has broken with 1.x-rc-5. Composer no longer wants to install it. Can this be rerolled? I'd like to avoid going to a dev branch of the module if at all possible.

Hi!

I can say that patch #17 works with the 1.0.0-rc5 and adds the 'published' checkbox in all the groups you have created so far.
By disabling it, access to the group page is forbidden (default settings, havent played yet with the permissions).

Patch no longer applies to latest 8.x-1.x:

$ wget https://www.drupal.org/files/issues/2019-11-19/group-status-2873212-17.patch
$ git apply group-status-2873212-17.patch
error: patch failed: group.install:453
error: group.install: patch does not apply
error: patch failed: src/Entity/Group.php:10
error: src/Entity/Group.php: patch does not apply

Automated re-roll of #17 on latest 8.x-1.x .

I applied #17 cleanly to 8.x-1.0-rc5.
Then committed into a branch, then rebased the branch on 8.x-1.x there were two small conflicts which I resolved manually. Therefore an interdiff is not really necessary or practical.

rebase in progress; onto bb5a855
You are currently rebasing branch '2873212-17-group-status' on 'bb5a855'.
  (fix conflicts and then run "git rebase --continue")
  (use "git rebase --skip" to skip this patch)
  (use "git rebase --abort" to check out the original branch)

Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
	modified:   group.group.permissions.yml
	modified:   src/Entity/Access/GroupAccessControlHandler.php
	modified:   src/Entity/GroupInterface.php
	new file:   tests/src/Functional/GroupAccessTest.php

Unmerged paths:
  (use "git restore --staged <file>..." to unstage)
  (use "git add <file>..." to mark resolution)
	both modified:   group.install
	both modified:   src/Entity/Group.php

diff --cc src/Entity/Group.php
index da0a608,85d6472..0000000
--- a/src/Entity/Group.php
+++ b/src/Entity/Group.php
@@@ -70,7 -71,7 +73,11 @@@
  class Group extends ContentEntityBase implements GroupInterface {
  
    use EntityChangedTrait;
++<<<<<<< HEAD
 +  use EntityOwnerTrait;
++=======
+   use EntityPublishedTrait;
++>>>>>>> 2873212-17-group-status

diff --cc group.install
index 289703e,e17cdba..0000000
--- a/group.install
+++ b/group.install
@@@ -455,16 -456,54 +456,69 @@@ function group_update_8017() 
  }
  
  /**
++<<<<<<< HEAD
 + * Set the 'owner' entity key and update the owner field.
 + */
 +function group_update_8018() {
++=======
+  * Added the status field to group entity type.
+  */
+ function group_update_8018() {
++>>>>>>> 2873212-17-group-status

Fix syntax error introduced accidentally in #26 with the manual conflict resolution after rebase on latest 8.x-1.x branch.

I apply the patch successfully, but a following drush cr results with this error:

PHP Fatal error: Class Drupal\group\Entity\Group contains 3 abstract methods and must therefore be declared abstract or implement the remaining methods (Drupal\Core\Entity\EntityPublishedInterface::isPublished, Drupal\Core\Entity\EntityPublishedInterface::setPublished,

Patch #27 failed to apply on the latest head.
Uploading the correct one!

Patch #30 failed to apply correctly, because group_update_8021 has been added in release 1.0.0. This is a reroll to change the update hook to group_update_8022.

None of the above patches are applying for me against 1.x-dev@dev.

This issue is over three years old. This is embarrassingly sad. This module needs more than one maintainer.

#31 is working for me and Views integration is just working!

Thanks!

It was missing a status column on the group listing page.

Thanks heddn

Last patch #36 works great! I made a few bunch of tests, everything looks fine with Group 8.x-1.3.

As brooke_heaton said, it would be great to include this feature in the next release

Thanks for the continued efforts here.

Just a small heads up that this issue is part of a larger effort to add proper revision support to groups module. While not required, it would be super helpful to update the patch on the parent issue #2829966: Support for Revisions on groups with the changes here, since there are integrations (including tests) in that issue that interact with the code here to properly cover the feature set that integrates published/unpublished state with permissions to access revisions.

From comment #52 on the parent issue:

1. Revision-based permissions in #3029907 depend on status permissions in #2873212.
('View the latest version' 'Requires the "View any unpublished group" or "View own unpublished group" permission')

It doesn't make sense to set this one back to NW, so that the parent ticket can be fixed, so I'm going to do that on the parent ticket instead.

thanks a lot for this patch, works fine with group 1.3 and drupal 8.9.11. i'm still missing an authorization, but it would be important to tell who is allowed to set the status.

so we only have two possibilities:

* group member takes out the publish and can't access the group anymore
* only administrator can republish the unpublished group

therefore, another authorization would be important, for example, that only administrators can see the status checkbox at all, or even a certain group role.

then this patch would be really perfect and should be included in the group core :)

+++ b/src/Entity/Access/GroupAccessControlHandler.php
@@ -19,9 +19,22 @@ class GroupAccessControlHandler extends EntityAccessControlHandler {
+            $access_result = GroupAccessResult::allowedIfHasGroupPermission($entity, $account, 'view own unpublished group');

As soon as you start checking per user, the result needs to be cached per user.

Given the access implications when someone unpublishes a group, I'd too consider gating the toggle behind a separate permission (publish group/unpublish group) so that you can still assign editors without having to worry that they might lock themselves out. Having said that, core currently also behaves in a way that someone might unpublish content they too can no longer see when unpublished. So I'd like to hear some opinions on that matter.

I think adding an extra granularity of permission (A "(un)publish group" permission) only adds benefit, and doesn't come with cons, so it seems like a good permission to add to me, regardless of whether core does it. On that note, separately from this issue it may be a nice patch for core as well.

@kristian: Reviewed your comment re:cacheing

GroupAccessControlHandler::checkAccess is what you have in there and consequently GroupAccessResult::allowedIfHasGroupPermission being called. allowedIfHasGroupPermission seems to inherently add cache context via: addCacheContexts(['user.group_permissions'].

I may be missing some context, but it would seem this would cache most any cached situation for a user? May not have my memory right on cache tags though.

View own implies it's per user. Which the code shows when you check the UID. In that case, you need to add the user cache context.

The current patch does not implement query access at all, leading to a security issue. Attached proof of concept fixes that. Also got rid of the browser test and will add in kernel and/or unit tests instead.

Small mistake re tracking memberships, let's try again.

Trying to get testbot to run near end of day is always a sure bet for typos.

This fixes tests and adds query tests for unpublished users. Now direct access tests and perhaps expanding query access test with an admin permission test and an update/delete only test.

This massively expands query tests, also for previous functionality to ensure everything both new and old is working as intended. The only thing remaining that needs testing is non-query access stuff; i.e.: direct access checks.

Thank you Kristian for looking at this in detail. The #50 patch still works very well, both for groups and subgroups the checkbox is displayed (Drupal 9.1.8, Group 1.3, Subgroup 1.0.2) . However, I have not been able to discover a new access authorisation option anywhere. Or has this not yet been taken into in the patch?

It would be ideal if, for example, you could give a group role permission in a group type. Let's say we have two roles: manager and editor. The manager is allowed to unpublish and republish the group, the editor should not see the checkbox at all.

When thinking about this, the question naturally arises as to what happens to the associated group content. But this is certainly not the issue with this patch.

Re #51 I still have the new permission in the back of my head, but this patch is being sponsored for approval right now and therefore we need to keep the scope to the work at hand. A follow-up for the new permission(s) would be perfect, though.

Associated group content is indeed out of scope, I've asked the question in this very issue before and no-one seems to have a good answer (yet?) so I'm not sticking my neck out for that one until we come up with a proper plan :)

This should be the final version: Kernel tests, Unit tests, regular access, query access, update hook similar to the ones core has in place, etc.

Associated group content is indeed out of scope, I've asked the question in this very issue before and no-one seems to have a good answer (yet?) so I'm not sticking my neck out for that one until we come up with a proper plan :)

Seems to me that this should be a setting, either for all groups, or for each given group.
That said, I think it would be a nice feature to look at after this one. Thanks for your awesome work on this one!

Fixed, thanks so much to Global Game Jam for sponsoring this work!

Thanks to you kristiaanvandeneynde, and thanks to Global Game Jam!

Sorry if I missed it, but I haven't found any conversation on the next release. This feature is essential for a site, so I'd love to see a release with this in it soon.