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

Cleanup and do not list current user shares in getShares too #16789

Merged
merged 12 commits into from
Oct 26, 2019

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Aug 19, 2019

  • getSharesInDir was properly excluding shares where shared_with was the current user (returns reshares in API #14605) but we were missing the check in getShares too
  • Added more comments on the code
  • Cleanup and move duplicate code into one function

I could use some help on the tests :)

@skjnldsv skjnldsv added 3. to review Waiting for reviews 2. developing Work in progress and removed 2. developing Work in progress 3. to review Waiting for reviews labels Aug 19, 2019
@rullzer rullzer mentioned this pull request Aug 23, 2019
@skjnldsv skjnldsv force-pushed the fix/sharing/do-not-list-sharedWith-me-shares branch from 2f9c3ab to a4c4733 Compare August 26, 2019 09:10
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 26, 2019
@skjnldsv skjnldsv requested a review from danxuliu August 26, 2019 09:11
@skjnldsv

This comment has been minimized.

@rullzer rullzer mentioned this pull request Aug 29, 2019
16 tasks
@skjnldsv skjnldsv force-pushed the fix/sharing/do-not-list-sharedWith-me-shares branch from a4c4733 to ff5d724 Compare August 30, 2019 09:09
@rullzer rullzer modified the milestones: Nextcloud 17, Nextcloud 18 Sep 5, 2019
@rullzer

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@skjnldsv skjnldsv force-pushed the fix/sharing/do-not-list-sharedWith-me-shares branch from ff5d724 to 75ee2b2 Compare October 4, 2019 06:19
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 4, 2019

Same here everyone, please let's get this in so I can continue the sharing revamp!

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

🐘

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 24, 2019
@skjnldsv skjnldsv force-pushed the fix/sharing/do-not-list-sharedWith-me-shares branch from 3492fd9 to bf6209c Compare October 24, 2019 12:38
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

There are several failing tests, not ready to release yet. I will take a look.

@danxuliu danxuliu added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Oct 24, 2019
danxuliu and others added 11 commits October 25, 2019 10:48
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
"ShareManager::getSharesBy()" already checks if the share provider
exists before returning the shares and, if the provider does not exist,
it returns an empty array. Therefore it is not needed to explicitly
check if the provider exists or not.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Getting the shares of a file no longer returns shares with the current
user for consistency with the results when getting the shares including
subfiles.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@danxuliu danxuliu force-pushed the fix/sharing/do-not-list-sharedWith-me-shares branch from ce6c693 to 8a06720 Compare October 25, 2019 20:48
@danxuliu
Copy link
Member

Sorry, it took me way longer than expected :-(

I have added more unit and integration tests, split the fixes and the refactorings, did some minor cleanups and fixed the pending issues (I hope :-P ).

@skjnldsv I moved if (!$resharingRight && $this->shareProviderResharingRights($this->currentUser, $share, $path)) { back to its original position because in the previous code it was called after if ($share->getSharedWith() === $this->currentUser) {, and given that getShareWith() is checked too in shareProviderResharingRights() that might change the behaviour (although no test was affected, and I was not able to make a test that caused a behaviour change... so if you changed it for some specific reason please provide a test that shows it :-) ).

I have also added some integration tests in a last commit that show the current behaviour in some strange scenarios. After working on this so much time I no longer know what works as expected or not... so I just document it and I let others decide for me :-P And if the tests themselves do not make sense either just drop the last commit; I sense a bug (not from this pull request, but coming already from the previous code) lurking around in the code that returns a reduced set of shares when there are not enough resharing rights, but as much as I tried I could not find it. So maybe everything is just fine :-P

@skjnldsv
Copy link
Member Author

so if you changed it for some specific reason please provide a test that shows it

Argh, I cannot recall! :(
I think aside from cleaning the code and not showing your own shares in the getShares function, that was all this pr was about.

The issue I noticed was on the new sidebar, when listing the shares of a shared-with-me folder, I could also see the share that made the folder available to me.
Not sure if that's really clear.
But to sums it up, if userA share a folder with resharing rights to userB, and then as userB I open the folder, I see all the shares that userA shared with others on this folder. Since I'm userB, I should not see the initial userA->userB share, I can still unshare it from the files list (instead of delte, that's how we do it).

I have added more unit and integration tests, split the fixes and the refactorings, did some minor cleanups and fixed the pending issues (I hope :-P ).

That's is an incredible work!!
Thanks a lot Daniel!!! 🤗 ❤️

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 25, 2019

After working on this so much time I no longer know what works as expected or not

that is the issue here.
That's why I split my 4 previous pr into 4 because it's so complicated! :O

did some minor cleanups and fixed the pending issues (I hope :-P ).

So far:

--- Failed scenarios:
699 |  
700 | /drone/src/build/integration/sharing_features/sharing-v1.feature:308
701

EDIT: fixed 🚀

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@skjnldsv skjnldsv force-pushed the fix/sharing/do-not-list-sharedWith-me-shares branch from 8a06720 to d0b205d Compare October 26, 2019 12:22
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Oct 26, 2019
@skjnldsv
Copy link
Member Author

Failed integration test now passes.
Acceptances failures are timeouts and were passing on the previous one

[Composer\Downloader\TransportException]
The "https://packagist.org/p/provider-2015%24388aba92c0dcf5c49c5ef6281aee51336f0df71b3d6a4111efbe8558d462bb03.json" file could not be downloaded (HTTP/1.1 404 Not Found)

🚀

@skjnldsv skjnldsv merged commit 9348fd5 into master Oct 26, 2019
@skjnldsv skjnldsv deleted the fix/sharing/do-not-list-sharedWith-me-shares branch October 26, 2019 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug enhancement feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants