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

return defauldt Provider for Share_Type_Remote_Group #16

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

navid-shokri
Copy link

set a default share provider for SHARE_TYPE_REMOTE_GROUP

Related Issue

pondersource/oc-opencloudmesh#19

Motivation and Context

federated groups and opencloudmeshapplication

Copy link

@yasharpm yasharpm left a comment

Choose a reason for hiding this comment

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

This is a dilemma.

FederatedShareProvider does not support SHARE_TYPE_REMOTE_GROUP. Even though the tests might pass, we have to make sure that this change does not break any functionality of the OC. For example, in another section of code, all shares for all share types might be queried. In such an instance, if there is a remote user share, the OC might show it twice because it received it once from the FederatedProvider for SHARE_TYPE_REMOTE and another type from the same object but for the SHARE_TYPE_REMOTE_GROUP.

A cleaner approach to solve this issue, I believe, is to add a function to IManager to return all the supported share types. And then change the behavior of apps/dav/lib/Connector/Sabre/SharesPlugin.php to query the supported share types from the IManager instead of using a hard coded list. But this will further complicate the chance of our PR into OC to be accepted.

The current fix is quick and sample (if it works) but logically inaccurate. While my counter proposal might complicate the PR process but more elegant.

I think the next course of action is:

  1. Make sure the suggested test does not result in bugs in OC
  2. Consult @michielbdejong and @thepeak99 to make a decision on the elegance vs PR merge difficulty.

@navid-shokri
Copy link
Author

So, this task is also in BLOCKED state by @yasharpm 's comment.
I agree with @yasharpm and also agree with my fix 😎

it seems there is a trade-off and we should discuss about it.

@navid-shokri navid-shokri merged commit 049b9a8 into accept-ocm-to-groups Jun 27, 2023
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.

owncloud files app shows no files with accept-ocm-to-groups branch and this app not installed
2 participants