Commit lock author

app/controllers/shipit/commits_controller.rb

@@ -1,7 +1,14 @@
 module Shipit
   class CommitsController < ShipitController
     def update
-      commit.update(params.require(:commit).permit(:locked))
+      if update_params[:locked].present?
+        if Shipit::CastValue.to_boolean(update_params[:locked])
+          commit.lock(current_user)
+        else
+          commit.unlock
+        end
+      end
+
       head :ok
     end
 
@@ -14,5 +21,9 @@ def commit
     def stack
       @stack ||= Stack.from_param!(params[:stack_id])
     end
+
+    def update_params
+      @update_params ||= params.require(:commit).permit(:locked)
+    end
   end
 end

app/controllers/shipit/stacks_controller.rb

@@ -19,7 +19,7 @@ def show
       @tasks = @stack.tasks.order(id: :desc).preload(:since_commit, :until_commit, :user).limit(10)
 
       commits = @stack.undeployed_commits do |scope|
-        scope.preload(:author, :statuses, :check_runs)
+        scope.preload(:author, :statuses, :check_runs, :lock_author)
       end
 
       next_expected_commit_to_deploy = @stack.next_expected_commit_to_deploy(commits: commits)

app/helpers/shipit/stacks_helper.rb

@@ -78,5 +78,13 @@ def pull_request_link(pull_request_or_commit)
     def render_raw_commit_id_link(commit)
       link_to(commit.short_sha, github_commit_url(commit), target: '_blank', class: 'number')
     end
+
+    def unlock_commit_tooltip(commit)
+      if commit.lock_author.present?
+        t('commit.unlock_with_author', author: commit.lock_author.name)
+      else
+        t('commit.unlock')
+      end
+    end
   end
 end

app/models/shipit/commit.rb

@@ -23,6 +23,7 @@ class Commit < ActiveRecord::Base
 
     belongs_to :author, class_name: 'User', inverse_of: :authored_commits
     belongs_to :committer, class_name: 'User', inverse_of: :commits
+    belongs_to :lock_author, class_name: :User, optional: true, inverse_of: false
 
     def author
       super || AnonymousUser.new
@@ -32,6 +33,10 @@ def committer
       super || AnonymousUser.new
     end
 
+    def lock_author
+      super || AnonymousUser.new
+    end
+
     scope :reachable, -> { where(detached: false) }
 
     delegate :broadcast_update, :github_repo_name, :hidden_statuses, :required_statuses, :blocking_statuses,
@@ -288,6 +293,24 @@ def deploy_requested_at
       end
     end
 
+    def lock(user)
+      update!(
+        locked: true,
+        lock_author_id: user.id,
+      )
+    end
+
+    def self.lock_all(user)
+      update_all(
+        locked: true,
+        lock_author_id: user.id,
+      )
+    end
+
+    def unlock
+      update!(locked: false, lock_author: nil)
+    end
+
     private
 
     def message_parser

app/models/shipit/stack.rb

@@ -240,21 +240,23 @@ def status
     end
 
     def lock_reverted_commits!
-      commits_to_lock = []
-
       backlog = undeployed_commits.to_a
+      affected_rows = 0
+
       until backlog.empty?
         backlog = backlog.drop_while { |c| !c.revert? }
-        if revert = backlog.shift
-          commits_to_lock += backlog.reverse.drop_while { |c| !revert.revert_of?(c) }
-        end
-      end
+        revert = backlog.shift
+        next if revert.nil?
 
-      unless commits_to_lock.empty?
-        if commits.where(id: commits_to_lock.map(&:id).uniq).update_all(locked: true) > 1
-          touch
-        end
+        commits_to_lock = backlog.reverse.drop_while { |c| !revert.revert_of?(c) }
+        next if commits_to_lock.empty?
+
+        affected_rows += commits
+                         .where(id: commits_to_lock.map(&:id).uniq)
+                         .lock_all(revert.author)
       end
+
+      touch if affected_rows > 1
     end
 
     def next_expected_commit_to_deploy(commits: nil)

app/views/shipit/commits/_commit.html.erb

