| Project: | OG User Roles |
| Version: | 6.x-4.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | bschilt |
| Status: | needs review |
Issue Summary
This issue is spawned from this issue here: http://drupal.org/node/496608. The maintainer ask to make this a separate issue.
I would like to change the way user roles are added and removed from the user. Currently when the og_user_roles_page_form is submitted all roles for every user in that group are deleted. Then the roles are re-added based upon what was returned by the form submission.
There should be some code processing that happens before roles are added or deleted that figures out which roles need to be added and which ones need to be deleted. I have come up with a solution that works and I'll post that code below. But I would like to change the code even more so that the database doesn't get queried when getting the user's current roles. I would like to actually pass the original role values for each user to the submit handler by way of form values.
Here is the code that I currently have that works:
<?php
function og_user_roles_page_form_submit($form, &$form_state) {
$gid = $form['#node']->nid;
foreach ($form_state['values']['user_roles'] as $uid => $new_roles) {
//get the user's current roles from the db.
$original_roles = og_user_roles_get_roles_by_group($gid, $uid);
//determine which roles to add
$add_roles = array_diff($new_roles, $original_roles);
$add_roles = array_filter($add_roles);
//determine which roles to delete
$delete_roles = array_diff($original_roles, $new_roles);
foreach ($delete_roles as $rid => $checked) {
og_user_roles_role_delete($gid, $uid, $rid);
}
foreach ($add_roles as $rid => $checked) {
og_user_roles_role_add($gid, $uid, $rid);
}
}
drupal_set_message(t('The changes have been saved.'));
}
?>Any thoughts?
Comments
#1
Here is a patch that solves the issue described above. This solution drastically reduces database I/O by only inserting and deleting the necessary roles for each user if a checkbox was toggled on the og_user_roles_page_form page.
Each user's original role settings are saved as form values and passed to the submit handler. This patch changes the following two functions:
<?php
/**
* Form builder for group user roles assignment.
*/
function og_user_roles_page_form($form_state, $node, $roles, $result) {
$form['#node'] = $node;
// Used for theming.
$form['#id'] = 'og-user-roles';
$form['#roles'] = $roles;
// Unset role titles.
foreach ($roles as $rid => $name) {
$roles[$rid] = '';
}
$form['user_roles'] = array('#tree' => TRUE);
while ($account = db_fetch_object($result)) {
$form['user_roles'][$account->uid]['user'] = array(
'#value' => theme('username', $account),
);
$form['user_roles'][$account->uid]['roles'] = array(
'#type' => 'checkboxes',
'#default_value' => og_user_roles_get_roles_by_group($node->nid, $account->uid),
'#options' => $roles,
'#parents' => array('user_roles', $account->uid),
);
// save the user's original roles. Compared with the values in $form['user_roles'][$uid]['roles']['new']
// to determine which roles were added and deleted. Used in the submit handler.
$form['user_roles'][$account->uid]['original_roles'] = array(
'#type' => 'value',
'#value' => og_user_roles_get_roles_by_group($node->nid, $account->uid),
);
}
$form['submit'] = array('#type' => 'submit', '#value' => t('Save'));
return $form;
}
/**
* Form submit handler for group user roles assignment form.
*/
function og_user_roles_page_form_submit($form, &$form_state) {
$gid = $form['#node']->nid;
foreach ($form_state['values']['user_roles'] as $uid => $roles) {
$original_roles = ($roles['original_roles']) ? $roles['original_roles'] : array();
$new_roles = array();
foreach($roles as $rid => $checked) {
if(is_numeric($rid)) {
$new_roles[$rid] = $checked;
}
}
//determine which roles to add
$add_roles = array_diff($new_roles, $original_roles);
$add_roles = array_filter($add_roles);
//determine which roles to delete
$delete_roles = array_diff($original_roles, $new_roles);
foreach ($delete_roles as $rid => $checked) {
og_user_roles_role_delete($gid, $uid, $rid);
}
foreach ($add_roles as $rid => $checked) {
og_user_roles_role_add($gid, $uid, $rid);
}
}
drupal_set_message(t('The changes have been saved.'));
}
?>
#2
#3
#4
+++ og_user_roles.pages.inc 6 Jul 2009 17:19:13 -0000@@ -118,6 +118,12 @@ function og_user_roles_page_form($form_s
'#parents' => array('user_roles', $account->uid),
@@ -129,12 +135,27 @@ function og_user_roles_page_form($form_s
function og_user_roles_page_form_submit($form, &$form_state) {
...
+ foreach ($form_state['values']['user_roles'] as $uid => $roles) {
+ $original_roles = ($roles['original_roles']) ? $roles['original_roles'] : array();
+ $new_roles = array();
+ foreach($roles as $rid => $checked) {
+ if(is_numeric($rid)) {
+ $new_roles[$rid] = $checked;
+ }
+ }
ok, a few issues here:
The definition of #parents in the form['roles'] ensures that we can actually iterate over the $uid => $roles like we do here.
Since you added 'original_roles' now, I'm not entirely sure whether this foreach still works at all?
In any case, you can remove the inner foreach if you would just unset($roles['original_roles']) after copying it into $original_values.
Minor note (which no longer applies then): Missing space behind "foreach" and "if".
+++ og_user_roles.pages.inc 6 Jul 2009 17:19:13 -0000@@ -118,6 +118,12 @@ function og_user_roles_page_form($form_s
+ // save the user's original roles. Compared with the values in $form['user_roles'][$uid]['roles']['new']
+ // to determine which roles were added and deleted. Used in the submit handler.
@@ -129,12 +135,27 @@ function og_user_roles_page_form($form_s
+ //determine which roles to add
...
+ //determine which roles to delete
The first comment exceeds 80 chars. Comments should also start with a capital letter and end with a period (full-stop), i.e. form a proper sentence. The last two are missing a leading space.
+++ og_user_roles.pages.inc 6 Jul 2009 17:19:13 -0000@@ -118,6 +118,12 @@ function og_user_roles_page_form($form_s
+ '#value' => og_user_roles_get_roles_by_group($node->nid, $account->uid),
It looks like we could vastly speed this up if og_user_roles_get_roles_by_group() would be altered to make the second argument optional, so it returns all current group assignment, keyed by uid (but only when the second argument has been left out, i.e. explicitly test for NULL).
+++ og_user_roles.pages.inc 6 Jul 2009 17:19:13 -0000@@ -129,12 +135,27 @@ function og_user_roles_page_form($form_s
+ $original_roles = ($roles['original_roles']) ? $roles['original_roles'] : array();
When using the ternary operator syntax, please always wrap the entire statement in parenthesis (instead of the condition) to clarify what belongs to the statement and what not.
+++ og_user_roles.pages.inc 6 Jul 2009 17:19:13 -0000@@ -129,12 +135,27 @@ function og_user_roles_page_form($form_s
+
...
+
Trailing white-space here (blank lines should be blank).
+++ og_user_roles.pages.inc 6 Jul 2009 17:19:13 -0000@@ -129,12 +135,27 @@ function og_user_roles_page_form($form_s
+ $add_roles = array_diff($new_roles, $original_roles);
+ $add_roles = array_filter($add_roles);
This can be written on one line. A better variable name would be $added_roles (past tense).
This review is powered by Dreditor.
#5
I have updated this patch to work with the latest HEAD code. The solution is a bit simpler than it was before. Please review.
#6
Looks better now (although trailing white-space all over the place).
However, the tests are failing when running them with this patch applied.
#7
I'll take out the trailing spaces. I haven't run tests before. I'll review simple_test and see what needs to be changed.
#8
Ok, here is the updated patch, I've removed the white space and it passes all tests.
#9