[D7] SalesCloud

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

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 Helgi Andri Jónsson,

I understand it takes great effort to make your code comply to drupal standards :)

One thing I noticed while manually reviewing your code :

FILE : salescloud.admin.inc
1) Add a blank line at end of salescloud_admin_form() function.

I think it will be good to run your module against the auto code sniffer at pareview.sh and iron out the very few standardization issues you may find there.

Hello Helgi,

Coding standards in your code are followed in correct way.
But there are some things that I am concerned about.

1. I see that you've implemented 7 functions that just load variables. In general variable_get function is a common way to set form element's default value or set if condition. That will reduce your code with 43 lines.
   I suppose that you used them to beautify your code. In my opinion more efficient way is to have a single function that returns static array variable with all values.
2. I am a bit concerned about salescloud() function. Each time it is executed a new SalesCloud object is created. Is this neccessary? I think that better way is to use Singleton pattern, or at least put this object to a static variable.
3. Please put required modules dependencies into salescloud.info file.
4. Try to put salescloud.api.inc file into files[] section of module info file instead of using require_once.

Thanks for your work!

Hi Helgi. Here's a manual code review, based on the Project Application Review Template.

Recommendations/Questions are in bold italics

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation, as I could not find any other Drupal SalesCloud integrations.

Master Branch

[Yes: Follows] the guidelines for master branch, and the correct git clone command

Licensing

[Yes: Follows] the licensing requirements, as sandbox project uses GNU GPL v2, and there are no other licenses cited.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code, as none are included. One question: It looks like the Oauth2 Guzzle plugin is included in composer.json, but I didn't see Guzzle 5, though that is listed as a dependency too on the project page. Was that forgotten, or is it included somehow?

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template, with all the necessary information in README.txt.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[Yes: Meets] the security requirements as far as I can tell, though I am not familiar with the SalesCloud or Guzzle APIs and best practices. There are no writes to the db and proper handling of user input via standard drupal admin form.

Coding style & Drupal API usage

[List of identified issues in no particular order]

• You can specify a minimum PHP version in .info, instead of needing to do so in hook_requirements.

Nothing big, I can't see any blockers. Good work!

Some more reviews in addition to #9

   *
   * @return SalesCloud
   */
  public static function Instance($client_id, $secret, array $scope, $username, $password) {
    static $inst = null;
    if ($inst === null) {
      $inst = new SalesCloud($client_id, $secret, $scope, $username, $password);
    }

A better approach would be to declare a class level instance, instead of the function/method level static you are using:

  static $instance;

  public static function Instance($client_id, $secret, array $scope, $username, $password) {
    if (static::$instance === null) {
      static::$instance = new SalesCloud($client_id, $secret, $scope, $username, $password);
    }
    return static::$instance;

Also, nitpick, Drupal standards require method names to start with lowercase and use camelCase

  private function __construct($client_id, $secret, array $scope, $username, $password) {
    $this->base = 'https://salescloud.is';
    .
    .
    .
  public function connect() {
    $return = array();

    $client = $this::client();

    try {
      $res = $client->post($this->base . '/api/2.0/system/connect', array('json' => array()));

Rather than hardcoding api paths, it would make sense to have them as settings (or at least constants) for more flexibility.

I also echo #9.1 about the use of variables namespace. It would be nicer if you bunched all of those into one 'salescloud_auth' setting or so. It's good practice to avoid overbloating the variables table. Consider if every module did the same thing?

@Helgi, these are not blockers, just recommendations.

@almaudoh: anything else that you found or should this now be RTBC?

@klausi, I think this should be RTBC, even though I didn't test the API all the way through since I have no salescloud account.

@Helgi, in the salescloud.api.inc, where http requests are made in the try block, the code in the catch block assumes that there is always a response. I had an instance where there wasn't a response, so I had to change the code to something like below to make it work.

    }
    catch (RequestException $e) {
      if ($e->getResponse()) {
        $return['headers'] = $e->getResponse()->getHeaders();
        $return['url'] = $e->getResponse()->getEffectiveUrl();
        $return['reason'] = $e->getResponse()->getReasonPhrase();
        $return['status_code'] = $e->getResponse()->getStatusCode();
        $return['protocol'] = $e->getResponse()->getProtocolVersion();
        $return['data'] = array();
      }
    }

Review of the 7.x-1.x branch (commit 363f4d1):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/salescloud.api.inc
  ----------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  ----------------------------------------------------------------------
   24 | ERROR | Visibility must be declared on property "$instance"
  ----------------------------------------------------------------------
  
  Time: 627ms; Memory: 12Mb
  

• 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. "variable_set('salescloud_settings', $settings);": all variables defined by your module need to be removed in hook_uninstall(). Looks like you are now using only one variable?
2. salescloud.api.inc: why do you build the $return arrays everywhere? I think it would be much more useful if you just return the actual response objects that people can use in a more readable manner.

Otherwise looks good to me.

Thanks for your contribution, Helgi!

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.