Conversation
…ad of waiting up to 12h CapabilitiesWorker always returned Result.success() even when the capabilities fetch failed, so a single-user fetch (e.g. right after login) that failed was never retried until the next periodic run, leaving user.capabilities null in the meantime. A response with a null capabilities payload was also silently swallowed without posting a failure event. Now a targeted single-user fetch returns Result.retry() on failure, and AccountVerificationActivity enqueues it as unique work with exponential backoff and a network constraint so it retries automatically once connectivity is back. Bulk (periodic/app-start chain) runs keep returning success to avoid blocking dependent work. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Covers the success path plus every failure branch, including the previously-silent case where the capabilities response has no ocs/data/ capabilities payload (now posts a CAPABILITIES_FETCH failure event instead of doing nothing). updateUser() is made package-visible (@VisibleForTesting) so it can be called directly, bypassing doWork()'s Dagger field-injection. The Worker instance itself is built via Robolectric + work-testing's TestListenableWorkerBuilder, since Worker's constructor still requires a real Context/WorkerParameters pair. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…es and split by retry contract CapabilitiesWorker (Java, RxJava2) handled both a best-effort refresh across every account and a single-account fetch that should retry with backoff, picking its Result semantics based on whether an internal user id happened to be present in the input data. That implicit branching made the class's actual contract hard to read at a glance. Split into: - CapabilitiesFetcher: the actual fetch/persist logic (suspend, coroutines, a new NcApiCoroutines.getCapabilities endpoint), shared by both workers and testable directly without WorkManager/Robolectric scaffolding. - CapabilitiesSyncWorker: best-effort refresh for every stored account, used by the app-start chain, the periodic sync, and the manual Settings refresh. Always returns Result.success() so it never blocks dependent work. - CapabilitiesFetchWorker: single-account fetch used right after login, returns Result.retry() on failure so WorkManager retries with the backoff/constraints the caller already configures. If the target account no longer exists it now fails outright instead of silently falling back to syncing every account, since that fallback only existed to paper over the old shared code path. CapabilitiesFetcherTest replaces CapabilitiesWorkerTest, testing the plain CapabilitiesFetcher class directly with mocks instead of building a Worker via Robolectric/work-testing, which is no longer needed. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…apabilities refresh fails storeProfile() already persists the capabilities fetched during findServerTalkApp()'s hasTalk check when it creates the account. The CAPABILITIES_FETCH step that follows right after is a redundant re-fetch of the same data, but a failure there called abortVerification(), which deletes the account that was just correctly created - turning a transient network hiccup on a throwaway re-check into full data loss. Treat a failed capabilities refresh here the same as the sibling PUSH_REGISTRATION/SIGNALING_SETTINGS failures: report it and continue the login instead of aborting. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…round-tripping through EventBus PROFILE_STORED and PUSH_REGISTRATION were posted to the global EventBus just so AccountVerificationActivity's own onMessageEvent could react by calling the next step - neither event ever crossed into a Worker, so the bus added nothing but indirection and forced onMessageEvent to filter out unrelated accounts' events (internalAccountId != eventStatus.userId). storeProfile() now calls fetchAndStoreCapabilities() directly, and the push-registration paths call a new onPushRegistrationFinished() directly. CAPABILITIES_FETCH and SIGNALING_SETTINGS stay on EventBus since those are genuinely posted from a background Worker - switching those to WorkInfo observation was considered but would regress behavior (CapabilitiesFetchWorker retries with backoff so its WorkInfo never reaches a terminal state promptly, and SignalingSettingsWorker attaches no outputData to convey success/failure). PROFILE_STORED is now unused anywhere and is removed from EventStatus.EventType. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…o coroutines Replaces the Observer/MaybeObserver callback chains for server status, capabilities, profile fetch/store, push registration, and account activation/deletion with sequential suspend functions launched via lifecycleScope. UserManager's RxJava calls are bridged with kotlinx-coroutines-rx2's await()/awaitSingle(), wrapped in withContext(Dispatchers.IO) since UserManager doesn't specify its own scheduler - the original code relied on the caller subscribing on IO or already running on a background EventBus thread. Adds getServerStatus() and getUserProfile() to NcApiCoroutines so the Retrofit calls in this flow can be suspend directly; Retrofit's own coroutine support means no explicit dispatcher wrapping is needed for those. Removes the disposables/dispose()/onDestroy() bookkeeping entirely, since lifecycleScope coroutines are cancelled automatically when the Activity is destroyed. Also drops the runOnUiThread wrapping throughout, since each suspend function now defaults back to Dispatchers.Main after any IO hop. WorkManager/LiveData usage and the EventBus-based CAPABILITIES_FETCH/ SIGNALING_SETTINGS handling are untouched - neither is RxJava. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Extracts repeated patterns into small helpers: getUser()/getAllUsers() for
the scattered withContext(Dispatchers.IO) { userManager... } blocks,
appendProgressMessage() for the repeated progress-text-append blocks,
reportServerWithoutTalk() for a byte-for-byte duplicated pair of branches
in findServerTalkApp(), and navigateToServerSelectionAfterDelay() for a
duplicated pair in abortVerification(). Also simplifies the scheme
handling in determineBaseUrlProtocol() and adds a class-level KDoc
summarizing the verification pipeline.
Simplifies findServerTalkApp()'s hasTalk check from four repeated !!
force-unwrap chains to one safe-call chain - this also means a null ocs/
data now falls through to "no Talk" and cleanly aborts instead of
crashing, a small but deliberate behavior improvement alongside the
readability cleanup.
Assisted-by: Claude Code:claude-sonnet-5
Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…count verification storeProfile() already embeds the capabilities fetched moments earlier by findServerTalkApp() into the new account (added in b6e9c9d, "Use already fetched capabilities for user", to fix a null-capabilities crash right after signup). The separate CapabilitiesFetchWorker step that followed was the account's *original* sole source of capabilities before that fix, and was left in place instead of being removed - it duplicated a request to the same endpoint that had just succeeded, and its own failure was already made non-fatal, since the account's capabilities were never actually at risk. storeProfile() now calls setupPushNotifications() directly. Removed the CAPABILITIES_FETCH step, its EventBus case, CapabilitiesFetchWorker (its only caller), and related now-dead code/imports. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
mahibi
changed the title
mahibi
changed the title
Contributor
📱 QA build
The QA build installs alongside a released Nextcloud app, so you can keep Downloading the file requires a GitHub account, so open this link on the |
AndyScherzinger
approved these changes
Sep 20, 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
2 participants
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.
Investigates and fixes the various ways
Capabilitiescould end up null/stale for a user, and cleans up the account-verification flowalong the way.
Capabilities fetching (
CapabilitiesWorker→ split)CapabilitiesWorkeralways returnedResult.success()even when the fetch failed, so a failed single-account fetch (e.g. rightafter login) silently waited up to 12h for the next periodic run before retrying. It also silently swallowed a response with no
ocs/data/capabilitiespayload — no retry, no error.CapabilitiesSyncWorker: best-effort refresh across every account (app-start chain, 12h periodic sync, manual Settings refresh) —always succeeds, never blocks dependent work.
CapabilitiesFetcher: the shared fetch/persist logic, kept as a plain class so it's testable with mocks, noWorkManager/Robolectric needed.
CapabilitiesFetcherTestcovering the success path and every failure branch, including the previously-silent one.Account verification (
AccountVerificationActivity)and a transient failure of that throwaway re-check called
abortVerification(), which deletes the account — even though itscapabilities were already correctly stored one step earlier. Fixed by treating that failure as non-fatal, like the sibling
push-registration/signaling-settings steps already do.
storeProfile()has embedded the already-fetched capabilities directly since Oct 2022(
b6e9c9d56, a fix for a different null-capabilities crash), which made the entire separate capabilities re-fetch step redundant — itwas left in place instead of being removed. Removed it outright:
storeProfile()now proceeds straight to push-notification setup, andCapabilitiesFetchWorker(its only caller) is deleted.EventBus-basedPROFILE_STORED/PUSH_REGISTRATIONself-signaling with direct calls — neither ever crossed into aWorker, so routing them through a global bus only added indirection and forced manual filtering of unrelated accounts' events.
CAPABILITIES_FETCH(now removed)/SIGNALING_SETTINGSstayed onEventBussince those are genuinely posted from a backgroundWorker.AccountVerificationActivity's RxJava (Observer/MaybeObservercallback chains) to coroutines/lifecycleScope,removing the manual
Disposablebookkeeping andrunOnUiThreadwrapping in the process.getUser()/getAllUsers(),appendProgressMessage(),reportServerWithoutTalk(),navigateToServerSelectionAfterDelay()), simplified a fragile!!-chain into one safe-call expression,added a class-level overview doc.
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)