I wanted to print my schedule for OSCMS. I have found no such screen.

CommentFileSizeAuthor
#5 signup-1.1.2.28-admin_view_schedule.patch1.53 KBdww

Comments

hunmonk’s picture

Assigned: Unassigned » hunmonk
Status: Active » Fixed

fixed. link to print a user's schedule now appears in the user signup block and the user's account page in the signup section. unfortunately, i don't see an easy way to integrate what i did w/ the 'print' module, and i don't want to output an unthemed page directly--so this will have to do for now...

dww’s picture

Status: Fixed » Active

i think there's a bug in this feature: it only works for users w/ "admin signups" access. seems like any user that can create signups (with "access signups" rights) should be allowed to view their own schedule. if i hadn't been up all night, i might have had the energy to generate a patch. as it is, i need some sleep. but, this should be an easy fix, assuming this behavior is just a minor oversight, not a "feature". ;)

basically, in signup_menu(), you just need to change this:

$items[] = array('path' => 'user/' . $user->uid . '/signups', 'access' => ($access || ($user->uid == arg(1))), 'type' => MENU_CALLBACK,
    'callback' => 'signup_user_schedule');

to this:

$items[] = array('path' => 'user/' . $user->uid . '/signups', 'access' => ($user_access('allow signups') || ($user->uid == arg(1))), 'type' => MENU_CALLBACK,
    'callback' => 'signup_user_schedule');

(i think) ;)

thanks,
-derek

hunmonk’s picture

no, that won't work. that gives everybody access to everybody's signup stuff. not good. i'll look at it later.

hunmonk’s picture

Status: Active » Fixed

doh! menu lessons 101:

you can't put a menu item that changes in the 'may_cache' section :)

fixed in 4.6 and HEAD.

dww’s picture

Category: feature » bug
Status: Fixed » Needs review
StatusFileSize
new1.53 KB

i hate to keep reopening this issue, but i just found another bug in this feature (in revision 1.1.2.28). once again, it's due to badness in signup_menu() where we define the callback for signup_user_schedule(). you went out of your way to add support for only viewing the signup schedule if either a) it's your own schedule or b) you've got 'admin signups' access. however, this code only defines the callback for a single path, namely "user/{current_user}/signups":

    // user signup schedule callback
    $items[] = array('path' => 'user/' . $user->uid . '/signups', 'access' => ($access || ($user->uid == arg(1))), 'type' => MENU_CALLBACK,
    'callback' => 'signup_user_schedule');

say the current user trying to view a page is uid 3. even if that user is a signup-admin, once they navigate to "user/5/signups", they'll just get sent to "user/5", since no callback or path was defined for user/5/signups. :( i've tried this as a regular user, as a signup-admin, with and without caching, etc, so i'm pretty sure i'm not crazy. this is just a think-o in how this callback is being defined. it seems there are only 3 possible solutions:

  1. forget about anyone ever being able to view anyone else's signup schedule, even a signup admin (this seems lame)
  2. do a DB query inside signup_menu() to pull all the uids, and for anyone with 'allow signups', create another row in the $items array as a callback to user/{uid}/signups. we should only do this DB query and define all these callbacks if the current user has 'admin signups' access, to only incur the cost of the query if we need it
  3. redesign this callback entirely so that it's not using the uid in the URL, but so it gets passed in as an argument. then, there's just a single path (/user/signups) and a single callback, but the callback figures out what uid you care about based on an argument in the URL or via hidden form mojo.

i think #1 would be a shame. i don't really understand this code well enough to take a shot at #3. #2 seems reasonable, though that could potentially create a large number of callbacks, especially on huge sites with tons of users. i don't have a good sense of drupal scalability concerns, if doing something like this is a big no-no, etc.

for now, i coded up a small patch to revision 1.1.2.28 to implement #2. the attached patch changes the lines i quoted above to become this:

    // user signup schedule callback
    if ($access) {
      // signup admins need callbacks registered for all users on the
      // system that are able to create signups at all
      $results = db_query("SELECT uid FROM users");
      while ($user_rec = db_fetch_object($results)) {
        if (user_access('allow signups', $user_rec)) {
          $items[] = array('path' => 'user/' . $user_rec->uid . '/signups', 'access' => TRUE, 'type' => MENU_CALLBACK, 'callback' => 'signup_user_schedule');
        }
      }
    } elseif (user_access('allow signups')) {
      // this user can view but not admin signups, so only register a
      // callback for their own schedule
      $items[] = array('path' => 'user/' . $user->uid . '/signups', 'access' => TRUE, 'type' => MENU_CALLBACK, 'callback' => 'signup_user_schedule');
    }

if i understand hook_menu() correctly, by only conditionally adding rows to the $items array based on the access rights of the current user, we prevent these callback paths from doing anything for other users. at least, that's how it works via my testing. users without 'admin signups' can't view the schedules of other users, since when they try to go to user/{not_them}/signups, they just get dumped back to /user/{not_them}.

i hope this all makes sense. let me know if there's an easy way to do something more along the lines of my suggestion #3 above. i consider #2 a lesser evil than #1, but i don't think it's ideal. however, i didn't want to attempt major surgery to make #3 work.

thanks!
-derek

p.s. checkout http://drupal.org/node/47735 for info about that "user_access('allow signups', $user_rec)" call. sadly, that only works in 4.6, since someone broke (from my perspective) user_access() in HEAD to make it impossible to do efficient checking of access for a list of users like this. :(

hunmonk’s picture

Status: Needs review » Fixed

that was me doing something stupid, and the fix was easy. check the cvs site if you'd like to see how i did it--it's a nice menu trick...

dww’s picture

ahh, very cool. good to know. thanks.

-d

Anonymous’s picture

Status: Fixed » Closed (fixed)