PTV-1886 staging area limit file size #3312

eapearson · 2023-07-10T19:43:21Z

Description of PR purpose/changes

The primary motivation for this set of changes is to ensure that the staging area widget applies a file size limit of 5GB to user uploads. This is achieved by setting that value in the Narrative config template, and propagating that value to the dropzone widget. This code was in place, although the max file size value was inaccurate.

In addition, there are changes around ensuring that this and related errors are properly displayed. And around this are changes to ensure that the staging upload widget communicates this file size limit to the user.

One related set of fixes applies to the usage of decimal and binary measurements of file size. In several cases these units were mixed and up and communicating incorrect information to the user. The important distinction is that users will be aware of decimal file size (e.g. MB, GB) as that is what is reported by most operating systems and tools that rely upon them. On the other hand, the dropzone widget and nginx work in binary file sizes (e.g. MiB, GiB). So there is code to convert back and forth and ensure that users see file sizes in decimal. Generally all code uses file size measurements in bytes.

A document attached shows the before and after ui states.

Jira Ticket / Issue

Related Jira ticket: https://kbase-jira.atlassian.net/browse/DATAUP-X

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:

Tests pass locally and in GitHub Actions
Changes available by spinning up a local narrative and navigating to X to see Y

Dev Checklist:

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)

…-1886]

… icon, add hover effect for the button (affordance), in detail show info tab by default, remove unnecessary bluebird, rename import that conflicts with built-ins, minor formatting improvements [PTV-1886]

Fixes case of 0-length file, uses Alert rather than showing warning in same style as file snippet, handle null as head/tail snippet as well as the bespoke text value "not text flie" [PTV-1886]

…n, set maxFileSize for dropzone widget, support decimal and binary file sizes explicitly (and in appropriate places), handle 413 (request entity too big) errors, generally improve dropzone layout and styling, display max file size in dropzone, fix globus link handling and use a dialog before opening a new window to catch and display errors [PTV-1886]

… it internally, especially when debugging tests! [PTV-1886]

note - need another pass at tests to cover changes during PR-preparation

some test cases added, some unused code removed, some code simplified

…rea-limit-file-size

codecov · 2023-07-10T19:54:47Z

Codecov Report

Merging #3312 (e45e547) into develop (3bccc58) will increase coverage by 0.04%.
The diff coverage is 79.05%.

❗ Current head e45e547 differs from pull request most recent head bb4f7e0. Consider uploading reports for the commit bb4f7e0 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3312      +/-   ##
===========================================
+ Coverage    29.22%   29.27%   +0.04%     
===========================================
  Files          495      495              
  Lines        50482    50545      +63     
===========================================
+ Hits         14755    14798      +43     
- Misses       35727    35747      +20

Impacted Files	Coverage Δ
...ets/narrative_core/kbaseNarrativeStagingDataTab.js	`93.18% <ø> (ø)`
...widgets/narrative_core/upload/stagingAreaViewer.js	`73.74% <25.00%> (-1.19%)`	⬇️
.../widgets/narrative_core/upload/fileUploadWidget.js	`84.45% <83.20%> (+7.53%)`	⬆️
...-extension/static/kbase/js/util/bootstrapDialog.js	`97.43% <100.00%> (+0.06%)`	⬆️
...ion/static/kbase/js/widgets/common/AlertMessage.js	`100.00% <100.00%> (ø)`

... and 4 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

eapearson · 2023-07-10T20:57:49Z

Before and after user interface:

PTV-1886_ Before and After.pdf

…rea-limit-file-size

eapearson · 2023-07-19T17:27:44Z

@Bill @ialarmedalien resolved merge conflicts, merged the repaired tox.ini into .flake8, some code quality fixes to make the bots happier. Don't know why the integration tests aren't passing. I spot checked one of the narratives involved for a failing public data test, and it looks fine. (Such integration tests will occasionally fail due to changes in ref data since they use live CI services.)

briehl

There's a lot happening here, but this looks good.
Got a couple minor changes and suggestions for you.

briehl · 2023-07-21T21:01:22Z

docs/developer/data_import.md

