[D7] ActionKit

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

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.

Hi Nicole Benes,

Please do proper indentation(https://drupal.org/coding-standards ).

Check your code with https://drupal.org/project/coder module there is so many warnings.

Also 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

Hi Nicole,

Now there are no more errors in pareview.sh: http://pareview.sh/pareview/httpgitdrupalorgsandboxnicolebenes2085167git

I've found a little error with the coder (https://drupal.org/project/coder):

actionkit.petition.inc

Line 546: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
  $file = file_save_data($csv->data, "public://" . $_POST["page_id"] . "-" . rand(0, 1000000) .  ".csv", FILE_EXISTS_REPLACE);

Also, as Niteshp already said, you are still using the master branch. Please, follow the steps 1, 5, and 6 of https://drupal.org/node/1127732

Manual review:

• Array indentation in actionkit_menu() (actionkit.module) and some others functions seems to be a little different from the standard. Please, take a look on these standards: https://drupal.org/coding-standards#array
• There are some string missing t(). Some examples:
  • actionkit.donate.inc: Missing t() on 'Thank You Message' at line 57
  • actionkit.mail.inc: Missing t() on 'Fund this campaign' at line 48
  • actionkit.petition.inc: Missing t() on 'Petition Name' at line 24
• Theme files
• Javascript code could be removed from template files and organized in .js files
• Forms could be created using FAPI (https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...)

I'll add the translate functions to the strings. Thanks Rodrigo.

Added the t() functions, now I'm going to see if I can fix the array formatting.

I took a stab at making the js in the tpl files external, but I didn't finish. Hopefully Nicole can fix my mistakes.

http://drupalcode.org/sandbox/NicoleBenes/2085167.git/commit/cbe72d5

If you can fix up these last few items, your project will be in pretty good shape.

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
Review of the 7.x-1.x branch:

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

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.

FILE: /var/www/drupal-7-pareview/pareview_temp/includes/actionkit.general.inc
--------------------------------------------------------------------------------
FOUND 10 ERROR(S) AFFECTING 10 LINE(S)
--------------------------------------------------------------------------------
 261 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
 263 | ERROR | else must start on a new line
 272 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
 274 | ERROR | else must start on a new line
 285 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
 287 | ERROR | else must start on a new line
 299 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
 301 | ERROR | else must start on a new line
 310 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
 312 | ERROR | else must start on a new line
--------------------------------------------------------------------------------


FILE: /var/www/drupal-7-pareview/pareview_temp/js/nodeakdonation.js
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 190 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

Source: http://pareview.sh/ - PAReview.sh online service

PAReview.sh test looks good now!

Just one thing found from brief code review: Do you need this block in "node--actionkit_donation.tpl.php:" ?

Hi.

Git link in description is not fixed yet.
Please change
git clone --branch master NicoleBenes@git.drupal.org:sandbox/NicoleBenes/2085167.git
to
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/NicoleBenes/2085167.git actionkit

That is not an application blocker, please do a real review of the source code.

Hi Nicole,

I've just take a look at this, and PAreview is throwing a bunch of warnings - mainly around formatting of documentation comments - so should be pretty easy to clean up.

http://pareview.sh/pareview/httpgitdrupalorgsandboxnicolebenes2085167git

Other issues I noted from a review of the code:

• In actionkit_uninstall() in actionkit.install.inc the comments don't match the code - see lines 150,158,161,169.
• In actionkit.user.inc, line 20 has a hardcoded URL which should almost certainly not be, also the text here should be marked as translatable
• In actionkit.petition.inc it looks like you're passing SQL to a 3rd party endpoint (Line 384). While this doesn't impact on the security of this module per-se I'd be severely concerned about the security of that API if it allows sites to pass it arbitrary SQL to run. You should review that URGENTLY if it's your API endpoint.
• Also in actionkit.petition.inc, line 545 it looks like you're dumping a file into the public filesystem, can you re-assure us that the contents of that file are suitable for being available externally - it's not too clear from reading the code what that specifically contains.
• In actionkit.general.inc, line 203 it would be nice to make this list alterable so other modules can add / amend this list. I can imagine other countries may want to change the list of words that are considered profane
• In actionkit.donate.inc, line 217, this message contains reference to a specific donation cause - this whole block of text should be configurable / translatable and/or just updated to pull in the names correctly.
• In theme/node--actionkit_donation.tpl.php, line 39 - these variables should all have a module-specific prefix to avoid conflicts. Ideally you should use Drupal.settings to pass these.
• In nodeakdonation.js, line 29 it looks like you're submitting data direct to a standalone script that then bootstraps Drupal - I can't see a good reason for this rather than just submitting to a proper ajax endpoint handled through Drupal's normal menu / routing. The same applies to all of the files in the php/ folder.
• Also in nodeakdonation.js - it looks like you're handling credit card data, but there's nothing that checks that the URLs are operating over SSL which there should be - or these details should be sent to the actionkit endpoint from JS rather than posting to a potentially insecure server.
• Not sure what the purpose of the files in the SQL folder is - can they be removed?

Hi Nicole,

Thanks for replying and no worries about the PAreview errors - I know how easy it is for those to crop up :)

I had a look at your comments, and the code changes you'd made. Your replies cover most things I raised - thank you for the explanation of the public file storage. On the basis of what you've said - that seems OK in the public filesystem.

I'd be nervous about marking this as RBTC while the module is posting SQL to a 3rd party endpoint, hopefully someone else will have a stronger opinion on whether that's acceptable or not. I understand that it's not something that is the fault of the module, but it might be helpful if you have any documentation that covers this to post a link here - does the 3rd party have a statement about this?

I also can't see that the following points are covered - I'd personally consider these blockers to getting this approved at least without a solid description of why it's necessary to do it this way.

• In nodeakdonation.js, line 29 it looks like you're submitting data direct to a standalone script that then bootstraps Drupal - I can't see a good reason for this rather than just submitting to a proper ajax endpoint handled through Drupal's normal menu / routing. The same applies to all of the files in the php/ folder.
• Also in nodeakdonation.js - it looks like you're handling credit card data, but there's nothing that checks that the URLs are operating over SSL which there should be - or these details should be sent to the actionkit endpoint from JS rather than posting to a potentially insecure server.
• I read your notes about the files in the sql/ folder, but I'm still no clearer. It doesn't look like the module code uses them - are they just documentation? If so, they should probably be moved into documentation - either in the readme file, or an install.txt, or a documentation page on drupal.org

I've left this as "needs review" rather than setting back to "needs work" as another reviewer may have a different opinion, but I'd suggest that some more detail, or changes on those points would help.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2327911

Project 2: https://www.drupal.org/node/2131947

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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