Tests for Simple-Data-Grapher #6104

IshaGupta18 · 2019-08-04T16:56:22Z

Here are a few of the tests I have written for csvfiles_controller.rb file and I will add more as and when required.
Since I have done most of my testing (logical and UI) in Simple-Data-Grapher as well, I think these will be good to go for now.
Also, this gives more confidence to #5993 which can be merged soon, now that we have tests to back it up.
@jywarren @gauravano @sagarpreet-chadha @namangupta01 kindly review this and let me know what else can be done!

IshaGupta18 · 2019-08-04T16:56:46Z

PS: I would be fixing the linting soon!

IshaGupta18 · 2019-08-04T17:02:55Z

Here only the test file bears important, rest all is from #5993 https://github.com/publiclab/plots2/pull/6104/files#diff-b6813d8a7c58e0cf4118a4a9e17873cd

IgorWilbert · 2019-08-05T00:54:28Z

app/assets/javascripts/simple-data-grapher.js

+
+}
+function graphMaker(data,divId){
+    var obj = data["sdgobject"];


I think it would be possible to use let instead of var here, because limiting the scope could avoid future global variable-related bugs :) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let

IgorWilbert · 2019-08-05T00:55:24Z

app/controllers/editor_controller.rb

@@ -38,6 +43,10 @@ def rich
    image if params[:i]
  end

+  # def tempfunc


Is there any reason to leave this commented code?

IgorWilbert · 2019-08-05T00:55:59Z

app/helpers/csvfiles_helper.rb

@@ -0,0 +1,2 @@
+module CsvfilesHelper


More changes in the future I guess

IgorWilbert · 2019-08-05T00:59:26Z

app/views/csvfiles/new.html.erb

+    var headerContainer = document.getElementsByClassName("body-container")[0];
+    SimpleDataGrapherObject = new SimpleDataGrapher("first");
+    var value = '<%= current_user %>';
+    <% if current_user %>


Sorry, I did not understand why you have JS code in an embedded ruby file. Would it make sense to move the JS code to a separate file?

Actually, this file is very much like the rich.html.erb , which integrates the PL Editor in the same way as Simple-Data-Grapher, that is why it's required here!

IgorWilbert · 2019-08-05T01:01:00Z

app/views/csvfiles/user_files.html.erb

+</table>
+
+<style>
+.main_heading_data{


A separate file for these css styles would be great I think

IgorWilbert · 2019-08-05T01:04:27Z

test/functional/csvfiles_controller_test.rb

+		assert_redirected_to '/login'
+	end
+
+	test 'should save graph object' do


Can you think of other unhappy path tests for this? For example, is there any circumstance in which the save/fetch graph object should fail? (e.g. user not logged in)

For fetching, maybe yes! But for saving, I have checked for the login redirect!

IgorWilbert · 2019-08-05T01:05:15Z

Thank you @IshaGupta18 ! I left some comments, I hope they are helpful :)

jywarren · 2019-08-05T05:39:50Z

Hi!!! Does this include all the commits from #5993? If so, I'd prefer to merge this instead of that, to do it all in one. That way the tests and functionality are linked!

IshaGupta18 · 2019-08-05T05:41:18Z

Yes this does, however I have a couple of bugs to fix in this one, then it's good for merge! Thanks a lot!

…

On Mon, Aug 5, 2019, 11:09 AM Jeffrey Warren ***@***.***> wrote: Hi!!! Does this include all the commits from #5993 <#5993>? If so, I'd prefer to merge this instead of that, to do it all in one. That way the tests and functionality are linked! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6104?email_source=notifications&email_token=AJXHQZ6CW3NCM2YY6X2G6P3QC64KVA5CNFSM4IJFYZLKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3QXQ5Y#issuecomment-518092919>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJXHQZ6EL3CYM734YF3OHADQC64KVANCNFSM4IJFYZLA> .

jywarren

Just wondering a bit about the flow and left a question above, but also would you consider adding some system tests to protect the basic workflow here?

