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

add filter by sharetype in the ocs API #2050

Merged
merged 3 commits into from
Oct 5, 2021

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Sep 7, 2021

Added a query parameter to the OCS API to filter the received shares by type.

@C0rby C0rby force-pushed the sharetype-filter branch 3 times, most recently from e593447 to eacfb8a Compare September 7, 2021 16:08
@C0rby C0rby marked this pull request as ready for review September 7, 2021 16:25
@C0rby C0rby requested a review from labkode as a code owner September 7, 2021 16:25
@C0rby C0rby self-assigned this Sep 7, 2021
@C0rby C0rby requested review from butonic and refs September 7, 2021 16:25
labkode
labkode previously approved these changes Sep 8, 2021
Copy link
Member

@labkode labkode left a comment

Choose a reason for hiding this comment

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

Looks good to me but I need @butonic, @refs to approve as well.

@butonic
Copy link
Contributor

butonic commented Sep 8, 2021

The share_type parameter was introduced to oc10 with owncloud/core#38053

butonic
butonic previously approved these changes Sep 8, 2021
Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

could you add the filter to the sql driver in

if f.Type == collaboration.Filter_TYPE_RESOURCE_ID {

and maybe in
if f.Type == collaboration.Filter_TYPE_RESOURCE_ID {
as well?

if not track it in a new issue.

@C0rby
Copy link
Contributor Author

C0rby commented Sep 8, 2021

could you add the filter to the sql driver in

Ok let me try.

@C0rby C0rby dismissed stale reviews from butonic and labkode via 8f0de6e September 8, 2021 09:29
@C0rby C0rby requested review from butonic and labkode September 8, 2021 09:30
@C0rby
Copy link
Contributor Author

C0rby commented Sep 8, 2021

@butonic @labkode, I added the filters to the sql share managers. Please have a look at my changes. Also I couldn't test these changes since I don't know how/what to setup for the sql share managers. If you have some documentation or some tips, that would be great.

@lgtm-com
Copy link

lgtm-com bot commented Sep 8, 2021

This pull request introduces 2 alerts when merging 8f0de6e into f2109fc - view on LGTM.com

new alerts:

  • 2 for Useless assignment to local variable

@C0rby C0rby force-pushed the sharetype-filter branch 2 times, most recently from 1c991f3 to 5f7b705 Compare September 8, 2021 09:58
@C0rby
Copy link
Contributor Author

C0rby commented Sep 8, 2021

This pull request introduces 2 alerts when merging 8f0de6e into f2109fc - view on LGTM.com

new alerts:

* 2 for Useless assignment to local variable

I resolved this but the comment doesn't disappear and I also can't remove it.

rhafer
rhafer previously approved these changes Sep 8, 2021
Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

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

LGTM. At least I didn't find something obviously wrong here. (Take this with a grain of salt, as I am still pretty new to the code base)

butonic
butonic previously approved these changes Sep 8, 2021
refs
refs previously approved these changes Sep 13, 2021
Copy link
Member

@refs refs left a comment

Choose a reason for hiding this comment

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

Just a pedantic comment from mine, nice PR and nice expected failures cleanup! Thanks!

pkg/cbox/share/sql/sql.go Outdated Show resolved Hide resolved
pkg/cbox/share/sql/sql.go Show resolved Hide resolved
@C0rby C0rby dismissed stale reviews from refs, butonic, and rhafer via 216679b September 13, 2021 13:10
@C0rby C0rby requested review from butonic and refs September 13, 2021 13:11
@C0rby C0rby requested review from refs and butonic September 17, 2021 14:57
@C0rby C0rby force-pushed the sharetype-filter branch 2 times, most recently from 33b27dd to 06bce1c Compare September 20, 2021 09:41
@C0rby
Copy link
Contributor Author

C0rby commented Sep 20, 2021

The tests fail now because of this PR #2072. Because of that PR rejected shares are not shown in ListReceivedShares responses and there is no way to re-accept or even list a rejected share.

@labkode, how do we want to go forward with this? I think setting the filter in the ocdav layer would be better and then making that behavior configurable. Then an admin could choose if rejected shares should be shown or not.

cc @micbar @butonic

@labkode
Copy link
Member

labkode commented Sep 28, 2021

Hi @C0rby that PR should do nothing about accepting or denying shares as we know them today. That PR handles a specific type of share that denies access (DENIAL). That type of share cannot be accepted or rejected by the receiver, the receiver basically can't do anything about it.

The filter set in that PR is to not shown this type of special shares to the receiving user, NOT to filter out reject shares.

Let me know if you need further clarifications.

@C0rby
Copy link
Contributor Author

C0rby commented Sep 28, 2021

The filter set in that PR is to not shown this type of special shares to the receiving user, NOT to filter out reject shares.

I see, then I misunderstood this filter. I will rewrite my code then to reflect the correct behavior.

@C0rby
Copy link
Contributor Author

C0rby commented Sep 28, 2021

@labkode, how do I recognize such deny shares?
Is a share a deny share when DenyGrant is set to true or how can I filter them out in the json and memory manager?

@C0rby C0rby force-pushed the sharetype-filter branch 2 times, most recently from c88c57d to d901f66 Compare September 28, 2021 13:02
@labkode
Copy link
Member

labkode commented Sep 28, 2021

Hi @C0rby, in the SQL manager we set a DenyShare as having permissions=0, so you can do the same on the other drivers, I think is semantically meaningful.

Copy link
Contributor

@ishank011 ishank011 left a comment

Choose a reason for hiding this comment

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

Hi @C0rby this is really nice! Just left a couple of comments

pkg/cbox/share/sql/sql.go Outdated Show resolved Hide resolved
pkg/share/manager/json/json.go Outdated Show resolved Hide resolved
@C0rby C0rby force-pushed the sharetype-filter branch from 65d8cfc to 46f3b4b Compare October 4, 2021 15:22
@C0rby C0rby force-pushed the sharetype-filter branch 3 times, most recently from 5c6046b to c39d980 Compare October 5, 2021 11:50
@C0rby C0rby marked this pull request as draft October 5, 2021 12:16
@C0rby
Copy link
Contributor Author

C0rby commented Oct 5, 2021

Let me just add some unit tests for the new functions and then this PR is good to go. :)

@C0rby C0rby force-pushed the sharetype-filter branch from c39d980 to d9dfebe Compare October 5, 2021 12:29
@C0rby C0rby marked this pull request as ready for review October 5, 2021 12:29
@C0rby C0rby requested a review from ishank011 October 5, 2021 12:30
@C0rby
Copy link
Contributor Author

C0rby commented Oct 5, 2021

It's ready now.

@ishank011 ishank011 merged commit c40fcc8 into cs3org:master Oct 5, 2021
@C0rby C0rby deleted the sharetype-filter branch October 5, 2021 14:23
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