Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error handling for catalog not available 503 #960

Merged

Conversation

BPerlakiH
Copy link
Collaborator

Fixes: #959

Solution:

Additional enum case for error, and re-use existing translation key for displaying the error.

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2024

Codecov Report

Attention: Patch coverage is 30.95238% with 29 lines in your changes missing coverage. Please review.

Project coverage is 38.51%. Comparing base (2fdfacb) to head (d27282f).

Files with missing lines Patch % Lines
Views/Settings/Settings.swift 0.00% 11 Missing ⚠️
Views/Library/ZimFilesCategories.swift 0.00% 10 Missing ⚠️
Views/Library/ZimFilesNew.swift 0.00% 5 Missing ⚠️
Views/BuildingBlocks/Message.swift 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #960      +/-   ##
==========================================
- Coverage   38.58%   38.51%   -0.07%     
==========================================
  Files         108      108              
  Lines        6223     6244      +21     
==========================================
+ Hits         2401     2405       +4     
- Misses       3822     3839      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BPerlakiH IMHO you should have a library URL.builder which should be totally disconnected from the libraryView... and then reuse it in your observer. Havin to hardcode URL wherever you need it is not a good sign.

@BPerlakiH
Copy link
Collaborator Author

BPerlakiH commented Sep 1, 2024

@BPerlakiH IMHO you should have a library URL.builder which should be totally disconnected from the libraryView... and then reuse it in your observer. Havin to hardcode URL wherever you need it is not a good sign.

I think it's not that bad as it seems. Formerly we had the url hard coded in a single place in the LibraryViewModel (which is not a view). The only change is that I moved it up higher to the top of the file, so it's easier to find. That's about it, when it comes to the production code. (I also updated the production url to be private for that class).

In tests, now I updated to use a mock url, and it works equally good with that as well. The responses are also mocks, which is the key part of those tests, the url can be anything really.

To sum up: we do not use the production URL in more than 1 place.

@kelson42 kelson42 merged commit 58bfb76 into main Sep 2, 2024
4 checks passed
@kelson42 kelson42 deleted the 959-show-user-error-when-libkiwixorg-is-not-available branch September 2, 2024 13:09
@kelson42 kelson42 modified the milestones: 3.6.0, 3.5.2 Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show user error when libkiwix.org is not available
3 participants