Skip to content

accept group shares with federated manager#4

Open
soltanireza65 wants to merge 2 commits into
surf-devfrom
acceptGroupShares
Open

accept group shares with federated manager#4
soltanireza65 wants to merge 2 commits into
surf-devfrom
acceptGroupShares

Conversation

@soltanireza65
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@navid-shokri navid-shokri left a comment

Choose a reason for hiding this comment

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

it needs refactoring to become more straightforward.

$f = $this->uid;
$externalGroupManager = new \OCA\FederatedGroups\FilesSharing\External\Manager($a, $b, $c, $d, $e, $f);

if ($this->externalManager->acceptShare((int) $id)) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reza, you just copy one code twice and it is not acceptable. you should just switch between managers per requests like this:

if ($shareType == "group"){
   $manager = $this->externalGroupManager;
} else {
   $manager = $this->$manager;
}

and then use $manager in the rest of the code.
please refer to core/apps/files_sharing/Controllers/ExternalController.php

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I cannot see the changes in the front part of your pull request. I expected more file changes. can you make sure that all changes are pushed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

for client-side changes,
those are merged before this PR
but if you have anything more, I'm ready to go forward to code them as well

@soltanireza65
Copy link
Copy Markdown
Author

@navid-shokri
do you agree with the latest changes I have made?

@soltanireza65
Copy link
Copy Markdown
Author

soltanireza65 commented Mar 6, 2023

@navid-shokri
please give a comment , if we can merge and close

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.

2 participants