Centralize Add Plates modal dialog javascript and html #468
Conversation
…lates dialog from 7 pages that use them, put into these files
…log with call to external shared javascript function and import of external shared html snippet.
…log with call to external shared javascript function and import of external shared html snippet. Move code from old plateList.js file into this page as this is the only place it is used.
|
@AmandaBirmingham This sounds really nice! If you don't mind, I will finish up a WIP first before I review this one - should be in the next day or so. |
|
Quick note for pulling this down, the branch is in "upstream" (i.e., $ git remote -v | grep upstream
upstream [email protected]:jdereus/labman.git (fetch)
upstream [email protected]:jdereus/labman.git (push)
$ git checkout -b issue_468
$ git pull --rebase upstream Issue246and351 |
|
Will do!
…On Fri, Apr 19, 2019 at 1:35 PM Daniel McDonald ***@***.***> wrote:
Quick note for pulling this down, the branch is in "upstream" (i.e.,
jdereus). So for pulling:
$ git remote -v | grep upstream
upstream ***@***.***:jdereus/labman.git (fetch)
upstream ***@***.***:jdereus/labman.git (push)
$ git checkout -b issue_468
$ git pull --rebase upstream Issue246and351
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#468 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AKFU7E2LOCLUIJGNR56A5QDPRIUIDANCNFSM4HHB23QQ>
.
|
|
Thanks, @AmandaBirmingham! I've gone through the requested pages. Below are the comments that I wasn't sure warranted separate issues. Doing a code review now. Major:
AN ERROR HAS OCCURED!
Please copy the following into an issue in our github repository
Error
Cannot compress 5 gDNA plates. Please provide 1 to 4 gDNA plates
Traceback
Traceback (most recent call last):
File "/Users/dtmcdonald/miniconda3/envs/labcontrol/lib/python3.7/site-packages/tornado/web.py", line 1697, in _execute result = method(*self.path_args, **self.path_kwargs)
File "/Users/dtmcdonald/miniconda3/envs/labcontrol/lib/python3.7/site-packages/tornado/web.py", line 3174, in wrapper return method(self, *args, **kwargs)
File "/Users/dtmcdonald/ResearchWork/software/labman/labcontrol/gui/handlers/process_handlers/gdna_compression_process.py", line 50, in post self.current_user, plates, plate_ext_id, Equipment(robot))
File "/Users/dtmcdonald/ResearchWork/software/labman/labcontrol/db/process.py", line 773, in create 'gDNA plates' % len(plates))
ValueError: Cannot compress 5 gDNA plates. Please provide 1 to 4 gDNA plates Minor:
|
| addPlate(plateId); | ||
|
|
||
| // Disable the button to add the plate to the list | ||
| $('#addBtnPlate' + plateId).prop('disabled', true); |
There was a problem hiding this comment.
Should the button be disabled before entering the try block?
|
Thanks, @wasade -- this is great! Re #468 (comment) , I tracked down the issue you found with adding 5 plates to the compression page and verified that it is not due to adding plates fast and exists in master, so I created a new issue for it at #477 ; fortunately it looks super easy to fix and the fix can be added easily to this PR. With that red herring out of the way, could you say more about the race condition you observed? ("If the user is adding plates fast, the modal pops up and disappears before the user can do anything"). I have not been able to reproduce it--perhaps I am not fast enough? :) If you could provide either instructions to reproduce or a gif of it happening (on a page other than compression bc we know that one is screwed up) it would really help me track this down. I will look into the search functionality ... |
|
Per #468 (comment) , I checked into the search functionality. It is provided by the external DataTables javascript library; here is the description of its search API: https://datatables.net/reference/api/search() . Essentially, it appears to be a pretty greedy search that returns results that contain within them all of the strings entered, in any order, case-insensitively, across the union of all searchable fields . Since the study name is a searchable field and since "Identification of the Microbiomes for Cannabis Soils" contains the strings fo (in "for"), so (in "Soils"), ca (in "Cannabis"), and de (in "Identification"), and since the plate name is a searchable field and contains the string la (in "plates"), the results returned for the search input "fo so ca la de" are consistent with the DataTables search rules. From the DataTables documentation, it looks like we can tweak the search functionality in various ways, e.g. below, pulled from the link above. @wasade, please provide feedback on whether you feel this is necessary.
|
|
@AmandaBirmingham race condition may not be the most accurate term but felt approximately like what was observed :) I haven't had this happen on other pages, so it may have been isolated to the compression page. #473 and the gif contained highlights a possibly related issue though where the modal can be opened even though the loading is still happening which can be misleading That makes total sense re: search. Let's drop that item. |
|
Putting this in the PR, since I'm testing these code changes: When I try to create a new sample plate, I find that I cannot. Has anyone else seen this issue? Possible it's something wrong with my configuration; in any case, it's odd that I have no idea why:
|
|
@charles-cowart wooo, that is weird. @wasade can you reproduce in the PR branch? I cannot, although I will keep trying ... |
|
@AmandaBirmingham @wasade and I couldn't reproduce it in his dev environment, implying that it's something with my local configuration; although it's not immediately obvious to the both of us what it is. Confirmed I'm on commit fca5b..... I will look at a few other things. In the meantime, I took a screenshot of the developer console:
|
|
I tested add plate functionality on all of the requested pages, and this is what I encountered: normalization.html -> normalize compression.html -> gdna_compression extraction.html -> gdna_extraction library_pooling.html -> poollibraries library_prep_shotgun.html -> library_prep_shotgun library_prep_16S.html -> library_prep_16S parse_quantification.html -> parse_quantify I've attached animated GIFs for most of the errors I encountered. More than 3 seemed redundant: |
|
I also noticed some steps were not available in subsequent steps after I created them, but I will dig deeper on those and report anything as a separate issue. |
|
@charles-cowalt, please recall that 404 errors for plate names that don’t exist yet are in fact *required*: they are LabControl’s way of informing the front end that the plate name is available for use. That is why they are silently hidden rather than reported.
Given this, it appears to me that the only “real” :) error shown in these screenshots is the plating page’s TypeError when the code is unable to find the constructor for the Slick.CellSelectionModel object. I’d make a small bet that this is linked to your inability to create a sample plate (probably the error in the upstream JavaScript prevents later code that would enable the “Create” button from being executed).
If I recall correctly, you were doing some development work aimed at determining if we could remove some of the SlickGrid components from the plating page. Is it possible that some file or some file contents were removed or modified on your system during this investigation, and that some portion of those changes remain on your machine? This might speak to why the plating issue is occurring in your dev environment but mine or @wasade ‘s ...
|
|
@AmandaBirmingham Ah, of course! :D Thanks for the clarification. It's a fresh pull from GH so I don't think anything should be removed or modified, but I'll look into it shortly - thanks for the tip! |
|
Found it! It looks like the 'slickgrid.plugins' directory didn't get installed into my '~/miniconda3/envs/labman/lib/python3.5/site-packages/labcontrol/gui/static/vendor/js' directory. Once I copied it manually from the source tree to the installed base, everything worked fine. I'll check out the install script to see if that's the cause. Thanks again! |
|
@AmandaBirmingham The slickgrid.plugins issue has been resolved w/ #484. No other issues w/this PR outside of what is currently being discussed. |
…utton disabling to beginning of method to prevent user from trying to add same plate or another from modal while existing addition is in process.
…ateGUI for case where plate ids come in from querystring). Added more comments to clarify workings of prepopulateGUI (mostly to myself).
|
@wasade Have updated pull request, please re-review when you can. |
|
@AmandaBirmingham, a static view of the commits looks great, thank you!! Are you requesting a re-review of this PR in a live dev environment as well for the pages noted above, or are you comfortable with a static review? |
|
@wasade Urgh. I think I'm going to say static review .... Giving optimism a try :) That said, if you felt the need to review in a live dev environment, I certainly wouldn't try to stop you. |
|
@wasade Just realized I'm not sure where we stand with this. Assuming we are doing static review only, do you approve this PR? If so, I'd like to merge it soon; if not, would like to know what changes to make. |
|
Sorry for the delay. I approve :)
…On Thu, Apr 25, 2019, 1:49 PM Amanda Birmingham ***@***.***> wrote:
@wasade <https://github.com/wasade> Just realized I'm not sure where we
stand with this. *Assuming* we are doing static review only, do you
approve this PR? If so, I'd like to merge it soon; if not, would like to
know what changes to make.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#468 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AADTZMV67C2PQTLFP26LR4LPSIKNRANCNFSM4HHB23QQ>
.
|
Fixes #246, #351, #466 and #458.
I request that the reviewers actually download this PR as a branch and install it, and try adding plates on each of the SEVEN different pages that are affected (see list below).
I realize this is a big pain; however, as far as I know LabControl has no front-end javascript unit testing approach, and the code changes here affect PRIMARILY the front-end javascript ... so the fact that the PR passes the back-end unit-tests provides approximately no assurances about its fitness. I have trialed the plate addition on each of these pages in my own test environment, but I certainly don't trust myself to catch all problems.
Affected pages to test "Add Plate" functionality on:
normalization.html
compression.html
extraction.html
library_pooling.html
library_prep_shotgun.html
library_prep_16S.html
parse_quantification.html