Skip to content

Conversation

@JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Jun 21, 2022

Description

This feature allows sharing resources with multiple users at once via the following format: user1, user2, user3

If you have the guest app installed and active, owncloud/guests#506 is required to properly use this feature. Unfortunately, there is no way to overcome this as the guest app extends the sharing functionality in a very bad way by re-implementing it.

Related Issue

Screenshot

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@JammingBen JammingBen self-assigned this Jun 21, 2022
@ownclouders
Copy link
Contributor

ownclouders commented Jun 21, 2022

💥 Acceptance tests pipeline webUISharingIntGroup2-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/36110/150

@JammingBen JammingBen force-pushed the batch-share-action branch from 0a493cc to bfff40c Compare June 21, 2022 11:41
@mmattel
Copy link
Contributor

mmattel commented Jun 24, 2022

Docs relevant if merged. Pls file a docs-server issue when ready.

@JammingBen JammingBen marked this pull request as ready for review June 28, 2022 06:54
@phil-davis
Copy link
Contributor

phil-davis commented Jun 28, 2022

Users aaaa, bbbb and cccc exist

  1. login as aaaa
  2. create "afolder"
  3. start sharing
  4. type "bbbb, "
  5. the popup appears saying "No users or groups found for bbbb, "
  6. keep typing until you have typed "bbbb, cccc"
  7. Behind the "No user of groups found..." popup, there is a suggestion to "Add multiple users" but I can't see the details of the users that it has matched and it proposes to add.

Screenshot 2022-06-28 at 17-35-39 Files - ownCloud

a) the "No users or groups found for" popup should go away when I start typing more stuff (when I start typing "cccc"
b) actually, if I have typed "bbbb," and thus it looks like I am about to go on, the logic could still parse this comma-separated string and just check for "bbbb" (and understand that an empty entry at the end is OK (trailing comma and white-space could be ignored to make life easier for the user)
c) why do we insist on comma followed by space as the separator? The UI could allow any combination of spaces and commas, and that would make it easier for users to enter "whatever they like within reason".

e.g. bbbb ,cccc or bbbb,cccc or bbbb,,,, cccc - any of that sort of input can be parsed pretty easily.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I paste in a string like "qwerty, bbbb, ccc" and only user "bbbb" exists, it still suggests to add all 3 "users". Then I get a message about not being able to add "qwerty" and "ccc".

Could it validate the list up-front? And let the user know that some of the users are not found?

I tried some combinations using the user's display name in the list. That works. You just have to be careful to provide the exact display name.

@phil-davis
Copy link
Contributor

Note: this feature (currently) never shares with groups. I tried with users "bbbb" and "cccc", and groups "abc" and "cccc".

When I try to share with "abc, cccc" it shares with user "cccc" and tells me "Could not be shared with the following users: abc"

I guess that the feature is only for sharing with multiple users. It will be trickier to include groups that match, because there can be a user and group with the same name. So some system would have to be designed to help the user when such and ambiguity happened.

@JammingBen
Copy link
Contributor Author

