From a666389f726d38ddf987a76887b540b3d6dec05a Mon Sep 17 00:00:00 2001 From: Artur Neumann Date: Thu, 17 Feb 2022 14:03:10 +0545 Subject: [PATCH] [#40969] show correct error message in files tab https://community.openproject.org/work_packages/40969 Signed-off-by: Artur Neumann --- lib/Controller/OpenProjectAPIController.php | 24 +- lib/Service/OpenProjectAPIService.php | 76 ++++--- src/components/tab/EmptyContent.vue | 10 +- src/views/ProjectsTab.vue | 10 +- .../jest/components/tab/EmptyContent.spec.js | 17 ++ tests/jest/views/ProjectsTab.spec.js | 19 +- .../OpenProjectAPIControllerTest.php | 42 +++- .../lib/Service/OpenProjectAPIServiceTest.php | 210 +++++++++++++++++- 8 files changed, 345 insertions(+), 63 deletions(-) diff --git a/lib/Controller/OpenProjectAPIController.php b/lib/Controller/OpenProjectAPIController.php index 0cc1718df..87ccd69d8 100644 --- a/lib/Controller/OpenProjectAPIController.php +++ b/lib/Controller/OpenProjectAPIController.php @@ -129,20 +129,36 @@ public function getNotifications(?string $since = null): DataResponse { * @NoAdminRequired * * @param ?string $searchQuery + * @param ?int $fileId * * @return DataResponse */ - public function getSearchedWorkPackages(?string $searchQuery = null): DataResponse { + public function getSearchedWorkPackages(?string $searchQuery = null, ?int $fileId = null): DataResponse { if ($this->accessToken === '' || !OpenProjectAPIService::validateOpenProjectURL($this->openprojectUrl)) { - return new DataResponse('', Http::STATUS_BAD_REQUEST); + return new DataResponse( + 'invalid open project configuration', Http::STATUS_UNAUTHORIZED + ); } $result = $this->openprojectAPIService->searchWorkPackage( - $this->openprojectUrl, $this->accessToken, $this->refreshToken, $this->clientID, $this->clientSecret, $this->userId, $searchQuery + $this->openprojectUrl, + $this->accessToken, + $this->refreshToken, + $this->clientID, + $this->clientSecret, + $this->userId, + $searchQuery, + $fileId ); + if (!isset($result['error'])) { $response = new DataResponse($result); } else { - $response = new DataResponse($result, Http::STATUS_UNAUTHORIZED); + if (isset($result['statusCode'])) { + $statusCode = $result['statusCode']; + } else { + $statusCode = Http::STATUS_BAD_REQUEST; + } + $response = new DataResponse($result, $statusCode); } return $response; } diff --git a/lib/Service/OpenProjectAPIService.php b/lib/Service/OpenProjectAPIService.php index 74686dfcd..907bfac34 100644 --- a/lib/Service/OpenProjectAPIService.php +++ b/lib/Service/OpenProjectAPIService.php @@ -238,22 +238,28 @@ public function getNotifications(string $url, string $accessToken, * @param string $clientID * @param string $clientSecret * @param string $userId - * @param string $query + * @param string|null $query + * @param int|null $fileId * @param int $offset * @param int $limit - * @return array + * @return array + * @throws \OCP\PreConditionNotMetException * @throws \Safe\Exceptions\JsonException */ public function searchWorkPackage(string $url, string $accessToken, string $refreshToken, string $clientID, string $clientSecret, string $userId, - string $query, int $offset = 0, int $limit = 5): array { + string $query = null, int $fileId = null, int $offset = 0, int $limit = 5): array { $resultsById = []; + $filters = []; // search by description - $filters = [ - ["description" => ["operator" => "~", "values" => [$query]]], - ["status" => ["operator" => "!", "values" => ["14"]]] - ]; + if ($fileId !== null) { + $filters[] = ['file_link_origin_id' => ['operator' => '=', 'values' => [(string)$fileId]]]; + } + if ($query !== null) { + $filters[] = ['description' => ['operator' => '~', 'values' => [$query]]]; + $filters[] = ['status' => ['operator' => '!', 'values' => ['14']]]; + } $params = [ 'filters' => \Safe\json_encode($filters), 'sortBy' => '[["updatedAt", "desc"]]', @@ -263,28 +269,38 @@ public function searchWorkPackage(string $url, string $accessToken, $url, $accessToken, $refreshToken, $clientID, $clientSecret, $userId, 'work_packages', $params ); + if (isset($searchDescResult['error'])) { + return $searchDescResult; + } + if (isset($searchDescResult['_embedded'], $searchDescResult['_embedded']['elements'])) { foreach ($searchDescResult['_embedded']['elements'] as $wp) { $resultsById[$wp['id']] = $wp; } } // search by subject - $filters = [ - ["subject" => ["operator" => "~", "values" => [$query]]], - ["status" => ["operator" => "!", "values" => ["14"]]] - ]; - $params = [ - 'filters' => \Safe\json_encode($filters), - 'sortBy' => '[["updatedAt", "desc"]]', - // 'limit' => $limit, - ]; - $searchSubjectResult = $this->request( + if ($query !== null) { + $filters = [ + ['subject' => ['operator' => '~', 'values' => [$query]]], + ['status' => ['operator' => '!', 'values' => ['14']]] + ]; + $params = [ + 'filters' => \Safe\json_encode($filters), + 'sortBy' => '[["updatedAt", "desc"]]', + // 'limit' => $limit, + ]; + $searchSubjectResult = $this->request( $url, $accessToken, $refreshToken, $clientID, $clientSecret, $userId, 'work_packages', $params ); - if (isset($searchSubjectResult['_embedded'], $searchSubjectResult['_embedded']['elements'])) { - foreach ($searchSubjectResult['_embedded']['elements'] as $wp) { - $resultsById[$wp['id']] = $wp; + if (isset($searchSubjectResult['error'])) { + return $searchSubjectResult; + } + + if (isset($searchSubjectResult['_embedded'], $searchSubjectResult['_embedded']['elements'])) { + foreach ($searchSubjectResult['_embedded']['elements'] as $wp) { + $resultsById[$wp['id']] = $wp; + } } } @@ -386,14 +402,7 @@ public function request(string $openprojectUrl, string $accessToken, string $ref } else { return ['error' => $this->l10n->t('Bad HTTP method')]; } - $body = $response->getBody(); - $respCode = $response->getStatusCode(); - - if ($respCode >= 400) { - return ['error' => $this->l10n->t('Bad credentials')]; - } else { - return json_decode($body, true); - } + return json_decode($response->getBody(), true); } catch (ServerException | ClientException $e) { $response = $e->getResponse(); $body = (string) $response->getBody(); @@ -425,13 +434,20 @@ public function request(string $openprojectUrl, string $accessToken, string $ref } return [ 'error' => $e->getMessage(), - 'statusCode' => $e->getResponse()->getStatusCode(), + 'statusCode' => $response->getStatusCode(), ]; - } catch (ConnectException | Exception $e) { + } catch (ConnectException $e) { + $this->logger->warning('OpenProject connection error : '.$e->getMessage(), ['app' => $this->appName]); return [ 'error' => $e->getMessage(), 'statusCode' => 404, ]; + } catch (Exception $e) { + $this->logger->critical('OpenProject error : '.$e->getMessage(), ['app' => $this->appName]); + return [ + 'error' => $e->getMessage(), + 'statusCode' => 500, + ]; } } diff --git a/src/components/tab/EmptyContent.vue b/src/components/tab/EmptyContent.vue index 902ee3973..eedc8d841 100644 --- a/src/components/tab/EmptyContent.vue +++ b/src/components/tab/EmptyContent.vue @@ -33,6 +33,10 @@ export default { type: String, required: true, }, + errorMessage: { + type: String, + default: '', + }, }, data() { return { @@ -49,8 +53,12 @@ export default { emptyContentMessage() { if (this.state === 'no-token') { return t('integration_openproject', 'No OpenProject account connected') - } else if (this.state === 'error') { + } else if (this.state === 'connection-error') { return t('integration_openproject', 'Error connecting to OpenProject') + } else if (this.state === 'failed-fetching-workpackages') { + return t('integration_openproject', 'Could not fetch work packages from OpenProject') + } else if (this.state === 'error') { + return t('integration_openproject', 'Unexpected Error') } else if (this.state === 'ok') { return t('integration_openproject', 'No workspaces linked yet, search for work package to add!') } diff --git a/src/views/ProjectsTab.vue b/src/views/ProjectsTab.vue index 8f830b4e0..f27df4feb 100644 --- a/src/views/ProjectsTab.vue +++ b/src/views/ProjectsTab.vue @@ -77,19 +77,23 @@ export default { async fetchWorkpackages(fileId) { const req = {} - const url = generateUrl('/apps/integration_openproject/workpackages/' + fileId) + const url = generateUrl('/apps/integration_openproject/work_packages?fileId=' + fileId) try { const response = await axios.get(url, req) if (!Array.isArray(response.data)) { - this.state = 'error' + this.state = 'failed-fetching-workpackages' } else { this.state = 'ok' } } catch (error) { if (error.response && error.response.status === 401) { this.state = 'no-token' - } else { + } else if (error.response.status === 404) { + this.state = 'connection-error' + } else if (error.response.status === 500) { this.state = 'error' + } else { + this.state = 'failed-fetching-workpackages' } } }, diff --git a/tests/jest/components/tab/EmptyContent.spec.js b/tests/jest/components/tab/EmptyContent.spec.js index 01576cbef..2168b82aa 100644 --- a/tests/jest/components/tab/EmptyContent.spec.js +++ b/tests/jest/components/tab/EmptyContent.spec.js @@ -3,6 +3,15 @@ import { shallowMount, createLocalVue } from '@vue/test-utils' import EmptyContent from '../../../../src/components/tab/EmptyContent' const localVue = createLocalVue() +function getWrapper(propsData = {}) { + return shallowMount(EmptyContent, { + localVue, + mocks: { + t: (msg) => msg, + }, + propsData, + }) +} describe('EmptyContent.vue Test', () => { let wrapper const emptyContentMessageSelector = '.title' @@ -28,6 +37,7 @@ describe('EmptyContent.vue Test', () => { requestUrl: 'http://openproject/oauth/', }, }) + wrapper = getWrapper({ state: cases.state }) expect(wrapper.find(connectButtonSelector).exists()).toBe(cases.viewed) }) it.each([{ @@ -35,7 +45,13 @@ describe('EmptyContent.vue Test', () => { message: 'No OpenProject account connected', }, { state: 'error', + message: 'Unexpected Error', + }, { + state: 'connection-error', message: 'Error connecting to OpenProject', + }, { + state: 'failed-fetching-workpackages', + message: 'Could not fetch work packages from OpenProject', }, { state: 'ok', message: 'No workspaces linked yet', @@ -53,6 +69,7 @@ describe('EmptyContent.vue Test', () => { requestUrl: 'http://openproject/oauth/', }, }) + wrapper = getWrapper({ state: cases.state }) expect(wrapper.find(emptyContentMessageSelector).exists()).toBeTruthy() expect(wrapper.find(emptyContentMessageSelector).text()).toMatch(cases.message) }) diff --git a/tests/jest/views/ProjectsTab.spec.js b/tests/jest/views/ProjectsTab.spec.js index acc7c681f..2e0a0c04c 100644 --- a/tests/jest/views/ProjectsTab.spec.js +++ b/tests/jest/views/ProjectsTab.spec.js @@ -45,23 +45,10 @@ describe('ProjectsTab.vue Test', () => { }) describe('fetchWorkpackages', () => { it.each([ - { responseData: [], AppState: 'ok' }, - { responseData: 'string', AppState: 'error' }, - { responseData: null, AppState: 'error' }, - { responseData: undefined, AppState: 'error' }, - ])('sets states according to response content in case of success', async (cases) => { - axios.get.mockImplementationOnce(() => - Promise.resolve({ - data: cases.responseData, - }), - ) - await wrapper.vm.update({ id: 123 }) - expect(wrapper.vm.state).toBe(cases.AppState) - }) - it.each([ - { HTTPStatus: 400, AppState: 'error' }, + { HTTPStatus: 400, AppState: 'failed-fetching-workpackages' }, { HTTPStatus: 401, AppState: 'no-token' }, - { HTTPStatus: 404, AppState: 'error' }, + { HTTPStatus: 402, AppState: 'failed-fetching-workpackages' }, + { HTTPStatus: 404, AppState: 'connection-error' }, { HTTPStatus: 500, AppState: 'error' }, ])('sets states according to HTTP error codes', async (cases) => { const err = new Error() diff --git a/tests/lib/Controller/OpenProjectAPIControllerTest.php b/tests/lib/Controller/OpenProjectAPIControllerTest.php index f4a70d1c2..6cb99a3a6 100644 --- a/tests/lib/Controller/OpenProjectAPIControllerTest.php +++ b/tests/lib/Controller/OpenProjectAPIControllerTest.php @@ -176,9 +176,24 @@ public function testGetOpenProjectAvatarNoType() { } /** + * @return array + */ + public function searchWorkPackagesDataProvider() { + return [ + ['test', null, [['id' => 1], ['id' => 2], ['id' => 3], ['id' => 4], ['id' => 5]]], + ['test', 9090, [['id' => 1], ['id' => 2], ['id' => 3], ['id' => 4], ['id' => 5]]], + [null, 9090, [['id' => 1], ['id' => 2], ['id' => 3], ['id' => 4], ['id' => 5]]] + ]; + } + + /** + * @param string|null $searchQuery + * @param int|null $fileId + * @param array $expectedResponse * @return void + * @dataProvider searchWorkPackagesDataProvider */ - public function testGetSearchedWorkPackages():void { + public function testGetSearchedWorkPackages($searchQuery, $fileId, array $expectedResponse):void { $this->getUserValueMock(); $service = $this->getMockBuilder(OpenProjectAPIService::class) ->disableOriginalConstructor() @@ -186,33 +201,44 @@ public function testGetSearchedWorkPackages():void { ->getMock(); $service->expects($this->once()) ->method('searchWorkPackage') - ->willReturn([['id' => 1], ['id' => 2], ['id' => 3], ['id' => 4], ['id' => 5]]); + ->with( + $this->anything(), + $this->anything(), + $this->anything(), + $this->anything(), + $this->anything(), + $this->anything(), + $searchQuery, + $fileId + ) + ->willReturn($expectedResponse); $controller = new OpenProjectAPIController( 'integration_openproject', $this->requestMock, $this->configMock, $service, 'test' ); - $response = $controller->getSearchedWorkPackages('test'); + $response = $controller->getSearchedWorkPackages($searchQuery, $fileId); $this->assertSame(Http::STATUS_OK, $response->getStatus()); - $this->assertSame([['id' => 1], ['id' => 2], ['id' => 3], ['id' => 4], ['id' => 5]], $response->getData()); + $this->assertSame($expectedResponse, $response->getData()); } + /** * @return void */ - public function testGetSearchedWorkPackagesErrorResponse(): void { + public function testGetSearchedWorkPackagesNoAccessToken(): void { $this->getUserValueMock(''); $service = $this->createMock(OpenProjectAPIService::class); $controller = new OpenProjectAPIController( 'integration_openproject', $this->requestMock, $this->configMock, $service, 'test' ); $response = $controller->getSearchedWorkPackages('test'); - $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertSame(Http::STATUS_UNAUTHORIZED, $response->getStatus()); } /** * @return void */ - public function testGetSearchedWorkPackagesNoAccessToken(): void { + public function testGetSearchedWorkPackagesErrorResponse(): void { $this->getUserValueMock(); $service = $this->getMockBuilder(OpenProjectAPIService::class) ->disableOriginalConstructor() @@ -225,7 +251,7 @@ public function testGetSearchedWorkPackagesNoAccessToken(): void { 'integration_openproject', $this->requestMock, $this->configMock, $service, 'test' ); $response = $controller->getSearchedWorkPackages('test'); - $this->assertSame(Http::STATUS_UNAUTHORIZED, $response->getStatus()); + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); $this->assertSame(['error' => 'something went wrong'], $response->getData()); } diff --git a/tests/lib/Service/OpenProjectAPIServiceTest.php b/tests/lib/Service/OpenProjectAPIServiceTest.php index 9fd668c8e..d473e587d 100644 --- a/tests/lib/Service/OpenProjectAPIServiceTest.php +++ b/tests/lib/Service/OpenProjectAPIServiceTest.php @@ -10,6 +10,10 @@ namespace OCA\OpenProject\Service; use GuzzleHttp\Client as GuzzleClient; +use GuzzleHttp\Exception\BadResponseException; +use GuzzleHttp\Exception\ClientException; +use GuzzleHttp\Exception\ServerException; +use GuzzleHttp\Exception\ConnectException; use OC\Avatar\GuestAvatar; use OC\Http\Client\Client; use OCP\ICertificateManager; @@ -198,7 +202,7 @@ public function searchWorkPackageDataProvider() { * @return void * @dataProvider searchWorkPackageDataProvider */ - public function testSearchWorkPackageDescAndSubjectResponse( + public function testSearchWorkPackageOnlyQueryDescAndSubjectResponse( array $descriptionResponse, array $subjectResponse, array $expectedResult ) { $service = $this->getServiceMock(); @@ -229,6 +233,92 @@ public function testSearchWorkPackageDescAndSubjectResponse( $this->assertSame($expectedResult, $result); } + /** + * @return void + */ + public function testSearchWorkPackageByFileIdOnlyFileId() { + $service = $this->getServiceMock(); + $service->method('request') + ->withConsecutive( + [ + 'url','token', 'refresh', 'id', 'secret', 'user', 'work_packages', + [ + 'filters' => '[{"file_link_origin_id":{"operator":"=","values":["123"]}}]', + 'sortBy' => '[["updatedAt", "desc"]]', + ] + ], + ) + ->willReturnOnConsecutiveCalls( + ["_embedded" => ["elements" => [['id' => 1], ['id' => 2], ['id' => 3]]]] + ); + $result = $service->searchWorkPackage( + 'url', 'token', 'refresh', 'id', 'secret', 'user', null, 123 + ); + $this->assertSame([['id' => 1], ['id' => 2], ['id' => 3]], $result); + } + + /** + * @return void + */ + public function testSearchWorkPackageByFileIdQueryAndFileId() { + $service = $this->getServiceMock(); + $service->method('request') + ->withConsecutive( + [ + 'url','token', 'refresh', 'id', 'secret', 'user', 'work_packages', + [ + 'filters' => '[{"file_link_origin_id":{"operator":"=","values":["123"]}},{"description":{"operator":"~","values":["search query"]}},{"status":{"operator":"!","values":["14"]}}]', + 'sortBy' => '[["updatedAt", "desc"]]', + ] + ], + [ + 'url','token', 'refresh', 'id', 'secret', 'user', 'work_packages', + [ + 'filters' => '[{"subject":{"operator":"~","values":["search query"]}},{"status":{"operator":"!","values":["14"]}}]', + 'sortBy' => '[["updatedAt", "desc"]]', + ] + ] + ) + ->willReturnOnConsecutiveCalls( + ["_embedded" => ["elements" => [['id' => 1], ['id' => 2], ['id' => 3]]]], + ["_embedded" => ["elements" => [['id' => 4], ['id' => 5], ['id' => 6]]]] + ); + $result = $service->searchWorkPackage( + 'url', 'token', 'refresh', 'id', 'secret', 'user', 'search query', 123 + ); + $this->assertSame([['id' => 1], ['id' => 2], ['id' => 3], ['id' => 4], ['id' => 5], ['id' => 6]], $result); + } + + /** + * @return void + */ + public function testSearchWorkPackageRequestProblem() { + $service = $this->getServiceMock(); + $service->method('request') + ->willReturn(['error' => 'some issue', 'statusCode' => 404 ]); + $result = $service->searchWorkPackage( + 'url', 'token', 'refresh', 'id', 'secret', 'user', 'search query', 123 + ); + $this->assertSame(['error' => 'some issue', 'statusCode' => 404 ], $result); + } + + + /** + * @return void + */ + public function testSearchWorkPackageSecondRequestProblem() { + $service = $this->getServiceMock(); + $service->method('request') + ->willReturnOnConsecutiveCalls( + ["_embedded" => ["elements" => [['id' => 1], ['id' => 2], ['id' => 3]]]], + ['error' => 'some issue', 'statusCode' => 404 ] + ); + $result = $service->searchWorkPackage( + 'url', 'token', 'refresh', 'id', 'secret', 'user', 'search query', 123 + ); + $this->assertSame(['error' => 'some issue', 'statusCode' => 404 ], $result); + } + /** * @return void */ @@ -428,6 +518,40 @@ public function testRequestRefreshOAuthToken() { $this->assertSame(["_embedded" => ["elements" => [['id' => 1], ['id' => 2]]]], $result); } + /** + * @return void + */ + public function testRequestToNotExistingPath() { + $consumerRequest = new ConsumerRequest(); + $consumerRequest + ->setMethod('GET') + ->setPath('/api/v3/not_existing'); + + $providerResponse = new ProviderResponse(); + $providerResponse + ->setStatus(404); + + $this->builder + ->uponReceiving('an GET request to /api/v3/not_existing') + ->with($consumerRequest) + ->willRespondWith($providerResponse); + + $result = $this->service->request( + $this->mockServerBaseUri, + '1234567890', + '', + $this->clientId, + $this->clientSecret, + 'admin', + 'not_existing' + ); + $this->assertSame([ + 'error' => 'Client error: `GET http://localhost:7200/api/v3/not_existing` ' . + 'resulted in a `404 Not Found` response', + 'statusCode' => 404 + ], $result); + } + /** * @return void */ @@ -647,4 +771,88 @@ public function testGetOpenProjectOauthURL() { $result ); } + + /** + * @return array + */ + public function connectExpectionDataProvider() { + $requestMock = $this->getMockBuilder('\Psr\Http\Message\RequestInterface')->getMock(); + $responseMock402 = $this->getMockBuilder('\Psr\Http\Message\ResponseInterface')->getMock(); + $responseMock402->method('getStatusCode')->willReturn(402); + $responseMock403 = $this->getMockBuilder('\Psr\Http\Message\ResponseInterface')->getMock(); + $responseMock403->method('getStatusCode')->willReturn(403); + $responseMock500 = $this->getMockBuilder('\Psr\Http\Message\ResponseInterface')->getMock(); + $responseMock500->method('getStatusCode')->willReturn(500); + $responseMock501 = $this->getMockBuilder('\Psr\Http\Message\ResponseInterface')->getMock(); + $responseMock501->method('getStatusCode')->willReturn(501); + + return [ + [ + new ConnectException('a connection problem', $requestMock), + 404, + 'a connection problem' + ], + [ + new ClientException('some client problem', $requestMock, $responseMock403), + 403, + 'some client problem' + ], + [ + new ClientException('some client problem', $requestMock, $responseMock402), + 402, + 'some client problem' + ], + [ + new ServerException('some server issue', $requestMock, $responseMock501), + 501, + 'some server issue' + ], + [ + new BadResponseException('some issue', $requestMock, $responseMock500), + 500, + 'some issue' + ], + [ + new \Exception('some issue'), + 500, + 'some issue' + ], + + ]; + } + + /** + * @return void + * @param \Exception $exception + * @param int $expectedHttpStatusCode + * @param string $expectedError + * @dataProvider connectExpectionDataProvider + * + */ + public function testRequestException( + $exception, $expectedHttpStatusCode, $expectedError + ) { + $certificateManager = $this->getMockBuilder('\OCP\ICertificateManager')->getMock(); + $certificateManager->method('getAbsoluteBundlePath')->willReturn('/'); + + $ocClient = $this->getMockBuilder('\OCP\Http\Client\IClient')->getMock(); + $ocClient->method('get')->willThrowException($exception); + $clientService = $this->getMockBuilder('\OCP\Http\Client\IClientService')->getMock(); + $clientService->method('newClient')->willReturn($ocClient); + + $service = new OpenProjectAPIService( + 'integration_openproject', + $this->createMock(\OCP\IUserManager::class), + $this->createMock(\OCP\IAvatarManager::class), + $this->createMock(\Psr\Log\LoggerInterface::class), + $this->createMock(\OCP\IL10N::class), + $this->createMock(\OCP\IConfig::class), + $this->createMock(\OCP\Notification\IManager::class), + $clientService + ); + + $response = $service->request('', '', '', '', '', '', '', []); + $this->assertSame($expectedError, $response['error']); + $this->assertSame($expectedHttpStatusCode, $response['statusCode']); + } }