[D7] Nearby Places Search

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.

This is a manual review...

Manual Review

1. Basic application checks

   1.1 Ensure your application contains a repository and project page link.

   [Yes: Follows]

   1.2 Ensure your project is not duplication.

   [Yes: Does not cause] module duplication and/or fragmentation.

2. Basic repository checks

   2.1 Ensure the repository actually contains code.

   [Yes]

3. Security Review

   3.1 Ensure the project does not contain any security issues.

   [Yes: Meets] the security requirements. / No: List of security issues identified.

4. Licensing checks

   4.1 Ensure the repository does not contain a ‘LICENSE.txt’ file.

   [Yes: Follows] the licensing requirements.

   4.2 Ensure the repository does not contain any 3rd party (non-GPL) code.

   [Yes: Follows] the guidelines for 3rd party assets/code.

5. Documentation checks

   5.1 Ensure the project page contains detailed information.
   5.2 Ensure the repository contains a detailed README.txt.

   [Yes: Follows] the guidelines for in-project documentation and/or the README Template.

   5.3 Ensure the code contains a well-balanced amount of inline-comments.

   [Yes].

6. Coding standards and style

   6.1 Run an automated review and ensure there are no major issues.

   [Yes: Automated review is passed..]

7. API and best practices Review

   7.1 Ensure you are using Drupals API correctly.

   [Yes: Used Drupal API correctly.]

Hello. I've reviewed your module.

Some issues:

