Parent group assigned lost when group edited by user without access to parent element

Tested patch and it works great - when saving the group node the parent is not lost anymore. Small note: had to apply patch with patch -p6 < og_subgroups.module.patch - you should create patches from the root of the og_subgroups directory - not from Drupal root.

After looking at the patch and the original code, I think the problem is actually with the implementation of hook_nodeapi.

To begin with, the $node parameter is not passed by reference. As a result, when $op = 'load', the parent id is gotten and set but, because $node is not passed by reference nor is the $node returned, it never actually updates the $node object.

This:

function og_subgroups_set_parent($node, $parent = NULL) {

should be this:

function og_subgroups_set_parent(&$node, $parent = NULL) {

The second problem is in the switch statement. When the operation is 'load', the parent id is set in the $node object. When the operation is 'insert' or 'update', however, the parent id is not set resulting in og_subgroups_set_parent() being called with a null value for $node->og_parent which, then, removes the parent node as designed.

case 'insert':
case 'update':
  // Check if this is a group
  if (og_is_group_type($node->type)) {
    // Save the node parent, if one
    og_subgroups_set_parent($node, $node->og_parent);
    // Optionally force children to be private
    og_subgroups_force_private_children($node);
  }
  break;

The solution is to get and set the parent id.

case 'insert':
case 'update':
  // Check if this is a group
  if (og_is_group_type($node->type)) {
    // Attach the group's parent group, if one
    $parent = og_subgroups_get_group_parent($node);
    $node->og_parent = $parent ? $parent : NULL;
    // Save the node parent, if one
    og_subgroups_set_parent($node, $node->og_parent);
    // Optionally force children to be private
    og_subgroups_force_private_children($node);
  }
  break;

This removes the need to modify the og_subgroups_set_parent() function as the previous patch does (meaning, if I'm correct, that this patch should supercede the previous patch).

This patch may also affect #1346100: Audience settings removed when saving the node. and #1482408: Parent node being missed in node form under certain conditions. although I haven't tested those.

Changing status to 'needs review'.

Fixing patch. Previous patch did not respect $node->og_parent if it was already set in 'insert' and 'update' cases of switch statement in og_subgroups_nodeapi(). Added a test for existing $node->og_parent value.

case 'insert':
case 'update':
  // Check if this is a group
  if (og_is_group_type($node->type)) {
    // Attach the group's parent group, if one
    if (is_null($node->og_parent)) {
      $parent = og_subgroups_get_group_parent($node);
      $node->og_parent = $parent ? $parent : NULL;
    }
    // Save the node parent, if one
    og_subgroups_set_parent($node, $node->og_parent);
    // Optionally force children to be private
    og_subgroups_force_private_children($node);
  }
  break;

This worked great for me. Thank you so much.

--
V

With this solution it's impossible to remove the parent from the group.

One of our devs has suggested to solve the problem by modifying the 'if' statement in the nodeapi call for update/insert as follows (line 266):

if (og_is_group_type($node->type) && og_subgroups_can_edit_hierarchy($node)) {

This has the benefit of utilizing the already existing permission for whether the user is able to edit the hierarchy...

I haven't thoroughly tested it yet, but it seems good to me initially...more like how it should have been, since there is that permission...