Click to Schedule

Quick things:

1. All of the constants defined at the beginning of your module should be namespaced.
2. As for including Fancybox in your module, im not familiar with whats available in Drupal gardens, but the Fancy box library should be removed, and added back in using Libraries API.
3. The version should be 7.x-1.x.

A few items discussed over IRC:

Licensing issues

The Libraries API module is a recommended method for adding 3rd party dependencies (i.e. Fancybox) without directly including the code on Drupal.org.

Repository setup

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

files[] without classes or interfaces

It's only necessary to declare files[] if they declare a class or interface. (Only listing as a potential issue ... I didn't actually check.)

Please note: The Coder module currently has an unresolved flaw which will prompt you to add file declarations to your .info file even when it's not necessary to do so. Please do not try to make this warning go away by declaring files which do not contain classes or interfaces.

Automated Coding Standards Review

Run your code through the automated review tool at http://ventral.org/pareview (initial output at http://ventral.org/pareview/httpgitdrupalorgsandboxxenophyle1362590git) and update any coding style issues it flags. (Yes, it's long ... but ignore the fancybox items, which are 80% of the existing review; and will disappear once the library is removed.)

drupal_get_path()

Consider using drupal_get_path() instead of your CTS_PATH constant.

Filter all user-provide output

Mostly okay, but I believe I saw one place in the admin file where a user-provided admin config value was stored in a variable, then retrieved and displayed without sanitization.

Use '#attached'

Look into using the new Form API '#attached' property (introduced in D7) to attach Javascript or DSS to a form, instead of using drupal_add_js() or drupal_add_css().

Hardcoded dropbox id

If the dropbox id value (on line 241 of your module file) is not the same for all users/sites, then it should be a configurable variable ... if it is the same, then add a comment explaining this so that other reviewers don't flag this as an issue.

Cache_clear_all()

On line 515, this is like hitting a finishing nail with a 25 pound sledgehammer, and can cause temporary performance issues on heavy traffic sites with lots of users/nodes. :) I'd suggest updating the code to only clear the block cache.

Leave the issue at 'needs work' until your updates have been added to the repository ... just so that we don't get people double-reviewing the same code. ;)

Line 33, click_to_schedule.module:

case 'admin/help#click-to-schedule':

Should be:

case 'admin/help#click_to_schedule':

Line 9, click_to_schedule.info:

stylesheets[screen][] = fancybox/jquery.fancybox-1.3.4.css

Refers to a file that no longer exists ( probably needs to be removed now that you are using the Libraries API )

Line 13, click_to_schedule.install:

click_to_schedule_set_files();

If you enable the module before putting the fancybox scripts in the libraries folder, the message thrown by the click_to_schedule_set_files() function actually appears on the drupal "modules" page, but I missed it the first time around, since I went directly to the module config page afterwards. Subsequently, on the config page, the following error messages appear:

Notice: Undefined index: js in click_to_schedule_admin() (line 21 of /Applications/MAMP/htdocs/drupal7/sites/all/modules/click_to_schedule/click_to_schedule.admin.inc).
Notice: Undefined index: css in click_to_schedule_admin() (line 23 of /Applications/MAMP/htdocs/drupal7/sites/all/modules/click_to_schedule/click_to_schedule.admin.inc).

At this point, the variable "click_to_schedule_fancybox_files" is never set, even if you have now installed the fancybox script in the right place. The only way to set the "click_to_schedule_fancybox_files" variable is to call the "click_to_schedule_set_files", which only happens on install. In other words, if the user doesn't place the fancybox scripts *before* installing the module, the only way to get the module to work is to uninstall and re-install the module. Any easy fix might be to detect the files in click_to_schedule_admin():

Line 20, click_to_schedule.admin.inc:

$files = variable_get('click_to_schedule_fancybox_files', array());

Replace with:

$files = click_to_schedule_set_files();

Other than that, the module seemed to function well for me, and I was able to create a "click to schedule" button that worked.

xenophyle,

Sorry, I wasn't answering emails while at Drupalcon. In any case, your best bet, if you want to shortcut the process, is to find someone with 'create full project' access to create the project for you; but as noted above, there are a few outstanding items which should be addressed before it should be promoted (even if you were to take this shortcut).

Added http://git.drupal.org/sandbox/xenophyle/1362590.git

well, first someone needs to give you the status "reviewed and tested by the community" before such people will check and eventually grant you access. Short: You don't need to search for them, they will find you.

You should reread the project application docs ....

Here an auto-review:
Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

