Check if board visit is unique and refactoringbw-automatically-navigate-to-last-board-visited

9 files changed, 82 insertions, 39 deletions

diff --git a/app/controllers/groups/boards_controller.rb b/app/controllers/groups/boards_controller.rb
index 8e0efd534cf..cdc6f53df8e 100644
--- a/app/controllers/groups/boards_controller.rb
+++ b/app/controllers/groups/boards_controller.rb
@@ -5,17 +5,10 @@ class Groups::BoardsController < Groups::ApplicationController
 
   before_action :assign_endpoint_vars
   before_action :boards, only: :index
+  before_action :redirect_to_recent_board, only: :index
 
   def index
-    return respond_with_boards if request.format.json?
-
-    recently_visited = Boards::Visits::LatestService.new(group, current_user).execute
-
-    if recently_visited && includes_board?(recently_visited.board_id)
-      redirect_to(group_board_path(id: recently_visited.board_id), status: :found)
-    else
-      respond_with_boards
-    end
+    respond_with_boards
   end
 
   def show
@@ -46,4 +39,14 @@ class Groups::BoardsController < Groups::ApplicationController
   def includes_board?(board_id)
     boards.any? { |board| board.id == board_id }
   end
+
+  def redirect_to_recent_board
+    return if request.format.json?
+
+    recently_visited = Boards::Visits::LatestService.new(group, current_user).execute
+
+    if recently_visited && includes_board?(recently_visited.board_id)
+      redirect_to(group_board_path(id: recently_visited.board_id), status: :found)
+    end
+  end
 end
diff --git a/app/controllers/projects/boards_controller.rb b/app/controllers/projects/boards_controller.rb
index 6c03f624818..661455beaa8 100644
--- a/app/controllers/projects/boards_controller.rb
+++ b/app/controllers/projects/boards_controller.rb
@@ -8,17 +8,10 @@ class Projects::BoardsController < Projects::ApplicationController
   before_action :authorize_read_board!, only: [:index, :show]
   before_action :boards, only: :index
   before_action :assign_endpoint_vars
+  before_action :redirect_to_recent_board, only: :index
 
   def index
-    return respond_with_boards if request.format.json?
-
-    recently_visited = Boards::Visits::LatestService.new(project, current_user).execute
-
-    if recently_visited && includes_board?(recently_visited.board_id)
-      redirect_to(namespace_project_board_path(id: recently_visited.board_id), status: :found)
-    else
-      respond_with_boards
-    end
+    respond_with_boards
   end
 
   def show
@@ -54,4 +47,14 @@ class Projects::BoardsController < Projects::ApplicationController
   def includes_board?(board_id)
     boards.any? { |board| board.id == board_id }
   end
+
+  def redirect_to_recent_board
+    return if request.format.json?
+
+    recently_visited = Boards::Visits::LatestService.new(project, current_user).execute
+
+    if recently_visited && includes_board?(recently_visited.board_id)
+      redirect_to(namespace_project_board_path(id: recently_visited.board_id), status: :found)
+    end
+  end
 end
diff --git a/app/models/board_group_recent_visit.rb b/app/models/board_group_recent_visit.rb
index 68b3aa88c7f..92abbb67222 100644
--- a/app/models/board_group_recent_visit.rb
+++ b/app/models/board_group_recent_visit.rb
@@ -13,9 +13,10 @@ class BoardGroupRecentVisit < ActiveRecord::Base
   scope :by_user_group, -> (user, group) { where(user: user, group: group).order(:updated_at) }
 
   def self.visited!(user, board)
-    visit = find_or_initialize_by(user: user, group: board.group, board: board)
-    visit.updated_at = Time.now
-    visit.save!
+    visit = find_or_create_by(user: user, group: board.group, board: board)
+    visit.touch if visit.updated_at < Time.now
+  rescue ActiveRecord::RecordNotUnique
+    retry
   end
 
   def self.latest(user, group)
