fix: report files that could not be downloaded when the import finishes - #144
Merged
Merged
Conversation
A file that failed to download was dropped silently: the import carried on, nothing recorded the loss, and the completion notification reported the number of files *seen* - so the failed file was even counted as imported. For a data migration tool, silently missing files is the worst failure mode. Count download failures in a user setting accumulated across import batches, the same way the imported-files counter works. When the import finishes, the notification now reports the number of files actually downloaded and, if any, the number of files that could not be downloaded. Their names are already in the server log at warning level. Notifications queued before an upgrade carry no failure count and keep rendering as before. Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
…ication Two notification improvements on top of the failed-files count: Keep the names of the first ten files that could not be downloaded and list them in the notification body, so users without access to the server logs know which files are missing. The full list stays in the log. Count files that were skipped because they already existed and say so in the notification. Re-running an import into the same folder used to end with "0 files were imported", which read as a failure; it now explains itself. Distinguishing downloaded, existing and failed files properly also fixes a quirk where an imported empty file was never counted as imported. Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
The notifier throws UnknownNotificationException since #149. It extends InvalidArgumentException, so these assertions kept passing after the merge, but they would also pass if the notifier went back to the deprecated exception. Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
IRootFolder::getUserFolder() declares an IUserFolder return type since Nextcloud 36, so PHPUnit refuses the plain Folder mock these two tests returned. IUserFolder does not exist on older versions, which keep the Folder mock. Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
oleksandr-nc
force-pushed
the
fix/139/report-failed-downloads
branch
2 times, most recently
from
September 23, 2026 10:49
7273403 to
96bf4ff
Compare
The integration test imported a few files and stopped, so it never reached what this branch changes: nothing exercised a run in which files are already there, and the skipped and failed counters were not even reported. The job now imports a second time over the files the first run brought and expects them to be counted as skipped, with no failed download on a healthy drive. The drive behind the secrets holds more than 500 MB, so importing it completely is out of reach for CI; the second run therefore starts while the first import is still in progress, after clearing the two flags that a user would see expire an hour later. Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
oleksandr-nc
force-pushed
the
fix/139/report-failed-downloads
branch
2 times, most recently
from
September 23, 2026 11:43
b7a7926 to
cd04943
Compare
The failure path of the import could only be checked with unit tests: a real drive cannot be made to fail a download on demand, cannot produce an empty file at will and, being one small account, never returns a second listing page. The notification this branch rewrites was never rendered in a test that had actually imported anything either. A stub of the Graph endpoints the import uses closes that gap. It serves a fixed drive with a file that downloads, an empty one, one that fails with the URL from the listing and succeeds with a freshly fetched one, one that fails both times, one that the test puts in the target folder beforehand, and a subfolder, with the first listing page carrying a next link. The app is pointed at it through a new app config value that defaults to Microsoft Graph, so nothing changes outside tests. The job then checks the files on disk, the requests the app made, the notification text and that the counters are reset, and it needs no secrets, so it also runs on pull requests that cannot see them. Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
Importing a second time over the files of the first run reported nothing on one leg: every counter was zero and the import was over after one cron run. `importFiles()` asks for the drive quota first and returns straight away when that request fails, which makes the job take its finish path, reset the counters and log nothing at all. A large real drive that was just hammered with downloads is the wrong place to depend on that request succeeding. The stubbed Graph job covers the same behaviour without the flakiness: it puts a file in the target folder beforehand and asserts that the app never asks to download it. Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
oleksandr-nc
force-pushed
the
fix/139/report-failed-downloads
branch
from
September 23, 2026 11:53
d29bf32 to
87decb3
Compare
This was referenced Sep 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #139
Final part, following #142 and #143: a file that failed to download even after the fresh-URL retry was dropped silently, and the completion notification counted it as imported — it reported
totalSeenNumber, which increments before the download is attempted.What changes
nb_failed_filesuser setting, accumulated across import batches the same waynb_imported_filesalready works, and reset at import start and finish. The names of the first ten failed files are kept as well.getFile()also fixes a pre-existing quirk where a successfully imported empty file was never counted as imported.With this, all four problems from #139 are addressed: the indefinite hang (#142), the token leak into logs (#142), the expired listing URL (#143), and unreported missing files (this PR).
Tests
composer cs:check, psalm and the suite (22 tests) pass locally on NC 33 / PHP 8.3. All notification variants were additionally rendered live on a real NC 35 instance through the OCS API to verify the actual L10N output.