Google Data Visualization

Cool initiative and good intergration with views. Looking through the code it seems well documented. Also clear instructions on how to use the module.

There code style issues found, that are easy to fix:

http://ventral.org/pareview/httpgitdrupalorgsandboxsrutheesh1876988git

It seems to be a very cool tools!

So far no errors blocking my testing... a few things I'd like to mention.

- In the Views UI, I think it would be better to display Google Data Visualization instead of google_data_visualization.
- I know the display is done through JS, but could it be possible to display in the preview section?
- Played around, not too sure how to set fields in the X/Y axis, can you provide more document?

http://drupal.org/project/visualization
http://drupal.org/project/chart

This sounds like a feature that should live in the existing projects. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the appropriate issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

Hello,
Thank you for great module!

Manual review:

1. In File google_data_visualization.info you have declared twice dependencies[] = views, at Line: 3 and Line: 4
2. Then I enabled the module my local environment crashed.
Here is a snippet from apache error.log

[Thu Jan 10 10:47:45 2013] [error] [client 127.0.0.1] PHP Parse error:  syntax error, unexpected '=' in /var/www/drupal-7.18/sites/all/modules/google_data_visualization/includes/google_data_visualization.views.inc on line 85

Don't forget to add the "PAReview: review bonus" tag as indicated in #1410826: [META] Review bonus, otherwise you won't show up on my high priority list.

Reviewed module comment link updated.

Still, you forgot to add the "PAReview: review bonus" tag?

Removing the other strange tag.

See this screenshot where to add issue tags:

Please make sure to do more comprehensive manual reviews for your next review bonus, not just posting output of automated review tools.

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/google_data_visualization.info
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   7 | ERROR | It's only necessary to declare files[] if they declare a class or
     |       | interface.
   8 | ERROR | It's only necessary to declare files[] if they declare a class or
     |       | interface.
  --------------------------------------------------------------------------------
  

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:

1. google_data_visualization_help(): all user facing text must run through t() for translation.
2. google_data_visualization_init(): why do you need hook_init()? You should only add your JS to pages where you actually display charts?
3. google_data_visualization_views_api(): "'api' => 2.0,": Vies 2 does not exist in Drupal 7?
4. hook_google_data_visualization_skin_info(): Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php
5. google_data_visualization_google_data_visualization_skin_info(): empty function, so remove it.
6. theme_google_data_visualization(): use theme('item_list', ...) or similar instead of creating your own list markup.
7. google_data_visualization_views_add(): Views 2 support in Drupal 7 does not make sense?
8. Fatal error: Call to undefined method views_handler_field_node::is_value_empty() in includes/google_data_visualization.views.inc on line 314
9. README.txt: how do I setup my data structures? Where do I specify which fields of my entity should be used for the chart? Please add an example.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

I'm confused - which branch should I review, master or 7.x-1.x? Because the most recent commits seem to be on master?

Master branch is still there - did you forget to push your changes?

http://drupalcode.org/sandbox/srutheesh/1876988.git

I have removed all the updated files to 7.x-1.x branch and i have removed the master branch. I have incorperated all the mentioned changes to 7.x-1.x branch.

Master branch is still there - did you forget to push your changes? See http://drupal.org/node/1127732

http://drupalcode.org/sandbox/srutheesh/1876988.git

This is rad -- I'm working on a visualization distribution and this seems to fill a need I have. Will review it over the next hour.

Also and unrelated: I invite you to join my burgeoning Drupal Visualization group which discusses all things viz-y. We'd love to have you with us!

If you haven't yet, fix everything mentioned by Klausi in his earlier review! Please don't mark issues as "Ready for Review" again until you've done everything requested in previous reviews -- thanks!

✗ Automated Review

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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.