diff --git a/app/models/board_project_recent_visit.rb b/app/models/board_project_recent_visit.rb
index d5d2453927c..7cffff906d8 100644
--- a/app/models/board_project_recent_visit.rb
+++ b/app/models/board_project_recent_visit.rb
@@ -13,9 +13,10 @@ class BoardProjectRecentVisit < ActiveRecord::Base
   scope :by_user_project, -> (user, project) { where(user: user, project: project).order(:updated_at) }
 
   def self.visited!(user, board)
-    visit = find_or_initialize_by(user: user, project: board.project, board: board)
-    visit.updated_at = Time.now
-    visit.save!
+    visit = find_or_create_by(user: user, project: board.project, board: board)
+    visit.touch if visit.updated_at < Time.now
+  rescue ActiveRecord::RecordNotUnique
+    retry
   end
 
   def self.latest(user, project)
diff --git a/db/migrate/20181010235606_create_board_project_recent_visits.rb b/db/migrate/20181010235606_create_board_project_recent_visits.rb
index e6e946126f2..426f41e202a 100644
--- a/db/migrate/20181010235606_create_board_project_recent_visits.rb
+++ b/db/migrate/20181010235606_create_board_project_recent_visits.rb
@@ -13,5 +13,7 @@ class CreateBoardProjectRecentVisits < ActiveRecord::Migration
       t.references :project, index: true, foreign_key: { on_delete: :cascade }
       t.references :board, index: true, foreign_key: { on_delete: :cascade }
     end
+
+    add_index :board_project_recent_visits, [:user_id, :project_id, :board_id], unique: true, name: 'index_board_project_recent_visits_on_user_project_and_board'
   end
 end
diff --git a/db/migrate/20181016152238_create_board_group_recent_visits.rb b/db/migrate/20181016152238_create_board_group_recent_visits.rb
index 7cd4cf63d57..1e55dc8658e 100644
--- a/db/migrate/20181016152238_create_board_group_recent_visits.rb
+++ b/db/migrate/20181016152238_create_board_group_recent_visits.rb
@@ -14,5 +14,7 @@ class CreateBoardGroupRecentVisits < ActiveRecord::Migration
       t.references :group, references: :namespace, column: :group_id, index: true
       t.foreign_key :namespaces, column: :group_id, on_delete: :cascade
     end
+
+    add_index :board_group_recent_visits, [:user_id, :group_id, :board_id], unique: true, name: 'index_board_group_recent_visits_on_user_group_and_board'
   end
 end
diff --git a/db/schema.rb b/db/schema.rb
index 6f926a79fd2..7a75aafd7b0 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -214,18 +214,20 @@ ActiveRecord::Schema.define(version: 20181016152238) do
 
   add_index "board_group_recent_visits", ["board_id"], name: "index_board_group_recent_visits_on_board_id", using: :btree
   add_index "board_group_recent_visits", ["group_id"], name: "index_board_group_recent_visits_on_group_id", using: :btree
+  add_index "board_group_recent_visits", ["user_id", "group_id", "board_id"], name: "index_board_group_recent_visits_on_user_group_and_board", unique: true, using: :btree
   add_index "board_group_recent_visits", ["user_id"], name: "index_board_group_recent_visits_on_user_id", using: :btree
 
-  create_table "board_project_recent_visits", force: :cascade do |t|
+  create_table "board_project_recent_visits", id: :bigserial, force: :cascade do |t|
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
     t.integer "user_id"
     t.integer "project_id"
     t.integer "board_id"
-    t.datetime_with_timezone "created_at", null: false
-    t.datetime_with_timezone "updated_at", null: false
   end
 
   add_index "board_project_recent_visits", ["board_id"], name: "index_board_project_recent_visits_on_board_id", using: :btree
   add_index "board_project_recent_visits", ["project_id"], name: "index_board_project_recent_visits_on_project_id", using: :btree
+  add_index "board_project_recent_visits", ["user_id", "project_id", "board_id"], name: "index_board_project_recent_visits_on_user_project_and_board", unique: true, using: :btree
   add_index "board_project_recent_visits", ["user_id"], name: "index_board_project_recent_visits_on_user_id", using: :btree
 
   create_table "boards", force: :cascade do |t|
