Respect archiver limits on BatchAction download #9055

pascalwengerter · 2023-05-16T17:04:58Z

Related Issue

Fixes Limit batch download to a certain selection size #8456

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

Code changes
Needs clarification on fileSize calculation base (see comment)
Needs fix for action label reactivity
Unit tests added

Open tasks:

...

pascalwengerter · 2023-05-16T19:51:48Z

packages/web-app-files/src/composables/actions/files/useFileActionsDownloadArchive.ts

+
+    const selectedFilesAmount = resources.length
+
+    // TODO: selectedFilesSize doesn't traverse into folders, so currently limited to directly selected resources


@kulmann I don't guess there's an easy way to do this?

I don't get your TODO comment. 🙈

We can check the accumulated size of the selected files and folders against the archiver capability. A folder size always reflects the size of all the children. Tree size aggregation is properly happening backend side. That's what's happening in the reduce statement below and seems to be correct.

We can't check the accumulated number of files in the selection (because you would need a propfind with depth infinity for that). That's what you did in the line above and is incomplete with just checking the number of selected resources. You only count the number of selected resources while you would need to count the number of all children in the full tree of the selection.

To be honest I would just not look at the number of files. Doing the file size check already helps and is by far the more important check I think.

packages/web-app-files/src/composables/actions/files/useFileActionsDownloadArchive.ts

packages/web-pkg/src/components/ContextActions/ActionMenuItem.vue

ownclouders · 2023-05-16T21:09:39Z

Results for acceptance oCIS https://drone.owncloud.com/owncloud/web/35735/66/1
💥 The oCISSharingPublic2 tests pipeline failed. The build has been cancelled.

ownclouders · 2023-05-16T21:09:39Z

Results for acceptance oCIS https://drone.owncloud.com/owncloud/web/35735/56/1
💥 The oCISFiles3 tests pipeline failed. The build has been cancelled.

ownclouders · 2023-05-16T21:09:39Z

Results for acceptance oCIS https://drone.owncloud.com/owncloud/web/35735/58/1
💥 The oCISFiles5 tests pipeline failed. The build has been cancelled.

ownclouders · 2023-05-16T21:09:39Z

Results for acceptance oCIS https://drone.owncloud.com/owncloud/web/35735/53/1
💥 The oCISFavorites tests pipeline failed. The build has been cancelled.

ownclouders · 2023-05-16T21:09:39Z

Results for acceptance oCIS https://drone.owncloud.com/owncloud/web/35735/52/1
💥 The oCISSharingBasic tests pipeline failed. The build has been cancelled.

ownclouders · 2023-05-16T21:09:39Z

Results for acceptance oCIS https://drone.owncloud.com/owncloud/web/35735/55/1
💥 The oCISFiles2 tests pipeline failed. The build has been cancelled.

ownclouders · 2023-05-16T21:09:40Z

Results for acceptance oCIS https://drone.owncloud.com/owncloud/web/35735/54/1
💥 The oCISFiles1 tests pipeline failed. The build has been cancelled.

AlexAndBear · 2023-05-17T05:07:05Z

packages/web-app-files/src/composables/actions/files/useFileActionsDownloadArchive.ts

+    }
+
+    const selectedFilesAmount = resources.length
+


Couldn't we early return here, so we don't need to fire the accumulator if the limit is already reached ?

Surely, good idea :)

AlexAndBear · 2023-05-17T05:11:23Z

packages/web-app-files/src/services/archiver.ts

