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

handle OPDS catalog responses accordingly #5241

Merged
merged 1 commit into from Aug 22, 2019

Conversation

@edoput
Copy link
Contributor

commented Aug 22, 2019

this will fix #5240 and provide some more feedback to users trying to use OPDS
catalogs

  • handle moved permanently HTTP 301
  • handle authentication required HTTP 401
  • handle authentication errors HTTP 403
  • handle catalog not found HTTP 404

@edoput edoput force-pushed the edoput:master branch from 5f1202b to 58a54ef Aug 22, 2019

@Frenzie
Copy link
Member

left a comment

Thanks! A couple of small comments.

@@ -298,6 +298,22 @@ function OPDSBrowser:fetchFeed(item_url, username, password, method)
if xml ~= "" then
return xml
end
elseif code == 301 then
UIManager:show(InfoMessage:new{
text = T(_("Catalog has been permanently moved. Update catalog URL to %1"), headers['Location']),

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 22, 2019

Member

This could be handled automatically, but for a quickie it'll do.

Suggested change
text = T(_("Catalog has been permanently moved. Update catalog URL to %1"), headers['Location']),
text = T(_("The catalog has been permanently moved. Please update the catalog URL to '%1'."), headers['Location']),

(Not 100 % sure on single quotes around the URL.)

But particularly as far as redirects go the backend should Just Work regardless whether there's an associated update of the stored URL. For HTTPS this is relatively recent in our code, perhaps that's why it doesn't happen here.

})
elseif code == 401 then
UIManager:show(InfoMessage:new{
text = T(_("Authentication required for catalog, please add username and password")),

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 22, 2019

Member
Suggested change
text = T(_("Authentication required for catalog, please add username and password")),
text = T(_("Authentication required for catalog. Please add a username and password.")),
})
elseif code == 403 then
UIManager:show(InfoMessage:new{
text = T(_("Could not authenticate, check your username and password")),

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 22, 2019

Member
Suggested change
text = T(_("Could not authenticate, check your username and password")),
text = T(_("Could not authenticate. Please check your username and password.")),

Or possibly Failed to authenticate to the first part. Not because it's better in and of itself, but because that phrase already exists in our translation memory.

})
elseif code == 404 then
UIManager:show(InfoMessage:new{
text = T(_("Catalog not found")),

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 22, 2019

Member
Suggested change
text = T(_("Catalog not found")),
text = T(_("Catalog not found.")),

@edoput edoput force-pushed the edoput:master branch from 58a54ef to 4d22979 Aug 22, 2019

@edoput

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Ok all suggestions accepted!

@Frenzie Frenzie added this to the 2019.09 milestone Aug 22, 2019

@Frenzie Frenzie added the bug label Aug 22, 2019

else
UIManager:show(InfoMessage:new{
text = T(_("Cannot get catalog. Server code response %1"), code),
text = T(_("Cannot get catalog. Server code response %1."), code),

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 22, 2019

Member
Suggested change
text = T(_("Cannot get catalog. Server code response %1."), code),
text = T(_("Cannot get catalog. Server response code %1."), code),

Sorry, I overlooked this one.

handle OPDS catalog responses accordingly
- handle moved permanently HTTP 301
- handle authentication required HTTP 401
- handle authentication errors HTTP 403
- handle catalog not found HTTP 404

@edoput edoput force-pushed the edoput:master branch from 4d22979 to b1eb3b8 Aug 22, 2019

@Frenzie Frenzie merged commit 0906b69 into koreader:master Aug 22, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details
@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.