Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add batch action to allow sharing with multiple guests at once #506

Merged
merged 6 commits into from
Jul 18, 2022

Conversation

JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Jun 21, 2022

Description

This feature allows sharing resources with multiple guests at once via the following format: user1@mail.com,user2@mail.com,user3@mail.com

Requires owncloud/core#40155 to work. Merging this PR still keeps compatibility with older oC versions though.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@JammingBen JammingBen marked this pull request as ready for review June 28, 2022 06:53
@mmattel mmattel mentioned this pull request Jul 12, 2022
12 tasks
@jnweiger jnweiger mentioned this pull request Jul 12, 2022
42 tasks
@mmattel
Copy link
Contributor

mmattel commented Jul 15, 2022

Note that the requirement owncloud/core#40155 has been merged

Any progress?

@mmattel mmattel requested a review from jvillafanez July 15, 2022 10:42
@CLAassistant
Copy link

CLAassistant commented Jul 15, 2022

CLA assistant check
All committers have signed the CLA.

@jvillafanez
Copy link
Member

Should we use sets instead of arrays to hold the users? It should be more efficient than looping through an array multiple times, and the code might also be smaller and easier.

@mrow4a
Copy link
Contributor

mrow4a commented Jul 16, 2022

I wonder, how error handling works here? If 1 user in a batch fails for whatever reason, what happens with a batch? how is this presented to the user?

@JammingBen
Copy link
Contributor Author

Should we use sets instead of arrays to hold the users? It should be more efficient than looping through an array multiple times, and the code might also be smaller and easier.

@jvillafanez Do you mean here in L92: https://github.com/owncloud/guests/pull/506/files#diff-1e9748771effcb66e011a800b732cd0d897ea35001b2b40e9e9f7c970673ec7cR92?

I wonder, how error handling works here? If 1 user in a batch fails for whatever reason, what happens with a batch? how is this presented to the user?

In an error case, all users who succeed will still be added. Failed users will be left out and prompted to the user:

image

@jvillafanez
Copy link
Member

Yes. I assume that both the result and the existing shares are arrays, and we can't change it, but we're creating an array for the users from the search. I'm not sure if we can skip the user loop somehow to take full advantage of the set operations. The goal would be to reduce the number of loops we need to do.

Anyway, the expected data set for the users, shares and result shouldn't be too big, so it could be ok.

@JammingBen
Copy link
Contributor Author

Okay got it, I already tried it in owncloud/customgroups#521 (which is a similar care), unfortunately Sets do not support uniqueness with objects. Hence we would still need to loop through those. Or use some helpers from lodash, that probably do the same under the hood.

@sonarcloud
Copy link

sonarcloud bot commented Jul 18, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@mmattel mmattel merged commit cc47240 into master Jul 18, 2022
@delete-merged-branch delete-merged-branch bot deleted the batch-share-action branch July 18, 2022 10:11
@jnweiger
Copy link
Contributor

This is included in https://github.com/owncloud/guests/releases/tag/v0.11.0-rc.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants