Cleeng Content Monetization

Hello,

Only one module is reviewed as part of the CVS application process. By the time you've got through, you're OK to go...

Therefore, please state which module of these two you would like to be reviewed.

Hi,

I had a little look.

A few things for now:

a) The code (I guess) is great, but when you are submitting to the community it is so that other people can use it, so it needs both great documentation for end users and for other coders. Please add:
i) a README.txt (full guidance for users of your module)
ii) documentation for every function (full guidance for programmers wanting to change your module)

b) Install and run Coder module against the code:
Coder found 1 projects, 6 files, 543 normal warnings

See you later!

Hello, and thank you to apply for a CVS account.

As per requirements, the motivation needs to include a list of module features (more than two sentences), and a comparison with the existing solutions.

Ah, yes, it's slow in the queue here. I don't think there's anything particularly more to do. Just wait for one of the few people going through the queue to reach your issue in the list again (e.g. kiamlaluno). I don't think making the task "critical" is going to help; your application is, I think, no more urgent than all the others in this queue.

1. version = "6.x-1.5"
   core = "6.x"
   project = "cleeng"
   datestamp = "1246533502"
   

   Those lines needs to be removed.

2. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted, and the constant names be written.
3. Menu titles and descriptions (as well as database schema descriptions) should not passed to t().

4.     require_once(drupal_get_path('module', 'cleeng') . '/CleengClient.php');
       $client = new CleengClient(array('clientId' => variable_get('cleeng_clientid', 'b200b11ba9de'),
                       'clientSecret' => variable_get('cleeng_clientsecret', '9bb7b4e124549905585e'),
                       'callbackUrl' => url('cleeng/callback', array('absolute' => TRUE)),
                       'popupWindowMode' => variable_get('cleeng_popupwindowmode', TRUE)));
   

   There is a Drupal function that should be used to load PHP code from a file.

5. define('CLEENG_URL', $client->getUrl() . '/');
   define('CLEENG_LOGIN_URL', CLEENG_URL . 'login');
   define('CLEENG_ACCOUNT_URL', CLEENG_URL . 'my-account');
   define('CLEENG_ACCOUNT_UPGRADE_URL', CLEENG_URL . 'my-account/upgrade');
   

   Those lines are not actually defining constants, but dynamic values. Putting that code outside any functions could create problems, in some cases. It is probably better to put it inside hook_init().

6.       $form['cleeng'] = array(
           '#type' => 'fieldset',
           '#title' => t('Cleeng Content Widget'),
           '#collapsible' => TRUE,
           '#description' => t('Just select the content you want to protect in the editing window and press below button.'),
           '#attributes' => array('id' => 'cleeng-edit-panel'),
         );
   

   Form field titles should be in sentence case.

7.         $form['cleeng']['warning'] = array(
             '#type' => 'item',
             '#value' => t('Please setup the Cleeng clientId and clientSecret at <a href="!link">here</a>', array('!link' => url('admin/settings/cleeng'))),
           );
   

   The t()-placeholder for URLs doesn't start with !.

8.         $form['cleeng']['noslectionerror'] = array(
             '#type' => 'item',
             '#value' => '<div id="cleeng-connecting" style="display: none;"><h3>' . t('Connecting to Cleeng Platform') . '...</h3></div>
                   <div style="display:none;">
                     Welcome, <a title="Visit my account" target="_blank" id="cleeng-username" href="' . CLEENG_ACCOUNT_URL . '"></a>
                   <a href="javascript:" id="cleeng-logout" class="CleengWidget-auth-link">' . t('Log out') . '</a>
                   </div>
                   <div style="display: none;">
                     <a href="#" id="cleeng-login" class="CleengWidget-auth-link">Authenticate with Cleeng Platform</a>
                   </div>
                   <div style="display: none;" id="cleeng-notPublisher">' . t('You need to have a Publisher account before using this widget.') . ' 
                     <a href="' . CLEENG_ACCOUNT_UPGRADE_URL . '" target="_blank">' . t('Please upgrade your account here.') . '</a>
                       </div>
                     <div id="cleeng_SelectionError" style="color:red;display:none">' . t('Please make a selection first!') . '</div>',
           );
   

   The code is outputting HTML without to use a theme function.
   All the strings that are used in the user interface need to be passed to t(); this includes also the string used for the title attribute.

9.                 <div style="display:none;">
                     Welcome, <a title="Visit my account" target="_blank" id="cleeng-username" href="' . CLEENG_ACCOUNT_URL . '"></a>
                   <a href="javascript:" id="cleeng-logout" class="CleengWidget-auth-link">' . t('Log out') . '</a>
                   </div>
   

   Instead of concatenating strings, it would be better to use a single string passed to t(), and use the right placeholders.

     t("Welcome, <a title="@title" target="_blank" id="cleeng-username" href="@url"></a>
                   <a href="javascript:" id="cleeng-logout" class="CleengWidget-auth-link">Log out</a>",
     array('@title' => t('Visit my account'), '@url' => url(CLEENG_ACCOUNT_URL)));
   

   The <a> tag is empty, though. I don't think the user is going to see anything in the page.

10.         ob_start();
            include(drupal_get_path('module', 'cleeng') . '/cleeng.form.inc');
            $html = ob_get_contents();
            ob_clean();
    

    Form fields should be generated through the form API. I am not sure it's safe to generate them as the code does.

11.       if ($result === FALSE) {
            drupal_set_message('You are not authenticated to Cleeng', 'error');
          }
          elseif ($result !== TRUE) {
            drupal_set_message('Failed to update/create Cleeng content: ' . $result, 'error');
          } 
    

    What is wrong with that code (except that $result === FALSE could be probably be simplified)?

12.     if ($updated) {
          // update node content to database
          db_query("update  {node_revisions} set body='%s', teaser='%s' where vid='%s'", $text, $teaser, $node->vid);
        }
    

    SQL reserved words should be written in upper case.
    Why isn't the code using drupal_write_record()?

13.     ob_start();
        include(drupal_get_path('module', 'cleeng') . '/cleeng.render.tpl.php');
        $html = ob_get_contents();
        ob_clean();
    

    In a Drupal module, ob_start() and ob_clean() are generally not directly invoked.
    The code seems to not correctly use a Drupal template. Why isn't it using a theme function?

14.     try  {
          // if there is an available content id, do update
          $cid = 0;
          if (isset($attrs['id']) && $attrs['id']) {
            $cid = $attrs['id'];
            if ($cid<=0) {
              $cid = 0;
            }
          }
          
          if ($cid) {
            $data['contentId'] = intval($cid);
            $ret = $cleeng->updateContent(array($cid => $data));
          }
          else {
            $ret = $cleeng->createContent(array($data));
            $attrs['id'] = $ret[0]['contentId'];
            //$attrs['shortUrl'] = $ret[0]['shortUrl'];
          }
          
          return TRUE;
        } catch (Exception $e)  {
          watchdog('cleeng', $cleeng->getApiOutputBuffer());
          return $cleeng->getApiOutputBuffer();
        }
    

    PHP exceptions are handled only on PHP5, while Drupal is compatible with PHP4; if the module depends from PHP5, it should report this dependency.

15.     case 'getUserInfo' :
          header('content-type: text/json');
          if ($cleeng->isUserAuthenticated()) {
            try {
              echo json_encode($cleeng->getUserInfo(@$_REQUEST['backendWidget']));
            } catch (Exception $e) {
              print_r($cleeng->getApiOutputBuffer());
            }
          }
          else {
            echo json_encode(array('auth' => FALSE));
          }
          break;
    

    There is a Drupal function to use instead of header().
    The function json_encode() is only present on PHP5, while Drupal 6 is still compatible with PHP4.
    print_r() is normally used as debugging function; I am not sure that outputting the value returned from a method through that function has any purpose, if not helping the debugging. As reported by the requirements, any debugging code should be removed from the code that is used in a CVS application.

16.       if ($id && $key)  {
            $ret = $cleeng->autologin($id, $key);
            echo json_encode(array('success' => $ret));
            exit;
          }
    

    Before exit(), the code should invoke module_invoke_all('exit').

17. The cleeng.render.tpl.php is outputting many strings that are not translated.

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.

loom all good to me, could an admin give this thumbs up to get the module out of the sandbox and in to module repo?

Missed a couple of steps as listed in Migrating from CVS Applications to (Git) Full Project Applications ...

