-
Notifications
You must be signed in to change notification settings - Fork 1
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 errors in fetching RSS feeds more gracefully #604
base: main
Are you sure you want to change the base?
Conversation
8bb0928
to
bac2602
Compare
--color-green: green; | ||
--color-grey: grey; | ||
--color-orange: orange; | ||
--color-red: red; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't even try, because I assume you will have a stronger opinion than me 😉
bac2602
to
3491868
Compare
3491868
to
650b595
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already looks good. I think my biggest issue is the way we show the status: I would try an icon rather than relying on color for meaning
def error=(error_text) | ||
self.latest_error = error_text | ||
self.error_count = if error_text.present? | ||
error_count + 1 | ||
else | ||
0 | ||
end | ||
end | ||
|
||
def reset_error | ||
self.error = nil | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since both of these methods are private, I think we can only use error=
if there is an error, making the code a lot easier:
def error=(error_text) | |
self.latest_error = error_text | |
self.error_count = if error_text.present? | |
error_count + 1 | |
else | |
0 | |
end | |
end | |
def reset_error | |
self.error = nil | |
end | |
def error=(error_text) | |
self.latest_error = error_text | |
self.error_count = error_count + 1 | |
end | |
def reset_error | |
self.latest_error = nil | |
self.error_count = 0 | |
end |
save! | ||
end | ||
|
||
def should_refresh? | ||
error_count < 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried that 5 might be too few if a server has some outage. Maybe we make this limit a bit higher?
EDIT: Wrote this before I realised that we don't yet handle server-errors here. If we don't than 5 is certainly enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might also be good to place this value in a const at the top of the model
rescue Net::HTTPClientException => e | ||
self.error = e.message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to handle server errors here as well? In RefreshRssFeedJob
I added a few errors that would be discarded, but I think it would be better to handle those here as well
We could also do that in a next PR
@@ -27,6 +27,9 @@ en: | |||
next: "Next" | |||
previous: "Previous" | |||
nav_pagination_label: "Entry pagination" | |||
rss_feed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rss_feed: | |
rss_feeds: |
This would be more consistent with the other keys
@@ -0,0 +1,22 @@ | |||
.subscription__status { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a __status
element, means that we must have a subscription
block somewhere that this element is a descendant of. Since this status lives on its own, it can just be .subscription-status
# frozen_string_literal: true | ||
|
||
module SubscriptionsHelper | ||
def subscription_status_info(subscription) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you create a component for this status (fe. SubscriptionStatusComponent
), we can place this helper logic and the view that uses it close together (and we can test it more easily)
<%= content_tag :td, class: 'table__cell' do %> | ||
<%- if subscription.refreshable? %> | ||
<%- status_info = subscription_status_info(subscription) %> | ||
<i class="subscription__status <%= status_info[:class] %>" aria-label="<%= status_info[:label] %>" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fully convinced that colored dots are the right solution here. While we cover screenreaders with the aria-label, the color itself might be unclear to a user (let alone someone with some form of color-blindness).
Maybe we can use icons to indicate the special statuses (warning, error and not yet fetched)? Since we don't show anything for newsletters it would be consistent to render nothing if everything is ok
Fixes #180.