[D7] Weather Check

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

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 Vijaya Sankar,

I tried Weather Check module. Please see my observations below:

1) Add javascript in module files instead of adding in template file. I saw this in weather-status.tpl.php
<script type="text/javascript" src="http://maps.google.com/maps/api/js?sensor=false"></script>

2) Move css to the css file instead of adding as inline. weather-status.tpl.php Line no 2

3)Try to use of hooks to add module related js files. In weathercheck.module file line no 9-19 can move in to a hook and then it is more flexible to add condition like in which page need to add this js files. You can use hook_page_alter() hook to add these js.

$wc_latlong = array(
  'wc_lat' => variable_get('weathercheck_latitude', '10.8'),
  'wc_long' => variable_get('weathercheck_longitude', '78.7'),
  'wc_disp' => explode(';', variable_get('weathercheck_Disp_Info', '1;1;1;0;0;0;0;0;0;0;0;')),
);

drupal_add_js(array(
  'wc_latlong' => $wc_latlong,
), 'setting');
drupal_add_js('http://maps.google.com/maps/api/js?sensor=false', 'external');
drupal_add_js(drupal_get_path('module', 'weathercheck') . '/js/AdminMap.js');

4) What is the use of 'Update' button in configuration page? By default drupal system settings form is coming with 'Save Configuration' button. Update button is not working as expected. You can show a preview in right side when click on 'Update' button.

5) Once I save the configuration page, then Weather Check block only displaying the MAP not displaying any weather information.

6) Use Google Maps module to include map js. Getting error in console : https://www.drupal.org/files/issues/map_js_error_1.png

7) Use Drupal Behavior in your javascript code. Check weathercheck.js and AdminMap.js.
Refer:

• https://www.drupal.org/node/756722
• https://www.lullabot.com/articles/understanding-javascript-behaviors-in-...

Thanks,
ARUN AK

Hi Vijaya Sankar N,

Automated Review

There are still some problems on pareview.

Manual Review

Individual user account

Yes

No duplication

Yes

Master Branch

Yes

Licensing

Yes

3rd party assets/code

Yes

README.txt/README.md

Yes

Code long/complex enough for review

Yes

Secure code

Yes

Coding style & Drupal API usage

Regarding @ARUN AK at #6, Here are some other recommendations you could follow:

1. You should provide the hook_help to allow site builders to find information about your module using Drupal UI.
2. Naming your variables with lowercase, I think is better. for example: weathercheck_Disp_Select
3. In your codes, you include 3rd party libraries with drupal_add_js. It's better to use hook_libraries_info for this purpose. Or you could use other existing library modules.

This review uses the Project Application Review Template.

That's all, Thanks.

Hi Vijaya,

Errors in Automated Review:

js/AdminMap.js: line 13, col 24, Error - "google" is not defined. (no-undef)
js/AdminMap.js: line 14, col 21, Error - "google" is not defined. (no-undef)
js/AdminMap.js: line 17, col 20, Error - "google" is not defined. (no-undef)
js/AdminMap.js: line 19, col 24, Error - "google" is not defined. (no-undef)
js/AdminMap.js: line 24, col 7, Error - "google" is not defined. (no-undef)
js/AdminMap.js: line 31, col 3, Error - "google" is not defined. (no-undef)
js/weathercheck.js: line 10, col 3, Error - "angular" is not defined. (no-undef)
js/weathercheck.js: line 25, col 27, Error - Do not nest ternary expressions (no-nested-ternary)
js/weathercheck.js: line 52, col 24, Error - "google" is not defined. (no-undef)
js/weathercheck.js: line 53, col 21, Error - "google" is not defined. (no-undef)
js/weathercheck.js: line 56, col 20, Error - "google" is not defined. (no-undef)
js/weathercheck.js: line 58, col 24, Error - "google" is not defined. (no-undef)
js/weathercheck.js: line 63, col 7, Error - "google" is not defined. (no-undef)
js/weathercheck.js: line 64, col 9, Error - "angular" is not defined. (no-undef)
js/weathercheck.js: line 69, col 3, Error - "google" is not defined. (no-undef)

Please, change the git clone command, replacing weather_check to weathercheck.

Automated Review

Some errors found: http://pareview.sh/pareview/httpgitdrupalorgsandboxvijayasankarn2544636git

Manual Review

Individual user account

Yes

No duplication

Yes

Master Branch

Yes

Licensing

Yes

3rd party assets/code

Yes

README.txt/README.md

