Websockets on cart page #1058

squirrelo · 2015-04-13T02:11:34Z

~~Requires #1041 so merge that first.~~Merged
~~Depends on #1056 so merge that first~~Merged

This adds websocket-based removal of samples and processed data on the selected samples cart page. It also moves the analysis creation to that page as well.

coveralls · 2015-04-13T04:20:32Z

Coverage decreased (-0.1%) to 78.96% when pulling a828daa on squirrelo:websockets-on-cart-page into c1fc3c8 on biocore:cart-branch.

…ebsockets-on-cart-page

squirrelo · 2015-04-13T14:57:37Z

Ready for review

coveralls · 2015-04-13T16:02:33Z

Changes Unknown when pulling 795c748 on squirrelo:websockets-on-cart-page into * on biocore:cart-branch*.

ElDeveloper · 2015-04-17T22:08:06Z

argh, conflicts, can you pull from upstream master and upstream cart-branch.

squirrelo · 2015-04-17T22:32:20Z

Done, tests running now.

coveralls · 2015-04-17T23:02:34Z

Coverage decreased (-0.06%) to 79.26% when pulling 05b3837 on squirrelo:websockets-on-cart-page into 2409338 on biocore:cart-branch.

squirrelo · 2015-04-17T23:04:55Z

@biocore/qiita-dev Tests pass, ready for review!

ElDeveloper · 2015-04-17T23:56:26Z

Ok, now I'm reviewing this one.

ElDeveloper · 2015-04-18T00:21:25Z

qiita_pet/handlers/websocket_handlers.py

+        # parse into JSON
+        msginfo = loads(msg)
+        default = Analysis(self.current_user.default_analysis)
+        if 'samples' not in msginfo:


I think you can simplify this if/else block by using msginfo.get('samples'), which would return a None when 'samples' is not part of the dictionary. And perhaps, this is the way both arguments should be retrieved such that in case nothing was sent remove_samples would raise an exception (QiitaIncompetentDeveloper, I think).

if the object sent over the line always includes samples then the message is consistent. In this branch, the message would just need to have samples associated with []

ElDeveloper · 2015-04-18T00:32:59Z

This looks great!! Such an awesome improvement overall, a few things:

If you (1) select samples for an analysis, then (2) go to "view selected samples", then (3) remove all the samples and (4) try to create the analysis. You'll get a 405 error. Similarly if no samples are selected and you click "view selected samples", you'll get the following traceback:

  File "/Users/yoshikivazquezbaeza/.virtualenvs/qiita/lib/python2.7/site-package
s/tornado/web.py", line 1141, in _when_complete
    callback()

  File "/Users/yoshikivazquezbaeza/.virtualenvs/qiita/lib/python2.7/site-package
s/tornado/web.py", line 1162, in _execute_method
    self._when_complete(method(*self.path_args, **self.path_kwargs),

  File "/Users/yoshikivazquezbaeza/.virtualenvs/qiita/lib/python2.7/site-package
s/tornado/web.py", line 2297, in wrapper
    return method(self, *args, **kwargs)

  File "/Users/yoshikivazquezbaeza/git_sw/qiita/qiita_pet/handlers/analysis_hand
lers.py", line 220, in get
    proc_info=proc_data_info)

  File "/Users/yoshikivazquezbaeza/.virtualenvs/qiita/lib/python2.7/site-packages/tornado/web.py", line 538, in render
    html = self.render_string(template_name, **kwargs)

  File "/Users/yoshikivazquezbaeza/.virtualenvs/qiita/lib/python2.7/site-packages/tornado/web.py", line 645, in render_string
    return t.generate(**namespace)

  File "/Users/yoshikivazquezbaeza/.virtualenvs/qiita/lib/python2.7/site-packages/tornado/template.py", line 273, in generate
    return execute()

  File "analysis_selected_html.generated.py", line 276, in _tt_execute
    for pid in proc_datas:  # analysis_selected.html:79 (via sitebase.html:192)

UnboundLocalError: local variable 'proc_datas' referenced before assignment

