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

Fix #40966 #40967

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix #40966 #40967

wants to merge 1 commit into from

Conversation

michielbdejong
Copy link
Contributor

@michielbdejong michielbdejong commented Sep 1, 2023 •
edited

No description provided.

@yasharpm @michielbdejong
Fix owncloud#40966
05f34f9 
Signed-off-by: Michiel de Jong <michiel@pondersource.com>
@update-docs
Copy link

update-docs bot commented Sep 1, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@michielbdejong michielbdejong mentioned this pull request Sep 1, 2023
Closed
@sonarcloud
Copy link

sonarcloud bot commented Sep 1, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

22.2% 22.2% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@phil-davis
Copy link
Contributor

@michielbdejong somehow the CLA assistaqnt does not think that you have clicked the buttons to "sign" the CLA. I am surprised about that! But can you please click the button...

@pako81
Copy link
Contributor

pako81 commented Sep 14, 2023

@michielbdejong please also create a changelog for this.

@DeepDiver1975
Copy link
Member

@michielbdejong somehow the CLA assistaqnt does not think that you have clicked the buttons to "sign" the CLA. I am surprised about that! But can you please click the button...

CLA of Yashar is missing .... he is the author ...
image

jvillafanez
jvillafanez reviewed Sep 25, 2023
View reviewed changes
apps/files_sharing/lib/Controller/RemoteOcsController.php
@@ -111,6 +111,25 @@ public function acceptShare($id) {
);
}

// Allow the Federated Groups app to overwrite the behaviour of this endpoint
$managerClass = $this->config->getSystemValue('sharing.groupExternalManager');
Copy link
Member

Choose a reason for hiding this comment

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

There is some duplication getting the external manager. We can move that piece of code in a private method in order to reduce the duplication. I think this also affects to some code in this class not present in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvillafanez jvillafanez jvillafanez left review comments

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

None yet
7 participants
@michielbdejong @CLAassistant @phil-davis @pako81 @DeepDiver1975 @jvillafanez @yasharpm