[D7] Insightly Integration for Drupal

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

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.

Automated Review

All clear.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

No: Does not follow the licensing requirements. Please specify the license for your module.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template. I think the information needed is there, just need to apply the template to it.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements. Could not spot anything suspicious

Coding style & Drupal API usage

1. (*) Please use drupal style form hook ie "hook_validate" and "hook_submit" insted of "hook_submithook"
2. (+) Module name is not consistent, please choose either "insightly_integration_for_drupal" or "insightly_drupal" and rename folder/files/function/variables accordingly.

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.

Is necessary use insightly_integration_for_drupal name, why not use just insightlyas a module name?

Just a "clean code" comment:

  $url = 'https://api.insight.ly/v2.1/Contacts';
  $ch = curl_init($url);

  $user = variable_get('insightly_integration_for_drupal_apikey', 0);
  if ($user == 0) {
    watchdog('insightly_integration_for_drupal', 'API key not set!');
  }
  $user = base64_encode($user);

  curl_setopt($ch,
    CURLOPT_HTTPHEADER,
    array('Content-Type: application/json',
      'Authorization: Basic ' . $user));
  curl_setopt($ch, CURLOPT_HEADER, 0);
  curl_setopt($ch, CURLOPT_TIMEOUT, 30);
  curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);

  curl_setopt($ch, CURLOPT_POST, 1);
  curl_setopt($ch, CURLOPT_POSTFIELDS, $contact);

  curl_exec($ch);
  curl_error($ch);

  curl_close($ch);

Is better to reuse, you isolate this curl request on another function.

Automated Review:

################################ Coder Sniffer #################################

FILE: ...nsightly_integration_for_drupal/insightly_integration_for_drupal.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 4 | ERROR | The second line in the file doc comment must be " * @file"
 5 | ERROR | The third line in the file doc comment must contain a description
   |       | and must not be indented
--------------------------------------------------------------------------------

I agree. insightly would be a better name!

Automated Review

Problems found in coder:

File: @file block missing (Drupal Docs) [comment_docblock_file] severity: normalreview: style_string_spacing
Line 53: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
    $email_contact = '{"CONTACT_INFO_ID":1,"TYPE":"EMAIL","SUBTYPE":"Work","LABEL":null,"DETAIL":"'   . $email_address . '"}';

All fine in Pareview.sh

Manual Review

Individual user account

Yes, follows the guideline.

No duplication

Yes, does not duplicate

Master Branch

Yes, follows the Master Branch.

Licensing

Yes, follows

3rd party assets/code

Does not use third party assets

README.txt/README.md

Yes, follows the guide lines

Code long/complex enough for review

Yes, exactly 5 functions and more then 120 lines of code.

Secure code

Yes, could not find any security problems

Coding style & Drupal API usage

1. (*) Row 16 needs to be $form['actions']['submit']['#submit'][] = 'insightly_integration_for_drupal_validate';. Otherwise you are overriding other modules possibility to alter that submit.
2. (+) Instead of using you own submit function, why don't you use hook_form_alter, then you won't need to add a global value.
3. (+) Row 179 and 184, use drupal_get_path instead of dirname(__FILE__). https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_...
4. (+) Your choice, but Drupal 7 core does not require cURL, so if you want to follow that standard you should use drupal_http_request instead of cURL, see https://www.drupal.org/requirements/php and https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_...
5. On row 14 you should probably add an isset() since an entityform might be added after setting up your module. I have not tried this though.
6. I'm not sure if this point make sense, so disregard it if you want - all the written json, isn't it more structure to create PHP objects/arrays and use json_encode?
7. Maybe you should not name your submit hook with the ending _validate, it's kind of confusing because of https://api.drupal.org/api/drupal/modules!node!node.api.php/function/hoo... :)
8. Row 15 should be two rows?
9. Do you really need to run the cURL call if you don't have an API key?

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.

Hello,

To get promoted faster you should try and get a review bonus:
https://www.drupal.org/node/1975228

First of all, this is not a requirement for a release, but it's always nice to have a clean automated review:
http://pareview.sh/pareview/httpgitdrupalorgsandboxteemuaro2348091git

manual review
1.
Your function documentations are not following the standards. Please read about commenting hooks here:
https://www.drupal.org/node/1354

2.
In the file: insightly_integration_for_drupal.module (line 65) you have written a json string.
Please consider using the drupal_json_encode function (https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_js...) for readability.

3.
As you are using variables, please include an .install file to delete these variable upon uninstalling the module.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Should I sign up for my own API key or is there a dummy one you could provide? I received the following field module notices (Drupal 7.43) upon install which was otherwise fine:

Undefined index: module in FieldInfo->prepareInstanceWidget() (line 591...field.info.class.inc)
Undefined index: module in FieldInfo->prepareInstanceDisplay() (line 628...field.info.class.inc)
Undefined index: module in FieldInfo->prepareInstanceWidget() (line 591...field.info.class.inc)
Undefined index: module in FieldInfo->prepareInstanceDisplay() (line 628 of...field.info.class.inc).
Undefined index: module in FieldInfo->prepareInstanceWidget() (line 591 of...field.info.class.inc).
Undefined index: module in FieldInfo->prepareInstanceDisplay() (line 628 of.../field.info.class.inc).
Undefined index: module in FieldInfo->prepareInstanceWidget() (line 591 of .../field.info.class.inc).
Undefined index: module in FieldInfo->prepareInstanceDisplay() (line 628 of ...field.info.class.inc)

@teemuaro

I found following warning coder module:

FILE: ...pos/pareviewsh/pareview_temp/insightly_integration_for_drupal.module
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
12 | WARNING | Format should be "* Implements hook_foo().", "*
| | Implements hook_foo_BAR_ID_bar() for xyz_bar().",, "*
| | Implements hook_foo_BAR_ID_bar() for xyz-bar.html.twig.",
| | or "* Implements hook_foo_BAR_ID_bar() for
| | xyz-bar.tpl.php.".
--------------------------------------------------------------------------

You should now be able to promote this module to a full project.

I think, but am not sure, that to get security coverage, you still need to jump through all the hoops, and go get review bonuses.
You also need a full release, not an alpha, RC etc.

@teemuaro - it doesn't look like you've done any reviews of other projects, I think that's what's holding this up now.

Add links to your reviews in the summary here, and then add the PAreview bonus tag.

Automated Review

Issues found when testing with paraview. I think you just need to add brackets like hook_form_FORM_ID_alter().
https://pareview.sh/pareview/https-git.drupal.org-project-insightly_inte...

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
Yes: Does not cause module duplication and/or fragmentation.

Master Branch
Yes: Follows the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.

Secure code
Yes: Meets the security requirements.

Coding style & Drupal API usage
No issues found

You should also do what @kattekrab mentioned, it will help you a lot.

@teemuaro - there have been many reviews of your project now teemuaro - have you completed any reviews of other projects yet?

That reciprocity, doing the same kindness to others as these folk have done for you, will help you get the security coverage you're looking for.

If you HAVE done those reviews - link to them in the issue summary here, and post an update.

More information on this process is here:

https://www.drupal.org/node/539608

https://www.drupal.org/node/1975228

Great! good work @teemuaro - thanks so much for your reviews!

Hopefully this will bump your module up the priority list for security coverage review!

Good luck and thank you also for your patience and persistence :)

This module has been reviewed a number of times, and the author has conduced other reviews for the bonus.

Let's set it RTBC

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.