Fix generate_html method in googlecharts.rb #84

Prakriti-nith · 2018-03-08T19:46:16Z

Fix for the issue #85
This PR fixes the bug for the generate_html method in googlecharts.rb which I found while going through the code.
Wrong argument plots was passed as the argument in the generate_body method here: https://github.com/SciRuby/daru-view/blob/master/lib/daru/view/adapters/googlecharts.rb#L77
There was no id defined in the generate_html method which was further used while rendering template here: https://github.com/SciRuby/daru-view/blob/master/lib/daru/view/adapters/googlecharts.rb#L78
Also, the HTML of the chart contained only the div part, so I created another template to include the initial_script also.

Shekharrajak · 2018-03-09T06:48:08Z

lib/daru/view/adapters/googlecharts.rb

@@ -70,11 +71,13 @@ def show_in_iruby(plot)
        end

        def generate_html(plot)
-          # TODO: modify code
          path = File.expand_path('../../templates/googlecharts/chart_div.erb', __FILE__)


I think instead of this chart_div.erb file, you can use your static_html.erb file. Since we have initial_script and chart_script (you are using it as chart_div ).

Shekharrajak · 2018-03-09T06:49:04Z

lib/daru/view/adapters/googlecharts.rb

          path = File.expand_path('../../templates/googlecharts/chart_div.erb', __FILE__)
-          template = File.read(path)
+          template_div = File.read(path)
+          chart_script, id = generate_body(plot)


id is randomly generated, so no need to get id, isn't it?

Shekharrajak · 2018-03-09T06:49:34Z

lib/daru/view/templates/googlecharts/static_html.erb

+  <%= initial_script %>
+</head>
+<body><%= chart_div %></body>
+</html>


<enter> here.

Shekharrajak · 2018-03-09T06:50:36Z

Thanks for PR. But we must open an issue first (rather than PR), if you find any bug or problem in code, so that all issue related discussion goes there, also it will clarify the problem and approach to solve it. Because fixing a bug without any discussion may cause to waste of time.

Please look into this comment.

Shekharrajak · 2018-03-09T13:56:00Z

Please write testcases for all related methods. It is really needed.

Prakriti-nith · 2018-03-09T14:49:21Z

Sure, I will do it.

Prakriti-nith · 2018-03-10T13:01:58Z

I was trying to test the method export_html_file by matching the expected script with the one to be written in the file as:

  allow(File).to receive(:write)  
  @area_chart_chart.export_html_file
  path = File.expand_path('../../plot.html', __dir__) 
  expect(File).
    to have_received(:write).
    with(path, "<html lang='en'>...</html>").
    once

However, the script to test contains the random id generated, so it is difficult to match the expected output. I was thinking of using regex but IMO this is not a very good idea. What should I do?

Shekharrajak · 2018-03-11T05:54:56Z

You can match specific tags and Javascript code to test, it is generating the correct Javascript.

Although you can pass custom id in plot#to_html method. You may need to change in few upper level methods to pass custom id in export_html_file method.

Prakriti-nith · 2018-03-11T16:06:50Z

As you have suggested I have passed the custom id in the to_html method by changing few upper-level methods. IMO matching specific tags can be a bug-prone task. Should I need to change anything?

Shekharrajak · 2018-03-12T04:41:21Z

spec/adapters/googlecharts_spec.rb

+      allow(File).to receive(:write)
+      area_chart_chart.
+        export_html_file('./plot.html','f75b3117-7cb0-4451-add4-e0006cb10876')
+      path = File.expand_path('../../plot.html', __dir__)


I don't think that it is good to test whole html code here. As I said
we must test all related methods, so it is better to start test from the lower to upper level methods. Test for initial_script , chart_script i.e. #to_html, #generate_body ,#generate_html methods .

I think we must test for specific keys (in this case area chart, options, data, etc in javascript code) . Is it good to test whole file at once ? Ping @zverok @v0dro

As I was busy with the proposal I was unable to work on this.
I will test for the methods separately but I still think that testing the specific keys would be bug-prone. For instance, to_html method will return the rendered html in string format in ruby. So, extracting the chart, options and data in javascript code might be a diificult task.

