Posted by capellic on June 26, 2009 at 7:10pm
| Project: | Signup Status |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | reviewed & tested by the community |
Issue Summary
Noticed that when editing a signup (signup/edit/]nid]), Status only contains the values that are designated to "show on form". I can see how this would make sense from one perspective, but it doesn't make sense as an admin who wants to change a status. (Luckily I am able to change the status using VBO.)
What about using a permission to allow this field to show up?
Comments
#1
At first glance, I thought this was duplicate with #406414: not being able to adminster signup statuses, but you're talking about the interface for an admin to edit another user's signup. Yeah, in that case, we should probably include all the possible status values -- I don't think we want a new permission for this. We should know that's the version of the signup form we're altering, so it should be fairly easy to somehow figure that out inside signup_status_form_signup_edit_form_alter() and then add a second optional parameter to _signup_status_status_form_element() which would default to FALSE, but if set to TRUE, include all status codes in the form element. Feel free to roll a patch if you're up for it, though I'll be offline for the next 2 weeks so I won't be able to review or commit it until after that. However, other folks know about and care for this module, so maybe someone else can step in...
#2
I'll try to roll a patch later, but here is how I fixed this:
I added a new status form element to the signup_status.module
function _signup_status_edit_status_form_element($current_status = NULL) {$element = array();
$options = array();
foreach (signup_status_codes() as $cid => $code) {
$options[$cid] = $code['name'];
}
if (!empty($options)) {
if (!isset($current_status)) {
$options = array(-1 => t('- Please choose -')) + $options;
}
$element = array(
'#type' => 'select',
'#title' => t('Status'),
'#options' => $options,
'#weight' => 1,
'#required' => TRUE,
);
if (isset($current_status)) {
$element['#default_value'] = $current_status;
}
}
return $element;
}
and then changed the function signup_status_form_signup_edit_form_alter so that on line 269 it calls the new edit form element instead of the one for the main signup form.
$status_element = _signup_status_edit_status_form_element($signup->status);I haven't done a done a lot of testing, but it looks like it works so far.
#3
@ln282: Thanks. However, please don't cut and paste 95% of that code into a separate function. It'd be much better to follow the path I laid out in comment #1 where there's just an argument to this function that controls if it should use the full list of status options or the list restricted by the "show on form" checkboxes.
#4
Ok-- I actually came up with my fix before searching the issue queue, and that's why I didn't do it the way you described initially. Using an argument in the form element function for this is a whole lot better.
Anyhow, here's a patch that I think does what you had in mind.
#5
Great, thanks, that's a lot better. ;) A few minor problems:
A) You want
||(logical OR) not|(bitwise OR)B) $editform is a bit wonky as a name for this argument. Something like $show_all or $show_all_options would be better.
C) You need to document the new argument in the PHPDoc comment for this function.
D) The callsite doesn't need this:
+ $editform = TRUE;+ $status_element = _signup_status_status_form_element($signup->status, $editform);
This would be fine:
+ $status_element = _signup_status_status_form_element($signup->status, TRUE);Thanks!
-Derek
#6
Another try at it. :)
#7
This patch needs one more tweak. It needs to check for "manage signup status codes" permission before allowing all statuses to be shown to a user. That way, normal users could change their email address, phone number, etc., but not statuses set to "not shown on form".
Rather than passing TRUE in the function parameter, I just changed the line on or about line 246 from
$status_element = _signup_status_status_form_element($signup->status,TRUE);to
$status_element = _signup_status_status_form_element($signup->status,user_access(SIGNUP_STATUS_MANAGE_PERMISSION));This allows it to show for admins and not for "regular" users. I'm not sure this is the most efficient way to do it but it works.
#8
Followup: actually what I posted in #7 doesn't quite work because the status will revert if they go in and save their signup information. For instance, if it is set to Pending when they sign up and the admin sets it to Confirmed, only Pending shows and the Pending status will show, and it will revert to Pending when they save it. So what it really needs to do is just show only the current status if they don't have "manage signup status codes" permission. To make it generally applicable it would be appropriate to have another permission "manage own signup status codes" for websites that want the module to work as it does now without the patch.
#9
Attached patch shows all statuses for those with the SIGNUP_STATUS_MANAGE_PERMISSION permission. Users will also be able to see statuses that are not visible in the form, but has been set by admin, addressing #8.
#10
#9 Works for me!
#11
I did's saw this issue when I hit the problem :(
I have another patch with a different approach, just added the permission, maybe it's worth a look.
I don't change the RTBC status, but take care that the currently reviewed patch is at #9.
#12
Is this going to be commited?