FILE: ...eview/pareview_temp/includes/google_data_visualization_style_plugin.inc
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
  13 | ERROR | Class name must begin with a capital letter
  13 | ERROR | Class name must use UpperCamel naming without underscores
  17 | ERROR | Method name
     |       | "google_data_visualization_style_plugin::option_definition" is
     |       | not in lowerCamel format, it must not contain underscores
  17 | ERROR | No scope modifier specified for function "option_definition"
  41 | ERROR | Method name
     |       | "google_data_visualization_style_plugin::options_form" is not in
     |       | lowerCamel format, it must not contain underscores
  41 | ERROR | No scope modifier specified for function "options_form"
 159 | ERROR | No scope modifier specified for function "validate"
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

✗ Manual Review

• ✓ README.txt -- Good README. Some minor proofreading things that shouldn't stop RTBC:
  • ⓘ ln. 12, col. 25 -- "data" is the plural of "datum"; not "datas".
  • ⓘ ln. 12, col. 32 -- "Google" is spelled wrong.
  • ⓘ ln. 14, col. 63 -- You probably don't need to reiterate "on your Drupal site"; it's somewhat self-explanatory to just say "Install the Views module (url) if you haven't already."
  • ⓘ ln. 23, col. 14 -- Remove two extra spaces before "Continue".
  • ⓘ ln. 26, col. 30 -- Remove "the".
  • ⓘ ln. 27, col. 7 -- Should be "setting".
• ✓ google_data_visualization.info -- Good! Proofreading on description: "Provides a View style that displays rows as a Google chart. This API depends on Google Charts." -- Note, that the word "depends" in documentation generally means it requires another module to work! You can probably just cut the second sentence in that line.
• ✗ google_data_visualization.module -- In depth:
  • ⓘ ln. 19, 86 -- Proofreading: Remove two extra spaces.
  • ✗ google_data_visualization_help() starting at ln. 12 -- You define $fieldset, yet you don't do anything with it. You should probably just forego the variable and return your output directly to prevent possible notices (As per the comment at http://api.drupal.org/comment/18499#comment-18499). Regardless, you should wrap all of that text in t() functions. See: http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7
• ✓ js/google_data_visualization.js -- Just sets some values. Minor.
• ✗ includes/google-data-visualization-view.tpl.php -- You shouldn't have raw JavaScript here. You should use drupal_add_js() to add the JavaScript, and then use Drupal.settings to supply values to it. See: http://drupal.org/node/756722
• ⓘ includes/google_data_visualization.views.inc -- I can't help but feel assigning all of that stuff via $_REQUEST is unnecessary and is supplied elsewhere, somehow -- there might be some way of passing the $view object directly, though I'm pretty unfamiliar with Views programming and am not certain. Hopefully somebody else more knowledgeable can comment. Looks fine otherwise -- though I echo Klausi's point that you probably don't need Views 2 support!
• ⓘ includes/google_data_visualization_style_plugin.inc --
  • ⓘ ln. 24, did you mean "tahoma" and not "thahoma"?
  • ⓘ ln. 56 -- You probably shouldn't use markup in #value. Add "
    
    " and its closing tag to "#prefix" and "#suffix" respectively.
  • ⓘ ln. 64 -- This might have the same issue as ln. 56; granted, you're using #prefix a bit differently in this case.
  • ⓘ lns. 89, 98, 107, 116, 124, 130, 145, 153 -- Proofreading: remove spaces before and after the period.

  Note that anything marked as "proofreading" (Or otherwise has a "ⓘ" icon) is offered as a suggestion to improve readability and should not be seen as a blocking issue.

  Runthrough

  I created a new view of type Google Charts Visualization, added two fields to it (Differing date fields I had displaying as simply unixtime), turned pager off, visited view page: everything seems to be working.

  Earlier, I had a non-numeric field ("Title") as my first field and got the following notice:

  Notice: Undefined index: exclude in template_preprocess_google_data_visualization_view() (line 282 of /sites/all/modules/google_data_visualization/includes/google_data_visualization.views.inc).

  Again as per Klausi, having a more detailed walkthrough would help users a lot.

  Conclusion

  Looking good! Fix the various coding style issues and fix the JavaScript adding issue -- and you'll be on your way! But again, you should really look at Klausi's earlier review if you haven't yet (Some things like having support for Views 2 kind of suggests this...).