1. Why doesn't made you block for module?
2. Why you use bootstrap classes? Bootstrap is not default not specified in the list of dependencies.
3. Line 82: bad code - inline style.
4. Add settings button in module list.
5. Use $ for jQuery.
6. Use drupal behaviors in js (See: https://www.drupal.org/node/756722)

@D.push : Please change the status. Is this now RTBC after your review or are there other application blockers left?

Module looks good now. Changing the status to RTBC.

Hi ashwin.shaharkar,

I see some strings where you should use the t() function:
nearbyplace.module:122

'#markup' => '<button id="cust_btn" type="button" class="cust_btn">Search</button><br/>
              <div id="map-container"></div><br/>
              <input type="button" id="show_btn" value="show markers"/>',

to:

'#markup' => '<button id="cust_btn" type="button" class="cust_btn">' . t('Search') . '</button><br/>
              <div id="map-container"></div><br/>
              <input type="button" id="show_btn" value="' . t('show markers') . "/>',

js/nearbyplace.js
line:78
infowindow.setContent('<b>Name:&nbsp;</b>' + place.name + '<br><b>Address:&nbsp;</b>' + place.vicinity);
to:
infowindow.setContent('<b>' + Drupal.t('Name') + ':&nbsp;</b>' + place.name + '<br><b>' + Drupal.t('Address') + ':&nbsp;</b>' + place.vicinity);

line:114
else if (status === 'ZERO_RESULTS') {
instead you can use the built in constant string:
else if (status === google.maps.places.PlacesServiceStatus.ZERO_RESULTS) {

line:115, 194, 198
alert('No results found');
to:
alert(Drupal.t('No results found'));

Hi ashwin.shaharkar,

You should use placeholder here: (like in normal t() PHP function)
alert(Drupal.t('Geocoder failed due to: ' + status));
nearbyplace.js:183

see: https://www.drupal.org/node/323109

Hi ashwin.shaharkar,

I see more hardcoded strings which are not translatable,
for instance:
nearbyplace.module:128,133
'#attributes' => array('id' => 'latitude', 'placeholder' => 'Latitude'),
to:
'#attributes' => array('id' => 'latitude', 'placeholder' => t('Latitude')),
Sorry, I haven't realized earlier, so please check all of your files to find all of the hardcoded strings (if exist yet).

I think you should prefix variable name for types with your module name, to be sure your variable name quite unique in the variables table:
nearbyplace.module:59 (and all of the references for this)

$get_default_types = variable_get('types', array());// nearbyplace_types instead of types

Hi ashwin.shaharkar,

Great job.
Just one remaining thing:
I think it would be nice to create a module.install file and add uninstall hook where you delete those variables which are created by your module.
like this:

/**
 * Implements hook_uninstall().
 */
function nearbyplace_uninstall() {
  variable_del('nearbyplace_latitude');
  // And delete all of your variables.
}

I think it is not blocking problem, so I set RTBC status.

@ashwin.shaharkar,
what is the origin of all .png-image assets in http://cgit.drupalcode.org/sandbox-ashwin.shaharkar-2729005/tree/images ?

I've seen the same .png-files used on all sort of location applications all over the web for ages. Are these really your own work?

I am trying to work out the status of this item in the application review template:

It cannot be answered before we know whether these images are 3rd party or not.

I already knew those images came from Google (and I also knew they're free to download). I just wanted to make sure that you also knew that.

Please understand that Drupal.org has a policy on 3rd party libraries and content. We expect our users with access to upload to our git repo to respect that policy and we expect our reviewers in the PAreview queue to enforce it.

Here are the important parts from the page linked to above (with my empahsis):

• In general 3rd party libraries are forbidden, so do not commit any.
• The same policy applies to third party content, including media files (fonts, images, videos and sound) and text.

To progress, you need to remove all 3rd party files from the repo, and instruct your users in how to download them from Google themselves, and add them to your project after they've installed it.

Hi ashwin.shaharkar,

I edited my comment, please ignore it, I see you set the library as a dependency of your module.

I see trailing Tab characters in the README.md file (in line 19,22,25...)

Hi ashwin.shaharkar,

It seems OK, thanks.
(personally I like the new external icon version, because in this way we can replace icons easily if we want without touch the module code)

Removing review bonus tag as you have just posted the review template in the review https://www.drupal.org/node/2742721#comment-11263473
please do the proper reviews and add the review bonus tag again.

Hi ashwin.shaharkar,

Module looks good to me and works well with my installation.

Interesting project. Assigning to myself for a review.

Nice work with the module! And thank you for contributing!

Manual review

• As admin, I was only able to check/uncheck option 1 from the left checkboxes. Rest of the options were not selectable. Tested on Chrome + Firefox on Mac. I was able to select/unselect as anonymous user.
• Only one checkbox is selectable at a time (checkboxes are behaving like radios). Is this intentional? Can the form element type be changed to radios if only one category is supposed to be selected at a time?
• Rename the place_types.admin.inc file to prefix with module name.
• From the project page, consider removing the link to the project page, as you are on the same page :) Also, the git command is for your username. Rename it or remove it as it will not be needed once the module is promoted. Same with the README file.
• I found it tough to get the module working as getting the API key was not straight forward, and the link provided by the settings form pointed to the docs. Consider adding a direct link to get the key so that the setup is quick. Link - https://developers.google.com/maps/documentation/javascript/get-api-key
• Consider providing a validation function to the admin form to check if the key entered is valid.
• The markup of the module is generated directly from the form itself with #prefix/#suffix/#markup and the CSS file. Overriding this markup to fit the design requirements would be difficult. Might have to use !important to do it. Did you consider using a theme function to render your form. It can be done pretty easily - check http://drupal.stackexchange.com/questions/312/how-to-a-use-a-template-fi...

I would consider first couple of points that are blocking this application as of now. However, they could also be tracked as bugs in the issue queue. It would be good if these are resolved before creating a release.

P.S : I would suggest renaming the module to something like places_nearby, search_nearby_places, locations_nearby, search_nearby_locations to make it easily understandable. This is just a suggestion. The current name is fine too. If you are keeping the same name, it would be good if you could make it nearby_places_search (adding 's' to 'place').
Once fixed, you will not be able to change the the namespace of the module.

Also, good job with getting the review bonus!

Note: I am not using the complete Project Application Review Template, since it has been used in earlier reviews.

Some more observations:

• According to the coding standards, the hook function comment should be in format :
  

  /**
   * Implements HOOK_foo().
   **/
  

• I would recommend implementing hook_permission and defining module specific permission which you can use in your menu item.
• Consider implementing hook_help

Note: These are not application blockers.

Thank you! I will review it and update here.

Hello @ashwin.shaharkar,

This module is really interesting. I have installed and run your module successfully.
Getting some minor issue. Basically, it's a spelling error in the comment, which i got by running pareview.

Below is the link provided:
http://pareview.sh/pareview/httpsgitdrupalorgsandboxashwinshaharkar27290...

Please fix those error. Rest is ok for me.

@ashwin.shaharkar,

I have installed and run your module successfully with all new changes. I did not found any blocker.

So changing status to RTBC.

Thanks

Hi ashwin.shaharkar,

I installed and configured this module and see my comments below:

1. nearbyplace.markers library only providing marker images. I think this git library nearbyplace.markers.library is created by you only for this module. You can include one image by default within module, and then provide an admin config page to override default images for each type.
2. I installed and configured this module and assign Nearby Places Search block in to Sidebar first region. But then map content is overlapping with main site content. Even I assigned in to Sidebar first map is displaying inside main content. Please see attached screenshot.
3. It seems your javascript behaviors name is naturalessenceTheme. You can give a name related to your module.
4. Avoid multiple call to the same function drupal_get_path() in nearby_places_search_admin.inc.

Thanks,
ARUN AK

Hi ashwin.shaharkar,

I have done one more rodund of review. Please see my comments below.

1. It seems like you are using markup to create buttons. Use drupal form api button element to create button in form. Refer Form API Reference.
2. You are using nearby_places_search_admin.js file to make checkbox checked in admin configuration page. You can pass default values using variable_get() function itself like below.

   $get_default = variable_get('nearby_places_search_types', array('bank', 'atm', ...));

   There is no need to use of custom js file to set values. Found below in nearby_places_search_admin.js.

   (function ($) {
     'use strict';
     Drupal.behaviors.nearbyplacessearch = {
       attach: function (context, settings) {
         var chk_cnt = $('#adm-frm-types').find('input[type="checkbox"]:checked').length;
         if (chk_cnt === 0) {
           $('#edit-nearby-places-search-types-atm').attr('checked', 'checked');
           $('#edit-nearby-places-search-types-bank').attr('checked', 'checked');
           $('#edit-nearby-places-search-types-bus-station').attr('checked', 'checked');
           $('#edit-nearby-places-search-types-hospital').attr('checked', 'checked');
           $('#edit-nearby-places-search-types-park').attr('checked', 'checked');
           $('#edit-nearby-places-search-types-restaurant').attr('checked', 'checked');
           $('#edit-nearby-places-search-types-school').attr('checked', 'checked');
         }
       }
     };
   })(jQuery);

3. It seems like using custom library nearbyplace.markers.library for all marker images except main location marker. Main location marker image is hardcoded in js function.

   Found below in nearby_places_search.js line no 136.

   var imageUrl = 'http://chart.apis.google.com/chart?cht=mm&chs=24x32&chco=FFFFFF,008CFF,000000&ext=.png';

   It would be better you can provide admin configuration to alter marker images. Otherwise user/siteadmin should have FTP/File access to the server to alter a single marker image.

4. You are setting default address as "Pune, Maharashtra, India". But it is not marking by default in map after load the page/map. If not then remove default location from Address field/configuration.
5. I have a suggestion. It would be better you could remove the front-end css(nearby_places_search.css) from form elements. It is better you provide form elements with their default style, then it can easily adhere to the css of default front-end theme also users/developers can easily customize the elements by using their own css.

Thanks,
ARUN AK

Everyone, this project should not continue to be set back to Needs Work for little things. Per https://www.drupal.org/node/2453587, the only real reason something should be set back are for

Reviewers will manually review the project, checking only for problems related to licensing, security, or major problems with Drupal api use. Applications will not be blocked for minor coding standards violations, bugs, or project duplication.

Based on a quick read, nothing so far has been "major problems with Drupal API".

I checked again and seems like have fixed the issues mentioned in my previous comment. Also this module is satisfying the functionality defined in project page.

There are IMHO no more blockers. Moving to RTBC.

Thanks,
ARUN AK

Thanks for your contribution, ashwin.shaharkar!

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.