[D7] Trebutra - Trello Bug Tracker

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxMattWithoos2268803git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hello MattWithoos,
I can't clone your repo.

$ git clone --branch 7.x-1.x MattWithoos@git.drupal.org:sandbox/MattWithoos/2268803.git trebutra___trello_bug_tracker
Cloning into 'trebutra___trello_bug_tracker'...
MattWithoos@git.drupal.org's password:
Permission denied, please try again.

Manual Review.

Edit the description to add a link to http://pareview.sh/pareview/httpgitdrupalorgsandboxmattwithoos2268803git... This will make it easier for future reviewers.

Your CHANGELOG.txt has version numbers, but these don't exist yet.

Your hooks don't have the proper docblock. See https://drupal.org/coding-standards/docs#drupal, eg

/**
 * Implements hook_help().
 */
function trebutra_help($path, $arg) {

You have a settings form that defines variables, but no hook_uninstall to delete them().

You aren't using the second parameter to variable_get() to define a default value.

You should really move your setting form to a .admin.inc file

I would define my own permission for administering the module. This allows finer grain control for larger organizations.

Long term, I would also implement a hook_requirements() to warn about the configuration values.

Long term, you may want to move the settings into the block itself, in case you ever add support for multiple blocks to allow it to be used for multiple projects in a single site.

Why are you using cURL directly and not drupal_http_request()?

Your form has a submit handler, but that is not how you are actually submitting to Trello?

You are defining an #action for the form, but no path to handle it?

trebutra_api_call_to_trello() is calling mail() directly and not drupal_mail().

In trebutra_block_view, the second argument to drupal_get_form() is supposed to be an extra argument that gets passed to the form builder function. You are using this for a side effect to actually post to Trello.

You have unfiltered user input going to output (well, mail in this case). You need to filter this with check_plain(), and don't forget that this also needs to be done for any settings that come from the UI. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This looks like it could be a useful module, but you have some big problems with the Form API, and you are also bypassing the Drupal API as mentioned above.

Mathew, looking better.

Your check_plain() usage is wrong. You need to do something like

$title = check_plain($_POST['title']);

and then use $title in the code below. In other words, check_plain() returns the sanitized string, not modify it directly. See https://drupal.org/node/101495 and https://drupal.org/node/28984 for more info.

The alternate PHP syntax should really only be used in template files, so convert these control structures to use braces.

A comment in trebutra_api_call_to_trello() to the API docs for that endpoint would be nice.

Could you use https://drupal.org/project/trello as a dependency? Does your module duplicate any of this functionality?

You still have an no-op action in trebutra_bug_form():153. I think this can go.

I would rename trebutra_form() to trebutra_settings_form() to make it obvious what its function is

The email subject line should be run through t().

Your drupal_set_message() on line 103 should probably use the second argument to show it as a warning.

The check_plain() usage is a blocking issue; the rest are suggestions.

Please describe the differences to the existing trello project on your project page, so that users can make an educated decision which module to use in which case.

@Matt Withoos, first of all thanks for contribution!

Getting close!

Some minor suggestions, as @mpdonadio suggested above,

The alternate PHP syntax should really only be used in template files, so convert these control structures to use braces.

Still in your .module file you are using these in following function:

function trebutra_api_call_to_trello() {

  $trello_key          = variable_get('trebutra_key');
  $trello_api_endpoint = variable_get('trebutra_apiend');
  $trello_list_id      = variable_get('trebutra_listid');
  $trello_member_token = variable_get('trebutra_token');
  $enableemail         = variable_get('trebutra_emailsend');
  $reportemail         = variable_get('trebutra_email');
  if (isset($trello_key) && isset($trello_api_endpoint) && isset($trello_list_id) && isset($trello_member_token)):
    // Checks if form was submitted.
    if (isset($_POST['title'])):
      .....
      endif;
    endif;
  else:
    drupal_set_message(t("One or more Trebutra configuration settings are missing. Please set them before using the module."));
  endif;
}

You should move your setting form to a .admin.inc file as suggested by @mpdonadio also.

Your help is too small, try to extend this little bit more. And also move this function to top of module as this is general practice to keep hook_help in top of .module file.

/**
 * Implements hook_help().
 */
function trebutra_help($path, $arg) {
  switch ($path) {
    case "admin/help#trebutra":
      return '<p>' . t("Integrates Trello as a Bug Tracker") . '</p>';

  }
}

Rest looks good to me, please fix these changes and I'll move this to RTBC.

Thanks Again!

When I am trying to access admin/config/content/trebutra page, it is showing me following error.

Please fix this issue to review it further.

In module file, it should be something like this:

/**
 * Implements hook_menu().
 */
function trebutra_menu() {
  $items = array();
  $items['admin/config/content/trebutra'] = array(
    'title' => 'Trebutra',
    'description' => 'Configuration for Trebutra (Trello Bug Tracker) module.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('trebutra_settings_form'),
    'access arguments' => array('administer site configuration'),
    'type' => MENU_NORMAL_ITEM,
    'file' => 'trebutra.admin.inc'  
  );
  return $items;
}

Review of the 7.x-1.x branch (commit 103e992):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

manual review:

1. trebutra_api_call_to_trello(): doc block is wrong, this is not a hook so it should not start with "Implements ...". See https://www.drupal.org/coding-standards/docs#functions . Same for trebutra_bug_form(), see https://www.drupal.org/coding-standards/docs#forms
2. "'subject' => "New bug report for Epic Collaboration",": all user facing text must run through t() for translation. Same for "'#value' => 'Save',", please check all your strings.
3. trebutra_api_call_to_trello(): why do you access the global $_POST here? Is that a form submission? Why can't you use the usual Form API submit handlers? Ah, I see: in trebutra_bug_form_submit() you call this function without parameters. Instead, you should pass $form_state['values'] to it, which contains the submitted values.
4. trebutra.module: the TODO list is not up to date and you should use @todo, see https://www.drupal.org/coding-standards/docs#todo

The usage of $_POST is a pretty severe API misuse and a blocker right now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Few minor issues.
FILE: /var/www/drupal-7-pareview/pareview_temp/trebutra.module

--------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------------
104 | ERROR | else must start on a new line
177 | ERROR | Do not use t() in hook_menu()
178 | ERROR | Do not use t() in hook_menu()
--------------------------------------------------------------------------------

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*) I tested your module and assigned "Trello Bug Tracker" block on one page. I just submitted the form and get 404 page in response.
   
   
   

   I gone through your form code and found $form['#action'] = url('submit-bug'); but submit-bug page is not there in your code, that's why it producing 404. Also fix the notice Notice: Trying to get property of non-object in trebutra_api_call_to_trello() (line 87.