ElDeveloper · 2015-04-18T00:36:28Z

qiita_pet/templates/analysis_selected.html

@@ -63,6 +72,10 @@
 </div>
 {% end %}

+<button type="button" class="btn btn-primary btn-lg" data-toggle="modal" data-target="#create-analysis-modal-view">


This button would be better ( I think ) at the top.

And probably colored in "green"

I have it at the bottom to force people to look at what they selected before hitting create. I can make it green though.

ElDeveloper · 2015-04-18T00:38:05Z

When I select a few samples and try to create the analysis, I get a 405 error.

ElDeveloper · 2015-04-18T00:40:53Z

Last thing, now that I've been able to list several studies under the "/selected/" handler, I think it would be better if the studies were presented inside a data table. And the table shouldn't use the full width of the window, it looks weird.

wasade · 2015-04-18T00:44:16Z

qiita_pet/templates/analysis_selected.html

@@ -1,8 +1,15 @@
 {% extends sitebase.html %}
 {% block head %}
 <script type="text/javascript">
+var ws = new WebSocket('ws://' + window.location.host + '/analysis/selected/socket/');


It would be really nice if it were possible to reuse the moi init here since it is already doing browser checking.

Added an init_websocket function for taking care of boilerplate code.

josenavas · 2015-04-21T17:54:21Z

I've not gone over the code but I was testing this branch as @squirrelo suggested that this is the last PR on this branch.

One of the things that act weirdly is that I can select samples, go to the "View selected samples" page; and, when I go back to the study listing page, I have no feedback of which samples have been already selected. I would suggest that the checkbox should be checked if you've already chosen those samples. Alternatively, I would suggest changing the checkbox to a + sign (glyphicon); and use a - sign when that study has been already selected. Other options are using a color scheme, but I think this can look like a Christmas tree. Anyways, I think some kind of feedback is needed in the study selection page.

josenavas · 2015-04-21T17:55:53Z

Also, I cannot create an analysis. I added a new study, completely processed it, I only choose it's samples. When I click create analysis on the modal that appears, nothing happens...

As a note, nothing in my terminal or javascript console, so no clue on what is going on.

squirrelo · 2015-04-21T20:51:41Z

Fixed the issue with creation of new analyses, the modal was submitting to the wrong page.

As for previously selected samples, I agree that something needs to be done, but it's something that needs a lot of discussion and thought. For now, I think what we have will work, and it can be expanded upon when we come to a conclusion for how previously selected samples are shown. The big issue is that you aren't selecting by sample, but by processed data.

josenavas · 2015-04-21T20:53:56Z

Thanks for the comments @squirrelo
However, I don't think that we should merge all this branch into master (it can go to it's own branch). Based on the new Contributing.md document, this functionality is not complete, and leaving the system in a somewhat inconsistent state (it can be really misleading to the user - it was for me...), so we should discuss this and being able to make it for the alpha 0.1 release...

squirrelo · 2015-04-21T20:59:44Z

This is good to go as is for now though, since it's still on it's own branch. In light of that, can we get this a final review and merge?

josenavas · 2015-04-21T21:01:43Z

As @ElDeveloper suggests, I will be more comfortable if @wasade can review this, as he has more experience with websockets and he already had some comments on this PR.

squirrelo · 2015-04-21T21:57:46Z

This only changed a single number, so the tests will pass fine. It's good to review.

coveralls · 2015-04-22T01:15:08Z

Coverage decreased (-0.06%) to 79.26% when pulling 5b253ff on squirrelo:websockets-on-cart-page into 2409338 on biocore:cart-branch.

wasade · 2015-04-22T14:20:44Z

qiita_pet/templates/analysis_selected.html

+var ws = init_websocket(window.location.host + '/analysis/selected/socket/');
+ws.onmessage = function(evt) { if(evt.data != 'true') {ws.close();} };
+ws.onerror = function(evt) { $('#ws-error').html("<b>Server communication error. Sample removal will not be recorded. Please try again later.</b>"); };
+ws.onclose = function(evt) { $('#ws-error').html("<b>Server communication error. Sample removal will not be recorded. Please try again later.</b>"); };