- Moving the issue to the proper application queue
- Updating title
- Updating status to 'needs review' (Please note that, as outlined in the Application Workflow section of http://drupal.org/node/539608, the application should be set to 'needs review' after addressing any issues flagged by reviewers.)

* README.txt contains new line errors with '\r'. Use unix line endings ('\n').
* Remove the old $Id CVS helpers
* missing doc block on hook_init()
* why is there a template in cleeng.form.inc?

Sounds like this needs review. goalgorilla, in the future, make sure to change the status so issues don't get lost.

http://drupalcode.org/sandbox/goalgorilla/1089090.git/blob/refs/heads/ma... doesn't seem like it can be distributed - can you read the policy and comment on where the file can be downloaded? The same seems to be true for a lot of the files in the js directory. Similarly it would be good to clarify the source of all images in http://drupalcode.org/sandbox/goalgorilla/1089090.git/tree/refs/heads/ma... to confirm they are licensed properly and can be included in drupal.org's git repository. Overall a lot of the code and images seem like they are 1) questionable license to be re-licensed as GPL v2+ 2) 3rd party code which is available from elsewhere and should not be committed to drupal.org's git.

Is there ever a need to configure these values:
<?
543 'clientId' => 'b200b11ba9de',
544 'clientSecret' => '9bb7b4e124549905585e',
?>

drupal_http_build_query should be wrapped in "if (function_exists('drupal_http_build_query')) {" to prevent collisions in case some other module has also declared that function.

You should remove the "version = "6.x-1.0"" from your .info file. That will get added by the packaging script.

I'm slightly concerned about potential for CSRF in cleeng_callback and cleeng_ajax_request since they both deal with $_GET/$_REQUEST parameters directly. It seems like this is the same code you use elsewhere and nothing Drupal specific so I'll trust that you've dealt with that issue, but it could be good to doublecheck.

Hi goalgorilla,

Licenses on 3rd party files is only the first question. The policy outlines the rules, but in general, 3rd party libraries are not allowed to be hosted on Drupal.org if they can possibly be downloaded from a more original source. One reason for this is the high likelihood of conflicts between different modules directly including the same files. For example, if the Zero Clipboard module included those files, anyone trying to run both modules would have errors. The preferred solution to this is the Libraries API, which helps different modules check for the presence of any necessary 3rd party libraries in a central location.

In short, you should remove all of those files and instead include them via the Libraries API and/or download instructions.

Updated status - be sure to set it back when you feel it needs review.

• 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.
• CleengClient.php: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.
• README.txt: wrong line endings ('\r' but should be unix style '\n')
• lines in README.txt should not exceed 80 characters
• do not use hook_init() to define constants! just define them in your module file directly.
• "//Account settings form": There should be a space after "//" and comments should end with a ".".
• "Implementation of hook_menu()." should be "Implements hook_menu().". Also on other functions.
• "// check whether the type is supported": all comments should start capitalized.
• "if ($path = libraries_get_path('zeroclipboard')) {": indentation errors in this if block.
• "'clientSecret' => '9bb7b4e124549905585e',": hard-coded password??
• plugin.js: indentation should be 2 spaces per level, also in javascript files.

Review of the 6.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• Comments: there should be a space after "//". (also in many other places)
  

  includes/cleeng.page.inc:16:  //User login
  includes/cleeng.page.inc:67:        //print_r($cleeng->getApiOutputBuffer());
  includes/cleeng.page.inc:187:          //print_r($cleeng->getApiOutputBuffer());
  js/cleeng-backend.js:2:  //Initial settings
  js/cleeng-item-form.js:2:  //Price slider options
  README.txt:49://Installation finished if you don't use a wysiwyg editor -> Read further at "Usage"
  README.txt:51://ckeditor module
  README.txt:58://wysiwyg module (supported: ckeditor)
  

• ./cleeng.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function cleeng_clear_cookie() {
  --
  
  function cleeng_wysiwyg_plugin($editor, $version) {
  

• ./includes/cleeng.account.inc: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function cleeng_account_form_submit($form, &$form_state) {
  

• ./includes/cleeng.item_form.inc: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

   
  function cleeng_item_form($form_state) {
  --
  
  function cleeng_item_form_validate($form, &$form_state) {
  --
  
  function cleeng_item_form_submit($form, &$form_state) {
  --
    
  function price_array() {
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./README.txt:                                                           ASCII English text, with CRLF, LF line terminators
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• remove all debug statements from the repository, e.g. print_r().

Hi goalgorilla,

here's the latest automated review with pareview:

Review of the 6.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/cleeng.module:
   +36: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +114: [normal] The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.
   +284: [normal] The $string argument to t() should not begin or end with a space.
   +318: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +476: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +491: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +628: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +644: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
  
  Status Messages:
   Coder found 1 projects, 1 files, 8 normal warnings, 0 warnings were flagged to be ignored
  

• Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
  

  README.txt:50://Installation finished if you don't use
  README.txt:53://ckeditor module
  README.txt:60://wysiwyg module (supported: ckeditor)
  

• ./js/cleeng-backend.js: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

    // Process our "Cleeng it!" button also edit buttons, no submit action, open the modal frame! 
  

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./js/cleeng-backend.js:31:      var price = price.substr(1); // Strip sign
  

• ./cleeng.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function cleeng_wysiwyg_plugin($editor, $version) {
  

• ./cleeng.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function template_preprocess_cleeng_not_bought(&$variables) {
  function template_preprocess_cleeng_error(&$variables) {
  function template_preprocess_cleeng_bought(&$variables) {
  function template_preprocess_cleeng_owner(&$variables) {
  

• ./includes/cleeng.item_form.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function price_array() {
  

• ./includes/cleeng.class.inc: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  16-     */
  40-     */
  55-     */
  

• Assignments should have a space before and after the operator, see http://drupal.org/node/318#operators
  

  ./includes/cleeng.page.inc:128:        window.parent.location= window.parent.location;
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./cleeng.module ./plugins/cleeng/plugin.js ./README.txt ./includes/cleeng.item_form.inc ./includes/cleeng.class.inc ./includes/cleeng.account.inc ./includes/cleeng.admin.inc ./includes/cleeng.page.inc ./js/cleeng-frontend.js ./js/cleeng-backend.js ./js/cleeng-item-form.js ./templates/cleeng-error.tpl.php ./templates/cleeng-bought.tpl.php ./templates/cleeng-owner.tpl.php ./templates/cleeng-not-bought.tpl.php ./css/cleeng.frontend.css ./cleeng.info
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual review:

• Your project is really waiting a long time now. Nevertheless I recommend not only relying on automated code checks but also having an understanding of http://drupal.org/node/1354 as, in the end, coder and other scripts only try to find most issues with the standards documented there. This just as an explanation, as there are many questions and misunderstandings on that and, on top, these standards constantly evolve.
• May I add a suggestion for your project description? As it looks like, you and the many people you refer to, know what Cleeng is. What about users who don't but could possibly benefit from it as well? Perhaps a few additional words on its scope wouldn't be wrong? (You already do this in a brief way in your readme file.)
• Aside this, your project page already looks really informative. For I final polish, I would recommend these excellent tips for a great project page
• In your git master branch, there are still files. To avoid confusion, those should be replaced as described in http://drupal.org/node/1127732 at step 5.
• Although in general your code really shows evidence that you are already very inside Drupal coding standards, please have a look at http://drupal.org/node/322729 and http://drupal.org/node/322774 and re-check your translatables. Example: cleeng.module, line 277:

  $variables['message'] = t('Probably you have changed from sandbox to live. Please ' . l('relink your account', 'user/' . $user->uid . '/cleeng', array('attributes' => array('target' => '_blank'))) . '. Also Cleeng content from the sandbox can not be used on the live server so you have to delete the cleeng items and make them again.');

  should be

  $variables['message'] = t('Probably you have changed from sandbox to live. Please <a href="@url">relink your account</a>. Also Cleeng (...)', array("@url" => url('user/' . $user->uid . '/cleeng')));

  as this avoids lots of negative side effects in subsequent translation processes . Also note that I did not omit the "target" attribute by accident as it is not compliant with XHTML strict. Re-consider how important standard compliance is for you, otherwise you would add your attributes array the same way as in l().

Ok, so much for now. Hope you find it helpful!

-dave

(status)

Hi,

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

Review of the 6.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/cleeng.module:
   +336: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +500: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +515: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +651: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +667: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
  
  Status Messages:
   Coder found 1 projects, 1 files, 5 normal warnings, 0 warnings were flagged to be ignored
  

• Assignments should have a space before and after the operator, see http://drupal.org/node/318#operators
  

  ./includes/cleeng.page.inc:128:        window.parent.location= window.parent.location;
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./cleeng.module ./plugins/cleeng/plugin.js ./README.txt ./includes/cleeng.item_form.inc ./includes/cleeng.class.inc ./includes/cleeng.account.inc ./includes/cleeng.admin.inc ./includes/cleeng.page.inc ./js/cleeng-frontend.js ./js/cleeng-backend.js ./js/cleeng-item-form.js ./templates/cleeng-error.tpl.php ./templates/cleeng-bought.tpl.php ./templates/cleeng-owner.tpl.php ./templates/cleeng-not-bought.tpl.php ./css/cleeng.frontend.css ./cleeng.info
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Additions/manual review:

• Your pareview results look like there was some problem with installing the module and some more concerning file access etc.? Are you in a Windows or Unix environment?
• Aside from that you should provide docblocks for each non-hook function: Can you drop some lines for a better understanding of cleeng.module, lines 172-179? This menu page callback function looks pretty far from clean HTTP standards to me. I understand that some redirection to a previously viewed page is intended, but the normal Drupal way for this would be drupal_goto(). Not just because a visitor with Javascript disabled would face some strange results here. To make sure the user is taken back to a page before, you should really combine drupal_get_destination() with drupal_goto(). If, however, you want to do a redirect with Javascript, IMO you should nevertheless look for a better solution such aus Drupal.behaviors.yourfunction which is equivalent to jQuery(document).ready(function(){}); and could implement this behaviour, ideally combined with some non-JS fallback message. However, probably you can feed back on this? :)
• cleeng.module, lines 206-212 "Implements" syntax is ok (but not needed, just to mention). But if there's no return, you should omit the @return block as of Drupal cs.
• cleeng.module, line 289: I see that you have changed your translatable string, it looks already better, but is there a reason you still do it other than recommended in http://drupal.org/node/322774 ? Just for my understanding.
• cleeng.module, line 289 (again, but just a personal note): If I were registered with the site (i.e. not user 0) but not the site admin, just *some* user, customer, member, whatever: I would really wonder "what the heck sandbox is meant here"? Are you sure this message should be displayed to any registered site user?

OK, so much for the moment. HTH and if I can be of assistance, just let me know :)

Hi, I have just installed the most recent versions of drupalcs and pareview. There is currently much evolving with those. I have attached the results.

Coder should always be run in the D7 version, as it is common sense that most recent coding standards should also apply to new D6 related projects (and yes, anyone knows that hardly any existing D6 stuff does ;)).

HTH for the moment, dave

Hi, I just tried to find this line 62 you refer to in the repo viewer, but couldn't - in which file is that so I can check?

Thx, dave

Thanks for clarifying! ("Line 62" irritated me, as it's now in line 25.)

Ok, will do another check asap.

Sounds really strange. Ok. Maybe someone else can check before I can; too busy atm.

Just soliciting confirmation on a few items from previous reviews before marking this as ready ...

From Comment #15:
#5: Does not appear to have changed, but was marked as 'fixed'
#6: 'Sentence Case' refers to only capitalizing the first word of the phrase, not all the words. (Rather minor item, in my opinion.)
#8: You state you've added the t(), but have you considered adding a theme function so that themers can override the output?
#11: I have to admit ... the "If false, do x, else if not true, do y, else do z" logic confused me ... I'd suggest that an 'If true, else if false, else x' ordering of the logic might make it easier for people to follow. (Just a suggestion, not a requirement!)

From Comment #24:
Do you have an answer regarding the potential for CSRF attacks?

From Comment #33:
When you use the git clone command, you can specify the target directory as an argument following the 'git clone' command ... also, when a sandbox is promoted to a full project, you are given an opportunity to change/set the permanent shortname for the project at that time.

On a side note, I just wanted say thanks for your dedication, patience, and persistance with this application ... it's certainly earned my respect and appreciation!

Okay ... i still see a few minor coding standards violations (such as '}' not being on a seperate line inside your try/catch structures, and extraneous */ tags on the end of your inline comments), but nothing major.

I'm not thoroughly familiar with the code, but based on a quick review, and considering both the number of individuals who have viewed it and your assurances that the potential for CSRF is not a concern, let's set this one as ready and see what happens. :)

congrats! hard step to get first module in but then again, best quality