-
Notifications
You must be signed in to change notification settings - Fork 159
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
[full-ci] Show single (publicly) shared file in DetailsView #8446
Conversation
Results for e2e-tests oCIS https://drone.owncloud.com/owncloud/web/35562/12/1 💥 To see the trace, please open the link in the console ...
npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/35562/tracing/alice-can-share-this-weeks-meal-plan-with-all-parents-alice-2023-5-9-08-01-54.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/35562/tracing/alice-can-share-this-weeks-meal-plan-with-all-parents-brian-2023-5-9-08-02-26.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/35562/tracing/alice-can-share-this-weeks-meal-plan-with-all-parents-carol-2023-5-9-08-02-29.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/35562/tracing/quick-link-alice-2023-5-9-08-06-15.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/35562/tracing/quick-link-anonymous-2023-5-9-08-06-22.zip |
a165f15
to
1cbfbe1
Compare
Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/35562/13/1 |
1cbfbe1
to
aae96dc
Compare
If you keep our original logic, you don't need to worry whether it's a share where users can upload or not and becomes as simple as a single if condition. Basically, it's a single public share if 1) there's only 1 paginated resource and 2) there is no current folder. Since you create a "fake space" for the current folder, you can check whether it exists or not by checking that the fileId does not exist. So, this is enough: if (this.paginatedResources.length === 1 && !this.currentFolder.fileId) {
return true
}
return false (ps: for the CERN case we still need to check that |
56fabff
to
3c02f08
Compare
Hmmm we'd still need to check whether the receiver has upload rights for the current/shared folder, right? The way it's working right now is that
Or is that going against your initial feature request? |
@kulmann happy to receive a review here already, while figuring out the unit tests :) (acceptance tests are green) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused that you introduce so much options api vue code 😁 + see one comment. Works as expected and code looks good so far.
Do you want to add unit tests in a separate PR? I'd like to have them in this PR, but not sure how easy that's to resolve for you at the moment.
} | ||
if ( | ||
isPublicSpaceResource(this.space) && | ||
!this.currentFolder.canUpload({ user: this.user }) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the check on canUpload
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because (from my understanding) we'd show the table/tiles if the share/link receiver can upload - one of the usability complaints was that users don't understand that they can't upload when seeing a single shared file in the table view.
It's more of a PO decision, we could also use the ResourceDetails.vue
and enable the AppBar with create/upload buttons for single files and upload permissions, and then "switch over" to table/tiles once another resource is added.
Let me know, happy to finalize this either way :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, isn't the check of paginatedResources[0].isFolder
enough? Or do you have any case where you can upload and is not a folder? (that would be strange, no?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding was that we only ever show the single file view for files, never for folders. And for a file you will never be able to upload, like @diocas said. So isPublicSpaceResource(this.space) && !this.paginatedResources[0].isFolder
should be sufficient.
c978270
to
1e5eed6
Compare
1e5eed6
to
0a08c2c
Compare
0a08c2c
to
a5667fa
Compare
a5667fa
to
ac22873
Compare
ac22873
to
eb2a7c1
Compare
packages/web-app-files/src/components/SideBar/Shares/Links/CreateQuickLink.vue
Show resolved
Hide resolved
@@ -219,7 +219,7 @@ Feature: Share by public link with different roles | |||
And the user uploads folder "PARENT" using the webUI | |||
Then folder "PARENT" should be listed on the webUI | |||
And folder "CHILD" should be listed in the folder "PARENT" on the webUI | |||
And file "child.txt" should be listed in the folder "CHILD" on the webUI | |||
And file "child.txt" should be listed in the folder "CHILD" on the webUI as single share |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kulmann not sure if expected behavior product-wise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in "you're a user on a public link, create a folder, navigate into folder, create/upload a file and see this file as a single share (and not in a table view)"
@@ -231,7 +231,7 @@ Feature: Share by public link with different roles | |||
And the user uploads folder "PARENT" using the webUI | |||
Then folder "PARENT" should be listed on the webUI | |||
And folder "CHILD" should be listed in the folder "PARENT" on the webUI | |||
And file "child.txt" should be listed in the folder "CHILD" on the webUI | |||
And file "child.txt" should be listed in the folder "CHILD" on the webUI as single share |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kulmann not sure if expected behavior product-wise
c40770f
to
598844a
Compare
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Description
Needs
[ ] extracting components from thepackages/web-app-files/src/components/Sidebar/
directory (we decided that this is not necessary for now)2. IIRC there's also expected failures for this exact featurecouldn't find any)Related Issue