@@ -2333,6 +2335,9 @@ ActiveRecord::Schema.define(version: 20181016152238) do
   add_foreign_key "board_group_recent_visits", "boards", on_delete: :cascade
   add_foreign_key "board_group_recent_visits", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "board_group_recent_visits", "users", on_delete: :cascade
+  add_foreign_key "board_project_recent_visits", "boards", on_delete: :cascade
+  add_foreign_key "board_project_recent_visits", "projects", on_delete: :cascade
+  add_foreign_key "board_project_recent_visits", "users", on_delete: :cascade
   add_foreign_key "boards", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "boards", "projects", name: "fk_f15266b5f9", on_delete: :cascade
   add_foreign_key "chat_teams", "namespaces", on_delete: :cascade
diff --git a/spec/models/board_group_recent_visit_spec.rb b/spec/models/board_group_recent_visit_spec.rb
index 15fc9261e18..59ad4e5417e 100644
--- a/spec/models/board_group_recent_visit_spec.rb
+++ b/spec/models/board_group_recent_visit_spec.rb
@@ -24,15 +24,28 @@ describe BoardGroupRecentVisit do
       expect { described_class.visited!(user, board) }.to change(described_class, :count).by(1)
     end
 
-    it 'updates the timestamp' do
-      create :board_group_recent_visit, group: board.group, board: board, user: user, updated_at: 7.days.ago
+    shared_examples 'was visited previously' do
+      let!(:visit) { create :board_group_recent_visit, group: board.group, board: board, user: user, updated_at: 7.days.ago }
+
+      it 'updates the timestamp' do
+        Timecop.freeze do
+          described_class.visited!(user, board)
+
+          expect(described_class.count).to eq 1
+          expect(described_class.first.updated_at).to be_like_time(Time.zone.now)
+        end
+      end
+    end
 
-      Timecop.freeze do
-        described_class.visited!(user, board)
+    it_behaves_like 'was visited previously'
 
-        expect(described_class.count).to eq 1
-        expect(described_class.first.updated_at).to be_like_time(Time.zone.now)
+    context 'when we try to create a visit that is not unique' do
+      before do
+        expect(described_class).to receive(:find_or_create_by).and_raise(ActiveRecord::RecordNotUnique, 'record not unique')
+        expect(described_class).to receive(:find_or_create_by).and_return(visit)
       end
+
+      it_behaves_like 'was visited previously'
     end
   end
 
diff --git a/spec/models/board_project_recent_visit_spec.rb b/spec/models/board_project_recent_visit_spec.rb
index 58917cc5f35..275581945fa 100644
--- a/spec/models/board_project_recent_visit_spec.rb
+++ b/spec/models/board_project_recent_visit_spec.rb
@@ -24,15 +24,28 @@ describe BoardProjectRecentVisit do
       expect { described_class.visited!(user, board) }.to change(described_class, :count).by(1)
     end
 
-    it 'updates the timestamp' do
-      create :board_project_recent_visit, project: board.project, board: board, user: user, updated_at: 7.days.ago
+    shared_examples 'was visited previously' do
+      let!(:visit) { create :board_project_recent_visit, project: board.project, board: board, user: user, updated_at: 7.days.ago }
+
+      it 'updates the timestamp' do
+        Timecop.freeze do
+          described_class.visited!(user, board)
+
+          expect(described_class.count).to eq 1
+          expect(described_class.first.updated_at).to be_like_time(Time.zone.now)
+        end
+      end
+    end
 
-      Timecop.freeze do
-        described_class.visited!(user, board)
+    it_behaves_like 'was visited previously'
 
-        expect(described_class.count).to eq 1
-        expect(described_class.first.updated_at).to be_like_time(Time.zone.now)
+    context 'when we try to create a visit that is not unique' do
+      before do
+        expect(described_class).to receive(:find_or_create_by).and_raise(ActiveRecord::RecordNotUnique, 'record not unique')
+        expect(described_class).to receive(:find_or_create_by).and_return(visit)
       end
+
+      it_behaves_like 'was visited previously'
     end
   end