Code clean up of og_user_roles

This is fantastic. Thanks so much!

However, I just completed a series of changes to the 1.2 code and committed 1.3. I'm not sure if I can run this patch against the 1.3 code since there is at least one change that breaks it. I thought about patching 1.2, then applying my new patch of changes, but that won't work since the format will have changed drastically.

I've a attached a patch of the 1.3 changes that as applied to the 1.2 code.

Let me know what you suggest.

I have no idea what, if anything, you've decided to do. However, if you are looking to do something, I've committed a new 1.4 release which fixes a bug I inadvertently introduced in 1.3. If you do intend to submit a patch for 1.4 release, please let me know and I will freeze code development until module is patched.

Thanks!

From what I can see release 1.2, 1.3 and 1.4 are identical?!

In trying to patch 1.4 I got:

patching file og_user_roles.module
Hunk #12 FAILED at 556.
Hunk #13 succeeded at 824 (offset 24 lines).
Hunk #14 FAILED at 832.
2 out of 14 hunks FAILED -- saving rejects to file og_user_roles.module.rej

I can not get the patch "og_user_roles.module.070705.patch" to apply to this version of og_user_roles.module.

As it looks now my patch still applies.

Heh, the "applying" can be misinterpreted. Suppose you ment that the patch won't work, it's contents still need to be applied.

