[D7] Amazon SNS Endpoint

Multiple Applications

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

Project 1: http://drupal.org/node/1087242
Project 2: http://drupal.org/node/1929298

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.

Just for curiosity could you explain why you set variables in hook install instead of providing default value in variable_get()?
I'm not changing to 'needs work' because I'm not sure if there is a reason for that.

Hi @alexharries!

1) Project page

- Follow the Tips for a great project page. Add an Overview section, Requirements and Configuration. Try using <h3> tags for the titles of the sections.
- Give details about Requirements and Configuration. For example add the links for curl as also as for the Amazon SNS. You can provide a small description of this service also.
- Remove "1. Drupal 7" from requirements.

2) README.txt

- Try not to copy the text from the project page into the README.txt file. Provide here more details about Configuration as this module has to connect to a third party service.

3) Pareview

Please fix the errors from the Pareview test. They are easy to fix.

4) .info file

- Remove double quotes for the values. Follow instructions from Writing module .info files
- Remove the comment about configuration page ; Config path:.
- Move the configuration path under a group, probably "Web services" like this configure = admin/config/services/amazonsnsreceiver/settings. Change this path where necessary on other files.

5) .module file

- Don't use ':' at the end of the comments (minor). Examples on line 10, 13. See more about API documentation and comment standards
- Enable translations with t() for strings. Lines 47, 48, 110, 245 and maybe other.
- Remove useless (for coding) comments like this on line 60 (Code adapted...)
- Use plain english in comments. Don't use capitalized words. Example on line 149.

5) .install file

- Since there is a requirement for curl you must set it here. See how simplytest.install module is doing this check.
- When I disable the module the "amazon_sns_endpoint_path_original" variable is not deleted. You can add this variable on the hook_uninstall() function or make a general removall of all variables (just to be sure) like this:

function amazon_sns_endpoint_uninstall() {
  db_delete('variable')
  ->condition('name', 'amazon_sns_endpoint_%', 'LIKE')
  ->execute();
  cache_clear_all('variables', 'cache_bootstrap');
}

6) Real testing

- I get en error if I click the "Save Configuration" button on 'admin/config/amazonsnsreceiver' path.

Warning: Missing argument 3 for amazon_sns_endpoint_admin_form_submit(), called in MYDOMAIN\includes\form.inc on line 1464 and defined in amazon_sns_endpoint_admin_form_submit() (line 85 of MYDOMAIN\sites\all\modules\amazon_sns_endpoint\includes\amazon_sns_endpoint.admin.inc).

The correct code for this function is hook_form_submit($form, &$form_state).

Finally, can you provide a demo account/link to test this live? If you cannot I understand...

@alexharries I am afraid you forgot to update the git! :-)

Oh, I am really sorry! I updated another module! I will be back soon with with comments.

OK, I see no major errors now.
Just see again the warnings with Pareview (since they are about comments you can split these in more lines or minify them) and the ":" at the end of the comment lines.

You are right about menu title/description translations and watchdog translations (we are all new here!).

Well, I think I cannot check more things for this project. Mean, my knowledge stops here!
An administrator can review more the project (especially security aspects).

By the way, if I click on the test link (example.com/amazonsnsnotificationendpoint) I get some errors. Check that too.

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

manual review:

1. amazon_sns_endpoint_install(): no need to set variables on installation as you can make use of variable_get() with default values anyway. And storing NULL is totally redundant.
2. "variable_get('ost_customisations_sns_allowed_topic'));": wrong variable name? Also elsewhere.
3. "$json->TestToken == drupal_get_token('amazon_sns_endpoint_test')": use drupal_valid_token() instead?

Unfortunately I have to get some sleep now, but I smell a security issue in amazon_sns_endpoint_message_handler() as I don't see a mandatory check that verifies the authenticity of a request. Will get back to you later, setting back to "needs review" in the meantime.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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.

Since the variable names are wrong I cannot do useful security testing, please fix that first.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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