diff --git a/.github/workflows/integration-test.yml b/.github/workflows/integration-test.yml index f9e1f4f..a980681 100644 --- a/.github/workflows/integration-test.yml +++ b/.github/workflows/integration-test.yml @@ -228,6 +228,9 @@ jobs: # a request without an access token has to be refused, otherwise the test could pass # while the app stopped sending the token test "$(curl -s -o /dev/null -w '%{http_code}' http://127.0.0.1:8099/v1.0/me/drive)" = "401" + # start from a healthy drive whatever an earlier run left behind + curl -fs -X POST http://127.0.0.1:8099/control/repair-drive + echo - name: Run Nextcloud working-directory: server/ @@ -422,6 +425,52 @@ jobs: fi exit $fail + - name: Import with a drive that cannot be reached + working-directory: server/ + run: | + php occ user:setting admin ${{ env.APP_NAME }} onedrive_output_dir "/OneDrive stopped" + # drop what the previous imports sent, the one checked below has to be this import's + curl -s -X DELETE -u admin:admin -H 'OCS-APIRequest: true' \ + 'http://localhost:8080/ocs/v2.php/apps/notifications/api/v2/notifications' >/dev/null + curl -fs -X POST http://127.0.0.1:8099/control/break-drive + echo + # make sure the drive really is unreachable, otherwise the import would just work + test "$(curl -s -o /dev/null -w '%{http_code}' -H 'Authorization: bearer stub-access-token' http://127.0.0.1:8099/v1.0/me/drive)" = "403" + php occ ${{ env.APP_NAME }}:start-import admin + fail=0 + importing=1 + # a queued job is not always picked by the first cron run + for run in {1..4}; do + php cron.php + importing=$(php occ user:setting admin ${{ env.APP_NAME }} importing_onedrive 2>/dev/null | tail -1 || echo "1") + echo "round $run: importing=$importing" + if [ "$importing" = "0" ]; then break; fi + done + if [ "$importing" != "0" ]; then + echo "the import should have given up, importing_onedrive is '$importing'" + fail=1 + fi + notification=$(curl -s -u admin:admin -H 'OCS-APIRequest: true' -H 'Accept: application/json' \ + 'http://localhost:8080/ocs/v2.php/apps/notifications/api/v2/notifications') + expected='The import of your OneDrive files stopped before it was finished, check the server logs for details. | 0 files were imported from OneDrive storage.' + said=$(php -r 'foreach (json_decode(stream_get_contents(STDIN), true)["ocs"]["data"] as $n) { if (($n["app"] ?? "") === "integration_onedrive") { echo $n["subject"], " | ", $n["message"] ?? "", "\n"; } }' <<< "$notification") + echo "$said" + if [ "$(printf '%s\n' "$said" | grep -c .)" != "1" ]; then + echo "expected exactly one notification of this import" + fail=1 + fi + if [ "$said" != "$expected" ]; then + echo "expected: $expected" + fail=1 + fi + # the reason belongs in that line, it is what the notification sends the admin to + if ! grep -q 'import of admin stopped: .*accessDenied' data/nextcloud.log; then + echo "the log does not say why the import stopped" + grep 'import of admin stopped' data/nextcloud.log || true + fail=1 + fi + exit $fail + - name: Show the logs on failure working-directory: server/ if: failure() diff --git a/CHANGELOG.md b/CHANGELOG.md index bc6470d..c3e4428 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Stop counting the files an import downloaded itself among the files that were already there - Try a folder whose listing failed again in a later batch instead of leaving it out of the import - List a folder again from its first page when the page an import stopped in cannot be listed any more +- Tell the user when an import stops before it has seen the whole drive, it used to end in silence, and write the reason to the log +- Say nothing about an import the user cancelled while it was running, it used to report itself as finished - Test the import against a stubbed Graph API, including failed downloads, empty files and paging ## [3.5.2] - 2026-07-28 diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index 8ea772f..b242446 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -8,6 +8,7 @@ namespace OCA\Onedrive\Notification; use OCA\Onedrive\AppInfo\Application; +use OCP\IL10N; use OCP\IURLGenerator; use OCP\IUserManager; use OCP\L10N\IFactory; @@ -65,6 +66,28 @@ public function getName(): string { return $this->factory->get('integration_onedrive')->t('OneDrive'); } + /** + * The sentences an import adds about the files it did not bring, if there were any. + */ + private function whatElseHappened(IL10N $l, int $nbSkipped, int $nbFailed): string { + $content = ''; + if ($nbSkipped > 0) { + $content .= ' ' . $l->n( + '%n file was already there.', + '%n files were already there.', + $nbSkipped + ); + } + if ($nbFailed > 0) { + $content .= ' ' . $l->n( + '%n file could not be downloaded, check the server logs for details.', + '%n files could not be downloaded, check the server logs for details.', + $nbFailed + ); + } + return $content; + } + /** * @param INotification $notification * @param string $languageCode The code of the language that should be used to prepare the notification @@ -89,21 +112,8 @@ public function prepare(INotification $notification, string $languageCode): INot $nbSkipped = (int)($p['nbSkipped'] ?? 0); $failedFiles = is_array($p['failedFiles'] ?? null) ? $p['failedFiles'] : []; $targetPath = $p['targetPath']; - $content = $l->n('%n file was imported from OneDrive storage.', '%n files were imported from OneDrive storage.', $nbImported); - if ($nbSkipped > 0) { - $content .= ' ' . $l->n( - '%n file was already there.', - '%n files were already there.', - $nbSkipped - ); - } - if ($nbFailed > 0) { - $content .= ' ' . $l->n( - '%n file could not be downloaded, check the server logs for details.', - '%n files could not be downloaded, check the server logs for details.', - $nbFailed - ); - } + $content = $l->n('%n file was imported from OneDrive storage.', '%n files were imported from OneDrive storage.', $nbImported) + . $this->whatElseHappened($l, $nbSkipped, $nbFailed); if ($failedFiles !== []) { $names = implode(', ', $failedFiles); @@ -118,6 +128,22 @@ public function prepare(INotification $notification, string $languageCode): INot ->setIcon($this->url->getAbsoluteURL($this->url->imagePath(Application::APP_ID, 'app-dark.svg'))) ->setLink($this->url->linkToRouteAbsolute('files.view.index', ['dir' => $targetPath])); return $notification; + case 'import_onedrive_stopped': + /** @var array{nbImported?: string, nbFailed?: string, nbSkipped?: string, targetPath: string} $p */ + $p = $notification->getSubjectParameters(); + $nbImported = (int)($p['nbImported'] ?? 0); + $nbFailed = (int)($p['nbFailed'] ?? 0); + $nbSkipped = (int)($p['nbSkipped'] ?? 0); + $targetPath = $p['targetPath']; + $notification + ->setParsedSubject($l->t('The import of your OneDrive files stopped before it was finished, check the server logs for details.')) + ->setParsedMessage( + $l->n('%n file was imported from OneDrive storage.', '%n files were imported from OneDrive storage.', $nbImported) + . $this->whatElseHappened($l, $nbSkipped, $nbFailed) + ) + ->setIcon($this->url->getAbsoluteURL($this->url->imagePath(Application::APP_ID, 'app-dark.svg'))) + ->setLink($this->url->linkToRouteAbsolute('files.view.index', ['dir' => $targetPath])); + return $notification; default: // Unknown subject => Unknown notification => throw throw new UnknownNotificationException(); diff --git a/lib/Service/OnedriveAPIService.php b/lib/Service/OnedriveAPIService.php index dac92f1..ea5ae47 100644 --- a/lib/Service/OnedriveAPIService.php +++ b/lib/Service/OnedriveAPIService.php @@ -233,8 +233,10 @@ public function request(string $userId, string $endPoint, array $params = [], st } } } catch (ServerException|ClientException $e) { - $this->logger->warning('OneDrive API error : ' . $e->getResponse()->getBody(), ['app' => Application::APP_ID]); - return ['error' => $e->getResponse()->getBody()]; + // a string, so that whoever reports this error does not have to read a stream + $body = (string)$e->getResponse()->getBody(); + $this->logger->warning('OneDrive API error : ' . $body, ['app' => Application::APP_ID]); + return ['error' => $body]; } catch (ConnectException $e) { $this->logger->warning('OneDrive API connection error : ' . $e->getMessage(), ['app' => Application::APP_ID]); return ['error' => $e->getMessage()]; diff --git a/lib/Service/OnedriveStorageAPIService.php b/lib/Service/OnedriveStorageAPIService.php index 7aadc76..c65a1ba 100644 --- a/lib/Service/OnedriveStorageAPIService.php +++ b/lib/Service/OnedriveStorageAPIService.php @@ -215,11 +215,15 @@ public function importOnedriveJob(string $userId): void { try { $result = $this->importFiles($userId, $targetPath, $batchSize, $alreadyImportedSize, $alreadyImportedNumber, $importTree); } catch (Exception|Throwable $e) { + $this->logger->error('OneDrive import job failed: ' . $e->getMessage(), ['app' => Application::APP_ID, 'exception' => $e]); $result = [ - 'error' => 'Unknow job failure. ' . $e->getMessage(), + 'error' => $e->getMessage(), ]; } if (isset($result['error']) || (isset($result['finished']) && $result['finished'])) { + // the settings page cancels an import by clearing this while a batch runs: the + // user knows it is over and does not need to hear about it + $cancelled = $this->config->getUserValue($userId, Application::APP_ID, 'importing_onedrive', '0') !== '1'; // read the counters accumulated over all batches before resetting them $nbImported = (int)$this->config->getUserValue($userId, Application::APP_ID, 'nb_imported_files', '0'); $nbFailed = (int)$this->config->getUserValue($userId, Application::APP_ID, 'nb_failed_files', '0'); @@ -232,8 +236,10 @@ public function importOnedriveJob(string $userId): void { $this->config->setUserValue($userId, Application::APP_ID, 'nb_skipped_files', '0'); $this->config->deleteUserValue($userId, Application::APP_ID, 'failed_files'); $this->config->setUserValue($userId, Application::APP_ID, 'last_onedrive_import_timestamp', '0'); - if (isset($result['finished']) && $result['finished']) { - $this->config->deleteUserValue($userId, Application::APP_ID, 'import_tree'); + $this->config->deleteUserValue($userId, Application::APP_ID, 'import_tree'); + if ($cancelled) { + $this->logger->info('The OneDrive import of ' . $userId . ' was cancelled', ['app' => Application::APP_ID]); + } elseif (isset($result['finished']) && $result['finished']) { $this->onedriveApiService->sendNCNotification($userId, 'import_onedrive_finished', [ 'nbImported' => $nbImported, 'nbFailed' => $nbFailed, @@ -241,6 +247,16 @@ public function importOnedriveJob(string $userId): void { 'failedFiles' => $failedFiles, 'targetPath' => $targetPath, ]); + } else { + // the import ends here without having seen the whole drive, say so instead + // of leaving the user with an import that stopped for no visible reason + $this->logger->error('OneDrive import of ' . $userId . ' stopped: ' . $result['error'], ['app' => Application::APP_ID]); + $this->onedriveApiService->sendNCNotification($userId, 'import_onedrive_stopped', [ + 'nbImported' => $nbImported, + 'nbFailed' => $nbFailed, + 'nbSkipped' => $nbSkipped, + 'targetPath' => $targetPath, + ]); } } else { // save progress diff --git a/tests/integration/graph-stub.php b/tests/integration/graph-stub.php index f60989d..93c87f0 100644 --- a/tests/integration/graph-stub.php +++ b/tests/integration/graph-stub.php @@ -23,6 +23,9 @@ * already.txt is never downloaded, the test puts it in the target folder beforehand * sub/ a folder, holding nested.txt, which downloads * + * A request to /control/break-drive makes the drive itself unreachable, which is how an + * import that stops before it is finished is produced. + * * The first listing page carries an @odata.nextLink, so paging is covered as well. */ @@ -73,6 +76,14 @@ function baseUrl(): string { return 'http://' . ($_SERVER['HTTP_HOST'] ?? '127.0.0.1:8099'); } +/** + * The file that says the drive has been made unreachable on purpose. It carries the port + * so that two stubs on one machine, or a later run, do not inherit it. + */ +function brokenDriveFlag(): string { + return sys_get_temp_dir() . '/graph-stub-broken-drive-' . ($_SERVER['SERVER_PORT'] ?? 'x'); +} + function respond(array $body, int $status = 200): void { http_response_code($status); header('Content-Type: application/json'); @@ -113,6 +124,18 @@ function serveDownload(string $id): void { $path = parse_url($_SERVER['REQUEST_URI'] ?? '/', PHP_URL_PATH) ?: '/'; $path = rawurldecode($path); +// the test asks for the drive to be broken and repaired, no access token involved +if ($path === '/control/break-drive') { + touch(brokenDriveFlag()); + respond(['broken' => true]); + return; +} +if ($path === '/control/repair-drive') { + @unlink(brokenDriveFlag()); + respond(['broken' => false]); + return; +} + // downloads carry their own authorisation in the URL, everything else needs the access token if (!str_starts_with($path, '/download/') && !preg_match('/^bearer .+/i', $_SERVER['HTTP_AUTHORIZATION'] ?? '')) { respond(['error' => ['code' => 'unauthenticated', 'message' => 'no access token']], 401); @@ -125,6 +148,11 @@ function serveDownload(string $id): void { } if ($path === '/v1.0/me/drive') { + if (file_exists(brokenDriveFlag())) { + // what a revoked consent looks like: the import cannot even read the drive + respond(['error' => ['code' => 'accessDenied', 'message' => 'the drive is not yours any more']], 403); + return; + } respond(['id' => 'stub-drive', 'quota' => ['total' => 1073741824, 'used' => 42, 'remaining' => 1073741782]]); return; } diff --git a/tests/unit/Notification/NotifierTest.php b/tests/unit/Notification/NotifierTest.php index fe712d5..f76f2f6 100644 --- a/tests/unit/Notification/NotifierTest.php +++ b/tests/unit/Notification/NotifierTest.php @@ -141,4 +141,23 @@ public function testThrowsForUnknownSubject(): void { $this->expectException(UnknownNotificationException::class); $this->notifier->prepare($notification, 'en'); } + + public function testStoppedImportSaysWhatItGotDone(): void { + $this->prepare( + ['nbImported' => 12, 'nbFailed' => 0, 'nbSkipped' => 0, 'targetPath' => '/OneDrive import'], + 'The import of your OneDrive files stopped before it was finished, check the server logs for details.', + '12 files were imported from OneDrive storage.', + 'import_onedrive_stopped' + ); + } + + public function testStoppedImportCountsWhatItSkippedAndCouldNotDownload(): void { + $this->prepare( + ['nbImported' => 1, 'nbFailed' => 2, 'nbSkipped' => 3, 'targetPath' => '/OneDrive import'], + 'The import of your OneDrive files stopped before it was finished, check the server logs for details.', + '1 file was imported from OneDrive storage. 3 files were already there.' + . ' 2 files could not be downloaded, check the server logs for details.', + 'import_onedrive_stopped' + ); + } } diff --git a/tests/unit/Service/OnedriveStorageAPIServiceTest.php b/tests/unit/Service/OnedriveStorageAPIServiceTest.php index 9669b9b..df5f82f 100644 --- a/tests/unit/Service/OnedriveStorageAPIServiceTest.php +++ b/tests/unit/Service/OnedriveStorageAPIServiceTest.php @@ -723,4 +723,120 @@ public function testTheImportIsResumedFromWhatTheBatchWroteToTheConfig(): void { ); $this->assertArrayNotHasKey('import_tree', $this->configStore); } + + public function testAnImportThatCannotReachTheDriveSaysSo(): void { + $this->useStatefulConfig([ + 'importing_onedrive' => '1', + 'nb_imported_files' => '5', + 'nb_skipped_files' => '2', + ]); + $this->apiService->method('request')->willReturnCallback( + static fn (string $userId, string $endPoint) => ['error' => 'Too many requests'] + ); + $folder = $this->createUserFolderMock(); + $folder->method('nodeExists')->willReturn(true); + $folder->method('get')->willReturnSelf(); + $folder->method('isShared')->willReturn(false); + $this->rootFolder->method('getUserFolder')->willReturn($folder); + $this->logger->expects($this->once()) + ->method('error') + ->with($this->stringContains('Too many requests'), ['app' => 'integration_onedrive']); + $this->apiService->expects($this->once()) + ->method('sendNCNotification') + ->with('user1', 'import_onedrive_stopped', [ + 'nbImported' => 5, + 'nbFailed' => 0, + 'nbSkipped' => 2, + 'targetPath' => '/OneDrive import', + ]); + $this->jobList->expects($this->never())->method('add'); + + $this->service->importOnedriveJob('user1'); + + $this->assertSame('0', $this->configStore['importing_onedrive'], 'the import is over'); + $this->assertSame('0', $this->configStore['nb_imported_files']); + } + + public function testAJobThatThrowsIsLoggedWithItsReason(): void { + $this->useStatefulConfig(['importing_onedrive' => '1']); + $errors = []; + $this->logger->method('error')->willReturnCallback( + static function (string $message) use (&$errors): void { + $errors[] = $message; + } + ); + // the date of the file cannot be parsed, which throws inside the import + $this->apiService->method('request')->willReturnCallback( + static function (string $userId, string $endPoint) { + if ($endPoint === 'me/drive') { + return ['quota' => ['used' => 1000]]; + } + if ($endPoint === 'me/drive/root/children') { + return ['value' => [[ + 'name' => 'photo.jpg', + 'id' => 'id-photo', + 'file' => [], + 'lastModifiedDateTime' => 'the day before yesterday', + '@microsoft.graph.downloadUrl' => 'https://dl.example.org/photo.jpg', + ]]]; + } + return ['lastModifiedDateTime' => '2026-09-01T10:00:00Z']; + } + ); + $this->apiService->method('fileRequest')->willReturn(['success' => true]); + $file = $this->createMock(File::class); + $file->method('fopen')->willReturnCallback(static fn () => fopen('php://temp', 'w+')); + $file->method('stat')->willReturn(['size' => 10]); + $dirFolder = $this->createMock(Folder::class); + $dirFolder->method('nodeExists')->willReturn(false); + $dirFolder->method('newFile')->willReturn($file); + $topFolder = $this->createMock(Folder::class); + $topFolder->method('isShared')->willReturn(false); + $topFolder->method('nodeExists')->willReturn(true); + $topFolder->method('get')->willReturn($dirFolder); + $userFolder = $this->createUserFolderMock(); + $userFolder->method('nodeExists')->willReturn(true); + $userFolder->method('get')->willReturn($topFolder); + $this->rootFolder->method('getUserFolder')->willReturn($userFolder); + $this->apiService->expects($this->once()) + ->method('sendNCNotification') + ->with('user1', 'import_onedrive_stopped', $this->anything()); + + $this->service->importOnedriveJob('user1'); + + $this->assertNotEmpty( + array_filter($errors, static fn (string $m) => str_contains($m, 'import job failed')), + 'the exception was logged: ' . implode(' / ', $errors) + ); + $this->assertSame('0', $this->configStore['importing_onedrive']); + } + + public function testAnImportCancelledWhileItRunsIsNotReported(): void { + $this->useStatefulConfig(['importing_onedrive' => '1']); + // the settings page clears the flag while the batch is walking the drive + $this->apiService->method('request')->willReturnCallback( + function (string $userId, string $endPoint) { + if ($endPoint === 'me/drive') { + return ['quota' => ['used' => 1000]]; + } + if ($endPoint === 'me/drive/root/children') { + $this->configStore['importing_onedrive'] = '0'; + return ['value' => []]; + } + return ['lastModifiedDateTime' => '2026-09-01T10:00:00Z']; + } + ); + $folder = $this->createUserFolderMock(); + $folder->method('nodeExists')->willReturn(true); + $folder->method('get')->willReturnSelf(); + $folder->method('isShared')->willReturn(false); + $this->rootFolder->method('getUserFolder')->willReturn($folder); + $this->apiService->expects($this->never())->method('sendNCNotification'); + $this->jobList->expects($this->never())->method('add'); + + $this->service->importOnedriveJob('user1'); + + $this->assertSame('0', $this->configStore['importing_onedrive']); + $this->assertArrayNotHasKey('import_tree', $this->configStore); + } }