This module retrieves weather data from the Wunderground API and display this in two block, one for current weather conditions and one for weather forecasts. The data is obtained via a REST call and is cached, so that the number of call to the API is limited. It contains and easy settings form to fill in the api key and to select the location for which the weather data should be displayed.
Project page: https://drupal.org/sandbox/jeroen_drenth/2104663
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/jeroen_drenth/2104663.git wunderground_weather

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

AjitS’s picture

Status: Needs review » Needs work

Hi, please change the git link in your original post for non-maintainers. Cloning using the original link asks for your d.o user password.

Also, check for the issues reported by pareview :
http://pareview.sh/pareview/httpgitdrupalorgsandboxjeroendrenth2104663git

jeroen_drenth’s picture

Issue summary: View changes

changed git link for non-maintainers.

jeroen_drenth’s picture

Issue summary: View changes

changed git to branche.

jeroen_drenth’s picture

Hi AjitS,

I've changed the link and also solved the issues reported by pareview.

jeroen_drenth’s picture

Status: Needs work » Needs review
t14’s picture

Status: Needs review » Needs work

Hi

You have a few spelling errors in your comments on line 81, 113, 130.

You may want to use filter_xss() at some some point on the data you are obtaining from the API calls. Just to be on the safe side.

Personally I would cache the data rather than store it in your own custom tables. This would be a better light weight solution than creating a hook_schema on install and relying on a cron job. It will also allow you to set a time when you want the cache to expire and make a new API call. You can set it within the code or make it possible for your users to decide via you admin settings form.

So rather than save to the database on wunderground_weather_get_forecast_data() and wunderground_weather_get_current_data() you can make use of cache_set and cache_get ()
https://api.drupal.org/api/drupal/includes!cache.inc/function/cache_set/7
https://api.drupal.org/api/drupal/includes!cache.inc/function/cache_get/7
Much less code and more functionality.

If you do decide to stick with the cron method, you will need to remove the “TODO” in your schema definitions and give a valid description for each field. You may also want to consider implementing hook_cron_queue_info to queue your cron jobs. Will allow your functionality to work successfully in something like a big site that has a lot of task to do during a cron job, hence meaning that your cron job may not run if cron timeouts.
Here is a good tutorial for implementing hook_cron_queue_info
http://www.metaltoad.com/blog/handling-long-running-background-tasks-dru...

Hope that was of help to you.
T

jeroen_drenth’s picture

Status: Needs work » Needs review

Hi T,
Thank you for your feedback. It was very useful. The module is now using cache instead of custom tables. The user can also now choose wether he wants to cache the data or not, by checking a box on the settings page.

jeroen_drenth’s picture

Issue summary: View changes

Updated git link

PA robot’s picture

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.

PA robot’s picture

Issue summary: View changes

Updated module description.

Enxebre’s picture

Hi jeroen_drenth,
I think you are using hook_theme in a wrong way.

There should be just one implementation of hook_theme nor two, this is the correct one for you "wunderground_weather_theme()", line 55.

The "function wunderground_weather_forecast_theme " should be called "function theme_wunderground_weather_forecast($variables)"

