[D7] Acquia Cloud Dashboard

Looks like this needs review? See https://drupal.org/node/532400

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.

Updated Reviewed Proj List

Hello saitanay,

Unfortunately I don't have an Acquia cloud login, but I have some notes here that might help in the mean time until you get a better review.

Issues Summary

• PAReview, as of 3:14am GMT has the following minor error:
  There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
• There is a security concern - it appears you store the Acquia Cloud username and password in plaintext.
• (minor) adding doxygen comments where you list parameters, return types for all of your functions are really helpful to help us understand how your functions work (unless they are hook implementations) - the coding standards site has some good guidance on this - https://drupal.org/node/1354.
• Add a hook_uninstall() function in a "acquia_cloud_dashboard.install" file that would remove variables you create in your module.

Basic application checks

• Application has a git repo and link to the project page.
• Project doesn't seem to be a duplication of existing projects, but admittedly I don't know enough about Acquia cloud to make such a determination. The Acquia Insight (https://drupal.org/project/acquia_insight) and Acquia Network Connector (https://drupal.org/project/acquia_connector) might be related, but the author has explained their intentions in the project page.
• A cursory search didn't reveal multiple applications.

Repository checks

• Git repo does contain code.
• There is a 7.x-1.x revision, but the "master" branch still exists. See the "Issues" for more information.
• There's over 900 lines of code and more than 5 functions so there's enough handwritten code for review.

Security review

• PAReview and coder didn't return any security warnings, but does your module store the Acquia Cloud access credentials in plaintext in the database? That might be a security concern if the Drupal site's database gets compromised (I think Drupal salts and hashes the regular account passwords so if the database is compromised the adversary doesn't get the plaintext passwords).

Licensing checks

• There's no license.txt file in the Git repo.
• A visual inspection doesn't find any 3rd party/non-GPl code

Documentation checks

• PAReview didn't return any errors, but running your module through the coder module raised a "comment_comment_docblock_missing" for most functions when viewing minor errors (so very low priority fix). The Drupal doxygen page https://drupal.org/node/1354 has some guidance on documenting functions, parameters, return variables etc.
• README.txt file has a lot of detail - well done.
• There's a good amount of inline comments, but my feedback would be to be consistent with your conventions: you change between inline and multiline comments for in-code comments. Some functions in your "acquia_cloud_dashboard.pages.inc" file like "acquia_cloud_dashboard_report()", "acquia_cloud_dashboard_update_ssh_keys()" and "acquia_cloud_dashboard_update_dbs()" do some complex operations, so some inline comments would help.

Coding standards

• Coder/PAReview don't reveal any major issues.

API and best practices review

• I noticed you create several variables in the Drupal variable table, but I could not find a hook_uninstall() function to remove those variables. See the following link https://api.drupal.org/api/drupal/modules!system!system.api.php/function... for more information.

Hope this helps saitanay, and good luck.
ajosephau

Great work @saitanay. Happy to see a sandbox module with full documentation covered. Here is some manual code review comments:

1.   $items['admin/config/cloud-api'] = array(
       'title' => 'Acquia Cloud Dashboard',
       'description' => 'Control Acquia Cloud Hosting right from Drupal Site',
       'position' => 'left',
       'page callback' => 'system_admin_menu_block_page',
       'access arguments' => array('access cloud api report'),
       'file' => 'system.admin.inc',
       'file path' => drupal_get_path('module', 'system'),
     );
   

   Not sure why we need this? I don't get any page on this location. if it is just to have a separate section in config page, I guess we don't need the callback and file details.

2. Just a thought: when entering username/password at admin/config/cloud-api/configure it doesn't really do a validation on server side. Does it worth having?
3. $url = "https://cloudapi.acquia.com/v1/" . $method . ".json"; this can be a variable, so that if version change or sandbox/live environment changes can be managed easily.
4. Can't move CURD of domain/key (may be that needs username/password?)

Thanks for your quick reply on them @saitanay. Here is my updates on individual items in #15

#1 - awesome :)
#2 - It makes lots of sense now. Does it worth documenting it somewhere/add a note on the configuration page about 'check report page to see it work' or something.
#3 - Well, it still can be variable without UI (or just '#markup'). The reason is, consider if we get a dev & prod URIs and we have to make it as part of our build recipe for dev.
#4 - Ok thanks.

Review of the 7.x-1.x branch:

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

  
  FILE: /home/klausi/pareview_temp/acquia_cloud_dashboard.pages.inc
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   6 | ERROR | File doc comments must be followed by a blank line.
  --------------------------------------------------------------------------------
  

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.

1. acquia_cloud_dashboard_curl_caller_post(): Why can't you use drupal_http_request() and must use cURL? Please add a comment. Also you might want do a hook_requirements() if you require the cURL PHP extension, for example like http://api.drupal.org/api/drupal/modules!simpletest!simpletest.install/f...
2. "array("data" => "Site Name");": all user facing text must run through t() for translation, please check all your strings. This is not done in a couple of places like table headers.
3. "/* Get Task List for Site": inline single line comments should use the "//" syntax, see https://drupal.org/node/1354#inline
4. acquia_cloud_dashboard_update_generate_html(): this is vulnerable to XSS exploits. The third party data you retrieve from Acquia must be considered untrusted user provided input, so it needs to be sanitized before printing. Example: "$task_row[] = $value['description'];" should be "$task_row[] = check_plain($value['description']);". Make sure to read https://drupal.org/node/28984 again.

Otherwise looks pretty good! The security issue is a blocker right now.

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

Excellent. Thanks for taking time to make this module. Setting to RTBC, as it's been through enough reviews.

Your 2 constants should be namespaced to the module, NOT_AUTHORIZED_TEXT_RESPONSE_FROM_API should be ACQUIA_CLOUD_DASHBOARD_NOT_AUTHORIZED_TEXT_RESPONSE_FROM_API for example.
In acquia_cloud_dashboard_curl_caller_post() you can use http_build_query() to handle creating a param string from an array.

Those are not major issues though, looks like a great module!

Thanks for your contribution, saitanay!

I updated your account to let you 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 get 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.

----
Top Shelf Modules - Crafted, Curated, Contributed.