Refactoring

.travis.yml

@@ -23,6 +23,11 @@ gemfile:
 
 matrix:
   fast_finish: true
+  # Run Danger only once
+  include:
+    - rvm: 2.5
+      gemfile: gemfiles/rails_5_2.gemfile
+      script: bundle exec danger
   exclude:
     - gemfile: gemfiles/rails_5_0.gemfile
       rvm: 2.1

Dangerfile

@@ -0,0 +1,30 @@
+# --------------------------------------------------------------------------------------------------------------------
+# Has any changes happened inside the actual library code?
+# --------------------------------------------------------------------------------------------------------------------
+has_app_changes = !git.modified_files.grep(/lib/).empty?
+has_spec_changes = !git.modified_files.grep(/spec/).empty?
+
+# --------------------------------------------------------------------------------------------------------------------
+# You've made changes to lib, but didn't write any tests?
+# --------------------------------------------------------------------------------------------------------------------
+if has_app_changes && !has_spec_changes
+  warn("There're library changes, but not tests. That's OK as long as you're refactoring existing code.", sticky: false)
+end
+
+# --------------------------------------------------------------------------------------------------------------------
+# You've made changes to specs, but no library code has changed?
+# --------------------------------------------------------------------------------------------------------------------
+if !has_app_changes && has_spec_changes
+  message('We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!', sticky: false)
+end
+
+# Mainly to encourage writing up some reasoning about the PR, rather than
+# just leaving a title
+if github.pr_body.length < 10
+  fail "Please provide a summary in the Pull Request description"
+end
+
+# --------------------------------------------------------------------------------------------------------------------
+# Have you updated CHANGELOG.md?
+# --------------------------------------------------------------------------------------------------------------------
+changelog.check

doorkeeper.gemspec

@@ -2,29 +2,31 @@ $LOAD_PATH.push File.expand_path('../lib', __FILE__)
 
 require 'doorkeeper/version'
 
-Gem::Specification.new do |s|
-  s.name        = 'doorkeeper'
-  s.version     = Doorkeeper.gem_version
-  s.authors     = ['Felipe Elias Philipp', 'Tute Costa', 'Jon Moss', 'Nikita Bulai']
-  s.email       = %w([email protected])
-  s.homepage    = 'https://github.com/doorkeeper-gem/doorkeeper'
-  s.summary     = 'OAuth 2 provider for Rails and Grape'
-  s.description = 'Doorkeeper is an OAuth 2 provider for Rails and Grape.'
-  s.license     = 'MIT'
+Gem::Specification.new do |gem|
+  gem.name        = 'doorkeeper'
+  gem.version     = Doorkeeper.gem_version
+  gem.authors     = ['Felipe Elias Philipp', 'Tute Costa', 'Jon Moss', 'Nikita Bulai']
+  gem.email       = %w([email protected])
+  gem.homepage    = 'https://github.com/doorkeeper-gem/doorkeeper'
+  gem.summary     = 'OAuth 2 provider for Rails and Grape'
+  gem.description = 'Doorkeeper is an OAuth 2 provider for Rails and Grape.'
+  gem.license     = 'MIT'
 
-  s.files         = `git ls-files`.split("\n")
-  s.test_files    = `git ls-files -- spec/*`.split("\n")
-  s.require_paths = ['lib']
+  gem.files         = `git ls-files`.split("\n")
+  gem.test_files    = `git ls-files -- spec/*`.split("\n")
+  gem.require_paths = ['lib']
 
-  s.add_dependency 'railties', '>= 4.2'
-  s.required_ruby_version = '>= 2.1'
+  gem.add_dependency 'railties', '>= 4.2'
+  gem.required_ruby_version = '>= 2.1'
 
-  s.add_development_dependency 'capybara', '~> 2.18'
-  s.add_development_dependency 'coveralls'
-  s.add_development_dependency 'grape'
-  s.add_development_dependency 'database_cleaner', '~> 1.6'
-  s.add_development_dependency 'factory_bot', '~> 4.8'
-  s.add_development_dependency 'generator_spec', '~> 0.9.3'
-  s.add_development_dependency 'rake', '>= 11.3.0'
-  s.add_development_dependency 'rspec-rails'
+  gem.add_development_dependency 'capybara', '~> 2.18'
+  gem.add_development_dependency 'coveralls'
+  gem.add_development_dependency 'danger', '~> 5.0'
+  gem.add_development_dependency 'danger-changelog', '~> 0.3'
+  gem.add_development_dependency 'grape'
+  gem.add_development_dependency 'database_cleaner', '~> 1.6'
+  gem.add_development_dependency 'factory_bot', '~> 4.8'
+  gem.add_development_dependency 'generator_spec', '~> 0.9.3'
+  gem.add_development_dependency 'rake', '>= 11.3.0'
+  gem.add_development_dependency 'rspec-rails'
 end

lib/doorkeeper.rb

@@ -67,6 +67,7 @@
 require 'doorkeeper/rails/helpers'
 
 require 'doorkeeper/rake'
+require 'doorkeeper/stale_records_cleaner'
 
 require 'doorkeeper/orm/active_record'
 

lib/doorkeeper/errors.rb

@@ -46,6 +46,7 @@ def initialize(response)
 
     UnableToGenerateToken = Class.new(DoorkeeperError)
     TokenGeneratorNotFound = Class.new(DoorkeeperError)
