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(teams): resolve undefined variable error and add logging #6290

Merged
merged 1 commit into from
Nov 11, 2024
Merged

fix(teams): resolve undefined variable error and add logging #6290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions lib/Controller/ContactController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
*/
namespace OCA\Calendar\Controller;

use Exception;
use OCA\Calendar\Service\ServiceException;
use OCA\Circles\Api\v1\Circles;
use OCA\Circles\Exceptions\CircleNotFoundException;
use OCP\App\IAppManager;
use OCP\AppFramework\Controller;
Expand All @@ -17,6 +19,7 @@
use OCP\Contacts\IManager;
use OCP\IRequest;
use OCP\IUserManager;
use Psr\Log\LoggerInterface;

/**
* Class ContactController
Expand All @@ -40,11 +43,14 @@ class ContactController extends Controller {
* @param IRequest $request
* @param IManager $contacts
*/
public function __construct(string $appName,
public function __construct(
string $appName,
IRequest $request,
IManager $contacts,
IAppManager $appManager,
IUserManager $userManager) {
IUserManager $userManager,
private LoggerInterface $logger,
) {
parent::__construct($appName, $request);
$this->contactsManager = $contacts;
$this->appManager = $appManager;
Expand Down Expand Up @@ -173,32 +179,32 @@ public function searchAttendee(string $search):JSONResponse {
* @param string $circleId CircleId to query for members
* @return JSONResponse
* @throws Exception
* @throws \OCP\AppFramework\QueryException
*
* @NoAdminRequired
*/
public function getCircleMembers(string $circleId):JSONResponse {
if (!$this->appManager->isEnabledForUser('circles') || !class_exists('\OCA\Circles\Api\v1\Circles')) {
if (!class_exists('\OCA\Circles\Api\v1\Circles') || !$this->appManager->isEnabledForUser('circles')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question, why are we checking if the class exists? Would the check for isEnabledForUser not be sufficient enough to determine if the circles app is installed, enabled on the system and permitted for this user?

Copy link
Contributor

Choose a reason for hiding this comment

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

The original pr mentions a problem with Psalm #5420 (review) ;)

I haven't done any profiling but would expect, especially now with the flipped condition, a little performance gain for systems without circles.

isEnabledForUser does a lot (fetch the installed app versions for all apps, see if the app is installed, fetch the user session, fetch the groups for a user, see if the user has a group that the app is enabled for).

Copy link
Contributor

@SebastianKrupinski SebastianKrupinski Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

isEnabledForUser does a lot (fetch the installed app versions for all apps, see if the app is installed, fetch the user session, fetch the groups for a user, see if the user has a group that the app is enabled for).

I checked the code... Thats not the case, I think the original author just didn't know there was a build in function to see if the app was enabled, and checked for the existence for the class instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats not the case

What is not the case?

$this->logger->debug('Circles not enabled');
miaulalala marked this conversation as resolved.
Show resolved Hide resolved
return new JSONResponse();
}
if (!$this->contactsManager->isEnabled()) {
$this->logger->debug('Contacts not enabled');
return new JSONResponse();
}

try {
$circle = \OCA\Circles\Api\v1\Circles::detailsCircle($circleId, true);
$circle = Circles::detailsCircle($circleId, true);
} catch (QueryException $ex) {
$this->logger->error('Could not resolve circle details', ['exception' => $ex]);
return new JSONResponse();
} catch (CircleNotFoundException $ex) {
return new JSONResponse();
}

if (!$circle) {
$this->logger->error('Could not find circle', ['exception' => $ex]);
return new JSONResponse();
}

$circleMembers = $circle->getInheritedMembers();

$contacts = [];
foreach ($circleMembers as $circleMember) {
if ($circleMember->isLocal()) {

Expand All @@ -207,7 +213,8 @@ public function getCircleMembers(string $circleId):JSONResponse {
$user = $this->userManager->get($circleMemberUserId);

if ($user === null) {
throw new ServiceException('Could not find organizer');
$this->logger->error('Could not find user with user id' . $circleMemberUserId);
throw new ServiceException('Could not find circle member');
}

$contacts[] = [
Expand Down
9 changes: 8 additions & 1 deletion tests/php/unit/Controller/ContactControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use OCP\IRequest;
use OCP\IUserManager;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\LoggerInterface;

class ContactControllerTest extends TestCase {
/** @var string */
Expand Down Expand Up @@ -42,8 +43,14 @@ protected function setUp():void {
$this->manager = $this->createMock(IManager::class);
$this->appManager = $this->createMock(IAppManager::class);
$this->userManager = $this->createMock(IUserManager::class);
$this->logger = $this->createMock(LoggerInterface::class);
$this->controller = new ContactController($this->appName,
$this->request, $this->manager, $this->appManager, $this->userManager);
$this->request,
$this->manager,
$this->appManager,
$this->userManager,
$this->logger,
);
}

public function testSearchLocationDisabled():void {
Expand Down
Loading