As I just checked, all 1.2 - 1.3 - 1.4 versions are identical (checked the releses from http://drupal.org/node/149373/release with WinMerge), except for the .info files which have only their "version" value updated. Some cvs problem maybe?

I wouldn't notice the lack of changes as I can't test it (my development server is still pretty messed up because of other drupal problems :S). Hope the correct versions are somewhere safe.

Looks like I failed to commit modifications to the last 3 releases.

Current release: og_user_roles 5.x-1.5 has all the latest changes.

http://drupal.org/node/157738

Oops!

Yes. Code development on hold. Thanks.

Any idea why this is failing?

[root@db og_user_roles]#  patch -p0 < og_user_roles_code_clean_up_2_1.patch
patching file README.txt
patching file og_user_roles.info
Hunk #1 succeeded at 1 with fuzz 1.
patching file og_user_roles.install
patching file og_user_roles.module
Hunk #12 FAILED at 821.
1 out of 12 hunks FAILED -- saving rejects to file og_user_roles.module.rej

Apparently there was one modification that didn't get into 1.5 upload. Corrected, and patch works. I now have a Drupal code standardized version of og_user_roles. Thanks!

However, in trying to test, the very first thing I noticed was that the "Organic groups user roles" settings link has disappeared from "Organic groups" admin page (admin/og).

I assume, but never asked, that you tested this patched version?

If so, then perhaps there's a simple config I'm missing on my site. When I copy the new module, install and info files to my site, I can no longer access the Organic groups user roles settings page.

OK, my bad again. I needed to delete cache/cache_menu. OG User Roles settings now appear.

You said originally:

Other things included in this patch:
* I rewrote function og_user_roles_menu(). Less code but I think all the needed functionality is there.
* I rewrote som SQL querys with proper db_query usage to avoid SQL injections.

Before I commit this, I have to go through and basically make sure everything is working.

Could you please do the following:

1. Explain the logic behind how you re-wrote the og_user_roles_menu()?
2. Give me the exact functions where you re-wrote SQL queries so I can specifically test them?
3. Let me know of any other areas where you actually modified code?

Thanks so much. This is it. Once I get past this unit testing, we should be good to go.

I understand most of the code was physically changed (removing tabs) to conform to Drupal standards.

What I'm asking for is for you to identify where you modified code logic and why. For example:

@@ -323,99 +259,67 @@ function og_user_roles_page($gid) {
 
   if (is_array($roles)) {
     // Retrieve list of all group users
-    $sql = og_list_users_sql(0);   
-    $result = pager_query($sql, 100, 0, NULL, $gid);    
-    $output .= theme('pager',NULL, 100);     
-    $output .= drupal_get_form('og_user_roles_page_form',$gid,$roles,$result);
-  } 
+    $sql = og_list_users_sql(0, 0, 'ou.is_admin DESC, ou.is_active ASC, u.name ASC');
+    $result = pager_query($sql, 100, 0, NULL, $gid);
+    $output .= theme('pager', NULL, 100);
+    $output .= drupal_get_form('og_user_roles_page_form', $gid, $roles, $result);
+  }

I don't know why you changed the $sql. In order to test this specifically, that would be good to know. Without knowing why, I don't know if this might not have introduced a potential bug. Anytime any of us change anything in a working program, that becomes a possibility.

It's a big program as you know, with lot's of stuff. I'm trying to make sure I don't overlook something that crops sometime in the future when we least expect it.

Thanks for your assistance.

Thank you! Exactly what I needed.

For example, should I get this error Not unique table/alias: as I have in the past, I'm now aware of another place to look?

Trying to unit test this on a couple of servers. If we don't run into any problems, will commit shortly.

Thanks so very much for your time and effort on this, frjo! I will endeavor to follow the guidelines set in this code from here on out.

-ron

Found the first really big problem. Like me, you probably only tested with admin user (#1). When I logged in as another user (the following are my internal notes, FYI):

Logged in as David. Went clicked on "Groups", then Habitat 825:

* user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '222,223,217,207,225,226,227,228,229,230,238,239,240,241,242,243,244,245)) ' at line 6 query: SELECT COUNT(*) FROM node_access na LEFT OUTER JOIN og_uid ogu on ogu.nid = na.nid LEFT OUTER JOIN og ogm on ogm.nid = na.nid WHERE (na.nid = 0 OR na.nid = 13) AND ((na.realm = 'og_public' and na.gid = 0) OR (na.realm = 'og_subscriber' AND na.gid in (12,13,47,218,,222,223,217,207,225,226,227,228,229,230,238,239,240,241,242,243,244,245)) OR (ogm.nid > 0 and (na.nid in (12,13,47,218,,222,223,217,207,225,226,227,228,229,230,238,239,240,241,242,243,244,245)) or ogm.directory = 1) OR (na.realm = 'term_access' AND na.nid in (12,13,47,218,,222,223,217,207,225,226,227,228,229,230,238,239,240,241,242,243,244,245))) AND (na.grant_view >= 1 OR (ogu.is_admin = 1 AND ogu.uid in /var/www/html/websites/drupal/brix/includes/database.mysql.inc on line 172.

I think the problem is this:

Original code:

if ($groups || $node->type == 'group') {

$sql = "
SELECT COUNT(*) FROM {node_access} na
LEFT OUTER JOIN {og_uid} ogu on ogu.nid = na.nid
LEFT OUTER JOIN {og} ogm on ogm.nid = na.nid
WHERE (na.nid = 0 OR na.nid = %d)
AND ((na.realm = 'og_public' and na.gid = 0)
OR (na.realm = 'og_subscriber' AND na.gid in ('".implode("','",$gids)."'))
OR (ogm.nid > 0 and (na.nid in ('".implode("','",$gids)."')) or ogm.directory = 1 )
OR (na.realm = 'term_access' AND na.nid in ('".implode("','",$gids)."')) )
AND (na.grant_$op >= 1 OR (ogu.is_admin = 1 AND ogu.uid = ".$uid.") )
";
} else {
$sql = "
SELECT COUNT(*) FROM {node_access} na
WHERE (na.nid = 0 OR na.nid = %d)
AND (na.realm = 'term_access' AND na.gid in ('".implode("','",$rids)."'))
AND na.grant_$op >= 1
";
}

Changed to this:

if ($groups || $node->type == 'group') {
$sql = "
SELECT COUNT(*) FROM {node_access} na
LEFT OUTER JOIN {og_uid} ogu on ogu.nid = na.nid
LEFT OUTER JOIN {og} ogm on ogm.nid = na.nid
WHERE (na.nid = 0 OR na.nid = %d)
AND ((na.realm = 'og_public' and na.gid = 0)
OR (na.realm = 'og_subscriber' AND na.gid in (%s))
OR (ogm.nid > 0 and (na.nid in (%s)) or ogm.directory = 1)
OR (na.realm = 'term_access' AND na.nid in (%s)))
AND (na.grant_$op >= 1 OR (ogu.is_admin = 1 AND ogu.uid = %d))
";
$group_ids = implode(',', $gids);
$result = db_query($sql, $node->nid, $group_ids, $group_ids, $group_ids, $uid);
}
else {
$sql = "
SELECT COUNT(*) FROM {node_access} na
WHERE (na.nid = 0 OR na.nid = %d)
AND (na.realm = 'term_access' AND na.gid in ('%s'))
AND na.grant_$op >= 1
";
$result = db_query($sql, $node->nid, implode(',', $rids));
}

I think the problem is that he forgot to put in the quotation marks, i.e. ('%s').

Trying to see what happens.

Yep, that was it. Corrected code to this:

if ($groups || $node->type == 'group') {
$sql = "
SELECT COUNT(*) FROM {node_access} na
LEFT OUTER JOIN {og_uid} ogu on ogu.nid = na.nid
LEFT OUTER JOIN {og} ogm on ogm.nid = na.nid
WHERE (na.nid = 0 OR na.nid = %d)
AND ((na.realm = 'og_public' and na.gid = 0)
OR (na.realm = 'og_subscriber' AND na.gid in ('%s'))
OR (ogm.nid > 0 and (na.nid in ('%s')) or ogm.directory = 1)
OR (na.realm = 'term_access' AND na.nid in ('%s')))
AND (na.grant_$op >= 1 OR (ogu.is_admin = 1 AND ogu.uid = %d))
";
$group_ids = implode(',', $gids);
$result = db_query($sql, $node->nid, $group_ids, $group_ids, $group_ids, $uid);
}
else {
$sql = "
SELECT COUNT(*) FROM {node_access} na
WHERE (na.nid = 0 OR na.nid = %d)
AND (na.realm = 'term_access' AND na.gid in ('%s'))
AND na.grant_$op >= 1
";
$result = db_query($sql, $node->nid, implode(',', $rids));
}

Looked to correct elsewhere. Same potential problems:

Original code:

$result = db_query("SELECT n.nid, n.title FROM {node} n WHERE n.type IN ('". $groupTypes ."') ORDER BY n.title");

changed to:

$result = db_query("SELECT n.nid, n.title FROM {node} n WHERE n.type IN ('%s') ORDER BY n.title", $group_types);

Original code:

$sql .= " WHERE (na.nid = 0 OR na.nid = %d) AND ((na.realm = 'term_access' AND na.gid in ('".implode("','",$rids)."')) OR (ogr.uid = " . $uid . " AND na.realm = 'term_access' AND oga.group_nid = ogr.gid) )";

changed to:

$sql .= " WHERE (na.nid = 0 OR na.nid = %d) AND ((na.realm = 'term_access' AND na.gid in ('%s')) OR (ogr.uid = %d AND na.realm = 'term_access' AND oga.group_nid = ogr.gid))";

As per you your recommendation, it is installed. I think I'm getting a false positive because I've removed the indent from the line in question. Nevertheless, looks like the code is in pretty good shape, pending results of testing.

I thought I had replaced all $node_ID with the commonly used $nid?

Something else that was done that's good to know about!

This particular patch of code as missing from the module you modified. I've changed it to conform to the $nid standard.

Hi all,
i did some cleanup on the current cvs version. Patch attached.

i did some cleanup on the current cvs version. Patch attached.

This is much appreciated, but you guys need to let me know if you're going to do this because this module is constantly being updated. The current CVS version is hopelessly outdated as the code has been modified by the earlier patch and I myself have been working on additional code cleanup and unit testing as well.

This version hasn't been updated because I need to be sure there's not anything that's been overlooked.

Once a new version is released, then that would be code to evaluate and submit a patch for if needed. Let me know that is your intention, and I'll hold off on code modifications until you're done.

Thanks!

frjo,

On the "Configure member roles" page, I used to have the assignable roles in 3 columns per user. Not any more. Does your version have the multiple columns?

Actually, I built multiple checkbox columns on "Configure member roles" page into code. It requires this snippet to be added to your theme's style.css:

.checkbox-columns .form-item {
  font-size:75%;
  width: 12em;
  margin-right: 1px;
  float: left;
  display: inline;
}
.checkbox-columns-clear .form-item {
  font-size:75%;
  width: 12em;
  margin-right: 1px;
  clear: left;
  float: left;
  display: inline;
}

I did a refresh of the page and bingo! See the attached .gif.

Because I didn't see them at first, I got worried that something else was broken. Sorry!

frjo, your code modification here:

/**
 * Called by og_user_roles_nodeapi('access')
 * To check og access permissions
 */
function og_user_roles_og_access_check($op, $node = NULL) {
  global $user;
  $uid = $user->uid;
  if ($op != 'create' && $node->nid && $node->status) {
    if (isset($user) && is_array($user->og_groups)) {
      $gids = array_keys($user->og_groups);
    }
    else {
      $gids[] = 0;
    }

    if (isset($user) && is_array($user->roles)) {
      $rids = array_keys($user->roles);
    }
    else {
      $rids[] = 1;
    }

    // if this node has groups or is a group, then use group sql, else use non-group sql
    $groups = $node->og_groups;
    if ($groups || $node->type == 'group') {
      $sql = "
        SELECT COUNT(*) FROM {node_access} na
        LEFT OUTER JOIN {og_uid} ogu on ogu.nid = na.nid
        LEFT OUTER JOIN {og} ogm on ogm.nid = na.nid
        WHERE (na.nid = 0 OR na.nid = %d)
        AND ((na.realm = 'og_public' and na.gid = 0)
        OR (na.realm = 'og_subscriber' AND na.gid in ('%s'))
        OR (ogm.nid > 0 and (na.nid in ('%s')) or ogm.directory = 1)
        OR (na.realm = 'term_access' AND na.nid in ('%s')))
        AND (na.grant_$op >= 1 OR (ogu.is_admin = 1 AND ogu.uid = %d))
        ";
      $group_ids = implode(',', $gids);
      $result = db_query($sql, $node->nid, $group_ids, $group_ids, $group_ids, $uid);
    }
    else {
      $sql = "
        SELECT COUNT(*) FROM {node_access} na
        WHERE (na.nid = 0 OR na.nid = %d)
        AND (na.realm = 'term_access' AND na.gid in ('%s'))
        AND na.grant_$op >= 1
        ";
      $result = db_query($sql, $node->nid, implode(',', $rids));
    }

    $output = (db_result($result));

    if ($output == 0) {
      return FALSE;
    }
    if ($output > 0) {
      return TRUE;
    }
  }
}

/**
 * Called by og_user_roles_nodeapi('access')
 * To check taxonomy_access access permissions.
 */
function og_user_roles_taxonomy_access_check($op, $node = NULL) {
  global $user;

  $uid = $user->uid;

  if ($op != 'create' && $node->nid && $node->status) {
  //  Discovered that I needed to continue using user->roles to get non-group roles.
  //  will get group roles using og_users_roles and og_ancestry

    if (isset($user) && is_array($user->roles)) {
      $rids = array_keys($user->roles);
    }
    else {
      $rids[] = 1;
    }

    $sql = "SELECT COUNT(*) FROM {node_access} na";
    $sql .= " LEFT OUTER JOIN {og_users_roles} ogr ON ogr.rid = na.gid";
    $sql .= " LEFT OUTER JOIN {og_ancestry} oga ON oga.nid = na.nid";
    $sql .= " LEFT OUTER JOIN {og_uid} ogu on ogu.nid = na.nid";
    $sql .= " WHERE (na.nid = 0 OR na.nid = %d) AND ((na.realm = 'term_access' AND na.gid in ('%s')) OR (ogr.uid = %d AND na.realm = 'term_access' AND oga.group_nid = ogr.gid))";
    $sql .= " AND (na.grant_$op >= 1 OR (ogu.is_admin = 1 AND ogu.uid = %d))";

    $result = db_query($sql, $node->nid, implode(',', $rids), $uid, $uid);
    $output = (db_result($result));

    if ($output == 0) {
      return FALSE;
    }
    if ($output > 0) {
      return TRUE;
    }
  }
}

Is failing in certain circumstances, causing OG User Roles to simply not work and give "Access denied" message to users who do, in fact, have permission to access content. I've tried to figure out where the problem is, but cannot. All I know is that when I copy back the code below, the problem goes away. So, I've reverted back to the code that does work:

/**
 * Called by og_user_roles_nodeapi('access')
 * To check og access permissions
 *
*/
function og_user_roles_og_access_check($op, $node = NULL) {
  global $user;
  $uid = $user->uid;

  if ($op != 'create' && $node->nid && $node->status) {

      if (isset($user) && is_array($user->og_groups)) {
		$gids = array_keys($user->og_groups);
      } 
      else {
        $gids[] = 0;
      }

      if (isset($user) && is_array($user->roles)) {
        $rids = array_keys($user->roles);
      } 
      else {
        $rids[] = 1;
      }

	//
	// if this node has groups or is a group, then use group sql, else use non-group sql
	//	

	$groups = $node->og_groups;

	if ($groups || $node->type == 'group') {

	    $sql = "
    	SELECT COUNT(*) FROM {node_access} na 
			LEFT OUTER JOIN {og_uid} ogu on ogu.nid = na.nid 
			LEFT OUTER JOIN {og} ogm on ogm.nid = na.nid 
	    	WHERE (na.nid = 0 OR na.nid = %d) 
    		AND ((na.realm = 'og_public' and na.gid = 0) 
    		OR (na.realm = 'og_subscriber' AND na.gid in ('".implode("','",$gids)."')) 
			OR (ogm.nid > 0 and (na.nid in ('".implode("','",$gids)."')) or ogm.directory = 1 ) 
	    	OR (na.realm = 'term_access' AND na.nid in ('".implode("','",$gids)."')) ) 
    		AND (na.grant_$op >= 1 OR (ogu.is_admin = 1 AND ogu.uid = ".$uid.") )
    		";
	} else {
	    $sql = "
    	SELECT COUNT(*) FROM {node_access} na 
    		WHERE (na.nid = 0 OR na.nid = %d) 
	    	AND (na.realm = 'term_access' AND na.gid in ('".implode("','",$rids)."')) 
    		AND na.grant_$op >= 1
    		";
	}

    $result = db_query($sql, $node->nid);
    $output = (db_result($result));
    if ($output == 0) return FALSE;
    if ($output > 0) return TRUE;
  }
}

/**
 * Called by og_user_roles_nodeapi('access')
 * To check taxonomy_access access permissions
 *
*/
function og_user_roles_taxonomy_access_check($op, $node = NULL) {
  global $user;
  $uid = $user->uid;

  if ($op != 'create' && $node->nid && $node->status) {
	// 	Discovered that I needed to continue using user->roles to get non-group roles.
	//	will get group roles using og_users_roles and og_ancestry
	//
    if (isset($user) && is_array($user->roles)) {
      $rids = array_keys($user->roles);
    } 
    else {
      $rids[] = 1;
    }

    $sql = "SELECT COUNT(*) FROM {node_access} na";
    $sql .= " LEFT OUTER JOIN {og_users_roles} ogr ON ogr.rid = na.gid";
    $sql .= " LEFT OUTER JOIN {og_ancestry} oga ON oga.nid = na.nid";
	$sql .= " LEFT OUTER JOIN {og_uid} ogu on ogu.nid = na.nid";
    $sql .= " WHERE (na.nid = 0 OR na.nid = %d) AND ((na.realm = 'term_access' AND na.gid in ('".implode("','",$rids)."')) OR (ogr.uid = " . $uid . " AND na.realm = 'term_access' AND oga.group_nid = ogr.gid) )";
    $sql .= " AND (na.grant_$op >= 1 OR (ogu.is_admin = 1 AND ogu.uid = ".$uid.") )";
    $result = db_query($sql, $node->nid);
    $output = (db_result($result));
    if ($output == 0) return FALSE;
    if ($output > 0) return TRUE;
  }
}

I know this isn't exactly up to standard, but I can't progress with important issues when my basic code is buggy. If you can see what the problem is, please let me know.

Thanks!

With the exception of this code: http://drupal.org/node/156939#comment-279098, the cleaned up module has now been released.

Many thanks to frjo for your hard work.