DiscussIt Live Moderator

Status change as per http://drupal.org/node/1158830#comment-4479054

Nice! You're moving along in the queue, but it may take a bit longer due to the gaps in time between updates. I was wondering if you abandoned this issue! But obviously not. I'm not going to assign this to myself but I'll try to check it out again when I get a chance and keep this thing moving.

In the mean time, some other things. As I mentioned, (and although I haven't checked it out again thoroughly) your module appears to contain a library from a third party, correct? It remains that you may be required to provide instructions on how to obtain and install the library if it cannot be hosted on drupal.org (which is often the case). I think this is dependent upon the licensing. Projects hosted on drupal.org are required to be licensed under GPLv3.

Secondly, I will do a search myself later, but are there any modules that you know of with similar functionality? If so, how does your module differ?

And as always, good luck! And thanks for taking the initiative to contribute.

Indeed, it does comply. But, even in that case it may be recommended that you use a dependency on a module like http://drupal.org/project/oauth_common instead of including the library in your module folder. This is the common Drupal way to do things, as it ensures that the library is not duplicated on sites that use several OAuth modules, with each module that relies on OAuth containing the library.

I understand they are porting the module now. They do have a development version out, and I would strongly suggest you work with that. If anything it would be better to contribute to that module to help the port to D7.

The reason you don't want to include the library in your own module is because of issues like http://drupal.org/node/776104 and http://drupal.org/node/929786. Obviously there are several modules that use the same library, so we would prefer that they find a central library to use for exactly these reasons.

> Can we not go live with our existing version an then modify it when a release version comes out?

This might be an option. I appreciate that you guys tried this avenue. I'll continue with the code review, but we will see how the git administrators feel once thins issue is marked RTBC.

Here are a few more things to do:

It's common practice to place administrative forms in an include called mymodule.admin.inc or front end pages in mymodule.pages.inc. You can use the 'file' attribute in your hook_menu() attributes.

Drupal coding standards (http://drupal.org/coding-standards) dictate that variables should be all lower case and underscores, no camelCase.

All functions need to be documented, documentation is very good where functions are documented.

Drupal variables that are set by your module must be deleted in hook_uninstall() in your .install file.

Hook implementations need to be documented as Implements hook_name(). I.e. hook_menu()

All functions must be namespaced in this sense function mymodule_function_name() no abbreviations or acronyms. Even private functions.

hook_help() is there and output is formatted properly.

overall, I think the code looks good! It makes good use of the Drupal API where possible, using t() and url() and parts of the user API. You should be well on your way after these small changes.

Hi there, I've been working on the module while Charlie's busy with other things.

I've just commited an updated version to the repository in which I think I've dealt with all the issues mentioned in your previous post.

I look forward to hearing from you.

Pete Bennett

The API directory contains 3rd party code which is not allowed in Drupal's git repository because it is easily available elsewhere. Please remove that code and add instructions during the installation process for how to download it.

The alternative is as jordojuice mentioned earlier to get the oauth module working for Drupal 7 so you can rely on it.

Please remove the $Id: from the files like discussit_live_moderator.css and discussit_live_moderator.info

The permission "access administration pages" is probably the wrong one to use for the menu callback because that permission implies a passive role in viewing information. It should really be "administer site configuration" which is an active role of changing configuration.

I see the module uses curl - have you considered drupal_http_request? That will make it work on more sites where curl is not available. If you have to use curl you should add a hook_requirements and a note about it in README.txt

Your discussit_live_moderator_admin_validate can probably be moved to for a slight performance improvement and better logical organization.

Instead of require_once('discussit_live_moderator.admin.inc' ); you can include it as a 'file' parameter to the menu declarations.

The direct use of $_POST in discussit_live_moderator_dicomment_admin is a little concern, but I guess that's how core does it.

Most of these things are suggestions. I'm inclined to grant you access but just want to get some feedback about the 3rd party code issue first.

I know jordojuice's description doesn't exactly match mine. Here's the page where I draw my policy http://drupal.org/node/422996

Thanks for your response.

We're currently working on new features for the module and have removed all the 3rd party libraries. We'll go through all the other points you've mentioned here before we commit the next version for review.

Pete B.

I wonder if you could give me some advice re. drupal_http_request:

We're connecting to our API using OAuth2 over SSL and drupal_http_request seems to be handling our requests OK but not terminating the stream. This is the error I get:

Warning: fread(): SSL: Connection reset by peer in drupal_http_request() (line 905 of /usr/local/zend/apache2/htdocs/drupal78/includes/common.inc).

Any ideas?

Thanks.

That is quite odd. I suggest trying it on a different host on a different network just to see if it's a network issue first.

If that doesn't help then using curl is fine, just add the hook_requirements and note in README.txt about it.

Removed 3rd party libs, added hook_requirements for CURL, removed "$Id:"s, changed permissions. Changed functionality.

Hi, automatic review results with klausi's script:

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.
Review of the master 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/discussit_live_moderator.module:
   +15: [normal] Functions should be called with no spaces between the function name and opening parentheses
   +16: [normal] curly braces { should end a line, not start one
   +19: [minor] There should be no trailing spaces
   +20: [minor] There should be no trailing spaces
   +21: [minor] There should be no trailing spaces
  (... most warnings removed regarding comment length ...)
   +657: [minor] Use an indent of 2 spaces, with no tabs
  
  Status Messages:
   Coder found 1 projects, 1 files, 56 normal warnings, 94 minor warnings, 0 warnings were flagged to be ignored
  

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
  

  ./api/discussit.php
  ./api/discussit.oauth.php
  

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

  api/discussit.php:14:  //var $apibase = 'https://account.discussit/API/v2_0/DiscussIt.svc/';
  api/discussit.oauth.php:9:	//var $apibase = 'https://account.discussit/API/v2_0/DiscussIt.svc/';
  api/discussit.oauth.php:19:			//if (!isset($_SESSION['access_token']))$this->refresh_access_token($refreshtoken);
  (... most warnings removed regarding comment length ...)
  discussit_live_moderator.module:113:      //else 
  discussit_live_moderator.module:142:    //Error thrown possibly because the message does not exist on DiscussIt.
  discussit_live_moderator.module:160:    //Error thrown possibly because the message does not exist on DiscussIt.
  discussit_live_moderator.module:376:  //discussit_live_moderator_di_addComment
  discussit_live_moderator.module:530:  //TODO: set up hidden diRt field
  discussit_live_moderator.module:570:	//edit-subject
  discussit_live_moderator.module:584:	//TODO: add form element with diRt
  discussit_live_moderator.module:587:	//jQuery("#di_replyto").text("hello!");
  

• ./discussit_live_moderator.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

        //$comment_body = $comment->subject . chr(13) . chr(10) . $comment->comment_body[$comment->language][0]['value'];
      // Published class is not needed. It is either 'comment-preview' or 'comment-unpublished'.
        //$form['#prefix'] .= 'jQuery("#txtdiURL").parent().parent().replaceWith("<input id=\"txtdiURL\" type=\"hidden\" value=\"' . $current_user->user_url . '\"/>");';
  

• ./api/discussit.oauth.php: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

  			//if (!isset($_SESSION['access_token']))$this->refresh_access_token($refreshtoken);
  

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

  ./api/discussit.oauth.php:103:	function get_access_token_from_code($code, $clientID, $clientSecret, $redirect="https://www.discussit.com/")//redirect?
  

• ./api/discussit.oauth.php: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  	
  function refresh_access_token($refreshCode, $redirect = '') {
  

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

  function refresh_access_token($refreshCode, $redirect = '') {
  

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

  232- * An associative array containing the current state of the form.
  234- * TRUE if the comments should be deleted, FALSE otherwise.
  385- * Username 
  387- * Password
  389- * boolean true or false.
  408- * Username 
  410- * Password
  412- * The id of a message to update.
  414- * The status to set on the message.
  416- * boolean true or false.
  445- * Username 
  447- * Password
  449- * The id of a message to update.
  451- * The comment body to set on the message.
  453- * boolean true or false.
  476- * Username 
  478- * Password
  480- * Url of the page
  482- * The id of a message to reply to.
  484- * The comment body to set on the message.
  486- * author name to comment as.
  488- * url to commentor profile or website.
  490- * email address of commentor
  492- * IP or commentor
  494- * boolean true or false.
  

• ./discussit_live_moderator.admin.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
  

  136- * Current path's fourth component: the type of overview form ('approval' or
  

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

  ./.gitignore:                         ASCII text, with no line terminators
  
  

• There should be no space after the opening "(" of a control structure, see http://drupal.org/node/318#controlstruct
  

  discussit_live_moderator.module:626:	for ( var b = 0; b < jQuery(".di_Social").length; b++)
  

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

  ./discussit_live_moderator.module ./discussit_live_moderator.install ./discussit_live_moderator.info ./api/discussit.php ./api/discussit.oauth.php ./discussit_live_moderator.admin.inc
  

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 addition:
Seems like you checked in the .gitignore file by accident?

HTH, Dave

Thanks doitDave!

I've run the module through coder and gone through the list of issues you supplied.

I hope I've done enough.

Cheers,

Pete B.

Hi pete,

I hope I've done enough.

As developers we both know that this is unachievable by design. ;) Ok, back from irony, all the automated checks are very helpful but not perfect. In my personal review flow, I regard them as a necessary filter that applies before in-depth manual checks and I guess most other active reviewers handle it in a similar way. It is, IMO, always a good and speeding up idea, to also match one's one application manually with the many Drupal coding standards documents. In addition, your own local installation of klausi's pareview script can really make finding issues a lot easier.

So let's go - are you sure you committed and pushed your recent changes completely?

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.
Review of the master 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/discussit_live_moderator.module:
   +18: [minor] There should be no trailing spaces
   +19: [minor] There should be no trailing spaces
   +20: [minor] There should be no trailing spaces
   +21: [minor] There should be no trailing spaces
   +24: [minor] There should be no trailing spaces
   +25: [minor] There should be no trailing spaces
   +26: [minor] There should be no trailing spaces
   +27: [minor] There should be no trailing spaces
   +28: [minor] There should be no trailing spaces
   +39: [minor] There should be no trailing spaces
   +45: [minor] Use an indent of 2 spaces, with no tabs
   +50: [minor] There should be no trailing spaces
   +63: [minor] There should be no trailing spaces
   +86: [minor] There should be no trailing spaces
   +149: [minor] There should be no trailing spaces
   +150: [minor] There should be no trailing spaces
   +151: [minor] There should be no trailing spaces
   +152: [minor] There should be no trailing spaces
   +155: [minor] There should be no trailing spaces
   +156: [minor] There should be no trailing spaces
   +157: [minor] There should be no trailing spaces
   +160: [minor] There should be no trailing spaces
   +161: [minor] There should be no trailing spaces
   +162: [minor] There should be no trailing spaces
   +163: [minor] There should be no trailing spaces
   +166: [minor] There should be no trailing spaces
   +167: [minor] There should be no trailing spaces
   +168: [minor] There should be no trailing spaces
   +175: [minor] There should be no trailing spaces
   +185: [minor] Use an indent of 2 spaces, with no tabs
   +190: [minor] There should be no trailing spaces
   +199: [minor] There should be no trailing spaces
   +205: [minor] There should be no trailing spaces
   +208: [minor] There should be no trailing spaces
   +215: [minor] There should be no trailing spaces
   +217: [minor] There should be no trailing spaces
   +230: [minor] There should be no trailing spaces
   +231: [minor] There should be no trailing spaces
   +232: [minor] There should be no trailing spaces
   +238: [minor] There should be no trailing spaces
   +244: [minor] There should be no trailing spaces
   +258: [minor] There should be no trailing spaces
   +264: [minor] There should be no trailing spaces
   +270: [minor] There should be no trailing spaces
   +276: [minor] There should be no trailing spaces
   +279: [minor] There should be no trailing spaces
   +285: [minor] There should be no trailing spaces
   +315: [minor] There should be no trailing spaces
   +346: [minor] There should be no trailing spaces
   +348: [minor] There should be no trailing spaces
   +349: [minor] There should be no trailing spaces
   +350: [minor] There should be no trailing spaces
   +352: [minor] There should be no trailing spaces
   +353: [minor] There should be no trailing spaces
   +355: [minor] There should be no trailing spaces
   +356: [minor] There should be no trailing spaces
   +357: [minor] There should be no trailing spaces
   +367: [minor] There should be no trailing spaces
   +390: [minor] There should be no trailing spaces
   +402: [minor] There should be no trailing spaces
   +429: [minor] There should be no trailing spaces
   +441: [minor] There should be no trailing spaces
   +460: [minor] There should be no trailing spaces
   +482: [minor] There should be no trailing spaces
   +483: [minor] There should be no trailing spaces
   +512: [minor] Missing period
   +514: [minor] There should be no trailing spaces
   +516: [minor] There should be no trailing spaces
   +524: [minor] Use an indent of 2 spaces, with no tabs
   +527: [minor] Use an indent of 2 spaces, with no tabs
   +535: [minor] Use an indent of 2 spaces, with no tabs
   +539: [minor] Use an indent of 2 spaces, with no tabs
   +543: [minor] There should be no trailing spaces
   +544: [minor] There should be no trailing spaces
   +551: [minor] There should be no trailing spaces
   +553: [minor] There should be no trailing spaces
   +556: [minor] There should be no trailing spaces
   +570: [minor] There should be no trailing spaces
   +572: [minor] There should be no trailing spaces
   +575: [minor] There should be no trailing spaces
   +577: [minor] There should be no trailing spaces
   +591: [minor] There should be no trailing spaces
   +592: [minor] There should be no trailing spaces
   +594: [minor] There should be no trailing spaces
   +599: [minor] Use an indent of 2 spaces, with no tabs
   +601: [minor] Use an indent of 2 spaces, with no tabs
   +603: [minor] Use an indent of 2 spaces, with no tabs
   +633: [minor] Use an indent of 2 spaces, with no tabs
   +635: [minor] Use an indent of 2 spaces, with no tabs
   +637: [minor] Use an indent of 2 spaces, with no tabs
   +639: [minor] Use an indent of 2 spaces, with no tabs
  
  Status Messages:
   Coder found 1 projects, 1 files, 91 minor warnings, 0 warnings were flagged to be ignored
  

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

  api/discussit.oauth.php:10:  //var $apibase = 'https://account.discussit/API/v2_0/DiscussIt.svc/';
  discussit_live_moderator.module:554:	//edit-subject
  discussit_live_moderator.module:568:	//TODO: add form element with diRt
  discussit_live_moderator.module:571:	//jQuery("#di_replyto").text("hello!");
  

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

  ./api/discussit.oauth.php:83:  $redirect = "https://www.discussit.com/") //redirect?
  

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

  ./.gitignore:                         ASCII text, with no line terminators
  
  

• Do not use t() in hook_schema(), this will only generate overhead for translators.
  

    'description' => t('Comment Lookup Table for DiscussIt Module'), 
    'drupal_cid' => array('description' => t('Primary Key: Drupal Comment ID'), 
    'discussit_cid' => array('description' => t('Discussit Comment ID'), 
  

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

  ./discussit_live_moderator.module ./discussit_live_moderator.install ./README.txt ./discussit_live_moderator.info ./api/discussit.php ./api/discussit.oauth.php ./discussit_live_moderator.admin.inc
  

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 note: I see that you have created a git TAG instead of a branch. Maybe that is related. Please check with http://drupal.org/node/1127732 again and let me know once you are ready for the next attempt. :)

I've gone through most of the points raised but I am still confused about the (\n) thing.

Also I've tried to move to a new branch but I get the following response:

Peter-Bennetts-MacBook-Pro:discussit_live_moderator peterbennett$ git checkout 7.x-1.0
Already on '7.x-1.0'
Peter-Bennetts-MacBook-Pro:discussit_live_moderator peterbennett$ git add -A
Peter-Bennetts-MacBook-Pro:discussit_live_moderator peterbennett$ git commit -m "moved to branch"
# On branch 7.x-1.0
nothing to commit (working directory clean)
Peter-Bennetts-MacBook-Pro:discussit_live_moderator peterbennett$ git push origin 7.x-1.0
pete.discussit@git.drupal.org's password: 
Everything up-to-date
Peter-Bennetts-MacBook-Pro:discussit_live_moderator peterbennett$ 

Yes that is all a little bit tricky. As for me, it took me about three attempts before I no longer overlooked the colon that prefixed the now emptied branch (git push origin :master). (See http://drupal.org/node/1127732). Sure you didn't miss it? ;)

Ok. It's in there now.

No, sorry, your master branch is still not empty: http://drupalcode.org/sandbox/charlie73/1133136.git/tree/refs/heads/master

Ok it's empty now.

It's not empty - you can see the contents at http://drupalcode.org/sandbox/charlie73/1133136.git/tree/refs/heads/master

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.

I have also attached another review as of the most recent versions of the reviewing tools. Maybe you want to check again.

Thanks Dave.

I've pushed the latest version for review at http://drupalcode.org/sandbox/charlie73/1133136.git/commit/2fce4068c3943...

And I've cleared the master branch.

Thanks Dave.

I've pushed the latest version for review at http://drupalcode.org/sandbox/charlie73/1133136.git/commit/2fce4068c3943...

And I've cleared the master branch.

Pete, I know this is fussy with git. And, congrats, the master branch is empty now. But still you will not be able to create releases on d.o, as you do not meet the naming conventions. For deploying and maintaining projects here it is really, really important to be familiar not just with git but also with the additional conventions here on d.o. Please check with the git docs again, at least with this: http://drupal.org/node/1015226 as there is no way around it. (You will understand why, once you start to promote your project.)

Aside this, there remain some minor complaints from drupalcs, but that's not much at all.

Unfortunately the queue is very full again, so I have to postpone a next deeper insight until your next fixes. Feel free to help a bit with the queue in the meantime to accelerate the process.

Thanks for all your help. I'll familiarise myself properly with the naming conventions before I do any more commits.

Ok. I've moved the incorrectly named (7.x-1.0) branch to dev release (7.x-1.x) branch as per: http://drupal.org/node/1015226

Do I need to add a release tag to it or does that come later?

Thanks.

You just have to create tags, if you want to create a release (what is only possible after your project is a full one)

See http://ventral.org/pareview/httpgitdrupalorgsandboxcharlie731133136git

I've dealt with all the issues thrown up by pareview.sh. see here:

http://ventral.org/pareview/httpgitdrupalorgsandboxcharlie731133136git

I've run it through coder on strict and the only issues left are ones I cannot fix, as that would require changing code on the API, which the developers are not willing to do.

Have I understood correctly that I do not need to create tags yet, and the git repository is in order?

Kind regards,

Pete Bennnett

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. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• discussit_live_moderator_requirements(): "Simple Paypal could not be installed" copy & paste error?
• discussit_live_moderator_install(): empty function, remove it.
• drupal_uninstall_schema() is done by Drupal 7 automatically, so remove that line.
• discussit_live_moderator.css: Replace the file comment with a standard @file comment.
• discussit_live_moderator_schema(): Take a look at the hook_schema() example for the formatting of the nested arrays.
• discussit_live_moderator_uninstall(): You are deleting your own info file here, why?
• "define('COMMENT_WITHDRAWN', 2);": all constants should start with the name of your module.

Thanks. I've gone through the points raised in: http://drupal.org/node/1145422#comment-5420508

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.

manual review:

• discussit_live_moderator_uninstall(): $base_url is unused.
• discussit_live_moderator.module: why do you need the global require_once statements. You should only include your code if you actually need it, not on every page request.
• "'access arguments' => array('administer site configuration'),": this permission is too generic, create your own.
• discussit_live_moderator.admin.inc: this file should be specified in hook_menu(), then you don't need to require it yourself.
• discussit_live_moderator_comment_insert(): indentation errors on the db_insert() call, try to make it more readable. Also, you should consider to use drupal_write_record() here instead.
• "$di = new discussit();" the class has been renamed, no? Please test all your code that it actually works with your changes.

I know, getting feedback on your application took long, you can speed up the process with #1410826: [META] Review bonus.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.