[D7] Comment QA

Your sandbox seems to be empty

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

Automated Review

http://ventral.org/pareview/httpgitdrupalorgsandboxjcmartinez1144278git

The automated review scripts are flagging you on quite a number of points. Please check the Drupal coding standards.

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

See attachment for more details

The module seems to be working fine. I could able to add question and answers as comments. But when I access admin/content/comment url there is a drupal_set_message with the comments array. It should be removed.

First off: Looks nice !

When you don't set Q&A capabilities for a content type it returns a notice:

Notice: Uninitialized string offset: 0 in comment_qa_form_alter() (line 127 of /sites/all/modules/review/comment_qa/comment_qa.module).

Use a isset($settings) to check if any settings are returned.

At the settings:

Defaul status for original (Q&A) comments
Defaul status for follow-up (Q&A) comments

change to

Default status for original (Q&A) comments
Default status for follow-up (Q&A) comments

Hi,
the code looks good.
manually reviewed on fresh drupal installation:

1) When you don't set Q&A capabilities for a content type it returns a notice:

Notice: Uninitialized string offset: 0 in comment_qa_form_alter() (line 127 of D:\xampp\virtual\drupal7.my\sites\all\modules\1144278\comment_qa.module).

2) When you don't set Q&A capabilities for a content type it returns a notice and tries to add a comment to a node:
You get

Error
The website encountered an unexpected error. Please try again later.

and details:

Notice: Uninitialized string offset: 0 in comment_qa_form_alter() (line 127 of D:\xampp\virtual\drupal7.my\sites\all\modules\1144278\comment_qa.module).
Notice: Undefined property: stdClass::$cqa in comment_qa_comment_insert() (line 282 of D:\xampp\virtual\drupal7.my\sites\all\modules\1144278\comment_qa.module).
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'cqa' cannot be null: INSERT INTO {comment_qa} (cid, cqa) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => 1 [:db_insert_placeholder_1] => ) in comment_qa_comment_insert() (line 284 of D:\xampp\virtual\drupal7.my\sites\all\modules\1144278\comment_qa.module).

Hi,

still can't "Add new comment" on an Article node on fresh Drupal installation - the `Comment QA` module was enabled - but I haven't appied any settings/configuration, just trying to add new comment produce an error:

Error
The website encountered an unexpected error. Please try again later. 

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'cqa' cannot be null: INSERT INTO {comment_qa} (cid, cqa) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => 2 [:db_insert_placeholder_1] => ) in comment_qa_comment_insert() (line 284 of D:\xampp\virtual\drupal7.my\sites\all\modules\1144278\comment_qa.module).

Reviewed manually, now works fine for me.

1. Your readme is quite detailed, please put the some more information onto your project page

2.  * 0 => usually means Disabled or Comments
    * 1 => usually means Questions
    * 2 => usually means Answers

   You should define() constants instead, that would really improve readability

3. variable_get('cqa_d...prefix all variables with the full module name
4. Your currently only deleting variable_del('comment_qa_' . $node_type); but there are more with cqa prefix
5. comment_qa_comment_viewif the hook does nothing, either remove it complete or comment it out completely, so it won't be implemented for nothing
6. * @todo Please document this function.better a bad description than none, some todos will never happen ;)
7. whenever possible you should use hook_form_FORM_ID_alter for all forms

That's it for now :)

1. .install: you don't have to uninstall the schema manually here, installing and removing schemas is handled automatically in d7
2. Note that when your module is installed through drush the links will not be clickable, generally I don't know how much sense such a message makes as pretty much any module needs permissions/configuration after it was installed. But that's your choice.