FILE: ...modules/pareview_temp/test_candidate/click_to_schedule-button-config.js
--------------------------------------------------------------------------------
FOUND 18 ERROR(S) AFFECTING 9 LINE(S)
--------------------------------------------------------------------------------
 53 | ERROR | There should be no white space after an opening "{"
 53 | ERROR | There should be no white space before a closing "}"
 55 | ERROR | There should be no white space after an opening "{"
 55 | ERROR | There should be no white space before a closing "}"
 56 | ERROR | There should be no white space after an opening "{"
 56 | ERROR | There should be no white space before a closing "}"
 64 | ERROR | There should be no white space after an opening "{"
 64 | ERROR | There should be no white space before a closing "}"
 66 | ERROR | There should be no white space after an opening "{"
 66 | ERROR | There should be no white space before a closing "}"
 67 | ERROR | There should be no white space after an opening "{"
 67 | ERROR | There should be no white space before a closing "}"
 89 | ERROR | There should be no white space after an opening "{"
 89 | ERROR | There should be no white space before a closing "}"
 91 | ERROR | There should be no white space after an opening "{"
 91 | ERROR | There should be no white space before a closing "}"
 92 | ERROR | There should be no white space after an opening "{"
 92 | ERROR | There should be no white space before a closing "}"
--------------------------------------------------------------------------------


FILE: ...sites/all/modules/pareview_temp/test_candidate/click_to_schedule.module
--------------------------------------------------------------------------------
FOUND 26 ERROR(S) AFFECTING 26 LINE(S)
--------------------------------------------------------------------------------
 254 | ERROR | Array indentation error, expected 4 spaces but found 0
 255 | ERROR | Array indentation error, expected 4 spaces but found 0
 256 | ERROR | Array indentation error, expected 4 spaces but found 0
 257 | ERROR | Array indentation error, expected 4 spaces but found 0
 258 | ERROR | Array indentation error, expected 4 spaces but found 0
 259 | ERROR | Array indentation error, expected 4 spaces but found 0
 260 | ERROR | Array indentation error, expected 4 spaces but found 0
 261 | ERROR | Array indentation error, expected 4 spaces but found 0
 262 | ERROR | Array indentation error, expected 4 spaces but found 0
 263 | ERROR | Array indentation error, expected 4 spaces but found 0
 264 | ERROR | Array indentation error, expected 4 spaces but found 0
 265 | ERROR | Array indentation error, expected 4 spaces but found 0
 266 | ERROR | Array indentation error, expected 4 spaces but found 0
 267 | ERROR | Array indentation error, expected 4 spaces but found 0
 268 | ERROR | Array indentation error, expected 4 spaces but found 0
 335 | ERROR | Array indentation error, expected 4 spaces but found 0
 336 | ERROR | Array indentation error, expected 4 spaces but found 0
 337 | ERROR | Array indentation error, expected 4 spaces but found 0
 338 | ERROR | Array indentation error, expected 4 spaces but found 0
 339 | ERROR | Array indentation error, expected 4 spaces but found 0
 465 | ERROR | Array indentation error, expected 4 spaces but found 0
 466 | ERROR | Array indentation error, expected 4 spaces but found 0
 467 | ERROR | Array indentation error, expected 4 spaces but found 0
 468 | ERROR | Array indentation error, expected 4 spaces but found 0
 469 | ERROR | Array indentation error, expected 4 spaces but found 0
 641 | ERROR | Doc comment for var $delta does not match actual variable name
     |       | $form at position 1
--------------------------------------------------------------------------------