@@ -27,7 +27,7 @@
       <%= link_to stack_commit_path(commit.stack, commit), class: 'action-lock-commit', data: {tooltip: t('commit.lock')} do %>
         <i class="icon icon--lock"></i>
       <% end %>
-      <%= link_to stack_commit_path(commit.stack, commit), class: 'action-unlock-commit', data: {tooltip: t('commit.unlock'), confirm: t('commit.confirm_unlock')} do %>
+      <%= link_to stack_commit_path(commit.stack, commit), class: 'action-unlock-commit', data: {tooltip: unlock_commit_tooltip(commit), confirm: t('commit.confirm_unlock')} do %>
         <i class="icon icon--lock"></i>
       <% end %>
     </div>

config/locales/en.yml

@@ -22,7 +22,8 @@
 en:
   commit:
     lock: This commit is safe to deploy. Click to mark it as unsafe.
-    unlock: This commit is unsafe to deploy. Click to mark it as safe.
+    unlock: This commit was marked as unsafe to deploy. Click to mark it as safe.
+    unlock_with_author: "%{author} marked this commit as unsafe to deploy. Click to mark it as safe."
     confirm_unlock: Mark this commit as safe to deploy?
   release:
     validate: Mark the release as healthy

db/migrate/20190502020249_add_lock_author_id_to_commits.rb

@@ -0,0 +1,5 @@
+class AddLockAuthorIdToCommits < ActiveRecord::Migration[5.2]
+  def change
+    add_column(:commits, :lock_author_id, :integer, limit: 4)
+  end
+end

lib/shipit.rb

@@ -48,6 +48,7 @@
 require 'shipit/environment_variables'
 require 'shipit/stat'
 require 'shipit/strip_cache_control'
+require 'shipit/cast_value'
 
 SafeYAML::OPTIONS[:default_mode] = :safe
 SafeYAML::OPTIONS[:deserialize_symbols] = false

lib/shipit/cast_value.rb

@@ -0,0 +1,9 @@
+module Shipit
+  module CastValue
+    def to_boolean(value)
+      ActiveModel::Type::Boolean.new.serialize(value)
+    end
+
+    module_function :to_boolean
+  end
+end

test/controllers/commits_controller_test.rb

@@ -5,14 +5,38 @@ class CommitsControllerTest < ActionController::TestCase
     setup do
       @stack = shipit_stacks(:shipit)
       @commit = shipit_commits(:first)
-      session[:user_id] = shipit_users(:walrus).id
+      @user = shipit_users(:walrus)
+      session[:user_id] = @user.id
     end
 
-    test "#update allows to lock a commit" do
-      refute_predicate @commit, :locked?
-      patch :update, params: {stack_id: @stack.to_param, id: @commit.id, commit: {locked: true}}
-      assert_response :ok
-      assert_predicate @commit.reload, :locked?
+    test "#update allows commits to be locked and sets the lock author" do
+      refute_predicate(@commit, :locked?)
+
+      patch(:update, params: {
+        stack_id: @stack.to_param,
+        id: @commit.id,
+        commit: {locked: true},
+      })
+
+      assert_response(:ok)
+      @commit.reload
+      assert_predicate(@commit, :locked?)
+      assert_equal(@user, @commit.lock_author)
+    end
+
+    test "#update allows commits to be unlocked and clears the lock author" do
+      @commit.lock(@user)
+
+      patch(:update, params: {
+        stack_id: @stack.to_param,
+        id: @commit.id,
+        commit: {locked: false},
+      })
+
+      assert_response(:ok)
+      @commit.reload
+      refute_predicate(@commit, :locked?)
+      assert_nil(@commit.lock_author_id)
     end
   end
 end

test/dummy/db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2018_10_10_150947) do
+ActiveRecord::Schema.define(version: 2019_05_02_020249) do
 
   create_table "api_clients", force: :cascade do |t|
     t.text "permissions", limit: 65535
@@ -76,6 +76,7 @@
     t.string "pull_request_title", limit: 1024
     t.integer "pull_request_id"
     t.boolean "locked", default: false, null: false