3. /**
    * Implements hook_help.
    *
    * Displays help and module information.
    *
    * @param path 
    *   Which path of the site we're using to display help
    * @param arg 
    *   Array that holds the current path as returned from arg() function
    */
   function current_posts_help($path, $arg) {

   This code will never be invoked because the prefixing module name is not correct.
   Also you don't have to make any documentation about parameters and returned variables of hooks if your using them the standart way (but okay if you like it that way).

4. it is unlikely to put php containing file into subdirectories (/includes) in drupal (especially if there's only a single file in that make more work then sense)
   By following this convention you can apply the following:
   Current: include_once DRUPAL_ROOT . '/' . $path . 'settings_comments.inc';
   Best practice: module_load_include('inc', 'comment_qa', 'settings_comments');
   Best performance: include_once dirname(__FILE__) . '/settings_comments.inc';
5. variable_get('comment_qa_' . $type, '');There's always an array expected from this, you should set array() as default value (even when this never happens)
6. don't use and/or, use &&/|| instead

7. if (arg(0) == 'node' && is_numeric(arg(1))) {
           $node = node_load(arg(1));....

   using menu_get_object() is probably a better solution here.

8. I actually don't like this comment_qa_purge() thing, I mean.. variables get created, then a variable gets created that contains the same variables, then we delete the other variables... this feels like a unnecessary overhead, IMHO you should find a better solution for this ;-)
   Also stuff like

   $defaults = comment_qa_elements_default();
   '#default_value' => isset($settings['comment_qa_switch']) ? $settings['comment_qa_switch'] : $defaults['comment_qa_switch'],
   

   makes your life complicated.
   just using variables the common way would make it more simple I think..
   generally you should not try to reinvent the configuration system ;-)

9. t(".... <em>@perm</em> ...", array('@perm' => 'administer comments')),
   If you want to keep the "administer comments" translatable you have to wrap it within t() separately. But I'm not sure whether inserting a hardcoded translatable string into a hardcoded translatable string makes sense (maybe you should just put them into one string). Anyway, you don't have to use em-tags here if you want to do it this way you can just use % instead of @.
10. array(0 => t("Comment"), 1 => t("Question"), 2 => t("Answer")) If you got the luck to have a list that is 1,2,3... you should make use of your luck and remove that numbers because php will count them this way automatically (and they look ugly and will make you more work if you got to change something later).
11. if I see this right your inserting the settings form when editing any content type, also if it's not configured yet and no type specific variable are set but are needed. So if you really want to use your configuration workaround functions, why not doing this:
    

    function comment_qa_get_settings($type) {
      return variable_get('comment_qa_' . $type, comment_qa_elements_default());
    }

    That would at least make it a little more simple.

12. Also keep an eye on the automated report: http://ventral.org/pareview/httpgitdrupalorgsandboxjcmartinez1144278git

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

i have install this module with core comment module. The question and answer commenting system is working fine but when you edit a question or answer comment and save it then it again appending a 'Q:' or 'A:' to comment subject. if want to make a existing comment to question or answer then this is not working.

i have attached a screen shot.

It passed PAReview, so that is good.

Manual Review:
I noticed that you are using get_t() in the install hook. In the install hook you should use st(). get_t() is for code that could be run durring install but doesn't have to be run at install time. Because this is the install hook and that will only run durring install time your should just use st().

In comment_qa_comment_load($comments) you are using case 1: case 2: etc. I would use defines to make the code a bit more readable. <- don't quote me on this, I am not sure of the recommended Drupal policy on defines.

Line 12 of the .module file has a @todo comment. You should resolve this @todo.

1. .module: 82: Your still generating the path - but you don't need and don't use it

2.   // Ensure translations don't break at install time.
     $t = st();

   Your doing that wrong - now install actually breaks ;-)
   Replace this with
   $t = get_t();
   Don't forget to test your module after you applied changes

3. As it makes no sense to show all settings on the content type edit form when "Add Question & Answer (Q&A) functionality to comments" is set to Disabled, I'd suggest you to hide all the other items by #states, See forms docu
4. The "Type of comment" selection looks pretty ugly, maybe you better use a select widget instead of radios
5. .module: 84: There's no reason to use a #hidden form value you can just put your value in directly into the form by prefixing it with '#'. that way there won't a html hidden field generated and no information exposed to the commenter.
6. point 7 of comment #17 was not addressed - did it not work out for you?
7. .module 158: your still using "or" instead of ||
8. please use more self-explaining variable names then $v, $k
9. consider using drupal_write_record() instead of handling with insert's directly - it possibly takes a lot of work of your shoulders
10. As the comment_qa_settings_submit() function belong to the comment form you should also put it into settings_comment.inc (btw the usual name of the setting-form containing file is $modulename.admin.inc)

After all I don't see major issues with this module, therefore (after one year) let's push this forward

(no further feedback for a week, I think I was nitpicking enough here)

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.