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.

Sign up for GitHub

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

Jump to bottom

[full-ci] Resharing #3904

Merged
merged 6 commits into from
Jun 16, 2022
Merged

[full-ci] Resharing #3904

merged 6 commits into from
Jun 16, 2022

Conversation

kobergj
Copy link
Collaborator

@kobergj kobergj commented Jun 1, 2022 •
edited
Loading

Description

Adds and activates resharing functionality for ocis

Related Issue

  • Fixes <issue_link>

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

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)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@kobergj kobergj mentioned this pull request Jun 1, 2022
Closed
9 tasks
@kobergj kobergj force-pushed the Resharing branch 3 times, most recently from cd636da to 4206509 Compare June 2, 2022 10:02
@ScharfViktor
Copy link
Contributor

ScharfViktor commented Jun 3, 2022 •
edited
Loading

Looks good, great work.
perhaps we should create a new ApiBasic directory and move resharing.feature there. Because ApiSpaces has only tests related project space

I would suggest to add cases:

  • re-sharing to group
  • re-sharing file
  • re-sharing with different permissions.

@kobergj
Copy link
Collaborator Author

kobergj commented Jun 3, 2022

@ScharfViktor thanks for reviewing! I (presumably) addressed all issues, could you check again?

Also I added test for file resharing and resharing with different permissions. Please let me know if I should add more testcases.

re-sharing to group will not work atm. I will add a test later

@ScharfViktor
Copy link
Contributor

@kobergj great job.

ps re-sharing to group works for me, my request:

curl --location --request POST 'https://localhost:9200/ocs/v1.php/apps/files_sharing/api/v1/shares' \
--header 'Authorization: Basic bWFyaWU6cmFkaW9hY3Rpdml0eQ==' \
--header 'Content-Type: application/x-www-form-urlencoded' \
--data-urlencode 'shareType=1' \
--data-urlencode 'permissions=17' \
--data-urlencode 'shareWith=physics-lovers' \
--data-urlencode 'path=/Shares/f4'

@kobergj kobergj force-pushed the Resharing branch 4 times, most recently from 22dc3e4 to aa65576 Compare June 13, 2022 12:13
kobergj added 2 commits June 14, 2022 10:28
@kobergj
enable resharing
4c2311b 
check for removegrant instead addgrant

Signed-off-by: jkoberg <[email protected]>

add e2e tests

Signed-off-by: jkoberg <[email protected]>

expected failures

Signed-off-by: jkoberg <[email protected]>

enable resharing

Signed-off-by: jkoberg <[email protected]>
@kobergj
update testing
42e51cb 
update testing

expected passes

Signed-off-by: jkoberg <[email protected]>

update tests

Signed-off-by: jkoberg <[email protected]>

update the tests

Signed-off-by: jkoberg <[email protected]>

more unexpected passes

Signed-off-by: jkoberg <[email protected]>

add more api tests

Signed-off-by: jkoberg <[email protected]>
@kobergj kobergj marked this pull request as ready for review June 14, 2022 08:28
@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/ocis/12507/38/6

runsh: Total unexpected passed scenarios throughout the test run:
apiSpaces/resharing.feature:80
apiSpaces/resharing.feature:81

those can be removed from expected-failures now - good.

@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/ocis/12507/47/6

runsh: Total unexpected failed scenarios throughout the test run:
apiWebdavEtagPropagation2/copyFileFolder.feature:188

Old closed issue #3732 - it looks like this Etag test has been a bit flaky in the past. If it fails again we need to chase it up.

kobergj added 3 commits June 14, 2022 11:38
@kobergj
update expected failures
e66c955 
Signed-off-by: jkoberg <[email protected]>

expected failures

Signed-off-by: jkoberg <[email protected]>
@kobergj
changelog
a65c7cc 
Signed-off-by: jkoberg <[email protected]>
@kobergj
bump reva version
47ee2aa 
bump custom reva version

Signed-off-by: jkoberg <[email protected]>

new reva

Signed-off-by: jkoberg <[email protected]>

bump reva

Signed-off-by: jkoberg <[email protected]>

bump reva

bump reva

more reva bumping

Signed-off-by: jkoberg <[email protected]>

newest reva

Signed-off-by: jkoberg <[email protected]>

bump reva

Signed-off-by: jkoberg <[email protected]>
@kobergj
Copy link
Collaborator Author

kobergj commented Jun 14, 2022

@phil-davis Thanks! It works smoothly now.

Regarding apiWebdavEtagPropagation2/copyFileFolder.feature:188 I had this failing a few times in the past. I will report when it happens again.

fschade
fschade approved these changes Jun 14, 2022
View reviewed changes
Copy link
Contributor

@fschade fschade left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

micbar
micbar reviewed Jun 14, 2022
View reviewed changes
Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

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

LGTM, some small nitpicks

extensions/graph/pkg/service/v0/drives.go
@@ -498,7 +498,7 @@ func (g Graph) cs3StorageSpaceToDrive(ctx context.Context, baseURL *url.URL, spa
tmp := id
identity := libregraph.IdentitySet{User: &libregraph.Identity{Id: &tmp}}
switch {
case perm.AddGrant:
case perm.RemoveGrant:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like a code comment why we check that grant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@micbar I added a comment. Please check again if it is clear enough

@micbar micbar mentioned this pull request Jun 14, 2022
Merged
@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 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@butonic butonic merged commit 8a3d0e1 into master Jun 16, 2022
@delete-merged-branch delete-merged-branch bot deleted the Resharing branch June 16, 2022 08:49
ownclouders pushed a commit that referenced this pull request Jun 16, 2022
@butonic
commit 8a3d0e1
0fce655 
Merge: 3a43daf 7d97802
Author: Jörn Friedrich Dreyer <[email protected]>
Date:   Thu Jun 16 08:49:53 2022 +0000

    Merge pull request #3904 from owncloud/Resharing

    [full-ci] Resharing
@rhafer rhafer mentioned this pull request Sep 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phil-davis phil-davis phil-davis left review comments

@micbar micbar micbar left review comments

@ScharfViktor ScharfViktor ScharfViktor left review comments

@butonic butonic butonic approved these changes

@fschade fschade fschade approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kobergj @ScharfViktor @phil-davis @butonic @micbar @fschade