refactors finder and correlated code

6 files changed, 62 insertions, 55 deletions

diff --git a/app/controllers/admin/impersonation_tokens_controller.rb b/app/controllers/admin/impersonation_tokens_controller.rb
index 43e718bcc97..448f2c881a1 100644
--- a/app/controllers/admin/impersonation_tokens_controller.rb
+++ b/app/controllers/admin/impersonation_tokens_controller.rb
@@ -1,13 +1,12 @@
 class Admin::ImpersonationTokensController < Admin::ApplicationController
-  before_action :user
+  before_action :user, :finder
 
   def index
     set_index_vars
   end
 
   def create
-    # We never want to non-impersonate a user
-    @impersonation_token = user.personal_access_tokens.build(impersonation_token_params.merge(impersonation: true))
+    @impersonation_token = finder.execute.build(impersonation_token_params)
 
     if @impersonation_token.save
       flash[:impersonation_token] = @impersonation_token.token
@@ -19,7 +18,7 @@ class Admin::ImpersonationTokensController < Admin::ApplicationController
   end
 
   def revoke
-    @impersonation_token = user.personal_access_tokens.impersonation.find(params[:id])
+    @impersonation_token = finder.execute(id: params[:id])
 
     if @impersonation_token.revoke!
       flash[:notice] = "Revoked impersonation token #{@impersonation_token.name}!"
@@ -36,14 +35,21 @@ class Admin::ImpersonationTokensController < Admin::ApplicationController
     @user ||= User.find_by!(username: params[:user_id])
   end
 
+  def finder
+    @finder ||= PersonalAccessTokensFinder.new(user: user, impersonation: true)
+  end
+
   def impersonation_token_params
     params.require(:personal_access_token).permit(:name, :expires_at, :impersonation, scopes: [])
   end
 
   def set_index_vars
-    @impersonation_token ||= user.personal_access_tokens.build
+    finder.params[:state] = 'active'
+    @impersonation_token ||= finder.execute.build
     @scopes = Gitlab::Auth::SCOPES
-    @active_impersonation_tokens = user.personal_access_tokens.impersonation.active.order(:expires_at)
-    @inactive_impersonation_tokens = user.personal_access_tokens.impersonation.inactive
+    finder.params[:order] = :expires_at
+    @active_impersonation_tokens = finder.execute
+    finder.params[:state] = 'inactive'
+    @inactive_impersonation_tokens = finder.execute
   end
 end
diff --git a/app/controllers/profiles/personal_access_tokens_controller.rb b/app/controllers/profiles/personal_access_tokens_controller.rb
index a7eca876c2f..2188350f2fd 100644
--- a/app/controllers/profiles/personal_access_tokens_controller.rb
+++ b/app/controllers/profiles/personal_access_tokens_controller.rb
@@ -1,10 +1,12 @@
 class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
+  before_action :finder
+
   def index
     set_index_vars
   end
 
   def create
-    @personal_access_token = current_user.personal_access_tokens.build(personal_access_token_params)
+    @personal_access_token = finder.execute.build(personal_access_token_params)
 
     if @personal_access_token.save
       flash[:personal_access_token] = @personal_access_token.token
@@ -16,7 +18,7 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
   end
 
   def revoke
-    @personal_access_token = current_user.personal_access_tokens.find(params[:id])
+    @personal_access_token = finder.execute(id: params[:id])
 
     if @personal_access_token.revoke!
       flash[:notice] = "Revoked personal access token #{@personal_access_token.name}!"
@@ -29,14 +31,21 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
 
   private
 
+  def finder
+    @finder ||= PersonalAccessTokensFinder.new(user: current_user, impersonation: false)
+  end
+
   def personal_access_token_params
     params.require(:personal_access_token).permit(:name, :expires_at, scopes: [])
   end
 
   def set_index_vars
-    @personal_access_token ||= current_user.personal_access_tokens.build
+    finder.params[:state] = 'active'
+    @personal_access_token ||= finder.execute.build
     @scopes = Gitlab::Auth::SCOPES
-    @active_personal_access_tokens = current_user.personal_access_tokens.active.order(:expires_at)
-    @inactive_personal_access_tokens = current_user.personal_access_tokens.inactive
+    finder.params[:order] = :expires_at
+    @active_personal_access_tokens = finder.execute
+    finder.params[:state] = 'inactive'
+    @inactive_personal_access_tokens = finder.execute
   end
 end