2. In trebutra_api_call_to_trello form you are using PHP cURL to send http get request, I would recommend you to use drupal_http_request() to hit the external URL.
3. Instead of using json_decode, use drupal_json_decode.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Currently first point looks blocker and stopping me to move it to RTBC, rest looks good to me.

Thankyou Matt Withoos for you hard work and patience!

manual review:

1. the points mentioned from the automated review are not fixed yet.
2. trebutra_api_call_to_trello(): using @description here for t() in the message body will result in double escaping, since you already made sure that all parts in $description are sanitized. You can use the "!" placeholder instead which will pass through the variables without sanitization. Not sure why you need the check_plain() calls anyway - whatever you are posting to trello should be sanitized by them anyway. Remember: only use check_plain() when you are printing something directly to HTML. Forwarding data does not count as printing to HTML.
3. trebutra_api_call_to_trello(): why does this function need to receive the whole $form_state? It has no business with it, only with the submitted values? If I want to use trebutra_api_call_to_trello() from another module I have to prepare a fake form state. Instead I would just like to pass the parameters it needs explicitly: the description, email etc.

But that are not critical application blockers, otherwise I think this is RTBC.

Assigning to mpdonadio as he might have time to take a final look at this.

Automated Review

Review of the 7.x-1.x branch (commit 592632a):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Could be expanded.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes/No. If "no", list security issues identified.

Coding style & Drupal API usage

Remove the .gitignore from the repo

Is there any validation you can add to trebutra_settings_form()?

Your code is a little dense. Some blank lines at appropriate places would make it easier to read, such as between the form elements in trebutra_settings_form()

trebutra_api_call_to_trello() has a bunch of variable_gets() w/o default values.

drupal_http_request() is preferred over curl().

drupal_http_build_query is preferred over http_build_query().

I would have a separate permission for you settings page.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Agree with @klausi's points. Not seeing any blocking issues.

Thanks for your contribution, Matt Withoos!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

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.

Thanks to the dedicated reviewer(s) as well.