Closing a web socket should be a normal operation, right? Why is this considered an error?

n this case, it should stay open until you leave the page in order to capture sample removals. If it closes prematurely that’s bad and should be treated as an error.

On Apr 22, 2015, at 7:20 AM, Daniel McDonald notifications@github.com wrote:

In qiita_pet/templates/analysis_selected.html #1058 (comment):

@@ -1,8 +1,15 @@
{% extends sitebase.html %}
{% block head %}
<script type="text/javascript"> +var ws = init_websocket(window.location.host + '/analysis/selected/socket/'); +ws.onmessage = function(evt) { if(evt.data != 'true') {ws.close();} }; +ws.onerror = function(evt) { $('#ws-error').html("Server communication error. Sample removal will not be recorded. Please try again later."); }; +ws.onclose = function(evt) { $('#ws-error').html("Server communication error. Sample removal will not be recorded. Please try again later."); }; Closing a web socket should be a normal operation, right? Why is this considered an error?

—
Reply to this email directly or view it on GitHub https://github.com/biocore/qiita/pull/1058/files#r28876200.

wasade · 2015-04-22T14:38:20Z

What is the reason for not reusing the moi client-side websocket code? It would require only a minor modification to moi.js (allow specifying an onerror method at construction, and the specific handler the websocket should use). This is an opportunity to reduce js code replication and centralize websocket handling client-side.

squirrelo · 2015-04-22T15:16:30Z

The MOI.js is specific to what MOI is doing. I can make it generalizable, but then it’s no longe the same asr MOI.js in the MOI repo and the changes will need to get moved there as well. This is a fast solution that allows us to have qiita-specific websocket code and MOI specific web socket code coexist.

On Apr 22, 2015, at 7:38 AM, Daniel McDonald notifications@github.com wrote:

What is the reason for not reusing the moi client-side websocket code? It would require only a minor modification to moi.js (allow specifying an onerror method at construction, and the specific handler the websocket should use). This is an opportunity to reduce js code replication and centralize websocket handling client-side.

—
Reply to this email directly or view it on GitHub #1058 (comment).

wasade · 2015-04-22T15:26:11Z

It really isn't specific. Is a PR for the minor changes against moi not possible? It literally does the exact same thing, is just more flexible.

That argument is analogous to writing a custom fasta parser every time we need to parse a fasta file. It's quick, allows for special casing whatever random garbage is in the comment field, but hurts us in the long run.

coveralls · 2015-04-22T15:34:07Z

Coverage decreased (-0.06%) to 79.26% when pulling 05c24c0 on squirrelo:websockets-on-cart-page into 2409338 on biocore:cart-branch.

squirrelo · 2015-04-22T15:46:29Z

Ok, broke out the web socket initialization and in the process may have fixed #993. If the MOI code looks good I’ll open a pull request to add it directly into MOI.

On Apr 22, 2015, at 8:26 AM, Daniel McDonald notifications@github.com wrote:

It really isn't specific. Is a PR for the minor changes against moi not possible? It literally does the exact same thing, is just more flexible.

That argument is analogous to writing a custom fasta parser every time we need to parse a fasta file. It's quick, allows for special casing whatever random garbage is in the comment field, but hurts us in the long run.

—
Reply to this email directly or view it on GitHub #1058 (comment).

wasade · 2015-04-22T15:52:01Z

Great, thank you
On Apr 22, 2015 9:46 AM, "Joshua Shorenstein" notifications@github.com
wrote:

Ok, broke out the web socket initialization and in the process may have
fixed #993. If the MOI code looks good I’ll open a pull request to add it
directly into MOI.

On Apr 22, 2015, at 8:26 AM, Daniel McDonald notifications@github.com
wrote:

It really isn't specific. Is a PR for the minor changes against moi not
possible? It literally does the exact same thing, is just more flexible.

That argument is analogous to writing a custom fasta parser every time
we need to parse a fasta file. It's quick, allows for special casing
whatever random garbage is in the comment field, but hurts us in the long
run.