diff --git a/app/finders/personal_access_tokens_finder.rb b/app/finders/personal_access_tokens_finder.rb
index ad3f7f4a437..7b9a2f6c0bb 100644
--- a/app/finders/personal_access_tokens_finder.rb
+++ b/app/finders/personal_access_tokens_finder.rb
@@ -1,41 +1,47 @@
 class PersonalAccessTokensFinder
-  def initialize(user, params = {})
-    @user = user
+  attr_accessor :params
+
+  def initialize(params = {})
     @params = params
   end
 
-  def execute
-    pat_id = token_id?
-    personal_access_tokens = @user.personal_access_tokens
-    personal_access_tokens = personal_access_tokens.impersonation if impersonation?
+  def execute(token: nil, id: nil)
+    tokens = by_impersonation
 
-    return find_token_by_id(personal_access_tokens, pat_id) if pat_id
+    return tokens.find_by_token(token) if token
+    return tokens.find_by_id(id) if id
 
-    case state?
-    when 'active'
-      personal_access_tokens.active
-    when 'inactive'
-      personal_access_tokens.inactive
-    else
-      personal_access_tokens
-    end
+    tokens = by_state(tokens)
+    tokens.order(@params[:order]) if @params[:order]
+
+    tokens
   end
 
   private
 
-  def state?
-    @params[:state].presence
-  end
-
-  def impersonation?
-    @params[:impersonation].presence
+  def personal_access_tokens
+    @params[:user] ? @params[:user].personal_access_tokens : PersonalAccessToken.all
   end
 
-  def token_id?
-    @params[:personal_access_token_id].presence
+  def by_impersonation
+    case @params[:impersonation]
+    when true
+      personal_access_tokens.with_impersonation
+    when false
+      personal_access_tokens.without_impersonation
+    else
+      personal_access_tokens
+    end
   end
 
-  def find_token_by_id(personal_access_tokens, pat_id)
-    personal_access_tokens.find_by(id: pat_id)
+  def by_state(tokens)
+    case @params[:state]
+    when 'active'
+      tokens.active
+    when 'inactive'
+      tokens.inactive
+    else
+      tokens
+    end
   end
 end
diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index 676e0832d54..22809fe1487 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -9,11 +9,10 @@ class PersonalAccessToken < ActiveRecord::Base
 
   before_save :ensure_token
 
-  default_scope { where(impersonation: false) }
-  scope :active, -> { where(revoked: false).where("expires_at >= NOW() OR expires_at IS NULL") }
+  scope :active, -> { where("revoked = false AND (expires_at >= NOW() OR expires_at IS NULL)") }
   scope :inactive, -> { where("revoked = true OR expires_at < NOW()") }
-  scope :impersonation, -> { unscoped.where(impersonation: true) }
-  scope :with_impersonation_tokens, ->  { unscoped }
+  scope :with_impersonation, -> { where(impersonation: true) }
+  scope :without_impersonation, -> { where(impersonation: false) }
 
   def revoke!
     self.revoked = true
diff --git a/app/views/profiles/personal_access_tokens/index.html.haml b/app/views/profiles/personal_access_tokens/index.html.haml
index 8d5008642c9..7b5121ed550 100644
--- a/app/views/profiles/personal_access_tokens/index.html.haml
+++ b/app/views/profiles/personal_access_tokens/index.html.haml
@@ -86,19 +86,6 @@
 
 
 :javascript
-  var $dateField = $('#personal_access_token_expires_at');
-  var date = $dateField.val();
-
-  new Pikaday({
-    field: $dateField.get(0),
-    theme: 'gitlab-theme',
-    format: 'yyyy-mm-dd',
-    minDate: new Date(),
-    onSelect: function(dateText) {
-      $dateField.val(dateFormat(new Date(dateText), 'yyyy-mm-dd'));
-    }
-  });
-
   $("#created-personal-access-token").click(function() {
     this.select();
   });
diff --git a/app/views/shared/_personal_access_tokens_form.html.haml b/app/views/shared/_personal_access_tokens_form.html.haml
index 546e90a1d62..074eeb7d038 100644
--- a/app/views/shared/_personal_access_tokens_form.html.haml
+++ b/app/views/shared/_personal_access_tokens_form.html.haml
@@ -29,7 +29,7 @@
   new Pikaday({
     field: $dateField.get(0),
     theme: 'gitlab-theme',
-    format: 'YYYY-MM-DD',
+    format: 'yyyy-mm-dd',
     minDate: new Date(),
     onSelect: function(dateText) {
       $dateField.val(dateFormat(new Date(dateText), 'yyyy-mm-dd'));