+
+For a custom 413 response, first define a location within the server block handling the `staging_server`.
+
+```nginx


Is this set up in the staging service? I know you were making changes there, too, with @bio-boris .

This will come from changes to the nginx proxy. I've included the instructions, at least for what worked for me.

I tested this out by setting the kbase-ui proxy up with a low limit to trigger the 413 for arbitrary file sizes.

Yes, there is a set of changes coming for the staging service too, but the Narrative changes (other than the handling of a custom 413, which is auto-detected) don't depend on them. However, for anyone sending files between 4.4 and 5GB, the current staging service will crash. The upcoming changes address this.

briehl · 2023-07-21T21:02:24Z

kbase-extension/scss/partials/_dropzoneDz.scss

@@ -78,8 +79,8 @@

 .kb-dropzone {
  border: 2px dashed use_color('mid-blue') !important;
-  margin-bottom: 5px;
-  max-height: 150px;
+  // margin-bottom: 5px;


You can just delete these. I don't think they need to come back.

Roger that.

briehl · 2023-07-21T21:02:29Z

kbase-extension/scss/partials/_dropzoneDz.scss

@@ -91,14 +92,23 @@
  .dz-message {
    color: rgb(0 0 0);
    font: normal 400 24px/28px $typeface-page-text;
-    margin: 2em 4.5em;
+    // margin: 2em 4.5em;


Yeah, ye ole "not sure this will work, keep an eye on it and remove later" that doesn't get removed.
Gone, and also rearranging a bit to put styles where they should be, and adding some of those "this is the full path of this style" comments. They do help clarify, as nested styles can be painful to follow...

Yeah, the nested styles are hard, having the full path as a comment is very helpful!

briehl · 2023-07-21T21:02:41Z

kbase-extension/scss/partials/_dropzoneDz.scss

  }

  // .kb-dropzone__message--upload
  &__message--upload {
    font-family: $typeface-page-text;
    font-weight: 700;
  }
-}
+
+  &.dropzone.dz-clickable button {


briehl · 2023-07-21T21:03:26Z

kbase-extension/scss/partials/_fileUploadWidget.scss

+    // Bootstrap overrides
+
+    // necessary?
+    .progress-bar {


It might not be necessary at this point, but Bootstrap's progressbar is occasionally an oddball, and was annoying to work against Dropzone.

bootstrap already sets the width for .progress-bar to 0, and I don't see anywhere in the KBase codebase that sets this class otherwise that would need overriding.

Also, in the widget code the width is set via direct style to '0' on reset, but should probably set it to '' to remove the style and let the class style take over.

Anyway, it is working, so I don't see any reason to change anything here, other than the fact that these files are being revised and who knows when the next opportunity will come.

briehl · 2023-07-21T21:12:41Z

test/unit/spec/narrative_core/upload/fileUploadWidget-413-custom-error-bad-spec.js

+            // Realistically, this should poll until the condition is met, or
+            // timeout. Reason being that in real usage (or if using a real service mock),
+            // the call that generates the error may take an indeterminate amount of time,
+            // and certainly will not be on the next tick of the event loop.


There's some tooling in testUtil that might be helpful to get the behavior you mention. The waitForElementState function might make the most sense here. Then you can make it an async test, await on that, and it'll fail if it times out.

The existing file upload widget tests don't use that, I guess, since it was made later. If this works and is stable for now, that's fine.

Yeah, it works, so I don't see a reason to just change it.
It was more of a note to preserve the fact that it only works because it is using a specific style of mock service. It would fail, e.g., in an integration test, or a mock service which is either a true http server or includes a modest delay to simulate a network call.
I had just been through evaluating the implementation with different browsers. If memory serves, I found that some browsers wait for a good long while before reporting back the 413, others immediately, but in all cases it is a network call to the service occurs first, which is certainly longer in duration than a 0ms timeout.

Okay, that sounds reasonable.

briehl · 2023-07-21T21:16:26Z

test/unit/spec/util/BootstrapDialogSimpleSpec.js

@@ -0,0 +1,107 @@
+define(['jquery', 'util/bootstrapDialog'], ($, Dialog) => {


briehl · 2023-07-21T21:17:59Z

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

@@ -1,14 +1,23 @@
-<form class="kb-dropzone dropzone" style="min-height:200px">
+<form class="kb-dropzone dropzone"
+    style="min-height:100px; display: flex; flex-direction: column; justify-content: center;">


Can you move the inline styles to SCSS?

eapearson · 2023-07-21T22:30:03Z

Thanks for looking at it, @briehl, working on it.

…-1886]

…886]

…rea-limit-file-size

sonarqubecloud · 2023-07-21T23:45:27Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
19.2% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

briehl · 2023-08-01T17:28:12Z

Let me know when this is ready for re-review, thanks!

eapearson added 15 commits June 20, 2023 09:44

pyyaml missing from requirements.txt [PTV-1886]

7c640a7

add "filesize" package for uniform display of file sizes w/units [PTV…

78c51cf

…-1886]

fixes include: make clickable detail button the button itself not the…

7902a54

… icon, add hover effect for the button (affordance), in detail show info tab by default, remove unnecessary bluebird, rename import that conflicts with built-ins, minor formatting improvements [PTV-1886]

improve file snippet display [PTV-1886]

79adc72

Fixes case of 0-length file, uses Alert rather than showing warning in same style as file snippet, handle null as head/tail snippet as well as the bespoke text value "not text flie" [PTV-1886]

propagate file size config to widget [PTV-1886]

6536c04

bootstrap alert wrapper default to 'info' type [PTV-1886]

8c2cae5

support "shown" event [PTV-1886]

54586c1

Set max file size to 5GB [PTV-1886]

412c5ac

spell out "button" - btn is bootstrap naming and bit confusing to use…

1b3406a

… it internally, especially when debugging tests! [PTV-1886]

fix test server startup detection [PTV-1886]

d17a9b5

add frontend unit tests for new and modified code [PTV-1886]

71ecedd

note - need another pass at tests to cover changes during PR-preparation

improve code and tests in response to testing [PTV-1886]

62ce65d

some test cases added, some unused code removed, some code simplified

Merge remote-tracking branch 'origin/develop' into PTV-1886-staging-a…

43e8b49

…rea-limit-file-size

add release note [PTV-1886]

f3d5a49

eapearson added 5 commits July 10, 2023 14:21

make deepscan happy, sonarcloud happey? [PTV-1886]

5ffcda3

more sonarcloud fixes [PTV-1886]

6911a4a

couple more [PTV-1886]

8182b1a

add documentation for nginx config for file size limits [PTV-1886]

21b2936

add note for local-docker.md [PTV-1886]

dde8402

eapearson requested a review from briehl July 10, 2023 23:28

eapearson added 6 commits July 13, 2023 12:40

Merge remote-tracking branch 'origin/develop' into PTV-1886-staging-a…

4e052d9

…rea-limit-file-size

Merge remote-tracking branch 'origin/develop' into PTV-1886-staging-a…

28d0b98

…rea-limit-file-size

merge flake8 config into tox.ini [PTV-1886]

277a634

replace tox with .flake8 [PTV-1886]

70fd434

address a few reported code quality issues [PTV-1886]

b4a3fb5

get this line ignored by SONAR and prettier. [PTV-1886]

e45e547

briehl requested changes Jul 21, 2023

View reviewed changes

move some direct styles in to scss [PTV-1886]

8dfab32

eapearson added 3 commits July 21, 2023 16:22

move descendant styles under the parent, add clarifying comments [PTV…

bfd0724

…-1886]

redundant direct style - should remove not override class here [PTV-1…

a4a3a2e

…886]

Merge remote-tracking branch 'origin/develop' into PTV-1886-staging-a…

bb4f7e0

…rea-limit-file-size

briehl mentioned this pull request Oct 3, 2024

DEVOPS-1440 Update dropzone_area.html #3654

Draft

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PTV-1886 staging area limit file size #3312

PTV-1886 staging area limit file size #3312

eapearson commented Jul 10, 2023 •

edited

Loading

codecov bot commented Jul 10, 2023 •

edited

Loading

eapearson commented Jul 10, 2023 •

edited

Loading

eapearson commented Jul 19, 2023

briehl left a comment

briehl Jul 21, 2023

eapearson Jul 21, 2023

briehl Jul 21, 2023

eapearson Jul 21, 2023

briehl Jul 21, 2023

eapearson Jul 21, 2023

briehl Jul 21, 2023

briehl Jul 21, 2023

briehl Jul 21, 2023

eapearson Jul 21, 2023

briehl Jul 21, 2023

briehl Jul 21, 2023

eapearson Jul 21, 2023

briehl Jul 21, 2023

briehl Jul 21, 2023

briehl Jul 21, 2023

eapearson commented Jul 21, 2023

sonarqubecloud bot commented Jul 21, 2023

briehl commented Aug 1, 2023


		For a custom 413 response, first define a location within the server block handling the `staging_server`.

		```nginx

		@@ -0,0 +1,107 @@
		define(['jquery', 'util/bootstrapDialog'], ($, Dialog) => {

PTV-1886 staging area limit file size #3312

Are you sure you want to change the base?

PTV-1886 staging area limit file size #3312

Conversation

eapearson commented Jul 10, 2023 • edited Loading

Description of PR purpose/changes

Jira Ticket / Issue

Testing Instructions

Dev Checklist:

Updating Version and Release Notes (if applicable)

codecov bot commented Jul 10, 2023 • edited Loading

Codecov Report

eapearson commented Jul 10, 2023 • edited Loading

eapearson commented Jul 19, 2023

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

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

eapearson commented Jul 21, 2023

sonarqubecloud bot commented Jul 21, 2023

briehl commented Aug 1, 2023

eapearson commented Jul 10, 2023 •

edited

Loading

codecov bot commented Jul 10, 2023 •

edited

Loading

eapearson commented Jul 10, 2023 •

edited

Loading