+    NoOrmCleaner = Class.new(DoorkeeperError)
 
     InvalidToken = Class.new BaseResponseError
     TokenExpired = Class.new InvalidToken

lib/doorkeeper/rake/db.rake

@@ -13,26 +13,26 @@ namespace :doorkeeper do
     namespace :cleanup do
       desc 'Removes stale access tokens'
       task revoked_tokens: 'doorkeeper:setup' do
-        cleaner = Doorkeeper::Orm::ActiveRecord::StaleRecordsCleaner.new(Doorkeeper::AccessToken)
+        cleaner = Doorkeeper::StaleRecordsCleaner.new(Doorkeeper::AccessToken)
         cleaner.clean_revoked
       end
 
       desc 'Removes expired (TTL passed) access tokens'
       task expired_tokens: 'doorkeeper:setup' do
         expirable_tokens = Doorkeeper::AccessToken.where(refresh_token: nil)
-        cleaner = Doorkeeper::Orm::ActiveRecord::StaleRecordsCleaner.new(expirable_tokens)
+        cleaner = Doorkeeper::StaleRecordsCleaner.new(expirable_tokens)
         cleaner.clean_expired(Doorkeeper.configuration.access_token_expires_in)
       end
 
       desc 'Removes stale access grants'
       task revoked_grants: 'doorkeeper:setup' do
-        cleaner = Doorkeeper::Orm::ActiveRecord::StaleRecordsCleaner.new(Doorkeeper::AccessGrant)
+        cleaner = Doorkeeper::StaleRecordsCleaner.new(Doorkeeper::AccessGrant)
         cleaner.clean_revoked
       end
 
       desc 'Removes expired (TTL passed) access grants'
       task expired_grants: 'doorkeeper:setup' do
-        cleaner = Doorkeeper::Orm::ActiveRecord::StaleRecordsCleaner.new(Doorkeeper::AccessGrant)
+        cleaner = Doorkeeper::StaleRecordsCleaner.new(Doorkeeper::AccessGrant)
         cleaner.clean_expired(Doorkeeper.configuration.authorization_code_expires_in)
       end
     end

lib/doorkeeper/stale_records_cleaner.rb

@@ -0,0 +1,18 @@
+module Doorkeeper
+  class StaleRecordsCleaner
+    CLEANER_CLASS = 'StaleRecordsCleaner'.freeze
+
+    def self.for(base_scope)
+      orm_adapter = "doorkeeper/orm/#{Doorkeeper.configuration.orm}".classify
+
+      orm_cleaner = "#{orm_adapter}::#{CLEANER_CLASS}".constantize
+      orm_cleaner.new(base_scope)
+    rescue NameError
+      raise NoOrmCleaner, "'#{Doorkeeper.configuration.orm}' ORM has no cleaner!"
+    end
+
+    def self.new(base_scope)
+      self.for(base_scope)
+    end
+  end
+end

spec/factories.rb

@@ -2,24 +2,24 @@
   factory :access_grant, class: Doorkeeper::AccessGrant do
     sequence(:resource_owner_id) { |n| n }
     application
-    redirect_uri 'https://app.com/callback'
-    expires_in 100
-    scopes 'public write'
+    redirect_uri { 'https://app.com/callback' }
+    expires_in { 100 }
+    scopes { 'public write' }
   end
 
   factory :access_token, class: Doorkeeper::AccessToken do
     sequence(:resource_owner_id) { |n| n }
     application
-    expires_in 2.hours
+    expires_in { 2.hours }
 
     factory :clientless_access_token do
-      application nil
+      application { nil }
     end
   end
 
   factory :application, class: Doorkeeper::Application do
     sequence(:name) { |n| "Application #{n}" }
-    redirect_uri 'https://app.com/callback'
+    redirect_uri { 'https://app.com/callback' }
   end
 
   # do not name this factory :user, otherwise it will conflict with factories

spec/lib/orm/active_record/stale_records_cleaner_spec.rb → spec/lib/stale_records_cleaner_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Doorkeeper::Orm::ActiveRecord::StaleRecordsCleaner do
+describe Doorkeeper::StaleRecordsCleaner do
   let(:cleaner) { described_class.new(model) }
   let(:models_by_name) do
     {

spec/requests/flows/authorization_code_spec.rb

@@ -66,7 +66,7 @@
     page.driver.post token_endpoint_url(code: authorization_code, client_id: @client.uid,
                                         redirect_uri: @client.redirect_uri)
 
-    expect(Doorkeeper::AccessToken).not_to exist
+    expect(Doorkeeper::AccessToken.count).to be_zero
 
     should_have_json 'error', 'invalid_client'
   end

spec/support/doorkeeper_rspec.rb

@@ -4,7 +4,8 @@ class RSpec
     # Doorkeeper configuration, etc.
     def self.print_configuration_info
       puts <<-INFO.strip_heredoc
-        ====> Doorkeeper ORM = #{Doorkeeper.configuration.orm}
+        ====> Doorkeeper ORM: '#{Doorkeeper.configuration.orm}'
+        ====> Doorkeeper version: #{Doorkeeper.gem_version}
         ====> Rails version: #{::Rails.version}
         ====> Ruby version: #{RUBY_VERSION} on #{RUBY_PLATFORM}
       INFO