Because for extracting the chart, options and data from the JavaScript code, I think I will have to search for this text in the string returned. Is there any way to directly get those keys from the JS code without implementing a search?

I believe pattern matching will be useful. You can explore how it is tested in google_visualr, lazy_high_charts or other gems like formtastic etc.

…glecharts

Shekharrajak

Please add example(s) in spec/dummy_iruby (and share the link) and in rails, nanoc, sinatra app.

When it will be merged we need to have one example in our demo rails app : https://github.com/Shekharrajak/daru_examples_io_view_rails

Shekharrajak · 2018-04-02T10:49:44Z

spec/adapters/googlecharts/display_spec.rb

+      expect(js).to match(
+        /data_table.addRow\(\[\{v: \"2013\"\}\]\);/i)
+    end
+  end


expecting testcases for show_script as well.

Shekharrajak · 2018-04-02T10:53:40Z

spec/adapters/googlecharts_spec.rb

+      expect(js).to match(
+        /data_table.addRow\(\[\{v: \"2013\"\}\]\);/i)
+    end
+  end


expecting similar test cases for init_script, export_html_file, try for init_iruby. Also please test with few other chart types as well.

Shekharrajak · 2018-04-02T10:57:29Z

lib/daru/view/templates/googlecharts/static_html.erb

@@ -0,0 +1,7 @@
+<html lang='en'>
+<head>
+  <title>Chart</title>


What if it is a google datatable ? I think we can add more information in title (id, chart type). We can extend it and display with table and chart both in html table ( mark as TODO. It must not be in this same PR).

Shekharrajak · 2018-04-02T10:58:38Z

lib/daru/view/templates/googlecharts/static_html.erb

+  <title>Chart</title>
+  <%= initial_script %>
+</head>
+<body><%= chart_script %></body>


I prefer

<body> <%= chart_script %> </body>

It already comes in bunch of js code. So alignment is important.

Prakriti-nith · 2018-04-02T22:07:27Z

What kind of examples do I need to add in dummy_iruby as I think it already has enough examples of google charts? Should I indicate the use of generate_html method in the examples?

Prakriti-nith · 2018-04-06T05:23:47Z

@Shekharrajak can you please review this PR?

Shekharrajak

For id part I am expecting some other way. Let me know if you have some idea otherwise it will be merged.

Shekharrajak · 2018-04-10T06:52:34Z

lib/daru/view/adapters/googlecharts.rb

+          plot.to_html[0]
+        end
+
+        def generate_id(plot, id=nil)


I am not satisfied with this kind of approach. Anyways I don't understand why there is id parameter and function name generate_id.

As the generated id is random, I have used the id parameter for the testing purpose here

Prakriti-nith · 2018-04-10T12:40:05Z

Random id is generated in to_html method and we have to use it in static_html.erb file in generate_html method (here). I have given a thought to it and couldn't find any other way other than by returning two values in to_html method. Do you know of any other way and should I make any further changes?

Shekharrajak · 2018-04-10T14:13:44Z

Consider #84 (comment) . #to_html method behavior must be same for all the adapters; so changes like this (just for getting id) is not looking good to me. Think about the other adapters#to_html method as well.

Shekharrajak · 2018-04-11T06:59:48Z

lib/daru/view/adapters/googlecharts.rb

          initial_script = init_script
+          id = plot.id_chart


It's html_id not id_chart (or some meaningful).

Shekharrajak · 2018-04-11T07:00:54Z

spec/adapters/googlecharts/display_spec.rb

@@ -22,7 +22,7 @@

  describe "#to_html" do
    it "generates valid JS of the Area Chart" do
-  	  js, id = area_chart_chart.chart.to_html("id")
+  	  js = area_chart_chart.chart.to_html("id")


check for "id" in div tag.

Shekharrajak · 2018-04-11T13:19:40Z

lib/daru/view/adapters/googlecharts/display.rb

@@ -14,6 +14,7 @@ def self.init_script(
  end

  module Display
+    attr_accessor :html_id


We must write about html_id. It will hold a value only when to_html method is invoked (i.e. html code of the chart is generated).

Ufff we are iterating too many time. You must check these things for your PR :

Code quality (Clear, understandable, Extensible, exception handling, ..)

Specs for the feature or your each line of code.

Documentation (It is always better to have it. There is lack of documentations already for internal methods). Please follow yard.

Shekharrajak · 2018-04-12T13:11:17Z

Looks good to go. Thanks!

Fix export_html_file method in googlecharts.rb

70dc281

Shekharrajak reviewed Mar 9, 2018

View reviewed changes

Prakriti-nith changed the title ~~Fix export_html_file method in googlecharts.rb~~ Fix generate_html method in googlecharts.rb Mar 9, 2018

Remove id as a return parameter

8cc1b0a

Test for export_html_file method

2e2d1fd

Prakriti-nith force-pushed the export_html branch from 2e6b5cf to 2e2d1fd Compare March 11, 2018 16:02

Shekharrajak reviewed Mar 12, 2018

View reviewed changes

Add tests for to_html, generate_html and generate_body methods in goo…

bfff94a

…glecharts

Shekharrajak reviewed Apr 2, 2018

View reviewed changes

Added tests for init_script, show_script and export_html_file methods

e73c088

Prakriti-nith force-pushed the export_html branch from 20fdb33 to e73c088 Compare April 3, 2018 11:52

Proper indentation

8ca2b53

Shekharrajak reviewed Apr 10, 2018

View reviewed changes

Remove generate_id method

8f18f8e

Make id as an attr_accessor in module Display

1612557

Shekharrajak reviewed Apr 11, 2018

View reviewed changes

Add check for id in div tag

da216ca

Shekharrajak reviewed Apr 11, 2018

View reviewed changes

Add documentation for html_id

696121a

Shekharrajak approved these changes Apr 12, 2018

View reviewed changes

Shekharrajak merged commit 5b5d7bc into SciRuby:master Apr 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix generate_html method in googlecharts.rb #84

Fix generate_html method in googlecharts.rb #84

Prakriti-nith commented Mar 8, 2018 •

edited

Loading

Shekharrajak Mar 9, 2018

Shekharrajak Mar 9, 2018

Shekharrajak Mar 9, 2018

Shekharrajak commented Mar 9, 2018

Shekharrajak commented Mar 9, 2018

Prakriti-nith commented Mar 9, 2018

Prakriti-nith commented Mar 10, 2018 •

edited

Loading

Shekharrajak commented Mar 11, 2018

Prakriti-nith commented Mar 11, 2018

Shekharrajak Mar 12, 2018

Prakriti-nith Mar 29, 2018

Prakriti-nith Mar 29, 2018

Shekharrajak Mar 30, 2018

Shekharrajak left a comment •

edited

Loading

Shekharrajak Apr 2, 2018

Shekharrajak Apr 2, 2018

Shekharrajak Apr 2, 2018

Shekharrajak Apr 2, 2018

Prakriti-nith commented Apr 2, 2018

Prakriti-nith commented Apr 6, 2018

Shekharrajak left a comment

Shekharrajak Apr 10, 2018

Prakriti-nith Apr 10, 2018

Prakriti-nith commented Apr 10, 2018

Shekharrajak commented Apr 10, 2018

Shekharrajak Apr 11, 2018

Shekharrajak Apr 11, 2018

Shekharrajak Apr 11, 2018

Shekharrajak commented Apr 12, 2018

Fix generate_html method in googlecharts.rb #84

Fix generate_html method in googlecharts.rb #84

Conversation

Prakriti-nith commented Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shekharrajak commented Mar 9, 2018

Shekharrajak commented Mar 9, 2018

Prakriti-nith commented Mar 9, 2018

Prakriti-nith commented Mar 10, 2018 • edited Loading

Shekharrajak commented Mar 11, 2018

Prakriti-nith commented Mar 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shekharrajak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Prakriti-nith commented Apr 2, 2018

Prakriti-nith commented Apr 6, 2018

Shekharrajak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Prakriti-nith commented Apr 10, 2018

Shekharrajak commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shekharrajak commented Apr 12, 2018

Prakriti-nith commented Mar 8, 2018 •

edited

Loading

Prakriti-nith commented Mar 10, 2018 •

edited

Loading

Shekharrajak left a comment •

edited

Loading