@@ -17,6 +17,8 @@ export interface ArchiverCapability {
  formats: string[]
  // eslint-disable-next-line camelcase
  archiver_url: string
+  max_num_files: string


Why are those strings ?

I'm just using what oCIS/reva provide on my recently pulled docker image ;) see https://github.com/cs3org/reva/blob/2be8a61879b22823c852757497c81f2d544f6f9d/internal/http/services/owncloud/ocs/data/capabilities.go#L114

Couldn't we parse them beforehand?

No, this only reflects the raw capabilities coming from the backend. Parsing makes sense in the capability composables though.

pascalwengerter · 2023-05-17T06:12:53Z

packages/web-app-files/src/composables/actions/files/useFileActionsDownloadArchive.ts

@@ -68,6 +90,12 @@ export const useFileActionsDownloadArchive = ({ store }: { store?: Store<any> }
        label: () => {
          return $gettext('Download')
        },
+        disabledTooltip: ({ resources }) => {
+          return archiverLimitsExceeded(resources)
+            ? $gettext('Unable to download that many files at once')


Open for suggestions on the final text here btw 😄

What about something like The selection exceeds the allowed archive size (max. xyz MB)? We should let the user know what limits they are working against. Also wouldn't mention the number of files here since we can't check that.

By the way, I believe that the backend behaviour is faulty and should not offer an archive file for download at all if it's incomplete. What's your take on that? If same we should open an issue in ocis / reva.

Not sure if the average non-tech user knows how to parse allowed archive size, but let's roll with it since feedback can easily be implemented at a later point

kulmann · 2023-05-17T12:10:22Z

changelog/unreleased/enhancement-respect-archiver-limits

@@ -0,0 +1,8 @@
+Enhancement: Respect archiver limits
+
+The archiver service can now announce a limit for both the amount of resources and 


Suggested change

The archiver service can now announce a limit for both the amount of resources and

The archiver service can now announce a limit for both the number and

kulmann · 2023-05-17T12:17:23Z

packages/web-app-files/src/composables/actions/files/useFileActionsDownloadArchive.ts

+
+    const selectedFilesAmount = resources.length
+
+    // TODO: selectedFilesSize doesn't traverse into folders, so currently limited to directly selected resources


I don't get your TODO comment. 🙈

We can check the accumulated size of the selected files and folders against the archiver capability. A folder size always reflects the size of all the children. Tree size aggregation is properly happening backend side. That's what's happening in the reduce statement below and seems to be correct.

We can't check the accumulated number of files in the selection (because you would need a propfind with depth infinity for that). That's what you did in the line above and is incomplete with just checking the number of selected resources. You only count the number of selected resources while you would need to count the number of all children in the full tree of the selection.

To be honest I would just not look at the number of files. Doing the file size check already helps and is by far the more important check I think.

kulmann · 2023-05-17T12:17:56Z

packages/web-app-files/src/composables/actions/files/useFileActionsDownloadArchive.ts

+    const selectedFilesSize = resources.reduce(
+      (accumulator, currentValue) =>
+        accumulator +
+        (typeof currentValue.size === 'string' ? parseInt(currentValue.size) : currentValue.size),


Suggested change

(typeof currentValue.size === 'string' ? parseInt(currentValue.size) : currentValue.size),

parseInt(`${currentValue.size}`),

kulmann · 2023-05-17T12:18:26Z

packages/web-app-files/src/composables/actions/files/useFileActionsDownloadArchive.ts

@@ -59,6 +59,28 @@ export const useFileActionsDownloadArchive = ({ store }: { store?: Store<any> }
      })
  }

+  const archiverLimitsExceeded = (resources: Resource[]) => {


Suggested change

const archiverLimitsExceeded = (resources: Resource[]) => {

const areArchiverLimitsExceeded = (resources: Resource[]) => {

kulmann · 2023-05-17T12:24:00Z

packages/web-app-files/src/composables/actions/files/useFileActionsDownloadArchive.ts

@@ -68,6 +90,12 @@ export const useFileActionsDownloadArchive = ({ store }: { store?: Store<any> }
        label: () => {
          return $gettext('Download')
        },
+        disabledTooltip: ({ resources }) => {
+          return archiverLimitsExceeded(resources)
+            ? $gettext('Unable to download that many files at once')


What about something like The selection exceeds the allowed archive size (max. xyz MB)? We should let the user know what limits they are working against. Also wouldn't mention the number of files here since we can't check that.

By the way, I believe that the backend behaviour is faulty and should not offer an archive file for download at all if it's incomplete. What's your take on that? If same we should open an issue in ocis / reva.

packages/web-pkg/src/components/ContextActions/ActionMenuItem.vue

sonarcloud · 2023-05-17T13:13:16Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

20.0% Coverage
0.0% Duplication

kulmann

Love it 🚀

kulmann · 2023-05-19T12:13:06Z

for the record, easier to test if one decreases the max file size allowed by the archiver. E.g. as follows:

FRONTEND_ARCHIVER_MAX_SIZE: "4194304"

pascalwengerter force-pushed the feature/8456 branch from e00649a to b562d5b Compare May 16, 2023 19:48

pascalwengerter commented May 16, 2023

View reviewed changes

packages/web-app-files/src/composables/actions/files/useFileActionsDownloadArchive.ts Show resolved Hide resolved

pascalwengerter commented May 16, 2023

View reviewed changes

packages/web-pkg/src/components/ContextActions/ActionMenuItem.vue Show resolved Hide resolved

pascalwengerter force-pushed the feature/8456 branch from b562d5b to a100463 Compare May 16, 2023 20:58

pascalwengerter marked this pull request as ready for review May 16, 2023 21:06

AlexAndBear requested changes May 17, 2023

View reviewed changes

pascalwengerter commented May 17, 2023

View reviewed changes

kulmann requested changes May 17, 2023

View reviewed changes

Respect archiver limits for batch download action (owncloud#8456)

8cfa4ff

pascalwengerter force-pushed the feature/8456 branch from a100463 to 950f2a8 Compare May 17, 2023 13:01

Apply suggestions from code review

950f2a8

pascalwengerter requested review from kulmann and AlexAndBear May 17, 2023 13:36

kulmann approved these changes May 19, 2023

View reviewed changes

kulmann mentioned this pull request May 19, 2023

Bug: Web Interface Downloading Fails for Multiple Files Totaling Over 1GB owncloud/ocis#6301

Closed

AlexAndBear approved these changes May 19, 2023

View reviewed changes

kulmann merged commit 4141738 into owncloud:master May 19, 2023

pascalwengerter added a commit to pascalwengerter/web that referenced this pull request May 22, 2023

Respect archiver limits on BatchAction download (owncloud#9055)

c43f7a9

pascalwengerter deleted the feature/8456 branch July 13, 2023 16:15

micbar mentioned this pull request Jul 24, 2023

Release 4.0.0 owncloud/ocis#6838

Closed

68 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect archiver limits on BatchAction download #9055

Respect archiver limits on BatchAction download #9055

pascalwengerter commented May 16, 2023

pascalwengerter May 16, 2023

kulmann May 17, 2023

ownclouders commented May 16, 2023 •

edited

Loading

ownclouders commented May 16, 2023

ownclouders commented May 16, 2023

ownclouders commented May 16, 2023

ownclouders commented May 16, 2023

ownclouders commented May 16, 2023

ownclouders commented May 16, 2023

AlexAndBear May 17, 2023

pascalwengerter May 17, 2023

AlexAndBear May 17, 2023

pascalwengerter May 17, 2023

AlexAndBear May 17, 2023

kulmann May 19, 2023

pascalwengerter May 17, 2023

kulmann May 17, 2023

pascalwengerter May 17, 2023

kulmann May 17, 2023

kulmann May 17, 2023

kulmann May 17, 2023

kulmann May 17, 2023

kulmann May 17, 2023

sonarcloud bot commented May 17, 2023

kulmann left a comment

kulmann commented May 19, 2023


		const selectedFilesAmount = resources.length

		// TODO: selectedFilesSize doesn't traverse into folders, so currently limited to directly selected resources

		@@ -0,0 +1,8 @@
		Enhancement: Respect archiver limits

		The archiver service can now announce a limit for both the amount of resources and

	(typeof currentValue.size === 'string' ? parseInt(currentValue.size) : currentValue.size),
	parseInt(`${currentValue.size}`),

	const archiverLimitsExceeded = (resources: Resource[]) => {
	const areArchiverLimitsExceeded = (resources: Resource[]) => {

Respect archiver limits on BatchAction download #9055

Respect archiver limits on BatchAction download #9055

Conversation

pascalwengerter commented May 16, 2023

Related Issue

Types of changes

Checklist:

Open tasks:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ownclouders commented May 16, 2023 • edited Loading

ownclouders commented May 16, 2023

ownclouders commented May 16, 2023

ownclouders commented May 16, 2023

ownclouders commented May 16, 2023

ownclouders commented May 16, 2023

ownclouders commented May 16, 2023

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

sonarcloud bot commented May 17, 2023

kulmann left a comment

Choose a reason for hiding this comment

kulmann commented May 19, 2023

ownclouders commented May 16, 2023 •

edited

Loading