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

515/better error handling #529

Merged
merged 36 commits into from
Apr 26, 2019
Merged

515/better error handling #529

merged 36 commits into from
Apr 26, 2019

Conversation

LeonMatthes
Copy link
Contributor

Closes #515

@LeonMatthes

This comment has been minimized.

@LeonMatthes LeonMatthes self-assigned this Mar 27, 2019
@LeonMatthes LeonMatthes added question Further information is requested WIP work is still in progess for this PR labels Mar 27, 2019
Fixes issue that caused logs not going to STDOUT due to some weird Meta-Programming-issues
@LeonMatthes
Copy link
Contributor Author

For some reason, the logger now seems to send all logs to the logfile, instead of STDOUT
I don't know why this is happening, but it's definitely not intended

This is now fixed by using a Formatter

@LeonMatthes LeonMatthes added Review Required and removed WIP work is still in progess for this PR question Further information is requested labels Mar 27, 2019
app/views/notifications/_list.html.erb Outdated Show resolved Hide resolved
config/environments/production.rb Outdated Show resolved Hide resolved
@LeonMatthes LeonMatthes requested a review from chrisma April 1, 2019 21:19
@chrisma
Copy link
Contributor

chrisma commented Apr 2, 2019

Thanks for the changes.
I could imagine that if an error occurs, it might occur multiple times in succession when using the application. This would result in the admin's notification list filling up with the same error multiple times.

It might be useful to prevent this behavior, by e.g.:

  • adding a counter to the notification (Occurred X times) when the notification is still unread,
  • or checking whether the last created notification is the same as the one being created currently and only updating the creation date of this last notification, instead of creating a new one

Copy link
Contributor

@chrisma chrisma left a comment

Choose a reason for hiding this comment

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

Nice changes! Another test for the error display might be helpful. (+ See my previous comment)

spec/helpers/hart_formatter_spec.rb Show resolved Hide resolved
@LeonMatthes
Copy link
Contributor Author

Thanks for the changes.
I could imagine that if an error occurs, it might occur multiple times in succession when using the application. This would result in the admin's notification list filling up with the same error multiple times.
It might be useful to prevent this behavior, by e.g.:

adding a counter to the notification (Occurred X times) when the notification is still unread,
or checking whether the last created notification is the same as the one being created currently and only updating the creation date of this last notification, instead of creating a new one

I opted for the counter version. I think its more transparent.
This now also works for all notifications, not just error notifications.

image

@LeonMatthes LeonMatthes requested a review from chrisma April 6, 2019 20:48
app/helpers/hart_formatter.rb Outdated Show resolved Hide resolved
app/models/notification.rb Outdated Show resolved Hide resolved
config/application.rb Outdated Show resolved Hide resolved
spec/models/user_spec.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved

it 'does create notifications if the duplicates are one minute apart' do
notify_user
new_time = Time.now + 61 # seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

This "magic number" (i.e. the 60 seconds in the model) might be better suited for the AppSettings.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Better error handling
3 participants