Conversation
| end | ||
|
|
||
| def create_user() | ||
| return User.create!({ |
There was a problem hiding this comment.
Don't need to add this to the database - this slows down the test suite
| describe "#index" do | ||
| it "returns something" do | ||
| get :index | ||
| expect(response).not_to be_empty |
There was a problem hiding this comment.
This is a great test to start with, but it doesn't test real product behavior, so it needs to evolve a little.
| describe PullRequestsController do | ||
| before do | ||
| user = create_user | ||
| allow(controller).to receive(:current_user).and_return(user) |
|
|
||
| describe PullRequestsController do | ||
| before do | ||
| user = create_user |
There was a problem hiding this comment.
If we're using rspec, let will be ... More elegant.
| @@ -0,0 +1,100 @@ | |||
| desc 'Sync pull requests from Github' | |||
| task :sync => :environment do | |||
There was a problem hiding this comment.
In six months I think you'll find yourself typing rake sync and wondering which of twelve syncs we're running. I would recommend a slightly more descriptive name, maybe even a nested thing - rake sync:github:pr might be an extreme example.
| @@ -0,0 +1,100 @@ | |||
| desc 'Sync pull requests from Github' | |||
| task :sync => :environment do | |||
| class ResponseError < StandardError | |||
There was a problem hiding this comment.
A part of me is surprised this isn't defined somewhere else in the codebase already - these error names are very generic and high level, so we want one of two things here:
- either we make task-specific error names
- either we move these out so the whole codebase can use these as common errors
If you're not sure which of the two to do, then let me know and we can have a conversation about it - you'll be able to make the decision afterwards.
| @repository = repository | ||
| end | ||
|
|
||
| # These errors seem to happen a lot, so avoid sending them to Bugsnag |
There was a problem hiding this comment.
..... why do these errors happen a lot?
Errors are a way for the server to communicate with us, so if we're doing something wrong, we probably shouldn't ignore it. Here, it seems to indicate we're asking for too much too fast. We should look for a tool that helps us stay within the acceptable bounds as defined by Github's API terms of use.
| attr_reader :response, :repository | ||
| end | ||
|
|
||
| BadGatewayError = Class.new(ResponseError) |
There was a problem hiding this comment.
I like this! Simple, concise, and super-clear.
| ).new(response, message, repository) | ||
| end | ||
|
|
||
| def response |
There was a problem hiding this comment.
This name is too generic, and it hides the fact that this code is making the HTTP call. I would suggest something like current_pull_requests_on_github (or whatever makes more sense) so that the next person reading this code is tipped off to the idea that an HTTP call is happening.
|
|
||
| @pull_requests = @pull_requests.tagged_with(params[:tag]) if params[:tag].present? | ||
|
|
||
| @pull_requests = @pull_requests.order(created_at: :desc).offset(params[:offset] || 0).limit(params[:limit] || 20) |
There was a problem hiding this comment.
These three definitions of @pull_requests here look like they are business logic (which PRs do we want and how?) and that is usually best handled by a scope in the model, rather than defined in the controller itself (bonus: much easier to test!).
3 participants
This change adds APIs for managing pull requests and comments on pull requests.
It also adds a Rake task that we will run every evening to sync Pull Requests