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

[full-ci] Resharing for ocis #7086

Merged
merged 1 commit into from
Jul 6, 2022
Merged

[full-ci] Resharing for ocis #7086

merged 1 commit into from
Jul 6, 2022

Conversation

fschade
Copy link
Collaborator

@fschade fschade commented Jun 2, 2022

enhance web to be able to re-share resources when using an ownCloud infinite scale backend. It now works for project and personal spaces as well as the sharing jail.

Besides that roles, space-ref and path send as separate values to the sharing api which simplifies the usage of it.

@update-docs
Copy link

update-docs bot commented Jun 2, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@fschade fschade changed the title [WIP] use ocis storageId whenever possible [WIP] Resharing Jun 2, 2022
@ownclouders
Copy link
Contributor

ownclouders commented Jun 2, 2022

Results for oCISSharingPublic2 https://drone.owncloud.com/owncloud/web/26681/69/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingPublicManagement-publicLinkIndicator_feature-L110.png

webUISharingPublicManagement-publicLinkIndicator_feature-L110.png

@fschade fschade changed the title [WIP] Resharing [full-ci][WIP] Resharing Jun 2, 2022
@fschade fschade force-pushed the resharing branch 5 times, most recently from 5075675 to 8c23df7 Compare June 9, 2022 13:45
@fschade fschade force-pushed the resharing branch 4 times, most recently from 034da3e to 728e296 Compare June 13, 2022 09:39
@ScharfViktor
Copy link
Contributor

just so we don't forget:
we cannot share space with "manager" role via web so we changed roles when we did re-sharing. will this be fixed in this PR?

@fschade fschade force-pushed the resharing branch 2 times, most recently from 1a9d7cf to 2fd0f04 Compare June 27, 2022 11:18
@fschade fschade changed the title [full-ci][WIP] Resharing [full-ci] Resharing Jun 27, 2022
@fschade fschade changed the title [full-ci] Resharing [full-ci] Resharing for ocis Jun 27, 2022
@fschade
Copy link
Collaborator Author

fschade commented Jun 27, 2022

@ScharfViktor not sure if i understand you correct, can you explain more detailed?

@fschade fschade marked this pull request as ready for review June 27, 2022 11:20
@ScharfViktor
Copy link
Contributor

@ScharfViktor not sure if i understand you correct, can you explain more detailed?

  • admin creates space
  • admin tries to add Maria as manager member of the space. Web sends request with the body: shareType=7&shareWith=richard&space_ref=spaceId&permissions=31

Actual: inpossible to add manager user: <ocs><meta><status>error</status><statuscode>400</statuscode><message>invalid role for space member</message></meta></ocs>

and case with changing role: #7163

@fschade
Copy link
Collaborator Author

fschade commented Jun 27, 2022

@ScharfViktor, thanks. Unfortunately this is not part of this pr :(

Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

Looking good code-wise, will test locally now

.drone.star Show resolved Hide resolved
@pascalwengerter
Copy link
Contributor

@fschade I run into SHARESTREE console errors when resharing from the shares jail (moss shared folder "foo" with einstein, einstein logs in and navigates into "foo" and shares subfolder "bar" with Richard). The shareIndicators also behave weirdly, resharing one resources with another user updates all resources with a "people share icon" but that doesn't persist page reloads 🤔

@ScharfViktor
Copy link
Contributor

ScharfViktor commented Jul 1, 2022

test falls all time in the same place only in oc10. I can reproduce it localy
Steps:

  • alice creates folder
  • alice click to the folder (go inside of the folder)
  • alice tries to go to startUrl:

Actual: index.php/apps/web/index.html#/files/spaces/personal/alice?items-per-page=100 doesn't load. Reload page or go to startUrl again or run test with SLOW_MO helps

Screenshot 2022-07-01 at 10 01 03

@JammingBen
Copy link
Collaborator

I removed outdated acceptance tests in 70f1317.

I also noticed the share indicators for received shares behave a little weird. Current behavior is that a received share always shows a share indicator. In oC10 that makes sense IMO, as received shares are being displayed in the personal files table. In oCIS however those are beding displayed in the share jail. That means, all resources in the share jail have the sharing indicator.

What's the preferred solution here? We could also differ between those two scenarios, although it would require some engineering.

@kulmann
Copy link
Member

kulmann commented Jul 1, 2022

@JammingBen I agree with your oc10 related argument, so I think for the share jail we only want to show share indicators for outgoing shares, i.e. for reshared resources. fine by me to use the spaces.share_jail capability for that differentiation. 🤔

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

See comments. One thing that irritates me a bit is that the usage of the storage id is not consistent throughout your touched files. There are a lot of cases where we still determine the currentStorageId in different way. Could you please doublecheck and strive for consistency here? To me it's not clear when to use the file id and when to use the storage id in a different way now.

changelog/unreleased/enhancement-resharing Outdated Show resolved Hide resolved
.drone.env Outdated Show resolved Hide resolved

return userShares?.length ? userShares[0] : undefined
// the root share has an empty key in the shares tree. That's the reason why we retrieve the share by an empty key here
return this.sharesTree['']?.find((s) => s.incoming)
Copy link
Member

Choose a reason for hiding this comment

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

But that's only true if we are in the share jail, right? For oc10 the old behaviour should be correct...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i just checked it again, it's the same behavior for classic too

Bildschirmfoto 2022-07-06 um 12 04 31

Copy link
Member

Choose a reason for hiding this comment

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

Still have questions:

  1. What if you have a group share and a user share (both incoming shares) for the same folder? Picking any incoming shares is not sufficient then, right?
  2. Still don't get why it would be sufficient in any case to check the incoming shares of the root node. What if the share is mounted in a deeply nested folder? E.g. you have Shares/the-world as default mount path for shares in oc10. Or you move a mounted share to another mount point.


// toDo: remove me
// @jannik: please have a look here what we can wait for to be sure that it's there
await new Promise((resolve) => setTimeout(resolve, 250))
Copy link
Member

Choose a reason for hiding this comment

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

Remove? ;-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fix ;)

@fschade
Copy link
Collaborator Author

fschade commented Jul 6, 2022

  • admin creates space

@ScharfViktor, the manager fix is now part of this pr, should be fixed 😽

@fschade
Copy link
Collaborator Author

fschade commented Jul 6, 2022

test falls all time in the same place only in oc10. I can reproduce it localy Steps:

  • alice creates folder
  • alice click to the folder (go inside of the folder)
  • alice tries to go to startUrl:

Actual: index.php/apps/web/index.html#/files/spaces/personal/alice?items-per-page=100 doesn't load. Reload page or go to startUrl again or run test with SLOW_MO helps

Screenshot 2022-07-01 at 10 01 03

fixed via, temporary workaround in the e2e tests, i start working on it now and try to fix it long term.

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Just gave it a spin, re-sharing an incoming share from the shared with me page, then navigating into the share, doesn't show any share indicators in the file list and doesn't show the sharees in the right sidebar of any of the files.

@ScharfViktor
Copy link
Contributor

ready for testing yet? If so, I can add my findings here

use roles whenever possible
use fileId for public link sharing
fix indirect sharing-indicators behavior
fix retrieving of shares from shares tree
fix reshare permissions on share roots
fix sharesTree errors for links, take parent permissions into account
add ocis resharing env var to docker compose config
load indicators in share jail when resharing is enabled
make use of resharing composable
refactor share loading out of link and share panel component into parent component to load the data only once
cleanup expected failures & acceptance tests
add gherkin debug helpers
bump sdk
bump ocis
@fschade
Copy link
Collaborator Author

fschade commented Jul 6, 2022

@ScharfViktor, yes. But if you find minor thinks please create a separate issue, i want to close this pr asap.

@fschade
Copy link
Collaborator Author

fschade commented Jul 6, 2022

@kulmann, created a separate pr for that Bug, #7220

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Discussed some followups. Good to merge 💪

@sonarcloud
Copy link

sonarcloud bot commented Jul 6, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

44.4% 44.4% Coverage
0.0% 0.0% Duplication

@fschade fschade merged commit b2af0b2 into master Jul 6, 2022
@delete-merged-branch delete-merged-branch bot deleted the resharing branch July 6, 2022 15:19
@ScharfViktor
Copy link
Contributor

  • admin creates space
  • admin tries to add Maria as manager member of the space. Web sends request with the body: shareType=7&shareWith=richard&space_ref=spaceId&permissions=31

Actual: inpossible to add manager user: <ocs><meta><status>error</status><statuscode>400</statuscode><message>invalid role for space member</message></meta></ocs>

and case with changing role: #7163

fixed. Thanks))

@@ -243,11 +243,6 @@ export default {
context.commit('CURRENT_FILE_OUTGOING_SHARES_ERROR', null)
context.commit('CURRENT_FILE_OUTGOING_SHARES_LOADING', true)

let spaceRef
if (storageId) {
spaceRef = `${storageId}${path}`
Copy link
Contributor

Choose a reason for hiding this comment

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

@fschade @kulmann could #7268 have been introduced here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Enhancement Add new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants