Port signup_status to Views2

Ok, committed that (with minor changes -- I moved the handlers into a signup_status/views/handlers subdir). However, I realized that doesn't include views support for the per-status limit data, so that's coming next...

Here's a port of the proof-of-concept VBO integration I originally posted at #330943-6: Drupal 6.x port of signup_status to the latest HEAD code. This still depends on #352328: Replace (optionally?) node/N/signups/admin with a views bulk operations view? in signup to really be useful, but this should be pretty close now.

Committed #2 to HEAD of signup_status, now that #352328: Replace (optionally?) node/N/signups/admin with a views bulk operations view? landed in signup. Setting this issue back to active, since now we should provide a default VBO view in signup_status to use at node/N/signups/admin which includes the status column, the alter status operation, and an exposed filter for the status. Speaking of exposed filters: #358787: Exposed filters don't work on embedded views without overriding the path ...

Over at #330943: Drupal 6.x port of signup_status miglius committed code to HEAD that wiped out my changes from #2 and replaced it with:

/**                                                                             
 * Implementation of hook_init().                                               
 */
function signup_status_init() {
  foreach (signup_status_codes() as $cid => $code) {
    eval('
      function signup_status_change_status_to_'. $cid .'($signup) {
        $cid = '. $cid .';
        $codes = signup_status_codes();
        db_query("UPDATE {signup_log} SET status = %d WHERE sid = %d", $cid, $signup->sid);
        watchdog(\'action\', "Set status of %signup_label to %status_name.", array(\'%signup_label\' => $signup->label, \'%status_name\' => $codes[$cid][\'name\']));
      }
   ');
  }
}

...

/**
 * Implementation of hook_action_info().
 */
function signup_status_action_info() {
  $info = array();
  foreach (signup_status_codes() as $cid => $code) {
    $info['signup_status_change_status_to_'. $cid] = array(
      'type' => 'signup',
      'description' => t('Change signup status to !status', array('!status' => $code['name'])),
      'configurable' => FALSE,
    );
  }
  return $info;
}

I really don't think that's a good idea. A) It makes the code more complicated. B) It makes the UI more complicated, since now there's an explosion of actions for each signup status you might define. C) This makes it more unwieldy to define a default signup_status admin VBO view, since we can't know ahead of time what status codes will be defined on the site that should be selected as distinct operations.

I'd strongly suggest removing this code, and restoring what I had from patch #2:

/**
 * Implementation of hook_action_info().
 */
function signup_status_action_info() {
  return array(
    'signup_status_alter_action' => array(
      'type' => 'signup',
      'description' => t('Alter signup status'),
      'configurable' => TRUE,
    ),
  );
}

function signup_status_alter_action(&$signup, $context) {
  $status_codes = signup_status_codes();
  $signup->status = $context['signup_status'];
  db_query("UPDATE {signup_log} SET status = %d WHERE sid = %d", $signup->status, $signup->sid);
  watchdog('action', 'Set status of %signup_label to %status_name.', array('%signup_label' => $signup->label, '%status_name' => $status_codes[$signup->status]['name']));
}

function signup_status_alter_action_form($context) {
  $options = array();
  foreach (signup_status_codes() as $cid => $code) {
    $options[$cid] = $code['name'];
  }
  $form['signup_status'] = array(
    '#type' => 'select',
    '#title' => t('Signup status'),
    '#options' => $options,
  );
  return $form;
}

function signup_status_alter_action_validate($form, $form_state) {
  if ($form_state['values']['signup_status'] == 0) {
    form_set_error('signup_status', t('You must select a status.'));
  }
}

function signup_status_alter_action_submit($form, $form_state) {
  return array('signup_status' => $form_state['values']['signup_status']);
}

@Miglius: can you explain why you made this change? Do you mind if we put it back how I had it originally?

Thanks,
-Derek

Thanks for replying.

It seems that you misunderstand how configurable actions work, and what I've said about how signup_status should interface with VBO and signup...

"My idea was to ease the configuration of the signup status module by auto creation of the actions for each signup status change. With my code you don't have to create each action, they automatically appear under vbo view style settings actions list."

My code just had a single action, which lets you dynamically choose the status to use. So, when you bulk select N signups at node/%/signups/admin, choose the "Alter signup status" action and press "execute", you go to a page with a drop-down select for all status codes and choose the one you want to alter to. You never have to create multiple actions this way, and, most importantly, you don't have to change your views when you add/remove status codes.

"B) I don't think that UI got more complicated - on the contrary - now there is a list of all actions under vbo view configuration displayed automatically - one for each signup status, so creation actions for the statuses steps are skipped. The name of the status is automatically put in the action's name. Otherwise all this has to be done manually."

The UI is more complicated because whenever you see a list of actions, you now have N actions related to signup_status when you really only need one. Any time you modify your list of status values, you have to change all the VBO views to add these new actions to your views, even if the actions themselves are created automatically. This action-per-status is what actually requires the manual effort, not the original configurable action code.

C) signup status does not have it's own vbo view, it just adds actions to signup's view. Why it's more unwieldy than when you manually create signup status actions?

signup_status doesn't yet have a default view -- that's why I set this issue back to active instead of fixed. ;) The idea is that signup_status should provide a single default view ("signup_status_user_vbo_admin_list") complete with a column to display the signup status, and the VBO actions to modify the status. It'd be a trivial drop-in replacement for the existing signup_user_vbo_admin_list view using the setting to control which view to embed at node/%/signups/admin. When a site turns on signup_status, all they have to do is visit admin/settings/signup, select this different default view, and they're done.

If there's no signup_status default view, they have to manually go in and reconfigure signup's default view. If there are separate actions per status, every time they change their set of status codes, they have to change this view (and any other views that let you modify the status), since the set of VBO actions each view is configured to support will have to be updated. You should look at signup/views/signup.views_default.inc and scroll down to signup_user_vbo_admin_list and the "selected_operations" attribute for a concrete example.

I hope that helps clarify why I think the current action code will actually cause a lot more trouble and work for site admins than the code that was present in revision 1.4 of signup_status.module. I also hope the plan for signup_status VBO integration is more clear.

Let me know what you think... If you agree, I'll revert the actions code, and work on the signup_status_user_vbo_admin_list default view as originally planned.

Thanks,
-Derek

So, when you bulk select N signups at node/%/signups/admin, choose the "Alter signup status" action and press "execute", you go to a page with a drop-down select for all status codes and choose the one you want to alter to.

Oh, and a single configurable action also makes it possible to do cool things like have a single view with this exposed action which can enforce permissions on which status values a given user is allowed to set. signup_status_alter_action_form(), which would re-use _signup_status_status_form_element() (from #227643-4: Allow each node to specify which status values should appear on its signup form ) would then have the same logic built in to limit the choices based on the permissions of the current user, etc.

If we have separate actions per status, there's no way to do this -- the actions a given view supports are global across all users and roles...

Here's 2 patches:
357565_signup_status_configurable_action.8.patch: Convert these actions back into a single configurable action
357565_signup_status.views_default.8.patch: Provide a default 'signup_status_user_vbo_admin' view

Please install these, try them out, and let me know what you think.

Thanks,
-Derek

Here's my dream for how the VBO UI should work for this view, though it's not possible yet: #363780: 1 configurable action and N non-configurable actions on the same page?. ;)

The patch was committed, everything is happy now. Just cleaning up the issue status accordingly.