DATAUP-210 File size error dropzone #1903

eamahanna · 2020-10-28T02:06:16Z

Description of PR purpose/changes

This PR adds a file too large message as detailed in DATAUP-210.

Jira Ticket / Issue

https://kbase-jira.atlassian.net/browse/DATAUP-210

Added the Jira Ticket to the title of the PR e.g. (DATAUP-69 Adds a PR template)

Testing Instructions

Details for how to test the PR:
execute make test-frontend-unit
Navigate to the data import tab on localhost. Add a file that exceeds 20 gb. You will now be able to see the error and click on the link to navigate to the globus docs or a globus account, depending on how you are signed in.

Dev Checklist:

My code follows the guidelines at https://sites.google.com/truss.works/kbasetruss/development
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I have run Black and Flake8 on changed Python Code manually or with git precommit (and the travis build passes)

Updating Version and Release Notes (if applicable)

Version has been bumped for each release
Release notes have been updated for each release (and during the merge of feature branches)

lgtm-com · 2020-10-28T02:16:20Z

This pull request introduces 2 alerts when merging c8b2532 into a30f5aa - view on LGTM.com

new alerts:

1 for Useless assignment to local variable
1 for Potentially unsafe external link

coveralls · 2020-10-28T03:17:53Z

Coverage increased (+0.01%) to 17.035% when pulling 48c6fbb on DATAUP-210-dz-file-size-error into ab028c9 on truss.

leia-sefkin · 2020-10-28T19:55:57Z

kbase-extension/static/kbase/js/widgets/narrative_core/upload/fileUploadWidget.js

-                        errorText = err.xhr.responseText;
+                    var $errorMessage = $errorElem.find('#error_message');
+
+                    // I don't know how to determine if the file was too big other than looking at the preview message


yeah looks like dropzone doesn't offer much help here, there's a maxfilesexceeded event but not for maxFilesize
https://www.dropzonejs.com/#events

this approach seems to make sense given that

The erroredFile is a standard WebAPI File object, so it has a size attribute which gives up the size in bytes. In theory, that should be the error triggered by the maxFilesize option way up above where the Dropzone is created. Maybe that's useful?

leia-sefkin

A couple of places I could see minor improvements, but looks good overall, and works when I tested locally

kbase-extension/static/kbase/js/widgets/narrative_core/upload/fileUploadWidget.js

leia-sefkin · 2020-10-28T19:59:25Z

kbase-extension/static/kbase/templates/data_staging/dropped_file.html

-        <div id="status-message" style="display:none"></div>
-        <div class="progress progress-striped active" role="progressbar" aria-valuemin="0" aria-valuemax="100" aria-valuenow="0" style="margin-bottom: inherit; margin-left: 5px">
-            <div class="progress-bar progress-bar-success" style="width:0%;" data-dz-uploadprogress></div>
+<div class="row file-row dz-file">


look at all those inline styles - gone 😿 so beautiful

leia-sefkin · 2020-10-28T20:00:01Z

kbase-extension/static/kbase/templates/data_staging/dropped_file.html

+        <div class="col-md-1 ">
+            <span class="size pull-right" data-dz-size></span>
+        </div>
+        <div id="upload_progress_and_cancel" style="display: inline;">


any way to remove this inline style?

briehl

Just a couple minor suggestions. But looks good!

briehl · 2020-10-28T22:01:18Z

kbase-extension/static/kbase/js/widgets/narrative_core/upload/fileUploadWidget.js

-                        errorText = err.xhr.responseText;
+                    var $errorMessage = $errorElem.find('#error_message');
+
+                    // I don't know how to determine if the file was too big other than looking at the preview message


The erroredFile is a standard WebAPI File object, so it has a size attribute which gives up the size in bytes. In theory, that should be the error triggered by the maxFilesize option way up above where the Dropzone is created. Maybe that's useful?

briehl · 2020-10-28T22:03:37Z

kbase-extension/static/kbase/js/widgets/narrative_core/upload/fileUploadWidget.js