jywarren · 2019-08-05T05:41:30Z

app/controllers/editor_controller.rb

+    if params[:id] && params[:uid]
+      @graphobject = [params[:id],params[:uid]]
+    end
+    # byebug


Hm, is this required?

jywarren · 2019-08-05T05:48:35Z

app/views/editor/rich.html.erb

+        </div>
+      <% else %>
+        <div class="ple-module-content col-lg-9">
+          <% x = "Power Tag: simple-data-grapher: i/" + @graphobject[0] + "/" + @graphobject[1] %>


Hm, can you walk me through exactly what's happening here, or link to the previous PR where it's explained? Thanks!

IshaGupta18 · 2019-08-05T06:47:12Z

Yes, I am will write a comment explaining the entire flow and also why both these code snippets highlighted by you are necessary!

…

On Mon, Aug 5, 2019, 11:21 AM Jeffrey Warren ***@***.***> wrote: ***@***.**** requested changes on this pull request. Just wondering a bit about the flow and left a question above, but also would you consider adding some system tests to protect the basic workflow here? ------------------------------ In app/controllers/editor_controller.rb <#6104 (comment)>: > @@ -20,6 +20,11 @@ def editor end def post + @graphobject = "" + if params[:id] && params[:uid] + @graphobject = [params[:id],params[:uid]] + end + # byebug Hm, is this required? ------------------------------ In app/views/editor/rich.html.erb <#6104 (comment)>: > </div> - - <div class="ple-module-content col-lg-9"> - <textarea id="text-input" class="ple-textarea form-control"><% if @node && @node.latest && @node.latest.body_rich %><%= @node.latest.body %><% else %><%= params[:body] %><% end %></textarea> - </div> + <% if @graphobject=="" %> + <div class="ple-module-content col-lg-9"> + <textarea id="text-input" class="ple-textarea form-control"><% if @node && @node.latest && @node.latest.body_rich %><%= @node.latest.body %><% else %><%= params[:body] %><% end %></textarea> + </div> + <% else %> + <div class="ple-module-content col-lg-9"> + <% x = "Power Tag: simple-data-grapher: i/" + @graphobject[0] + "/" + @graphobject[1] %> Hm, can you walk me through exactly what's happening here, or link to the previous PR where it's explained? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6104?email_source=notifications&email_token=AJXHQZ2P2KSZF7U4WFL3LJTQC65VNA5CNFSM4IJFYZLKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCAQH6YY#pullrequestreview-270565219>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJXHQZYCF3KWHQH3GGQYKQ3QC65VNANCNFSM4IJFYZLA> .

IshaGupta18 · 2019-08-06T13:13:16Z

Okay so here is the flow of things, a part of which I will be borrowing from my comment in publiclab/simple-data-grapher#65.

The User clicks on "Publish as a Research Note" button

When this happens, as suggested by @sagarpreet-chadha and @gauravano, I save a hash containing the data I will be needing to plot the same graph again. This is done by sending an AJAX to the controller action in csvfiles_controller.rb , which creates a new csvfiles object, and returns just the id and uid of this object.

We need this id and uid in order to create the Power Tag for simple-data-grapher.

Once we get this, we give the control to the editor_controller.rb file by doing this:

We have passed the id and uid in params here, which is received by the post function (which has the control now)

The global variable @graphobject is available to the corresponding view that post function fetches ie rich.html.erb

There, we just use a simple if condition, to check if we need to insert a power tag for simple-data-grapher or not.

The remaining flow is as described in this comment
ie. we have a function in node_shared.rb (which is yet to be modified) and we have a js file for plotting the graph etc.

And voila! We have a chart here!

I know this sounds a little complicated at first, but throughout my research in being able to accomplish this, this is the best method I have been able to find, without hampering much of the existing code. The remaining flow is pretty standard, I have just introduced 2-3 extra lines in editor controller and rih.html.erb. Let me know if you follow this!

jywarren · 2019-08-15T18:44:37Z

