7714 configure download all ds #7806

sekmiller · 2021-04-15T13:24:36Z

What this PR does / why we need it: Modifies messaging when a download exceeds the limit for zipping. Previously we would zip as many files as possible and put the names of the missing files in the manifest file. This was confusing and annoying to users who assumed they got all of the files requested. With this change we will inform the user that they have selected files in excess of the limit and let them know what their options are.

Which issue(s) this PR closes:

Closes #7714 Configuration for Availability of "Download All"

Special notes for your reviewer:

Suggestions on how to test this:
The Access Dataset Button will display in various ways depending on the size of the files relative to the ZipDownloadLimit.
Assuming the user may download files the Access Dataset button will be available, but when the user clicks into it instead of a download link they may see a note saying the files are too large to be downloaded. Also if there are tabular files present and the original format is too large, while the archival version is smaller than the limit they will see one download link and one note saying the format is too large (or vice versa).

In order to exercise the various scenarios it will be useful to put various values in:
curl -X PUT -d 5000000 http://localhost:8080/api/admin/settings/:ZipDownloadLimit
to test

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
https://docs.google.com/presentation/d/1aSTQKu5t2fEM9oGUuvKo0yyj-wbjKfYuYqrsmdhe4RQ/edit#slide=id.g7673497ae2_0_0

Is there a release notes update needed for this change?:

Additional documentation:

scolapasta

Nice work. A lot of suggested small tweaks, and some changes due to calls being made from rendered attributes.

scolapasta · 2021-04-15T16:37:42Z

src/main/java/edu/harvard/iq/dataverse/DatasetPage.java

