Import gff ui #16

yaoyuyang · 2017-11-13T17:16:04Z

Add UI for import gff as genome in the database. Feature includes upload form validation and importing status and link to the uploaded genome after importing is finished.

yaoyuyang · 2017-11-13T18:27:20Z

@aruginkgo keep you in the loop. I added the import gff UI here if you want to learn how I added them since I remember at one point you were working on something similar.

myw

This is great!
My comments point out some ideas for improvement, some consistency/style points, and some minor errors.

myw · 2017-11-13T23:07:19Z

.gitignore

@@ -64,4 +64,6 @@ test.db*
 ncbi/

 # compiled static files
-static/
+src/edge/static/edge/assets


these lines do not need to be added here. Asset compilation has changed; all compiled assets that should be ignored now go in src/edge/static/edge/assets, the compiled assets are ignored in the other .gitignore file, src/.gitignore (I know, this is confusing; i didn't set this up, I just tried not to break it and keep the boundaries consistent.

these three lines should be removed; my original line should not be added back, however. that was just something i forgot to remove from when i was trying something different.
also, feel free to remove src/edge/static/edge/edge.css in your local tree you won't need it.

myw · 2017-11-13T23:08:32Z

src/edge/static/edge/src/js/app.js

+            var model = $parse(attrs.fileModel);
+            var modelSetter = model.assign;
+
+            element.bind('change', function(){


spaces after closing paren for anon funcs, as above; please make it consistent throughout with whatever the existing convention is

why not just use a regular ng-model for this? is it because ngModel doesn't allow you to actually assign to sub-attributes?

This is to get the file in the controller's scope so I can process them easily. I learned this from here. https://www.uncorkedstudios.com/blog/multipartformdata-file-upload-with-angularjs/
Not sure if this is the best way to do but it works fine.

myw · 2017-11-13T23:33:30Z

src/edge/static/edge/src/js/controllers.js

+    $scope.add_genome_error = undefined;
+    $scope.addGenome = function(){
+        var file = $scope.gffFile;
+        $scope.add_genome_error = undefined;


should use camelCase here for consistency; pretty much all JS has now moved to this convention.

You are right! Fixed. i_have_too_much_love_for_snake_case. Ruby was my first love and it has a profound impact on my coding style.

myw · 2017-11-14T01:57:53Z

src/edge/static/edge/src/js/controllers.js

+        $scope.add_genome_pending = undefined;
+        $scope.add_genome_result = undefined;
+        if (!file) {
+            $scope.add_genome_error = {};


can do this with a single object literal: $scope.addGenomeError = { message: 'Need to choose GFF file' }; also, prefer single quotes for consistency

myw · 2017-11-14T02:02:47Z

src/edge/static/edge/src/partials/header.html

@@ -0,0 +1,10 @@
+<header>


Well done. This is much cleaner!

myw · 2017-11-14T03:00:04Z

src/edge/views.py

    response = HttpResponse(content_type='text/plain')
    response['Content-Disposition'] = "attachment; filename=\"g%s.gff\"" % genome_id
    io.to_gff_file(response)
    return response


+def genome_import(request):


you should stick some REST validation around this, making sure it only gets called on POSTs, etc; you can do this with the @require_POST() decorator or a class-based view that only implements the post method.

Good thoughts! It's actually @require_http_methods(["POST"]) https://docs.djangoproject.com/en/1.11/topics/http/decorators/

myw · 2017-11-14T03:01:16Z

src/edge/views.py

    response = HttpResponse(content_type='text/plain')
    response['Content-Disposition'] = "attachment; filename=\"g%s.gff\"" % genome_id
    io.to_gff_file(response)
    return response


+def genome_import(request):
+    import tempfile


unless you have a great reason for doing otherwise, it's less confusing IMO to put all imports at the top of a file.

myw · 2017-11-14T03:02:33Z

src/edge/views.py

+            "id": g.id,
+            "name": g.name,
+        })
+        build_genome_blastdb.delay(g.id)


you should return the task id in the API, so that if someone wants to check on the status, they can

myw · 2017-11-14T03:04:12Z

src/edge/views.py

+def genome_import(request):
+    import tempfile
+    res = {
+      "imported_genomes": [],


currently, if your code throws any errors in this, you'll just return a 500 status; that might be fine, but there may be some errors you want to catch and report in your API instead, in an 'error' field.

myw · 2017-11-14T03:09:43Z

src/edge/views.py

+        build_genome_blastdb.delay(g.id)
+    return HttpResponse(json.dumps(res), content_type='application/json')
+
+


Are there tests for other views like this? Might make sense to add one if there are; shouldn't be so bad; i'd just mock up the task and import_gff, and make sure that both are called as expected based on the number of genomes that are submitted.

Should add a test for sure.

- gunzip ecoli-mg1655.gff for easier use as an example - Update styling - Fix label for="gff_file" that needs input id='gff_file'

yaoyuyang · 2017-11-15T19:15:11Z

@myw could you take another look? I've resolved most of your comments! Thank you for the detailed review. Learned quite a few things!

myw

Some HTML cleanup and a better way to write the GFF file.

myw · 2017-11-15T21:33:33Z

src/edge/static/edge/src/js/controllers.js

+    $scope.getBaseURL = function() { return '/edge/'; }
+    $scope.addGenome = function(){
+        var file = $scope.gffFile;
+        $scope.addGenomeStatus = {


nice. I like this approach

myw · 2017-11-15T21:33:53Z

src/edge/static/edge/src/js/controllers.js

+        }
+        $scope.addGenomeStatus.pending = true;
+        var fd = new FormData();
+        var gff_name = $scope.genome.name;


still wrong convention

myw · 2017-11-15T21:34:50Z

src/edge/static/edge/src/partials/import.html

+    </div>
+    <div class="form-group">
+        <label for="gff_file">GFF File (GFF3 format):</label><br/>
+        <input type="file" file-model="gffFile" id='gff_file'/>


html attributes should use double quotes; also, Ids typically come first

myw · 2017-11-15T21:35:45Z

src/edge/static/edge/src/partials/import.html

+
+    <div class="form-group">
+        <label for="genome_name">Genome Name:</label>
+        <input id="genome_name", name=name, type="text" ng-model="genome.name" required />


attributes should not have commas between them, and "name" should have quotes around it.

myw · 2017-11-15T21:37:03Z

src/edge/static/edge/src/partials/import.html

+        <label for="gff_file">GFF File (GFF3 format):</label><br/>
+        <input type="file" file-model="gffFile" id='gff_file'/>
+    </div>
+    <button class="btn btn-primary", type="submit", ng-disabled="userForm.$invalid", ng-click="addGenome()">Import</button>


no commas between attributes

Ah. Fixed. I misread your previous comments, literally oblivated not

myw · 2017-11-15T21:37:43Z

src/edge/static/edge/src/partials/import.html

+
+<div class="well" ng-show="addGenomeStatus.results">
+    <span class="text-success" ng-repeat='genome in addGenomeStatus.results.imported_genomes'>
+    Success: Added in Edge as <a ng-href="#/genomes/{{ genome.id }}">Genome {{ genome.id }}: {{ genome.name }}</a></span>


span tag alignment is still weird here

myw · 2017-11-15T21:38:56Z

src/edge/views.py

    response = HttpResponse(content_type='text/plain')
    response['Content-Disposition'] = "attachment; filename=\"g%s.gff\"" % genome_id
    io.to_gff_file(response)
    return response


+@require_http_methods(["POST"])


by convention, we stick with single quotes for non-docstrings, unless you need them for brevity when expressing string literals that contain single quotes, e.g. "I like to 'eat' foods"

myw · 2017-11-15T21:43:24Z

src/edge/views.py

+    }
+    for name in request.FILES:
+        with tempfile.NamedTemporaryFile(mode='rw+b', delete=False) as gff:
+            gff.write(request.FILES.get(name).read())


using .read() can cause OOM errors for large files. Since GFFs can be large, I suggest doing the django-recommended best-practice and using UploadedFile.chunks. See the docs for an example.

myw · 2017-11-15T21:45:36Z

src/edge/views.py

+    res = {
+      'imported_genomes': [],
+    }
+    for name in request.FILES:


this is more a matter of taste, but I find it much easier to read code with period line breaks in it to separate "paragraphs" or "thoughts". In here, to increase legibility, I'd stick extra lines after the dict literal, and before the return, and around each individual "component" of the for loop: the gff writing/deletion, the BLASTdb task creation, and the call to JsonResponse.

yaoyuyang · 2017-11-17T16:22:03Z

@myw can you take another look? Much appreciated!

myw

Just the commas;
commas betweet html attributes are not valid html

myw · 2017-11-17T16:24:03Z

src/edge/static/edge/src/partials/import.html

+
+    <div class="form-group">
+        <label for="genomeName">Genome Name:</label>
+        <input id="genomeName", name="name", type="text", ng-model="genome.name" required />


No commas in html attributes

yaoyuyang · 2017-11-17T16:34:33Z

@myw fixed. Another look? Thanks!

myw · 2017-11-17T16:37:15Z

All good. Ready for merge! Re: the commas; no worries. I figured that is exactly what happened as I saw that you added more.

yaoyuyang · 2017-11-17T16:46:44Z

Surprisingly, the commas did not cause any problems in html rendering in chrome even though it's clearly not the w3 standard. Maybe chrome is just forgiving about these errors.

yaoyuyang · 2017-11-17T16:47:04Z

Thanks for the thorough review @myw!

yaoyuyang added 4 commits November 12, 2017 19:31

Import gff ui working

4e51d45

Fix style

19cba09

Add import.html and header partial

663ba15

Remove unnecessary code

5b8a4db

yaoyuyang requested a review from myw November 13, 2017 17:16

myw suggested changes Nov 14, 2017

View reviewed changes

yaoyuyang added 6 commits November 14, 2017 23:17

Adjust .gitignore since src/.gitignore has it

da2fb97

Use camelCase and space after function()

0a1510a

Use JsonResponse and other styling

637da87

Add back HttpResponse

3184423

Add import view test, update to use addGenomeStatus

de74f27

- gunzip ecoli-mg1655.gff for easier use as an example - Update styling - Fix label for="gff_file" that needs input id='gff_file'

Update test gff file path

52eac68

myw suggested changes Nov 15, 2017

View reviewed changes

Fix html styling, use Django chucks instead of read

e230c91

yaoyuyang mentioned this pull request Nov 17, 2017

Add an import genome button #10

Closed

myw suggested changes Nov 17, 2017

View reviewed changes

No commas between html attributes!

92cd4a8

yaoyuyang merged commit b27c315 into master Nov 17, 2017

		build_genome_blastdb.delay(g.id)
		return HttpResponse(json.dumps(res), content_type='application/json')

Import gff ui #16

Import gff ui #16

Conversation

yaoyuyang commented Nov 13, 2017

yaoyuyang commented Nov 13, 2017

myw 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaoyuyang commented Nov 15, 2017

myw left a 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

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

yaoyuyang commented Nov 17, 2017

myw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaoyuyang commented Nov 17, 2017

myw commented Nov 17, 2017

yaoyuyang commented Nov 17, 2017

yaoyuyang commented Nov 17, 2017

myw left a comment •

edited

Loading