the "No users or groups found for" popup should go away when I start typing more stuff (when I start typing "cccc"

Yes, that's a general problem of the input field. But I might as well tackle it with this PR 👍

why do we insist on comma followed by space as the separator? The UI could allow any combination of spaces and commas, and that would make it easier for users to enter "whatever they like within reason".

Hmm I need to think about that one. Let's say you have a group called aaa, bbb. You couldn't share with this group if we remove the white spaces internally, so we'd need to search for both cases... Let me check again how it behaves, but if it makes things more complicated, I would leave it as it is.

Note: this feature (currently) never shares with groups. I tried with users "bbbb" and "cccc", and groups "abc" and "cccc".

Correct, the batch action only invites users. However, when having a group called aaa, bbb and a user aaa (and/or bbb), it should display 2 options for the user: share with user(s) or with the group.

If I paste in a string like "qwerty, bbbb, ccc" and only user "bbbb" exists, it still suggests to add all 3 "users". Then I get a message about not being able to add "qwerty" and "ccc".

Could it validate the list up-front? And let the user know that some of the users are not found?

Good idea, I think that's relatively easy to implement as we already have the failed users.

@phil-davis
Copy link
Contributor

Correct, the batch action only invites users. However, when having a group called aaa, bbb and a user aaa (and/or bbb), it should display 2 options for the user: share with user(s) or with the group.

That works:
Screenshot from 2022-06-28 18-56-55

I hadn't thought about that. So, the code should always send off to see if there is a match for the full literal string entered - it might be a group. And if there are comma(s) in the string (with or without white-space around the comma(s)) then ignore the white-space and suggest adding multiple users.

@phil-davis
Copy link
Contributor

The user and group suggestions are both blue. Single user suggestions are green. Is that the way the colors are intended to be?

I guess multiple users is a "group of users", so that's why it is blue?

@JammingBen
Copy link
Contributor Author

I believe these colors are random, no? Here I have two single users with different colors:

image

@sonarqubecloud
Copy link

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 18 Code Smells

55.2% 55.2% Coverage
0.0% 0.0% Duplication

@JammingBen JammingBen requested a review from phil-davis June 29, 2022 09:13
@phil-davis
Copy link
Contributor

Looking good - I have tried various combinations of user and group names and having comma-separator with and without a space after the comma. Works.

This situation is a bit odd:

  1. users aaaa bbbb cccc exist
  2. login as aaaa and create a folder
  3. try to share the folder with bbbb,unknown,cccc
  4. the suggestion comes up to share with multiple users bbbb,cccc - good "unknown" does not exist, so it is not in the suggested list of matches.
  5. click to share with those suggested users
  6. shares appear with bbbb and cccc - good
  7. A notification appears with "Could not be shared with the following users: unknown" - not so good.

A request to share with "unknown" should not be sent. The JS already knows that "unknown" has no match.

@phil-davis
Copy link
Contributor

There is an "i" information popup to the right of the share-with text box that has stuff about "Share with people on other ownClouds using the syntax..."

Maybe some instructions could be added there about the syntax to use to share with multiple users? It would be good to have some clue given in the UI that this feature exists.

@phil-davis
Copy link
Contributor

Note: I added issue #40178 to add some webUI automated test coverage. There is no big rush for that, if it is ready before this PR is merged then we can add a commit here. If this PR is already merged, then we add the test coverage in a separate PR to master.

@mmattel
Copy link
Contributor

mmattel commented Jul 5, 2022

Ping, CI is green, approvable? mergable?

@phil-davis
Copy link
Contributor

@JammingBen I made comment #40155 (comment)

Are you progressing this? Maybe just sort out that little thing, and we can merge.

@JammingBen
Copy link
Contributor Author

@JammingBen I made comment #40155 (comment)

Are you progressing this? Maybe just sort out that little thing, and we can merge.

I'm still on vacation until next Monday, I can work that stuff out then. Although I'm not sure about the expected behavior, IMO it's fine to display that the resource could not be shared with unknown. Why shouldn't it be displayed?

@phil-davis
Copy link
Contributor

I added @jvillafanez and @janackermann as reviewers.
My comments are only minor, and we need to move forward. Please review, and this can be merged.

@jvillafanez
Copy link
Member

While unlikely, it might be possible that a group contain the separator, for example a group music, pictures and arts. Such group should appear somehow.
Maybe it's easy to include an additional search without considering the separator in order to find that group.

@phil-davis
Copy link
Contributor

While unlikely, it might be possible that a group contain the separator, for example a group music, pictures and arts. Such group should appear somehow. Maybe it's easy to include an additional search without considering the separator in order to find that group.

See #40155 (comment)

If a group matches the comma-separated string then it is shown in the drop-down list as a possible thing to share with.

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