Tell the user when a project cannot be loaded - #100
Open
Pascal-SAPUI5 wants to merge 1 commit into
Open
Conversation
Changing a project's password on the server left PayForMe showing empty lists and no explanation. The same happened for a deleted project, an unreachable server, or an address that is not a Cospend or iHateMoney instance at all. The cause was structural. Both load publishers declared `Never` as their failure type, so every failure had to become a value before it could leave NetworkService: an HTTP status other than 200 was dropped by a `compactMap`, and a URLError was turned into `[]` by `replaceError`. An empty list is exactly what a project with no bills looks like, so nothing upstream could tell the two apart — and nothing did. Both publishers now fail with a `LoadError`, ProjectManager keeps the last failure, and ContentView shows it. The message names what happened: a rejected password suggests adding the project again with the new one, which is the action the reporter needed and could not guess. The type is called LoadError rather than ServerError because NetworkService already nests a `ServerError` of its own, and a second one under the same name resolves differently inside and outside the class. The two existing tests that pinned the old behaviour are rewritten rather than deleted — one of them already carried the note "should [emit] when proper feedback is implemented". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Fixes #37.
Changing a project's password on the server left PayForMe showing empty lists
and no explanation. The same silence covered a deleted project, an unreachable
server, and an address that is not a Cospend or iHateMoney instance at all.
Cause
The cause is structural rather than a missing check. Both load publishers
declared
Neveras their failure type:That is a promise not to fail, so every failure had to be turned into a value
before it could leave
NetworkService. An HTTP status other than 200 wasdropped by a
compactMapreturningnil, and aURLErrorbecame[]viareplaceError. An empty list is exactly what a project with no bills lookslike, so nothing upstream could tell a rejected password from an empty project
— and nothing did.
Publishers.ZipinProjectManagerthen simply neveremitted, and the UI kept whatever it had.
Change
Both publishers now fail with a
LoadError:ProjectManagerkeeps the last failure in@Published var loadingError,cleared by any load that works, and
ContentViewshows it. The message for arejected password names the fix — add the project again with the new password —
because that is the step the reporter needed and could not guess.
On the existing tests
Two tests pinned the old behaviour and are rewritten rather than deleted. One
of them already carried the note:
A further sixteen
sink { _ in … }calls had to become the two-closure form:sink(receiveValue:)only exists forFailure == Never. That compiler errorlist is a fair map of everything that could previously swallow a failure
unnoticed.
Changes
PayForMe/Services/NetworkService.swift—LoadError,validate(_:_:),both load publishers
PayForMe/Services/ProjectManager.swift— keep and clear the last failurePayForMe/Views/ContentView.swift— show it, and map each case to a messagePayForMe/Strings/*— six messages in all six localisationsPayForMeTests/LoadErrorTests.swift— newPayForMeTests/NetworkRequestTests.swift,TestHelpers.swift— adjustedVerifying
Seven new tests in
LoadErrorTests:testRejectedCredentialsReachTheCaller— the case from the issue: 401 arrivesas
.unauthorizedinstead of vanishingtestForbiddenCountsAsRejectedCredentials,testMembersFailTheSameWaytestAnUnexpectedStatusIsPassedThroughVerbatim— 500 stays 500testAnUnreadableBodyIsAnInvalidResponse— a 200 full of HTMLtestStatusCodesMapToTheRightCasetestEveryErrorHasATranslatedMessage— every case has its own message andnone falls back to the raw key, which would otherwise ship
"load_error_unauthorized" to the screen
By hand: add a project, change its password in Cospend, pull to refresh. Before,
the lists stayed as they were with no hint; now the app says the password was
rejected and what to do about it.
On the name
The type is
LoadError, notServerError, becauseNetworkServicealreadynests a
ServerErrorof its own. A second one under the same name resolves tothe nested type inside the class and to the new one outside it, which the
compiler reports as "cannot assign value of type 'NetworkService.ServerError'
to type 'ServerError'".
Note
This also covers part of #53, where "Could not find server" is shown when it is
the project that could not be found —
.notFoundnow says so.The scope is deliberately the load path. The write operations
(
postBillPublisherand friends) still report success as aBooland wouldbenefit from the same treatment, but that is a separate change.