Then You will be able to generate the html calling $your_html = theme('wunderground_weather_forecast, $variables)

or creating a render element (with #properties) which matchs with your definition in hook_theme and passing it through "drupal_render " function

https://drupal.org/node/933976

Hope this helps.

Regards!

Enxebre’s picture

Status: Needs review » Needs work
jeroen_drenth’s picture

Status: Needs work » Needs review

Thank you, Enxebre. I've changed the way the module uses hook_theme.

AjitS’s picture

Status: Needs review » Needs work

automation issues

Code Sniffer found the following issues.

FILE: ...trib/sites/all/modules/wunderground_weather/wunderground_weather.module
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
  52 | ERROR | Multi-line array contains a single value; use single-line array
     |       | instead
 261 | ERROR | Whitespace found at end of line
 264 | ERROR | Whitespace found at end of line
 304 | ERROR | Whitespace found at end of line
 307 | ERROR | Whitespace found at end of line
 336 | ERROR | Whitespace found at end of line
 339 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

manual review

You have a form function which will set the variables on form submit. You should also have an .install file which should implement hook_uninstall(), deleting all the variables using variable_del().

In the .info file, use only single full stop. Not a blocker but it is good to keep things clean :-)

In .module's hook_menu implementation, there is no need of / for the 'file' key.

Rest all looks good.

jeroen_drenth’s picture

Thanks Ajits. I added the hook_uninstall and changed the .info file.

AjitS’s picture

Status: Needs work » Needs review
Warning: Missing argument 1 for wunderground_weather_location_autocomplete() in wunderground_weather_location_autocomplete() (line 82 of /var/www/drupal_projects/contrib/sites/all/modules/wunderground_weather/wunderground_weather.admin.inc).
Notice: Undefined variable: text in wunderground_weather_location_autocomplete() (line 83 of /var/www/drupal_projects/contrib/sites/all/modules/wunderground_weather/wunderground_weather.admin.inc).
Warning: Missing argument 1 for wunderground_weather_location_autocomplete() in wunderground_weather_location_autocomplete() (line 82 of /var/www/drupal_projects/contrib/sites/all/modules/wunderground_weather/wunderground_weather.admin.inc).
Notice: Undefined variable: text in wunderground_weather_location_autocomplete() (line 83 of /var/www/drupal_projects/contrib/sites/all/modules/wunderground_weather/wunderground_weather.admin.inc).

These errors occured randomly, I got it at admin/config/user-interface/wunderground-weather menu item.
You should change the

function wunderground_weather_location_autocomplete($text) {

to

function wunderground_weather_location_autocomplete($text = '') {

which provides default value to $text and get rid of the warnings. These are not blockers, hence not changing the status to needs work.

jeroen_drenth’s picture

Thank you again! Changed it to $text = ''.

AjitS’s picture

Status: Needs review » Needs work

Tested this functionally, and works good. The only thing I notice is incomplete is README.txt file.
Please read Module documentation guidelines, on how the README.txt should be.
Also, you show help in UI by implementing hook_help() in your .module file. Also, some screenshots on the project page would be great.
Other than that this looks like RTBC to me :-) Good work.

AjitS’s picture

On further testing, I want to add some points to the above review:
When I don't input any Display name in the settings form. I see something like this in the screenshot in attachment #2 below (was not able to bring that up in the editor).
It shows a message like:

Weather forecast for

above the block. This is ambiguous. To avoid this, you should mark this field as '#required' => TRUE, in your hook_form() implementation.
Some suggestions for the fields that require some changes IMO are

  1. Wunderground API key - make it as a required field.
  2. Wunderground API url - make it not accessible. So, that user's don't change it by mistake.
  3. Display name - make it required, or change the text above the block to make sense.
  4. Get location path - Change the label to just Location Path.

In screenshot #1, there is a blank in the first list element. Should either be populated with something, or the element should be removed when no value is present.

I think this will be a very useful module. Might end up using this in some of the projects. Nice work :-)

jeroen_drenth’s picture

Status: Needs work » Needs review

Once again some very useful comments. Thanks again. I have altered the settings form as you suggested.

AjitS’s picture

Looks good. RTBC from my side :-)
Just one thing though, I think it would make much more sense if you move the configuration under Web Services (admin/config/services); as it makes use of an external service.
Lets wait for some other reviews too. You might want to apply for a review bonus to speed things up.

robinvdvleuten’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

kscheirer’s picture

Title: Wunderground weather » [D7] Wunderground weather
Status: Reviewed & tested by the community » Fixed
  • You should use drupal_strtoupper() in wunderground_weather_get_forecast_data().
  • In theme_wunderground_weather_forecast(), always call theme functions like theme('table', $variables);. That lets theme overrides work properly. If you call the function directly (theme_table()) then no overrides are possible. Same with theme_item_list() and theme_image().
  • In wunderground_weather_get_current(), don't break up the t() strings when creating the $items array - translators need context in order to do it properly. Instead, pass the entire string to t(), and use variable substitution to include your data.
  • I don't think you need the filter_xss() call in the wunderground_weather_api_key description in wunderground_weather_admin_form(). But its harmless.

Thanks for your contribution, jeroen_drenth!

I updated your account to let you 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:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get 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.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Highlighted git