Saving a group as "Anyone can join" is saved as "Joining requires admin approval"

Thanks for the bug report.

I was able to reproduce this issue. In fact, we saw this during the initial refactoring of group privacy settings , so it seems to have resurfaced.

Tagging for the Commons 3.3 radar.

I've managed to track this down but getting a working solution has proven more difficult than it should be!

Commons Groups' use case causes Node module to invoke both hook_node_update() and hook_node_insert() during node_save(). Also, hook_node_update() is always invoked before hook_node_insert().

A static variable is used in commons_groups_set_group_permissions() and _commons_groups_update_group_permissions() in order to prevent either function from being run more than once per node. However, since hook_node_update() is always invoked during node_save() and always invoked before hook_node_insert() and they both operate on the same node, hook_node_update() is always "locked out" of performing either commons_groups_*() operations.

It seems like the recommended solution would involve removing Commons Groups implementation of hook_node_insert() in favor of accomplishing all of the required tasks inside of the implementation of hook_node_update() (since it is run in all cases).

As a stop-gap, I've attached a patch that should work but is most likely a bad solution. :P

Marking as "needs work per"

As a stop-gap, I've attached a patch that should work but is most likely a bad solution. :P

Thanks for the thorough issue description!

Thanks for the patch.

+++ b/commons_groups.moduleundefined
@@ -667,6 +661,7 @@ function commons_groups_node_presave($node) {
+  $node = $node->original;

This looks wrong -- why not call the variable $original_node ?.

Thanks, Amitaibu. Aside from the variable name, can you comment on the overall approach here?

We've observed that putting in a return; at the top of _commons_groups_update_group_permissions allows the field to save correctly, but we understand that that would prevent the groups permissions checkboxes from synching with the subscription settings field.

Specifically, we're not clear on the expected values of the $permissions array here.

Commons Groups' use case causes Node module to invoke both hook_node_update() and hook_node_insert() during node_save(). Also, hook_node_update() is always invoked before hook_node_insert().

Sounds like this is the real cause of the problem, and I have a feeling it's a recent change, as I remember personally seeing it working properly. @Devin Carlson, were you able to track down the reason for this wrong hook execution?

I believe we should fix that, as the current bug is probably only the first symptom - update should run after insert ;)

I've narrowed this down further to commons_groups_og_role_change_permissions() which does:

/**
 * Implements hook_og_role_change_permissions().
 *
 * Update the subscription settings field on a group when the relevant
 * permissions are changed.
 */
function commons_groups_og_role_change_permissions($role) {
  if (!$role->gid || $role->name != OG_ANONYMOUS_ROLE) {
    return;
  }

  // Get all non members permissions.
  $permissions = og_role_permissions(array($role->rid => $role->name));
  if (!empty($permissions[$role->rid])) {
    _commons_groups_update_group_permissions($role, $permissions[$role->rid]);
  }
}

I've found that $permissions always returns subscribe (boolean) TRUE during node insertion no matter what the group's privacy settings are set to. The helper function _commons_groups_update_group_permissions() is then called which sets the field_og_subscribe_settings field to this value (subscribe) and saves it.

_commons_groups_update_group_permissions() is triggered whenever a node is inserted or updated because:

commons_groups_insert() and commons_groups_update() both call commons_groups_set_group_permissions() which calls og_role_change_permissions() which invokes hook_og_role_change_permissions(), triggering commons_groups_og_role_change_permissions(), which calls _commons_groups_update_group_permissions().

Ok, I see the problem and I think we should change the strategy here. Up until now when the permissions are changing, even programmatically we maintain a sync between them.

I think that instead, we should hide/ disable those permissions from the OG permissions UI, so it can only be changed via the field. Once we need to maintain that sync only from one direction (field -> permissions), we can remove commons_groups_og_role_change_permissions() and avoid those errors.

@ezra-g, what do you think about the above suggestion?

That suggestion sounds good to me in general. My only concern is whether there's ever a case where the permissions would be changed programaticall, causing them to become out of sync with the field.

If you're confident that's not likely to happen, this seems OK to me.

Another approach would be to detect which item (permissions or field) initiated the change by adding an arbitrary value to the $roles array or similar, and only sync one way in that case.

If you're confident that's not likely to happen, this seems OK to me.

Pretty confident, it's very unusual for a module to determine it's own permissions. I believe that's the right solution -- being able to change a setting only form a single place.

Another approach would be to detect which item (permissions or field) initiated the change by adding an arbitrary value to the $roles array or similar, and only sync one way in that case.

That's the first thing I tried to do, but looking at the code I realized it's not possible. The role object is lost before it reaches our function(s), and adding this context/ metdata in a different way seemed messy.

The attached patch removes our implementation of hook_og_role_change_permissions() and hides checkboxes that we set with field_og_subscribe_settings() when the group is private. However, after further thought, we should just hide these settings in all cases. Leaving as "needs work."

