[D7] Yandex Store Locator

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,

Automated Review

There is only one git issue. Look on results:
http://pareview.sh/pareview/httpsgitdrupalorgsandboxcharlsbharath2738515git

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

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

No: Does not follow the guidelines for master branch. Look on Documentation on setting a default 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, but I think it can be supplemented.

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

1. I think, it would be better to move 'drupal_add_css' and 'drupal_add_js' into preprocess function of the module file.

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.

Hi, thanks for your contribution here are my observations.
1)In .info file dependencies are mentioned as
dependencies[] = jquery_update:jquery_update
dependencies[] = libraries:libraries

it should be
dependencies[] = jquery_update
dependencies[] = libraries

2)It seems hook_help() is not there. Please add hook_help().

3)As per README.txt i placed Handlebars JS in the sites/all/libraries/handlerbar folder, but its showing following error
http://localhost/handlebars.min.js?oez64d
yandex_store_locator.js?oez64d:123 Uncaught ReferenceError: Handlebars is not defined

its taking path of Doc root.

4)Variables "yandex_store_locator_map_lat", "yandex_store_locator_map_lon", and "yandex_store_locator_json_url" are configuration variables and should be deleted on module un-installation. Check hook_unistall().

5)As per json data I entered Duis in search field, selected 100 kilometres and All Categories but its showing no stores found
Refer Attachment.

Thanks.

Hello @charls.bharath,

Command line version of Coder shows following warnings, please check it.

FILE: ...ampp\htdocs\d7\sites\all\modules\yandex_store_locator\README.txt
---------------------------------------------------------------------------------------
 16 | WARNING | [ ] Line exceeds 80 characters; contains 82 characters
 19 | WARNING | [ ] Line exceeds 80 characters; contains 94 characters
 22 | ERROR   | [x] Expected 1 newline at end of file; 0 found
---------------------------------------------------------------------------------------

FILE: ...es\all\modules\yandex_store_locator\yandex_store_locator.install
----------------------------------------------------------------------------------
 4 | ERROR | [x] The second line in the file doc comment must be "@file"
----------------------------------------------------------------------------------

FILE: ...tes\all\modules\yandex_store_locator\yandex_store_locator.module
----------------------------------------------------------------------
  60 | ERROR   | [x] Expected 1 space after IF keyword; 0 found
  60 | ERROR   | [x] There should be no white space before a closing  ")"
  60 | ERROR   | [x] Expected 1 space after closing parenthesis; found 0
  70 | ERROR   | [x] Expected one space after the comma, 0 found
  72 | ERROR   | [x] Expected one space after the comma, 0 found
 107 | ERROR   | [x] Expected 1 space after closing parenthesis; found 0
 109 | ERROR   | [x] Concat operator must be surrounded by a single space 
 109 | WARNING | [ ] Avoid backslash escaping in translatable strings when possible, use "" quotes instead
 109 | ERROR   | [x] Concat operator must be surrounded by a single space

Thank you,
AshwinSh

The automated review didn't return anything, everything's fine code style wise.

I took a more in-depth look at the code, here are my findings:

Manual Review

1. • In .install file:
   • The dependencies are still in project:module format as bhavesh.rohida pointed out; since your dependencies have the same name as their projects, it's not needed
   • The package property should be user for projects that contain more than 1 module; this should be removed
2. • Regarding libraries:
   • Is the handlebars dependency really required here ? I only see a few tags in templates that could easily be replaced with their PHP counterparts
   • If the answer to the previous question is yes, the dependency should be more obvious (at the moment there's only a reference in README.txt - and a minor typo in there 'handle=R=bar'); so maybe implement a hook_requirements, alongside with updating project's description; and maybe moving the library from /sites/all/modules/handlebar to handlebars to match library's name
3. • In .module (I tested this on my local machine, so results may be different on an public web server):
   • At line 67, $base_url shouldn't be needed to prefix the path to json; for me, it didn't work in place
   • Also, still on line 67, if the user didn't submit the admin settings form, there won't be a value for variable_get(); this should have a default like in admin settings form

I think this are all minor recommendations, none of them are blockers or get the project back to 'Needs work'. So maybe after/if they get applied, the project can move to RTBC. Goodluck.

Please change back the priority to Normal after doing a review.

Automated Review

Pareview details: https://pareview.sh/pareview/https-git.drupal.org-project-yandex_store_l...

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

• Your README.txt does not follow best practices (headings need to be uppercase). See https://www.drupal.org/node/2181737 .
• 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.

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

No: Does not follow the guidelines for in-project documentation and/or the README Template. See pareview

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

1. (*) in yandex_store_locator.info add spaces after equal signs:
   

   name = Yandex Store locator
   description = Yandex Store locator is module which is using Yandex maps API
   core = 7.x
   

2. (*) Use an indent of 2 spaces, with no tabs inyandex_store_locator.js and yandexstorelocator.css (https://www.drupal.org/docs/develop/standards/coding-standards#indenting)

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.

If you are still working on this application, you should fix all known problems and set the status to Needs review. (See also the project application workflow.)
Please don't change status of this application if you aren't sure you have time to dedicate to this application, or it will be closed again as won't fix.

I am closing this application due to lack of activity.