Yes - but it would be good to follow exact README template.

Code long/complex enough for review

Yes

Secure code

Yes

Coding style & Drupal API usage

1. Use lowercase for variable names.
2. As you are adding the CSS and JS files hook_page_alter, you should not add them in the .info file.
3. On weathercheck_form function, you should use t() function to translate the text on the #prefix attribute.
4. On weathercheck_form function, the form element called 'weathercheck_Disp_Info' value is not clear, how about refactoring that code so it's not a line of 978 characters? That way, your code will be clearer.
5. Add @file comments to weather-status.tpl.php

Hi Vijaya,

Avoid using remote files :

drupal_add_js('https://ajax.googleapis.com/ajax/libs/angularjs/1.0.7/angular.min.js', 'external');
drupal_add_js('http://maps.google.com/maps/api/js?sensor=false', 'external');

I also found some errors for some lat and long, block displays "undefined" and NaN.

you need to fix these things.

Changing status as errors need to be fixed for NaN and Undefined.

@Vijaya

I think instead of displaying NAN or defined in case of values not available you can simply hide those parameters or you can display some message "Data not available" or something like that.

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

• ESLint has found some issues with your code (please check the JavaScript coding standards).

js/AdminMap.js: line 13, col 24, Error - "google" is not defined. (no-undef)
js/AdminMap.js: line 14, col 21, Error - "google" is not defined. (no-undef)
js/AdminMap.js: line 17, col 20, Error - "google" is not defined. (no-undef)
js/AdminMap.js: line 19, col 24, Error - "google" is not defined. (no-undef)
js/AdminMap.js: line 24, col 7, Error - "google" is not defined. (no-undef)
js/AdminMap.js: line 31, col 3, Error - "google" is not defined. (no-undef)
js/weathercheck.js: line 10, col 3, Error - "angular" is not defined. (no-undef)
js/weathercheck.js: line 52, col 24, Error - "google" is not defined. (no-undef)
js/weathercheck.js: line 53, col 21, Error - "google" is not defined. (no-undef)
js/weathercheck.js: line 56, col 20, Error - "google" is not defined. (no-undef)
js/weathercheck.js: line 58, col 24, Error - "google" is not defined. (no-undef)
js/weathercheck.js: line 63, col 7, Error - "google" is not defined. (no-undef)
js/weathercheck.js: line 64, col 9, Error - "angular" is not defined. (no-undef)
js/weathercheck.js: line 69, col 3, Error - "google" is not defined. (no-undef)

14 problems

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.

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 Follows the guidelines for in-project documentation and/or the README Template.

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

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. weathercheck_page_alter : you are not doing altering the page render array here.So use hook_page_build.
2. weathercheck_page_alter : Please use #atttached over drupal_add_js
3. weathercheck_page_alter : Wrong arguments in hook_page_alter Please see https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...
4. Use the project page template for the project page https://www.drupal.org/node/2190239
5. weather-status.tpl.php : There is ng-app and ng-controller . is the module using angular for displaying map ? Please specify about the angular usage on the project page. Also add the additional dependency if module has ? Can you clarify this ?

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.

RTBC it because not found any blocker so far
please do adress rest issues in #25 and add comments on #25.5
Assigning to heddn for final review,.

1. It is not suggested that author's place their name into the code. This is open source, so anyone can contribute to it, even in a sandbox project.
   weather-status.tpl.php, weathercheck.install, etc.:
   Vijaya Sankar N <https://www.drupal.org/user/3238637>
2. No need to add variable_set's in an install. Use variable_get('variable_name', 'default_value') instead.
3. Instead of using drupal_add_js/css, use #attached in hook_page_build()
4. weathercheck_form: Using global $conf is not needed. Just use variable_get. It does it all for you.
5. weathercheck.css, the IDs in the css are very, very common. You are going to break a site if you leave them. i.e. #map, #AdminMap. Use drupal_html_id() to create a unique ID. And use CSS classes instead.
6. In reference to passing a static html id, you can pass it up using js settings. Or if it would just be easier, prefix the html id with the module name so as to reduce the possibility to of conflicts.

There's also a hard-coded '$scope.city = 'Trichy';' This shouldn't be.

Un-assigning for now. None of these things are individually blockers, but there's a lot of stuff that could be improved on this module before it could be used in a live site.

I still noticed a non-module namespaced html id of 'YourElementId', but this is much cleaner now and is definitely not a blocker, although it should probably be fixed.

Thanks for your contribution, Vijaya!

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.