I just notice that todays build (from this morning) does not have the ability to add the content privacy to groups. Is this a mistake and it should be there or has it been removed for some reason and content privacy is handled in a different way?

Do you have OG Access module enabled?

Can you confirm you packaged with build-commons-dev.make, rather than using the Drupal.org nightly development snapshot?

No problem :).

Here's a re-roll as proposed in #12.

Tested #17 which disabled the permission checkboxes but didn't prevent them from being rendered in the permissions table. Talking to @ezra-g, it sounds like the permissions should not be shown in the table at all, which would make disabling the permission checkboxes unnecessary.

Nice catch in #18.

Here's a re-roll. It feels unfortunate to unset() the form elements but #disabled can be overridden in the browser and setting #access = FALSE results in empty rows in the table.

Tested #19 with an existing install of Commons. While it did remove the permission checkboxes from the table, I believe that it did so for the wrong reason.

    foreach($form['checkboxes'] as $role => $checkboxes) {
      foreach (array('subscribe', 'subscribe without approval') as $permission) {
        if (isset($form['checkboxes'][$role][$permission]['#attributes'])) {
          unset($form['checkboxes'][$role][$permission]);
          unset($form['permission'][$permission]);
        }
      }
    }

The rows are only removed if $form['checkboxes'][$role][$permission] has any $attributes, which might not be the case depending on theming.

Unfortunately, I have yet to to come up with a suitable replacement to check and using $form['checkboxes'][$role][$permission] doesn't seem to work.

Marking this as needs work while I continue to look for a solution.

Thanks, Devin Carlson. Re-assigning per #20.

@RobKoberg - Correct. This issue is still accurately marked, "needs work" because it hasn't yet been fixed.

Is there some way to filter the issues list based on what outstanding patches are ahead of the latest version in the repo?

Sure. If an issue hasn't been marked "fixed" then it typically hasn't been committed. You may wish to review https://drupal.org/node/317 for more about conventions of the Drupal.org issue queue.

It seems strange to me that committers don't just commit instead of supplying a patch.

This issue is a good example of the reason why maintainers generally file patches: Because our proposed fixes aren't correct 100% of the time. Filing a patch allows others to review the patch from a code and functional perspective so that it's committed only once it passes that review. If we committed directly, we'd end up with many followup and reversed commits, which would mean a messy revision log and codebase.

This patch changes the offending line to

if (isset($form['checkboxes'][$role][$permission]) && is_array($form['checkboxes'][$role][$permission])) {

.

It feels unfortunate to unset() the form elements but #disabled can be overridden in the browser

I believe this is wrong, otherwise that would have been a security issue. Form API knows that element is disabled so doesn't process it.

Untested but, instead of fiddling with form_alter() OG already provides a mechanism to disable a checkbox per anon/ auth users (that's how it disables for example Subscribe permission from Auth users).

Try altering the permissions (hook_og_permissions_alter())to be with an empty role property.

  $items['subscribe without approval'] = array(
    'title' => t('Subscribe to group (no approval required)'),
    'description' => t('Allow non-members to join a group without an approval from group administrators.'),
    'roles' => array() // <-- something like this.
  );

Thanks, amitaibu. Marking as "needs work".

Re:

I believe this is wrong, otherwise that would have been a security issue. Form API knows that element is disabled so doesn't process it.

Looks like this is a change in behavior from Drupal 6 to 7, per the Form API reference:

In Drupal 6:

Description: Disables (greys out) a form input element. Note that disabling a form field doesn't necessarily prevent someone from submitting a value through DOM manipulation. It just tells the browser not to accept input.

in Drupal 7

Description: Disables (greys out) a form input element. Setting #disabled to TRUE results in user input being ignored, regardless of how the element is themed or whether JavaScript is used to change the control's attributes.

Yap, Drupal is becoming better in each version ;)

Here's a patch that implements hook_og_permission_alter() and a screenshot showing the changes to the permissions and description text.

It feels awkward to show a setting to the user that they'll never be able to adjust in this UI, but this is only one detail of the broader administrative experience in Commons that needs review, so this patch seems like an incremental improvement and we can circle back on this UI during the larger review.

Wrong patch #31 ;).

Could replicate with a fresh install, applied the patch and still got the 'require admin approval' while the group was in moderation. Once the group was made active, i could go back and select anyone to contribute.

On one hand this makes sense, but by default any group created by a non-administrator is going to default to needing approval before it gets approved.

Per #34, I believe we need to combine #33 with the removal of commons_groups_og_role_change_permissions().

Re-roll per #35.

Thanks for the prompt testing, RobKoberg!

#36 is committed. Thanks!

http://drupalcode.org/project/commons_groups.git/commit/b07b103