mcstrother [mcstrother]

Hello, and thank you for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

As per requirements, the motivation message should be expanded to contain more features of the proposed project. For themes, it should include also a screenshot of the theme, and (when possible) a link to a working demo site; for modules, it should include also a comparison with the existing solutions.

There is many "Control Structures" faults, so read http://drupal.org/coding-standards and correct those.

See comment #2; the motivation message should include a description of the module features, and a comparison with the existing projects.

Thank you for your reply.

1.

  update_sql('INSERT INTO {shift_scheduler_slot_roles} (slot_id, rid) VALUES ('. $row->slot_id .', 2)');

Not secure!! Read this http://drupal.org/writing-secure-code and change the code to avoid SQL injection attacks.

2.

      'primary key' => array('shift_id'), //TODO: figure out a way to index using the start time for fast lookups.  This table is going to get big, and we can't do linear time searches all over the place

Please, when applying for a CVS account supply code that you believe is complete.

So I don't think there should be todo:s.

3. shift_scheduler.module is missing CVS header "// $Id$".

4.

function _write_date_row(&$schedule_rows, $slot_cells, $timestamp, $slot_ids) {
function _schedule_format_name($uid) {
function _can_claim($user_uid, $shift_id) {
function _schedule_format_shift_text($uid, $shift_id, $start_time_t, $end_time_t) {

Functions and variables should be named using lowercase, and words should be separated with an underscore. Functions should in addition have the grouping/module name as a prefix, to avoid name collisions between modules.

5.

  $options_query = "SELECT {users}.uid, {users}.name, {users_roles}.rid FROM {users} INNER JOIN {users_roles} ON {users_roles}.uid = {users}.uid INNER JOIN {shift_scheduler_slot_roles} ON {users_roles}.rid = {shift_scheduler_slot_roles}.rid WHERE {shift_scheduler_slot_roles}.slot_id = %d ORDER BY {users}.name";

This is not wrong. But if you want to shorten it you can do like this instead

  $options_query = "SELECT u.uid, u.name, ur.rid FROM {users} AS u INNER JOIN {users_roles} AS ur ON ur.uid = u.uid INNER JOIN {shift_scheduler_slot_roles} AS sssr ON ur.rid = sssr.rid WHERE sssr.slot_id = %d ORDER BY u.name";

- - - - -

Havn't checked all the code, so there could be more similar errors in more places so please look over all your code and correct them.

+1 Subscribe (Watching this in anticipation of the startup of the Drupal Shift Scheduler module.)

So what is happening with this? Is the application process almost finished?

1.       $form[$i -1] = array(
           '#type' => 'select',
           '#title' => t('Role ' . $i),
           '#options' => $role_array,
           '#default_value' => $row->rid,
         );
   

   The first argument of t() is a literal string, not a dynamic value obtained from a function, a PHP variable, or concatenating two strings (even if they are two literal strings). If the first argument is not a literal string, then the script that extracts the string to translate will not add the string to the translation template, and the string will not be translated. Use t()-placeholders instead of concatenating strings.

2. Menu descriptions and titles (as well as schema descriptions) should not be passed to t().
3. The module doesn't implement hook_uninstall() to remove the Drupal variables it defines. Don't delete them using a SQL query that match any Drupal variable with a name starting with the module name.

4.   $row_data[] = array(
       'data' => '<b><p>' . date('l', $timestamp) . '</p><p>' . date('n/j/Y', $timestamp) . '</p></b>',
       'valign' => 'center',
     );
   

     if ($uid == 0 || $uid == NULL) {
       //if it's still an open shift, make it red and bold
       $shift_text = '<b><font  color="Red">' . $shift_text . '</font></b>';
     }
   

   <b> should be replaced by <strong>, and <font> should be replaced by a CSS style.

5. Dependencies from Drupal core modules that are not optional should not be declared. The module should report its dependencies from modules that need to be enabled before the module is enabled (it is not possible to disable user.module).
6. The project is not hosted on drupal.org; the update functions should be removed.
7. I am not sure why the database should have a field for each weekday when it would be enough to have a single field.

8.     $form[$i -1] = array(
         '#type' => 'select',
         '#title' => t('New Eligible Role'),
         '#description' => t('To add a new eligible role, select one from this menu.'),
         '#options' => $role_array,
       );
   

   Form field titles should be in sentence case, not title case.

9.     db_query("INSERT INTO {shift_scheduler_slots} (display_name, status) VALUES ('%s', 0)", $form_state['values']['name']);
       $slot_info = db_fetch_object(db_query("SELECT slot_id FROM {shift_scheduler_slots} WHERE display_name ='%s'", $form_state['values']['name']));
   

   Why isn't the code using drupal_write_record()?

10.   $form['slot_id'] = array(
        // use 'value' rather than 'hidden'
        '#type' => 'value',
        '#value' => $slot_id,
      );
    

    Comments should be written as sentences, with a final period.

11.     drupal_set_message(t('New default shift added'));
    

    Shown messages should have a final period.

12. function shift_scheduler_configure_submit($form, &$form_state) {
      variable_set('shift_scheduler_claim_open_shifts', (int) $form_state['values']['claim_open_shifts']);
      variable_set('shift_scheduler_relinquish_shifts', (int) $form_state['values']['relinquish_shifts']);
      variable_set('shift_scheduler_link_to_profile', (int) $form_state['values']['link_to_profile']);
    }
    

    That code would be not necessary, if the form would use system_settings_form().

Hi,
This is not a complete review. I installed your module on a fresh Drupal 6.20 installation. A few issues:

• Coder (Code Review) reports formatting issues.
• Please see the following http://drupal.org/node/539608 . In particular:
  

  Code like db_query('SELECT * FROM {table} t WHERE t.name = ' . $_GET['user'])
  is an open gate to security issues, as the content of $_GET['user'] could be ' OR 1, and would cause the query to show all the rows contained in the table.

  Please change the following statements located in shift_scheduler.module, method shift_scheduler_schedule() :

  if ($_GET['claim'] && $_GET['uid'] == $user->uid && $_GET['shift_id']
         && variable_get('shift_scheduler_claim_open_shifts', FALSE)
         && _shift_scheduler_can_claim($user->uid, $_GET['shift_id'])) {
      db_query('UPDATE {shift_scheduler_shifts} SET uid=%d WHERE shift_id=%d', $user->uid, (int) $_GET['shift_id']);
    }
    // Handle relinquishing shifts.
    elseif ($_GET['relinquish'] && $_GET['shift_id']
             && variable_get('shift_scheduler_relinquish_shifts', FALSE)) {
      $shift_info = db_fetch_object(db_query('SELECT uid FROM {shift_scheduler_shifts} WHERE shift_id = %d', $_GET['shift_id']));
      // Ensure that user is trying to relinquish his own shift.
      if ($shift_info->uid == $user->uid) {
        //Set to open shift.
        db_query('UPDATE {shift_scheduler_shifts} SET uid=%d WHERE shift_id=%d', NULL, $_GET['shift_id']);
      }
    }
  

• Please consider replacing the hard-coded database table values located in file listing shift_scheduler.module, method _shift_scheduler_schedule_format_name() :
  

  $last_name_row  = db_fetch_object(db_query("SELECT value FROM {profile_values} WHERE fid=2 AND uid=%d", $uid));
  $first_name_row = db_fetch_object(db_query("SELECT value FROM {profile_values} WHERE fid=1 AND uid=%d", $uid));
  

  What happens if first and last name are not mapped to fid 1 and 2 ?

Hope that helps.

+1 Subscribe

Hi,
I reviewed the version posted in comment #17 on Drupal 6.20 installation.

First, my sincere apology. My comment #16 , referring to use of $_GET vars and SQL was incorrect. Please disregard, you aren't concatenating, my mistake,

A couple other issues:

• Please set Coder's Selection form to "minor" and "Internationalization". Coder still reports some formatting issues and issues with usage of t().
• You do not have README.txt or a INSTALL.txt file in your package. I suppose you could notify folks who install your module that they have the option of configuring the profile module with the following field names, etc (in either the README.txt or INSTALL.txt files). In other words, you are still assuming that are profile fields defines with the name of profile_last_name and profile_first_name. As an alternative. please consider adding a configuration setting for an administrator that allows mapping of the profile fields.
• From reading other reviews, implementing hook_update_6xxx() in your install file is not appropriate for a package that is not currently available on Drupal.org. Obviously, once your project is hosted on Drupal,org, updates are fine.

Nice job! Really close.

Hope that helps.
Lorin

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

Just skimming over the code, writing down what I'm noticing. These are mostly just suggestions, you don't need to implement everything...

- ; $Id$
You don't need this line anymore, git doesn't use it.

- function shift_scheduler_schema() {
You should add a "Implementation of hook_schema()." docblock to all hook implementations. Note that it has changed to "Implements hook_schema()." in D7, which I prefer and usually also use in my D6 modules.

- $output = '< p >' . t("Displays the shift schedule managed by shift_scheduler") . '< /p >'; // Not too sure about this line. Does it need to be enclosed in a paragraph?
That is a good question, I don't know. I do know that the comment should be on a separate line however ;)

- That huge conditional thing in function _shift_scheduler_can_claim() is hard to read. My suggestion would be to split them into separate lines, store in a temporary variable and then just check these. Or write some api/helper functions. Also, you shouldn't select multiple columns nor use db_fetch_object() (use db_result() instead) when you just want to know if there is such a row or now. If these queries can return many rows, you also might want to use db_query_range() and only select a single row at most.

- " date_default_timezone_set('CST');"
That shouldn't be there I think. Not everyone lives in that timezone :)

- There is a huge amount of code in your .module file. Maybe you could move admin/pages callbacks to their own file to make it easier to read and save some memory.

- There are many many different queries all over the place. Wondering if that could be simplified a bit by creating some helper/api functions to re-use some of these. Haven't checked if they are different though.

- $volunteer_options_query = "SELECT uid, name FROM {users} WHERE uid > 1 ORDER BY name"; (in shift_scheduler_old_shifts_form)
That looks like a problematic think if a site has many users. Also, excluding uid 1 looks like a site-specific thing to me.

- function shift_scheduler_old_shifts_results()
You are mixing php and HTML in that function, should be moved to a separate theme function. Or maybe theme_table() could be used?

- function shift_scheduler_admin_slots() {
if (!arg(4)) {
Any reason why that is not a normal argument defined in hook_menu() ? Will make it easer to port to D7, because the number of path elements will likely be different.

- $items['admin/settings/shift_scheduler/slot'] = array(
This and a few others below admin/settings/shift_scheduler are all normal menu_items, is there a reason that they aren't local_tasks? I think that's more common when you have multiple settings pages.

Overall, the code looks good, it's just a huge amount of that.

Closing, feel free to re-open if this was a mistake.