@@ -168,6 +186,37 @@ define([
            $('#clear-all-btn').remove();
        },

+        makeGlobusErrorLink: function(globusUrlLinked) {
+            let url = 'https://docs.kbase.us/data/globus';


Should this be part of the uploadConfig object?

Also, this and $globusErrorLink should be const

briehl · 2020-10-28T22:04:45Z

kbase-extension/static/kbase/js/widgets/narrative_core/upload/fileUploadWidget.js

+            let $globusErrorLink = $("<a>")
+            .attr('id', 'globus_error_link')
+            .attr('href', url)
+            // .attr('target', '_blank')


can delete this comment

…ize-error

lgtm-com · 2020-10-28T22:56:20Z

This pull request introduces 1 alert when merging 58162c2 into ab028c9 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2020-10-29T20:37:20Z

This pull request introduces 1 alert when merging 69c834a into abd2004 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2020-10-29T20:48:26Z

This pull request introduces 1 alert when merging a3b35cc into abd2004 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2020-10-29T20:57:33Z

This pull request introduces 1 alert when merging 4a9890c into abd2004 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

sonarcloud · 2020-10-30T00:38:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

codecov · 2020-10-30T00:40:53Z

Codecov Report

Merging #1903 into truss will increase coverage by 0.00%.
The diff coverage is 44.44%.

@@           Coverage Diff           @@
##            truss    #1903   +/-   ##
=======================================
  Coverage   12.68%   12.68%           
=======================================
  Files         405      405           
  Lines       43725    43739   +14     
=======================================
+ Hits         5545     5550    +5     
- Misses      38180    38189    +9

Impacted Files	Coverage Δ
.../widgets/narrative_core/upload/fileUploadWidget.js	`74.74% <44.44%> (-1.73%)`	⬇️
...widgets/narrative_core/upload/stagingAreaViewer.js	`62.17% <0.00%> (-0.44%)`	⬇️
...s/widgets/narrative_core/kbaseNarrativeDataList.js	`35.19% <0.00%> (-0.42%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f10503...91510ce. Read the comment docs.

eamahanna marked this pull request as draft October 28, 2020 02:06

eamahanna marked this pull request as ready for review October 28, 2020 15:32

eamahanna requested review from briehl, leia-sefkin, bio-boris and ialarmedalien October 28, 2020 15:32

eamahanna marked this pull request as draft October 28, 2020 15:52

leia-sefkin reviewed Oct 28, 2020

View reviewed changes

leia-sefkin approved these changes Oct 28, 2020

View reviewed changes

briehl approved these changes Oct 28, 2020

View reviewed changes

eamahanna added 15 commits October 28, 2020 14:28

Add functionality

1cc0713

styling changes

9d5eefb

fix alignment

918c8f0

fix typo

09816dc

Fix success clear

1ef7c5f

Add test

c467f11

quick error fix

e3bd08e

fix quotes

04806f1

Fix tests

4a05a7c

review edits

cb4d6eb

add tests

9b48ea2

Review edits

e30abd2

review edit

7cad4f4

fix link path

320802b

review edits

0736142

eamahanna force-pushed the DATAUP-210-dz-file-size-error branch from 78729d2 to 0736142 Compare October 28, 2020 22:37

eamahanna marked this pull request as ready for review October 28, 2020 22:38

Merge remote-tracking branch 'origin/truss' into DATAUP-210-dz-file-s…

58162c2

…ize-error

eamahanna added 2 commits October 29, 2020 12:27

Fix progress bar

69c834a

Fix progress bar

a3b35cc

fix const

4a9890c

eamahanna added 3 commits October 29, 2020 13:19

fix if else

162f382

a little cleanup

48c6fbb

Merge branch 'truss' into DATAUP-210-dz-file-size-error

91510ce

eamahanna merged commit 7539b8d into truss Oct 30, 2020

eamahanna deleted the DATAUP-210-dz-file-size-error branch October 30, 2020 00:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DATAUP-210 File size error dropzone #1903

DATAUP-210 File size error dropzone #1903

eamahanna commented Oct 28, 2020

lgtm-com bot commented Oct 28, 2020

coveralls commented Oct 28, 2020 •

edited

Loading

leia-sefkin Oct 28, 2020

briehl Oct 28, 2020

leia-sefkin left a comment

leia-sefkin Oct 28, 2020

leia-sefkin Oct 28, 2020

briehl left a comment

briehl Oct 28, 2020

briehl Oct 28, 2020

briehl Oct 28, 2020

briehl Oct 28, 2020

lgtm-com bot commented Oct 28, 2020

lgtm-com bot commented Oct 29, 2020

lgtm-com bot commented Oct 29, 2020

lgtm-com bot commented Oct 29, 2020

sonarcloud bot commented Oct 30, 2020

codecov bot commented Oct 30, 2020 •

edited

Loading

DATAUP-210 File size error dropzone #1903

DATAUP-210 File size error dropzone #1903

Conversation

eamahanna commented Oct 28, 2020

Description of PR purpose/changes

Jira Ticket / Issue

Testing Instructions

Dev Checklist:

Updating Version and Release Notes (if applicable)

lgtm-com bot commented Oct 28, 2020

coveralls commented Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leia-sefkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briehl 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

lgtm-com bot commented Oct 28, 2020

lgtm-com bot commented Oct 29, 2020

lgtm-com bot commented Oct 29, 2020

lgtm-com bot commented Oct 29, 2020

sonarcloud bot commented Oct 30, 2020

codecov bot commented Oct 30, 2020 • edited Loading

Codecov Report

coveralls commented Oct 28, 2020 •

edited

Loading

codecov bot commented Oct 30, 2020 •

edited

Loading