[D7] The City Group Map

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

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.

Multiple Applications

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

Project 1: https://drupal.org/node/2090475

Project 2: https://drupal.org/node/2219823

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.

After fixing above listed issues.
Instead of using the below statement in module file.
require_once(DRUPAL_ROOT . '/sites/all/libraries/city-api/lib/jnet-ca-main.php');
Please use libraries api module and declare it as dependent .(use libraries_load).

A littile confusion I am referring to your master branch.Please delete your master branch and keep only the 7.x-1.x .And mention the updated branch issue description as I can see cleanup branch also:).
@ branch 7.x-1.x
@line 145 -- tc_group_map.admin.inc

/**
 * CG Group Admin form submission callback.
 */
function tc_group_map_admin_form_submit($form, &$form_state) {

It is recommended to use drupal commenting
For example:
use
(/** Implements hook_form_submit ......) and write the

/**
 * CG Group Admin form submission callback.
 */

comment below or inside the function.
Its applicable for all the hooks you used in the tc_group_map.admin.inc file (if any,in the complete module).

Go to your project -> version control ->Version to work from * select 7.x-1.x -> Make it default branch -> try to delete master branch using command line. It should work.

For coding standards:
Refer Hook implementation Documentation uses a short format:
https://drupal.org/coding-standards/docs

Thanks.

I have reviewed and it looks good.

Second that. I've reviewed the code and shared a few minor coding standard notes with @ericthelast. Things look good here from my perspective.

To set a default branch use https://drupal.org/node//edit/default-branch, so it should be https://drupal.org/node/2213255/edit/default-branch for you.

To delete the master branch both locally and remote use:

git branch -d master
git push origin :master

Yes, you are prohibited because it's still the default and you don't see the option to set another default branch, because you are on the wrong page. You are on https://drupal.org/project/2213255/git-instructions, but you should be on https://drupal.org/node/2213255/edit/default-branch

The link is on your project page at "edit" -> "default branch"

Okay, it's really not there. For me there is a second level menu below "edit" which has two links: "project" - the page you see, and "default branch". Did you activate git access in your profile? Otherwise, I don't know I am just a normal drupal user. You might want to ask on IRC or report a bug.

Eric, Sorry for the delay. I can't approve your application, but I can give this another look and a thorough review.

Automatic Review:

FILE: /var/www/drupal-7-pareview/pareview_temp/tc_group_map.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
144 | WARNING | Unused variable $tag_id.
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/tc_group_map.city_webhook.inc
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
16 | WARNING | Line exceeds 80 characters; contains 94 characters
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/tc_group_map.module
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
239 | ERROR | Data type of return value is missing
362 | ERROR | Data type of return value is missing
395 | ERROR | Data type of return value is missing
507 | ERROR | No key specified for array entry; first entry specifies key
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/tc_group_map.pages.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
54 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
54 | ERROR | Function comment short description must end with a full stop
--------------------------------------------------------------------------------

Address these when you can; all look minor.

Manual Review

Project page looks good, but separate out and elaborate on the installation instructions. I would also provide sample paths for the third-party libraries, as some people have trouble extracting these and often end up with an extra directory.

(*) Your README is really lean. Mirroring the project page would be a good start. Also mention the new paths in admin/content

(*) tc_group_map_admin_form_submit() has a syntax error on line 162; a paren is in the wrong place.

(*) I get some errors when I try to edit a group? Looks like the form name is wrong.

Your configuration URL should be specified in the .info

If possible, the code should reference the TC API docs when you make calls out / get requests back. It will help
others contribute to the project.

(*) You have a settings form w/ variables, but no hook_uninstall() to delete them.

Does anything in the settings form need to be required?

tc_group_map_groups_admin_form(), line 83, why is the URL commented out?

The classes in the .css file look a little too generic; they may interfere with other modules/themes.
I would suggest making them a little more specific.

There are some untranslated strings in the Javascript. May want to pass these in through a setting.

You are missing the passed in context in a few places in the jQuery.

JS needs some .once() to make it AJAX friendly.

(*) Why does $items['tc-group-map/group/%tc_group_map_group/%'] need 'access argument' == TRUE?

(*) tc_group_map_menu_title_callback() returns an unfiltered string. When you define your own title callback, you need
to sanitize it.

Is DRUPAL_NO_CACHE really needed in tc_group_map_block_info()? What makes this uncachable? You can manually clear the $cid for that block when the group content changes.

Your tc_group_map_block_view() should use #attached instead of drupal_add_js()

In tc_group_map_group_address_geocode(), don't build up the URL with string functions like that. Use the query
option in url(), or use drupal_http_build_query(). Also, avoid curl(), and try to use drupal_http_request().

I don't think you need the sensor=true in tc_group_map_library().

Your use of request_path() is a little weird. Don't think I have ever see it used this way. I think you will have problems on sites where it isn't installed in the root directory, or on path-based multilingual sites.

You may eventually want a hook_requirements() to warns users on the Status Report if the library isn't installed
properly.

In tc_group_map_group_request_form(), you have an untranslated string.

What is your hook_tc_group_map_group_request used for? The standard practice to to make a tc_group_map.api.php with
some stub examples.

Markers. Personally, I don't like building up the infoWindow in JS. Making a theme function to render them out, and
then adding them to the settings for each marker may be better. The JS would then just take this HTML and stuff it
into the infoWindow.

(*) The data going to the group page callbacks (eg, tc_group_map_group_page) is unfiltered and vulnerable to XSS.
To test this, set the name / other data to <script>alert('XSS')</script>
in your sample_data.json and see what happens before and after the check_plains(). I think the group data going to
the map via settings is OK because of the implicit drupal_json_encode (and I think the Google Maps filters this, too),
but I ran into too many errors to really test this out. Double check all of that. Please don't remove the security tag, we keep that for statistics and to show examples
of security problems.

The starred items are blockers because of documentation, security, and API issues, and also because the errors make it
harder to test; the rest are suggestions.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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