Simple Data Grapher: Integration + Research Note + Tests #6113
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hmm, error is: |
|
Have you modified that file at all? Could this possibly be something related to @namangupta01's Notifications work? Thanks! |
|
Yeah, so this happens a lot with Travis, the screenshot tests give errors,
but on restarting Travis, they work fine.
…On Mon, Aug 19, 2019, 7:49 PM Jeffrey Warren ***@***.***> wrote:
Hmm, error is:
ERROR["test_viewing_the_settings_page", #<Minitest::Reporters::Suite:0x00007fd339b783b0 @name="SettingsTest">, 98.75577035500004]
test_viewing_the_settings_page#SettingsTest (98.76s)
ActionView::Template::Error: ActionView::Template::Error: undefined method `id' for nil:NilClass
app/views/users/settings.html.erb:10:in `_app_views_users_settings_html_erb__4235966138012236490_70272530884460'
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6113?email_source=notifications&email_token=AJXHQZ7XNK5EFIXPL7S4QIDQFKTXBA5CNFSM4IKDQJXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4TC65Q#issuecomment-522596214>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJXHQZZEVPYU27O2ODQK33LQFKTXBANCNFSM4IKDQJXA>
.
|
|
No no, it's not because of any modifications to the file. This is very
strange, I even discussed it with Cess. It so happens that Travis has
passed, and after linting a file, just removing spaces, it fails, and on
restarting it works.
…On Mon, Aug 19, 2019, 7:50 PM Jeffrey Warren ***@***.***> wrote:
Have you modified that file at all? Could this possibly be something
related to @namangupta01 <https://github.com/namangupta01>'s
Notifications work? Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6113?email_source=notifications&email_token=AJXHQZ6T3IWVVTKVUF3A5Q3QFKTZFA5CNFSM4IKDQJXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4TDA2Q#issuecomment-522596458>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJXHQZ7ZVWGJMX4ROM3ROALQFKTZFANCNFSM4IKDQJXA>
.
|
|
@jywarren can you please restart Travis once? |
|
@jywarren all tests pass! 🎉 |
|
OK, let's try to track this testing thing in a new issue... any links to Travis lines and builds that demonstrate it would be great. And there we can hear from other people on the issue as well. |
|
Sure, that sounds good!
…On Mon, Aug 19, 2019, 9:14 PM Jeffrey Warren ***@***.***> wrote:
OK, let's try to track this testing thing in a new issue... any links to
Travis lines and builds that demonstrate it would be great. And there we
can hear from other people on the issue as well.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6113?email_source=notifications&email_token=AJXHQZ5QDNA3TLHX2EDHLJLQFK5UBA5CNFSM4IKDQJXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4TMO6I#issuecomment-522635129>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJXHQZZ3JR5WSLA263K5KO3QFK5UBANCNFSM4IKDQJXA>
.
|
jywarren
reviewed
Aug 19, 2019
config/routes.rb
Outdated
| post 'graph/object' => 'csvfiles#setter' | ||
| post 'graph/note/graphobject' => 'csvfiles#add_graphobject' | ||
| get 'graph/prev_file' => 'csvfiles#prev_files' | ||
| get 'simple-data-grapher/data/:id' => 'csvfiles#user_files' |
There was a problem hiding this comment.
Oh, was there a reason you didn't change this one too? Just checking!
jywarren
reviewed
Aug 19, 2019
There was a problem hiding this comment.
This looks good. @IshaGupta18 what plan do you have for some system tests for this code? That would cover a LOT of the Ruby/JS integration that needs to work for this PR to succeed. What do you think? Thanks!
One question above. And if you can get a system test running in here too, I'd be really happy with merging - and that would link the full-stack system test with the entirety of this code!
|
Yeah, actually I thought for the routes user will visit, they can be a
little more descriptive, graph might be a little vague here. What do you
think?
…On Mon, Aug 19, 2019, 9:16 PM Jeffrey Warren ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In config/routes.rb
<#6113 (comment)>:
> @@ -123,6 +123,15 @@
get 'wiki/edit/:lang/:id' => 'wiki#edit'
get 'wiki' => 'wiki#index'
+ #routes for simple-data-grapher
+ get 'graph/fetch_graphobject' => 'csvfiles#fetch_graphobject'
+ get 'simple-data-grapher' => 'csvfiles#new'
+ post 'graph/object' => 'csvfiles#setter'
+ post 'graph/note/graphobject' => 'csvfiles#add_graphobject'
+ get 'graph/prev_file' => 'csvfiles#prev_files'
+ get 'simple-data-grapher/data/:id' => 'csvfiles#user_files'
Oh, was there a reason you didn't change this one too? Just checking!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6113?email_source=notifications&email_token=AJXHQZZINCXM5PGTR5CP5STQFK56NA5CNFSM4IKDQJXKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCB6VR7Q#pullrequestreview-276650238>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJXHQZZ5T4RV6MUTK7F5F5TQFK56NANCNFSM4IKDQJXA>
.
|
|
Oh, could you give me a couple of examples of where it has been done in
plots2 or something like that?
…On Mon, Aug 19, 2019, 9:18 PM Jeffrey Warren ***@***.***> wrote:
***@***.**** commented on this pull request.
This looks good. @IshaGupta18 <https://github.com/IshaGupta18> what plan
do you have for some system tests for this code? That would cover a LOT of
the Ruby/JS integration that needs to work for this PR to succeed. What do
you think? Thanks!
One question above. And if you can get a system test running in here too,
I'd be really happy with merging - and that would link the full-stack
system test with the entirety of this code!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6113?email_source=notifications&email_token=AJXHQZ53Z6WI5XPDY7JLCBDQFK6EXA5CNFSM4IKDQJXKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCB6VZ7Y#pullrequestreview-276651263>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJXHQZ4AYVYBBZU253MN6MLQFK6EXANCNFSM4IKDQJXA>
.
|
|
Sure! Here's a system test of writing a post and publishing it!
https://github.com/publiclab/plots2/blob/master/test/system/post_test.rb#L36
As to the routes, in general, let's put description in the title and help
text on the page itself; i think a shorter URL will be easier for people to
get to! Thank you!
On Mon, Aug 19, 2019 at 11:50 AM Isha Gupta <[email protected]>
wrote:
… Oh, could you give me a couple of examples of where it has been done in
plots2 or something like that?
On Mon, Aug 19, 2019, 9:18 PM Jeffrey Warren ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
>
> This looks good. @IshaGupta18 <https://github.com/IshaGupta18> what plan
> do you have for some system tests for this code? That would cover a LOT
of
> the Ruby/JS integration that needs to work for this PR to succeed. What
do
> you think? Thanks!
>
> One question above. And if you can get a system test running in here too,
> I'd be really happy with merging - and that would link the full-stack
> system test with the entirety of this code!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <
#6113?email_source=notifications&email_token=AJXHQZ53Z6WI5XPDY7JLCBDQFK6EXA5CNFSM4IKDQJXKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCB6VZ7Y#pullrequestreview-276651263
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AJXHQZ4AYVYBBZU253MN6MLQFK6EXANCNFSM4IKDQJXA
>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6113?email_source=notifications&email_token=AAAF6J47YZV5KX4QXZSHWC3QFK6I3A5CNFSM4IKDQJXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4TNBAI#issuecomment-522637441>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAF6JY5PVCLVZ4LRFDXILDQFK6I3ANCNFSM4IKDQJXA>
.
|
|
Okay, sure, I'll change the routes! |
|
Could you please suggest what kind of a system test and how many are needed for the integration? I am not very sure on how to proceed with it? @cesswairimu @jywarren please guide me here! |
|
Okay so I looked it up, and I have done a TON of System or UI testing in simple-data-grapher Do you think this will suffice, @jywarren ? |
|
If it does, are we good for merge? |
jywarren
approved these changes
Aug 20, 2019
|
Great it worked!
|
|
Congratulations!!! Let's test it in stable and confirm here! |
|
Great job @IshaGupta18 🎉 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the PR I would be working on and is a combination of #6102 #6104 #5993 with major bug fixes.
@jywarren @gauravano @sagarpreet-chadha please review! Thanks a lot!