-
+
+    public Long getSizeOfDatasetNumeric() {
+        //get min of orig/archival


let's have this call the methods in this class that do the specific thing (a little cleaner / easier to read)

scolapasta · 2021-04-15T16:38:01Z

src/main/java/edu/harvard/iq/dataverse/DatasetPage.java

+    }
+
+    public Long getSizeOfDatasetOrigNumeric() {
+        boolean original = true;


simplify to one line

scolapasta · 2021-04-15T16:38:10Z

src/main/java/edu/harvard/iq/dataverse/DatasetPage.java

+
+    public Long getSizeOfDatasetArchivalNumeric() {
+        boolean original = false;
+        return DatasetUtil.getDownloadSizeNumeric(workingVersion, original);


simplify to one line

scolapasta · 2021-04-15T16:38:43Z

src/main/java/edu/harvard/iq/dataverse/DatasetPage.java

+    }
+
+    public String getSizeOfSelectedMaxAsString(){
+        return FileSizeChecker.bytesToHumanReadable(Math.max(getSizeOfSelectedOrigNumeric(), getSizeOfDatasetArchivalNumeric()) );


Shouldn't the 2nd call here be for selected?

yes. good catch.

scolapasta · 2021-04-15T16:38:50Z

src/main/java/edu/harvard/iq/dataverse/DatasetPage.java

+    }
+
+    public Long getSizeOfSelectedOrigNumeric(){
+        boolean original = true;


simplify to one line

scolapasta · 2021-04-15T16:56:51Z

src/main/java/edu/harvard/iq/dataverse/dataset/DatasetUtil.java

+        return FileSizeChecker.bytesToHumanReadable(getDownloadSizeNumeric(dsv, original));
+    }
+
+    public static Long getDownloadSizeNumeric(DatasetVersion dsv, boolean original) {


we can just have this method call the next, passing in the file metadatas (deduplication of logic)

scolapasta · 2021-04-15T16:59:50Z

src/main/webapp/dataset.xhtml

                                                        <!-- DOWNLOAD -->
-                                                        <ui:fragment rendered="#{DatasetPage.canDownloadFiles()}">
+                                                        <ui:fragment rendered="#{DatasetPage.canDownloadFiles()  and !(DatasetPage.sizeOfDatasetNumeric > settingsWrapper.zipDownloadLimit) }">


renders get called multiple times, so we are doing the work here multiple times (please confirm this is true, by adding a temporary debug method in the method this calls); so we should store the values from the first time it's called (or even add the setting into init) to avoid that.

scolapasta · 2021-04-15T18:45:18Z

src/main/webapp/dataset.xhtml

-                                                            <li jsf:rendered="#{DatasetPage.versionHasTabular and DatasetPage.sizeOfDatasetOrig != '0 B' }">
-                                                                <p:commandLink update="@form" actionListener="#{DatasetPage.validateAllFilesForDownloadOriginal()}" styleClass="btn-download">
+                                                            <li jsf:rendered="#{DatasetPage.versionHasTabular and DatasetPage.sizeOfDatasetOrig != '0 B'  and !(DatasetPage.sizeOfDatasetOrigNumeric > settingsWrapper.zipDownloadLimit) }">
+                                                                <p:commandLink update="@form" actionListener="#{DatasetPage.validateAllFilesForDownloadOriginal()}" 


it probably make sense, but can you explain the reason behind this change (and the similar one below it?)

We don't want to render the download all here if the size exceeds the zip limit. Instead we show a message pointing the user to use the files table to select files.

Right, I guess I'm trying to understand why there's a rendered on the li but also the validate method called on the link.

The validate method on the link does a bunch of things like check to see if you have permission to download the files and compare the size of files you are asking for to the zip limit (which is more for the case where you select from the files table, but I put it in one method to prevent a lot of duplicate code from the prior version.) Then depending on whether you have selected files for which you don't have permission or too large of a download shows different popups - which is why your suggestion that we put the logic for displaying popups in the page may be hard to accomplish. i'll see what I can do there.

right, but then what is the rendering checking? I must be missing something, if it helps explain it, we could do a quick call later.

Re: the popups, we should be trying to minimize the popups as much as possible; i.e. single message popups that have just a close. I would suggest trying to see if you can use the validate message to set what the message is in the popup (making sure the page updates the popup) and then the code to show the popup happens on the page. Let me know if that makes sense.

The rendering here is checking for whether there are tabular files because it shows multiple links if there are (original or archival downloads). Maybe there's a different approach to take here, but I didn't really change it much except to add the various size tests(and I did set booleans in the init so that the renders won't have to do the same calculation multiple times)
Re: popups I thought you were objecting to the method actually opening them. I can work with the idea of having a single popup where the message changes - we already do that because we list the files that you don't have permission to download.
And Tania asked for the "just close" popup for when the size is too big - it also includes a link to the Download API in the guides.

scolapasta · 2021-04-15T18:45:55Z

src/main/java/edu/harvard/iq/dataverse/DatasetPage.java

@@ -3025,6 +3010,11 @@ public boolean isSelectAllFiles() {
    public void setSelectAllFiles(boolean selectAllFiles) {
        this.selectAllFiles = selectAllFiles;
    }
+
+    public void handleFileSelectClick(){


is this used anywhere? It's new, but I don't see any other reference.

Removed. I think I put it in in anticipation of giving immediate feedback when the files selected were too large for zip.

src/main/java/edu/harvard/iq/dataverse/DatasetPage.java

scolapasta

Some additional review.

scolapasta · 2021-04-23T20:18:34Z

src/main/java/edu/harvard/iq/dataverse/DatasetPage.java

+    }
+
+    public String getSizeOfDataset() {
+        boolean original = false;


not enough reason for the extra boolean here

scolapasta · 2021-04-23T20:28:47Z

src/main/java/edu/harvard/iq/dataverse/DatasetPage.java

-    
-    
+
+
    public void openDownloadPopupForMultipleFileDownload() {
        if (this.selectedFiles.isEmpty()) {
            //RequestContext requestContext = RequestContext.getCurrentInstance();
            PrimeFaces.current().executeScript("PF('selectFilesForDownload').show()");


should we consider removing this one while we're here as well? (or is that out of scope)

Yeah, it's only used for the download mixed popup (some restricted; some not) so we know there are valid files selected so we can just put the openguestbook popup in the onComplete.

scolapasta · 2021-04-23T20:30:06Z

src/main/java/edu/harvard/iq/dataverse/DatasetPage.java


        // There's a chance that this is not really a batch download - i.e.,
        // there may only be one file on the downloadable list. But the fileDownloadService
        // method below will check for that, and will redirect to the single download, if
        // that's the case. -- L.A.
+
        this.guestbookResponse.setDownloadtype("Download");
        //RequestContext requestContext = RequestContext.getCurrentInstance();
        PrimeFaces.current().executeScript("PF('downloadPopup').show();handleResizeDialog('downloadPopup');");


another one here.

I removed the whole method - see above.

scolapasta · 2021-04-23T20:33:34Z

src/main/webapp/dataset.xhtml

                                                            <li class="dropdown-header">#{bundle['dataset.accessBtn.header.download']} <span class="glyphicon glyphicon-download-alt"/></li>
                                                            <!-- NORMAL DOWNLOAD BUTTON (NO TABULAR FILES) -->
-                                                            <li jsf:rendered="#{!DatasetPage.versionHasTabular}">
-                                                                <p:commandLink update="@form" actionListener="#{DatasetPage.validateAllFilesForDownloadArchival()}" styleClass="btn-download">
+                                                            <li jsf:rendered="#{!DatasetPage.versionHasTabular and !(DatasetPage.tooLargeToDownload)}">                 


isn't the 2nd part of the rendered always true? (since it's in the ui fragment)

The difference here is that we render different command links depending on whether there are tabular files in the dataset - line 153 for no tabular files and 164 below for has tabular files.

I get that, but that explains the tabular check on the renders of the li tags. But I'm talking about the ui:fragment surrounding these li tags. Let me know if it's easier to discuss on a call, viewing the code.

Ok I get what you're saying now. I can remove it.

scolapasta · 2021-04-23T20:33:58Z

src/main/webapp/dataset.xhtml

                                                            <!-- DOWNLOAD ORIGINAL BUTTON (TABULAR FILES PRESENT) -->
-                                                            <li jsf:rendered="#{DatasetPage.versionHasTabular and DatasetPage.sizeOfDatasetOrig != '0 B' }">
-                                                                <p:commandLink update="@form" actionListener="#{DatasetPage.validateAllFilesForDownloadOriginal()}" styleClass="btn-download">
+                                                            <li jsf:rendered="#{DatasetPage.versionHasTabular and DatasetPage.sizeOfDatasetOrig != '0 B'  and !(DatasetPage.tooLargeToDownloadOriginal) }">


same. May make sense to review all the renders in these sections.

scolapasta

@sekmiller Thanks for making those latest changes - I think this hierarchical format is so miuch easier to understand (so maintainable).

I'll approve, but can you add to the QA notes to make sure to test all these different scenarios of original vs archival, etc? Thanks.

sekmiller and others added 21 commits March 29, 2021 16:17

Merge branch 'develop' into 7714-configure-download-all-ds

a0437b3

#7714 get size of files for download

b2e3ea8

Merge branch 'develop' into 7714-configure-download-all-ds

d5a52ff

Merge branch 'develop' into 7714-configure-download-all-ds

2ba7831

Merge branch 'develop' into 7714-configure-download-all-ds

f508a06

Merge branch 'develop' into 7714-configure-download-all-ds

33f43f1

#7714 refactor download all validation

fe9d014

Merge branch 'develop' into 7714-configure-download-all-ds

681b616

#7714 add messages for selected files too large

d889e72

Merge branch 'develop' into 7714-configure-download-all-ds

486ac79

#7714 fix too large popup logic

3407409

Merge branch 'develop' into 7714-configure-download-all-ds

668eb62

#7714 add granular download messages

04cc3e1

#7714 fix selected size issue

e79b640

#7714 fix margins on "too big" alerts

0a00ddf

Merge branch 'develop' into 7714-configure-download-all-ds

dbbe203

text updates

cc80f49

Merge branch 'develop' into 7714-configure-download-all-ds

b6d4b01

#7714 formatting and additional tests

4aa845b

Merge branch 'develop' into 7714-configure-download-all-ds

678b986

#7714 revise popup messaging

aee2d88

sekmiller added this to Review 🦁 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) via automation Apr 15, 2021

Merge branch 'develop' into 7714-configure-download-all-ds

b013581

scolapasta self-assigned this Apr 15, 2021

scolapasta requested changes Apr 15, 2021

View reviewed changes

scolapasta assigned sekmiller and unassigned scolapasta Apr 15, 2021

#7714 CR feedback changes

8225e18

djbrooke moved this from Review 🦁 to IQSS Team - In Progress 💻 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Apr 16, 2021

#7714 refactor download buttons/process

fc0e86b

Merge branch 'develop' into 7714-configure-download-all-ds

e029aac

sekmiller moved this from IQSS Team - In Progress 💻 to Review 🦁 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Apr 23, 2021

sekmiller removed their assignment Apr 23, 2021

Merge branch 'develop' into 7714-configure-download-all-ds

d512aaa

scolapasta reviewed Apr 23, 2021

View reviewed changes

sekmiller added 4 commits April 23, 2021 17:25

#7714 more PR feedback code cleanup

e7a2ad9

Merge branch 'develop' into 7714-configure-download-all-ds

a9d8924

#7714 remove redundant render logic

7088344

Merge branch 'develop' into 7714-configure-download-all-ds

fb7657a

djbrooke assigned scolapasta Apr 26, 2021

Merge branch 'develop' into 7714-configure-download-all-ds

80b5559

scolapasta assigned sekmiller Apr 30, 2021

sekmiller added 2 commits May 3, 2021 10:17

#7714 reorg/simplify download button/message renders

5a1160d

#7714 update comments

56a7b63

scolapasta approved these changes May 3, 2021

View reviewed changes

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Review 🦁 to QA 🔎✅ May 3, 2021

scolapasta removed their assignment May 3, 2021

sekmiller removed their assignment May 3, 2021

Merge branch 'develop' into 7714-configure-download-all-ds

2532dac

kcondon self-assigned this May 3, 2021

kcondon merged commit c56f4a8 into develop May 3, 2021

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA 🔎✅ to Done 🚀 May 3, 2021

kcondon deleted the 7714-configure-download-all-ds branch May 3, 2021 20:47

djbrooke added this to the 5.5 milestone May 3, 2021

djbrooke mentioned this pull request May 18, 2021

Download All Error Reporting - Hidden in Manifest if not all files are downloaded (4.9.4) #5588

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7714 configure download all ds #7806

7714 configure download all ds #7806

sekmiller commented Apr 15, 2021 •

edited

scolapasta left a comment

scolapasta Apr 15, 2021

scolapasta Apr 15, 2021

scolapasta Apr 15, 2021

scolapasta Apr 15, 2021

sekmiller Apr 15, 2021

scolapasta Apr 15, 2021

scolapasta Apr 15, 2021

scolapasta Apr 15, 2021

scolapasta Apr 15, 2021

sekmiller Apr 15, 2021

scolapasta Apr 15, 2021

sekmiller Apr 16, 2021

scolapasta Apr 16, 2021

sekmiller Apr 16, 2021

scolapasta Apr 15, 2021

sekmiller Apr 16, 2021

scolapasta left a comment

scolapasta Apr 23, 2021

scolapasta Apr 23, 2021

sekmiller Apr 23, 2021

scolapasta Apr 23, 2021

sekmiller Apr 23, 2021

scolapasta Apr 23, 2021

sekmiller Apr 23, 2021

scolapasta Apr 23, 2021

sekmiller Apr 26, 2021

scolapasta Apr 23, 2021

scolapasta left a comment



		public Long getSizeOfDatasetNumeric() {
		//get min of orig/archival

7714 configure download all ds #7806

7714 configure download all ds #7806

Conversation

sekmiller commented Apr 15, 2021 • edited

scolapasta 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

scolapasta 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

scolapasta left a comment

Choose a reason for hiding this comment

sekmiller commented Apr 15, 2021 •

edited