+    t.integer "lock_author_id", limit: 4
     t.index ["author_id"], name: "index_commits_on_author_id"
     t.index ["committer_id"], name: "index_commits_on_committer_id"
     t.index ["created_at"], name: "index_commits_on_created_at"

test/models/commits_test.rb

@@ -444,6 +444,66 @@ class CommitsTest < ActiveSupport::TestCase
       assert_predicate commit, :deployable?
     end
 
+    test "#lock sets the lock author and sets the locked flag" do
+      user = shipit_users(:shipit)
+
+      @commit.lock(user)
+
+      assert_predicate(@commit, :locked?)
+      assert_equal(user, @commit.lock_author)
+    end
+
+    test "#lock does not set the lock author if the user is anonymous" do
+      user = Shipit::AnonymousUser.new
+
+      @commit.lock(user)
+
+      assert_predicate(@commit, :locked?)
+      assert_nil(@commit.lock_author_id)
+    end
+
+    test ".lock_all sets the lock author and sets the locked flag" do
+      user = shipit_users(:shipit)
+
+      Commit.where(id: [@commit.id]).lock_all(user)
+
+      @commit.reload
+      assert_predicate(@commit, :locked?)
+      assert_equal(user, @commit.lock_author)
+    end
+
+    test ".lock_all does not set the lock author if the user is anonymous" do
+      user = Shipit::AnonymousUser.new
+
+      Commit.where(id: [@commit.id]).lock_all(user)
+
+      @commit.reload
+      assert_predicate(@commit, :locked?)
+      assert_nil(@commit.lock_author_id)
+    end
+
+    test "#lock_author defaults to AnonymousUser" do
+      assert_nil(@commit.lock_author_id)
+      assert_kind_of(Shipit::AnonymousUser, @commit.lock_author)
+
+      user = shipit_users(:shipit)
+      @commit.lock(user)
+
+      assert_kind_of(Shipit::User, @commit.lock_author)
+    end
+
+    test "#unlock clears the lock author and resets the locked flag" do
+      user = shipit_users(:shipit)
+      @commit.lock(user)
+      assert_predicate(@commit, :locked?)
+      assert_equal(user, @commit.lock_author)
+
+      @commit.unlock
+
+      refute_predicate(@commit, :locked?)
+      assert_nil(@commit.lock_author_id)
+    end
+
     expected_webhook_transitions = { # we expect deployable_status to fire on these transitions, and not on any others
       'unknown' => %w(pending success failure error),
       'pending' => %w(success failure error),

test/models/rollbacks_test.rb

@@ -35,23 +35,24 @@ class RollbackTest < ActiveSupport::TestCase
       )
 
       expected = [
-        ['Revert "Merge pull request #7 from shipit-engine/yoloshipit"', false],
-        ["whoami", false],
-        ['fix all the things', false],
+        ['Revert "Merge pull request #7 from shipit-engine/yoloshipit"', false, nil],
+        ["whoami", false, nil],
+        ['fix all the things', false, nil],
       ]
-      assert_equal expected, @stack.undeployed_commits.map { |c| [c.title, c.locked?] }
+      assert_equal(expected, @stack.undeployed_commits.map { |c| [c.title, c.locked?, c.lock_author_id] })
 
       rollback = deploy.trigger_revert
       rollback.run!
       rollback.complete!
 
+      user_id = reverted_commit.author.id
       expected = [
-        ['Revert "Merge pull request #7 from shipit-engine/yoloshipit"', false],
-        ["whoami", true],
-        ['fix all the things', true],
-        ['yoloshipit!', true],
+        ['Revert "Merge pull request #7 from shipit-engine/yoloshipit"', false, nil],
+        ["whoami", true, user_id],
+        ['fix all the things', true, user_id],
+        ['yoloshipit!', true, user_id],
       ]
-      assert_equal expected, @stack.undeployed_commits.map { |c| [c.title, c.locked?] }
+      assert_equal(expected, @stack.undeployed_commits.map { |c| [c.title, c.locked?, c.lock_author_id] })
     end
   end
 end