This looks great; can you actually merge these commits into the PR #5993, so that the tests and functionality are linked into one PR that shows both the functionality and the tests that secure them?

IshaGupta18 · 2019-08-15T18:46:07Z

@jywarren we have shifted to #6113, which is the latest PR for all the SDG work, so could you please take a look at that one?

jywarren · 2019-08-15T18:47:01Z

OK, great, good to know, and we can close this then, right? THat way people finding this PR aren't confused (as I was! lol)

jywarren · 2019-08-15T18:47:47Z

And likewise should #5993 be closed too and a comment left redirecting to #6113?

IshaGupta18 · 2019-08-15T18:48:59Z

Yes, I will be closing that one!

IshaGupta18 and others added 26 commits July 8, 2019 19:55

New Table for CsvFiles

4c063aa

belongs_to for CSV files

5f93182

CSV controller code

fb9baf0

routes for simple-data-grapher

0584a26

has_many relation for users

c156ba5

view files for csvfiles

bae5211

user data display

6d94e09

Added simple-data-grapher package

45ed76c

Delete Gemfile

340c516

Gemfile

1066318

Gemfile

cd1977c

code climate

a3c1ecb

Climate 2

cf2164d

climate 3

db59324

fix code climate issues

48bdba5

Update schema.rb.example

8a61b1e

Fixing changes

f5bc4b8

Error Handling

1de79be

Fixed the errors

8feb24d

linting

ef4bfcc

minor fix in alert

a332358

more linting

ab48bd4

work on research note

11335f0

Publish as a Research Note Ready!

e8d12d9

updating schema

dabe948

Tests for Csvfiles controller for Simple Data Grapher

01a17ca

IgorWilbert reviewed Aug 5, 2019

View reviewed changes

jywarren requested changes Aug 5, 2019

View reviewed changes

IshaGupta18 mentioned this pull request Aug 7, 2019

Simple Data Grapher: Integration + Research Note + Tests #6113

Merged

IshaGupta18 added gsoc summer-of-code labels Aug 11, 2019

IshaGupta18 closed this Aug 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for Simple-Data-Grapher #6104

Tests for Simple-Data-Grapher #6104

IshaGupta18 commented Aug 4, 2019

IshaGupta18 commented Aug 4, 2019

IshaGupta18 commented Aug 4, 2019

IgorWilbert Aug 5, 2019

IgorWilbert Aug 5, 2019

IgorWilbert Aug 5, 2019

IgorWilbert Aug 5, 2019

IshaGupta18 Aug 5, 2019

IgorWilbert Aug 5, 2019

IgorWilbert Aug 5, 2019

IshaGupta18 Aug 5, 2019

IgorWilbert commented Aug 5, 2019

jywarren commented Aug 5, 2019

IshaGupta18 commented Aug 5, 2019 via email

jywarren left a comment

jywarren Aug 5, 2019

jywarren Aug 5, 2019

IshaGupta18 commented Aug 5, 2019 via email

IshaGupta18 commented Aug 6, 2019

jywarren commented Aug 15, 2019

IshaGupta18 commented Aug 15, 2019 •

edited

Loading

jywarren commented Aug 15, 2019

jywarren commented Aug 15, 2019

IshaGupta18 commented Aug 15, 2019

Tests for Simple-Data-Grapher #6104

Tests for Simple-Data-Grapher #6104

Conversation

IshaGupta18 commented Aug 4, 2019

IshaGupta18 commented Aug 4, 2019

IshaGupta18 commented Aug 4, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorWilbert commented Aug 5, 2019

jywarren commented Aug 5, 2019

IshaGupta18 commented Aug 5, 2019 via email

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IshaGupta18 commented Aug 5, 2019 via email

IshaGupta18 commented Aug 6, 2019

jywarren commented Aug 15, 2019

IshaGupta18 commented Aug 15, 2019 • edited Loading

jywarren commented Aug 15, 2019

jywarren commented Aug 15, 2019

IshaGupta18 commented Aug 15, 2019

IshaGupta18 commented Aug 15, 2019 •

edited

Loading