Skip to content
This repository has been archived by the owner on Mar 29, 2022. It is now read-only.

Handle Github Errors Appropriately #409

Closed
mkcode opened this issue Nov 6, 2019 · 3 comments · May be fixed by #431
Closed

Handle Github Errors Appropriately #409

mkcode opened this issue Nov 6, 2019 · 3 comments · May be fixed by #431
Assignees
Labels
feature Accepted feature that needs to be built
Projects

Comments

@mkcode
Copy link
Contributor

mkcode commented Nov 6, 2019

Let's add a service to our app GithubErrorHandler.

It will have one method on it, with_error_handling which takes a block argument. We can call use it in our jobs to catch common errors.

This file can serve as a basic template: https://github.com/education/classroom/blob/master/lib/github/errors.rb. The main pieces that we want to copy are the with_error_handling method body and the big case statement that switches on errors. Though rather the just raising a different error, we will some appropriate action on the user.

Let's catch the following errors:

Let's add a new states to our state machine called dead. Also, let's add 1 new column to our user model, last_error, which will persist the last error.

In the ErrorHandler, let's have Octokit::AccountSuspended and UserNotFoundOnGithubError set the last_error column and move the user into the dead state. All other errors in the Handler that are not explicitly handled should be reraised.

We should also add code to not the TryUserTransition Job (and perhaps a few others) to not include Users in the dead state. We may want to use a default_scope to accomplish this, so long as it does not interfere with the state machine.

We may want to consider adding an error state later, or some additional way to handle our Ockokit::Unauthorized errors, but not in the scope of this PR.

@mkcode
Copy link
Contributor Author

mkcode commented Nov 6, 2019

@johndbritton - Would like your confirmation on this that we expect suspended github accounts to remain that way.

@jhaff jhaff self-assigned this Nov 6, 2019
@jhaff jhaff mentioned this issue Nov 6, 2019
9 tasks
@jhaff
Copy link
Contributor

jhaff commented Nov 7, 2019

Two updates to this:

  1. We're going to rename the proposed dead state to inactive, and the event to transition to this state will be called inactivate

  2. The error handler will be renamed to UserErrorHandler and take two arguments: the user and the block. This way the user comes in wherever with_error_handling is called and does not need to be attached to the errors that the error handler deals with. The UserErrorHandler will remain a module

@jhaff jhaff mentioned this issue Nov 12, 2019
9 tasks
@johndbritton johndbritton added the feature Accepted feature that needs to be built label Nov 12, 2019
@MattIPv4 MattIPv4 added this to Triage in 2020 via automation Jul 24, 2020
@katjuell katjuell self-assigned this Jul 24, 2020
@MattIPv4 MattIPv4 moved this from Triage to P1 - Must do in 2020 Jul 24, 2020
@MattIPv4
Copy link
Member

Done

@MattIPv4 MattIPv4 moved this from P1 - Must do to Done in 2020 Sep 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Accepted feature that needs to be built
Projects
2020
  
Done
Development

Successfully merging a pull request may close this issue.

5 participants