—
Reply to this email directly or view it on GitHub <
https://github.com/biocore/qiita/pull/1058#issuecomment-95236350>.

—
Reply to this email directly or view it on GitHub
#1058 (comment).

coveralls · 2015-04-22T15:57:39Z

Coverage decreased (-0.06%) to 79.26% when pulling a239792 on squirrelo:websockets-on-cart-page into 2409338 on biocore:cart-branch.

squirrelo · 2015-04-23T01:46:05Z

This is now fully MOI-ified so it should be ready for merge. I will add the cart info stuff to the top corner in a subsequent pull request, since this one is already growing rather large.

coveralls · 2015-04-23T01:57:13Z

Coverage increased (+0.03%) to 79.35% when pulling 0cc55c9 on squirrelo:websockets-on-cart-page into 2409338 on biocore:cart-branch.

wasade · 2015-04-23T02:50:35Z

👍

Websockets on cart page

squirrelo added 10 commits April 12, 2015 17:04

merge cart branch

ef223a1

add websocket handler

a4df330

add websocket use to cart page

f271e08

fix id name

bd3ec14

remove create analysis modal, change create link

3e9957f

move create analysis

50201fc

merge upstream/cart-branch

d616326

flake8

54da32b

fix websocket error message

5316632

update test

a828daa

Merge branch 'cart-branch' of https://github.com/biocore/qiita into w…

795c748

…ebsockets-on-cart-page

merge upstream/cart-branch

734a920

fix merge error

05b3837

ElDeveloper reviewed Apr 18, 2015
View reviewed changes

wasade reviewed Apr 18, 2015
View reviewed changes

address comments

399cead

fix create analysis submit page

5b253ff

wasade reviewed Apr 22, 2015
View reviewed changes

add space

05c24c0

centralize websocket creation

a239792

squirrelo mentioned this pull request Apr 22, 2015

Immediate selection/deselection of samples #226

Closed

squirrelo added 3 commits April 22, 2015 16:20

update moi.js for extended use

a20e3a0

update cart page to use MOI

e70015a

add moi use to study_list page

0cc55c9

antgonza added a commit that referenced this pull request Apr 23, 2015

Merge pull request #1058 from squirrelo/websockets-on-cart-page

460df34

Websockets on cart page

antgonza merged commit 460df34 into qiita-spots:cart-branch Apr 23, 2015

squirrelo mentioned this pull request Apr 28, 2015

WebSockets will not work over HTTPS #993

Closed

Websockets on cart page #1058

Websockets on cart page #1058

Uh oh!

Conversation

squirrelo commented Apr 13, 2015

Uh oh!

coveralls commented Apr 13, 2015

Uh oh!

squirrelo commented Apr 13, 2015

Uh oh!

coveralls commented Apr 13, 2015

Uh oh!

ElDeveloper commented Apr 17, 2015

Uh oh!

squirrelo commented Apr 17, 2015

Uh oh!

coveralls commented Apr 17, 2015

Uh oh!

squirrelo commented Apr 17, 2015

Uh oh!

ElDeveloper commented Apr 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElDeveloper commented Apr 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElDeveloper commented Apr 18, 2015

Uh oh!

ElDeveloper commented Apr 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas commented Apr 21, 2015

Uh oh!

josenavas commented Apr 21, 2015

Uh oh!

squirrelo commented Apr 21, 2015

Uh oh!

josenavas commented Apr 21, 2015

Uh oh!

squirrelo commented Apr 21, 2015

Uh oh!

josenavas commented Apr 21, 2015

Uh oh!

squirrelo commented Apr 21, 2015

Uh oh!

coveralls commented Apr 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wasade commented Apr 22, 2015

Uh oh!

squirrelo commented Apr 22, 2015

Uh oh!

wasade commented Apr 22, 2015

Uh oh!

coveralls commented Apr 22, 2015

Uh oh!

squirrelo commented Apr 22, 2015

Uh oh!

wasade commented Apr 22, 2015

Uh oh!

coveralls commented Apr 22, 2015

Uh oh!