1. JS should be put into .js files

  $form['js'] = array(
    '#markup' => '<script type="text/javascript" src="//asset0.zendesk.com/external/zenbox/v2.4/zenbox.js"></script>
    <style type="text/css" media="screen, projection">
      @import url(//asset0.zendesk.com/external/zenbox/v2.4/zenbox.css);
    </style>
    <script type="text/javascript">
      if (typeof(Zenbox) !== "undefined") {
        Zenbox.init({
          dropboxID:   "20032782",
          url:         "https://timetrade.zendesk.com",
          tabID:       "feedback",
          tabColor:    "gray",
          tabPosition: "Right",
          hide_tab:    true
        });
      }
    </script>',
  );

2. Text should always pass through t() function for translation:
'#markup' => '<div id="zenbox-link" onClick="Zenbox.show(); return false;">Leave feedback</div>',

3. ... also it is recommanded putting as much code markup into theme functions or tpl files (also at other places):

    <a href="' . $href . '">
    <img src="' . $button_img . '" alt="Make an Online Appointment" border="0"/></a>
    <br/>
    <a href="http://www.timetrade.com" style="font-size:10px;">Online appointment scheduling by Timetrade</a>
    </noscript>',

4a. This link should go to a Constant, or should even be configurable:
'http://player.vimeo.com/video/14251501?title=0&byline=0&portrait...',

4b. probably the same for http://www.timetrade.com/login and other external links, as they can change without knowledge of the module developer, who does not want to repackage the entire module because of a simple link

5. Links should be created with the l() function:

    <a href="' . $config['href'] . '">
    <img src="' . $button_img . '" alt="Make an Online Appointment" border="0"/></a>

6. String needs translation:
$('#fancybox-wrap').append('

');
$('body').append('

');

---
The response time for a review is now approaching 4 weeks.
Get a review bonus and we will come back to your application sooner.
See: http://drupal.org/node/1410826

You could for example start by evaluating my own project:
http://drupal.org/node/1302786

A bit more PAReview slavery, @xenophyle. ;-)

• sites/all/modules/pareview_temp/./test_candidate/click_to_schedule.admin.inc:
   +459: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar.
   +469: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar.
   +472: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar.

  All these come from the same pattern you're observing:

  drupal_set_message(t('Error connecting to TimeTrade:') . ' ' . filter_xss($e->getMessage()), 'error'); 

  It's the unranslated string literal triggering the error (i.e. ' '). You might try something like this instead:

  drupal_set_message(t('Error connecting to TimeTrade: !message', array(
    '!message' => filter_xss($e->getMessage())
  )), 'error'); 

  This way the whole string becomes translatable, which is better anyway. I see a number of other places across your code that you could use this pattern instead (and probably should, if you can)—in click_to_schedule_create_free_account_submit(), for example.

• FILE: click_to_schedule.module
  --------------------------------------------------------------------------------
  FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
  --------------------------------------------------------------------------------
   324 | ERROR | The $text argument to l() should be enclosed within t() so that
       |       | it is translatable
   452 | ERROR | The $text argument to l() should be enclosed within t() so that
       |       | it is translatable
   863 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
  --------------------------------------------------------------------------------
  

  These are all pretty easy fixes. On the first two, it would be cleaner to use theme_image() than to build the raw HTML yourself. Then you can make the alt text translatable, too.

A couple of miscellaneous comments:

• You might mention the libraries dependency in your README.txt.
• If your module depends on FancyBox, you should probably do some dependency checking to make sure it's there. Obviously, the libraries dependency by itself doesn't ensure it will be.

It's looking good!

You're welcome, @xenophyle. I think the moderators will take away the review bonus tag when it's time. :)

In the meantime, give the updated PAReview output a look.

Manual review of the 7.x-1.x branch:

1. click_to_schedule_can_create_new_account() and click_to_schedule_can_retry_login() are the same? You can re-use an access callback, you don't have to copy and paste it.
2. click_to_schedule_can_log_out(): why do you check "variable_get('click_to_schedule_email', '')" twice?
3. "t('Click to Schedule button !delta', array('!delta' => check_plain($config['delta'])))": you should use the "@" placeholder in t() instead of "!" which will automatically run check_plain() for you.
4. click_to_schedule_block_configure(): why do you need to build a fake form_state? Why can't you use drupal_get_form()? Why can't you just pass the delta or the configuration as parameter to click_to_schedule_block_configure_form()?
5. "'long-small' => 'Text on one line: Small'": all user facing text must run through t() for translation. Same for "array('internal' => 'Page on my Drupal site', 'external' => 'Web link'),", please check all you strings.
6. click_to_schedule_start_session(): indentation error, always use two spaces per level, also for multi line function/method calls.

But that are just minor issues, otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Your project page looks a little unstructured, you may have a look at the tips for a great project page.

Otherwise your module is well documented! Looks good,

Thanks for your contribution and welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

Having trouble with fancybox. It's installed correctly [checked on fancybox module], is there a line of text in one of these files I need to change? I'm pretty new to all of this so excuse me if the answer is posted above. I reread the thread several times and am still confused. I've also reinstalled click to schedule after getting fancy box installed correctly and still nothing. Also it's already asking me to upgrade, is this because I didn't create my account via the drupal form?

Please post all issues regarding this project to its issue queue: http://drupal.org/project/issues/click_to_schedule?categories=All

Added link to PAReview report on ventral.org.