Merge branch 'bw-automatically-navigate-to-last-board-visited' into 'master'

Automatically navigate to last board visited

See merge request gitlab-org/gitlab-ce!22430

app/controllers/groups/boards_controller.rb

@@ -5,6 +5,7 @@ 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
     respond_with_boards
@@ -13,6 +14,9 @@ class Groups::BoardsController < Groups::ApplicationController
   def show
     @board = boards.find(params[:id])
 
+    # add/update the board in the recent visited table
+    Boards::Visits::CreateService.new(@board.group, current_user).execute(@board) if request.format.html?
+
     respond_with_board
   end
 
@@ -31,4 +35,18 @@ class Groups::BoardsController < Groups::ApplicationController
   def serialize_as_json(resource)
     resource.as_json(only: [:id])
   end
+
+  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

app/controllers/projects/boards_controller.rb

@@ -8,6 +8,7 @@ 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
     respond_with_boards
@@ -16,6 +17,9 @@ class Projects::BoardsController < Projects::ApplicationController
   def show
     @board = boards.find(params[:id])
 
+    # add/update the board in the recent visited table
+    Boards::Visits::CreateService.new(@board.project, current_user).execute(@board) if request.format.html?
+
     respond_with_board
   end
 
@@ -33,10 +37,24 @@ class Projects::BoardsController < Projects::ApplicationController
   end
 
   def authorize_read_board!
-    return access_denied! unless can?(current_user, :read_board, project)
+    access_denied! unless can?(current_user, :read_board, project)
   end
 
   def serialize_as_json(resource)
     resource.as_json(only: [:id])
   end
+
+  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

app/models/board_group_recent_visit.rb

@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+# Tracks which boards in a specific group a user has visited
+class BoardGroupRecentVisit < ActiveRecord::Base
+  belongs_to :user
+  belongs_to :group
+  belongs_to :board
+
+  validates :user,  presence: true
+  validates :group, presence: true
+  validates :board, presence: true
+
+  scope :by_user_group, -> (user, group) { where(user: user, group: group).order(:updated_at) }
+
+  def self.visited!(user, board)
+    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)
+    by_user_group(user, group).last
+  end
+end

app/models/board_project_recent_visit.rb

@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+# Tracks which boards in a specific project a user has visited
+class BoardProjectRecentVisit < ActiveRecord::Base
+  belongs_to :user
+  belongs_to :project
+  belongs_to :board
+
+  validates :user,    presence: true
+  validates :project, presence: true
+  validates :board,   presence: true
+
+  scope :by_user_project, -> (user, project) { where(user: user, project: project).order(:updated_at) }
+
+  def self.visited!(user, board)
+    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)
+    by_user_project(user, project).last
+  end
+end

app/services/boards/visits/create_service.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+module Boards
+  module Visits
+    class CreateService < Boards::BaseService
+      def execute(board)
+        return unless current_user && Gitlab::Database.read_write?
+
+        if parent.is_a?(Group)
+          BoardGroupRecentVisit.visited!(current_user, board)
+        else
+          BoardProjectRecentVisit.visited!(current_user, board)
+        end
+      end
+    end
+  end
+end

app/services/boards/visits/latest_service.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+module Boards
+  module Visits
+    class LatestService < Boards::BaseService
+      def execute
+        return nil unless current_user
+
+        if parent.is_a?(Group)
+          BoardGroupRecentVisit.latest(current_user, parent)
+        else
+          BoardProjectRecentVisit.latest(current_user, parent)
+        end
+      end
+    end
+  end
+end

changelogs/unreleased/bw-automatically-navigate-to-last-board-visited.yml

@@ -0,0 +1,5 @@
+---
+title: Automatically navigate to last board visited
+merge_request: 22430
+author:
+type: changed

db/migrate/20181010235606_create_board_project_recent_visits.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class CreateBoardProjectRecentVisits < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :board_project_recent_visits, id: :bigserial do |t|
+      t.timestamps_with_timezone null: false
+
+      t.references :user, index: true, foreign_key: { on_delete: :cascade }
+      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

db/migrate/20181016152238_create_board_group_recent_visits.rb

@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+class CreateBoardGroupRecentVisits < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :board_group_recent_visits, id: :bigserial do |t|
+      t.timestamps_with_timezone null: false
+
+      t.references :user, index: true, foreign_key: { on_delete: :cascade }
+      t.references :board, index: true, foreign_key: { on_delete: :cascade }
+      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

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20181013005024) do
+ActiveRecord::Schema.define(version: 20181016152238) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -204,6 +204,32 @@ ActiveRecord::Schema.define(version: 20181013005024) do
   add_index "badges", ["group_id"], name: "index_badges_on_group_id", using: :btree
   add_index "badges", ["project_id"], name: "index_badges_on_project_id", using: :btree
 
+  create_table "board_group_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 "board_id"
+    t.integer "group_id"
+  end
+
+  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", 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"
+  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|
     t.integer "project_id"
     t.datetime "created_at", null: false
@@ -2306,6 +2332,12 @@ ActiveRecord::Schema.define(version: 20181013005024) do
   add_foreign_key "application_settings", "users", column: "usage_stats_set_by_user_id", name: "fk_964370041d", on_delete: :nullify
   add_foreign_key "badges", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "badges", "projects", on_delete: :cascade
+  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

doc/user/project/issue_board.md

@@ -176,6 +176,9 @@ Clicking on the current board name in the upper left corner will reveal a
 menu from where you can create another Issue Board and rename or delete the
 existing one.
 
+Clicking on the main issue board link will take you to the last board
+you visited.
+
 NOTE: **Note:**
 The Multiple Issue Boards feature is available for
 **projects in GitLab Starter Edition** and for **groups in GitLab Premium Edition**.

spec/controllers/groups/boards_controller_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Groups::BoardsController do
   let(:group) { create(:group) }
-  let(:user)    { create(:user) }
+  let(:user)  { create(:user) }
 
   before do
     group.add_maintainer(user)
@@ -22,6 +22,27 @@ describe Groups::BoardsController do
         expect(response.content_type).to eq 'text/html'
       end
 
+      it 'redirects to latest visited board' do
+        board = create(:board, group: group)
+        create(:board_group_recent_visit, group: board.group, board: board, user: user)
+
+        list_boards
+
+        expect(response).to redirect_to(group_board_path(id: board.id))
+      end
+
+      it 'renders template if visited board is not found' do
+        visited = double
+
+        allow(visited).to receive(:board_id).and_return(12)
+        allow_any_instance_of(Boards::Visits::LatestService).to receive(:execute).and_return(visited)
+
+        list_boards
+
+        expect(response).to render_template :index
+        expect(response.content_type).to eq 'text/html'
+      end
+
       context 'with unauthorized user' do
         before do
           allow(Ability).to receive(:allowed?).with(user, :read_cross_project, :global).and_return(true)
@@ -35,12 +56,30 @@ describe Groups::BoardsController do
           expect(response.content_type).to eq 'text/html'
         end
       end
+
+      context 'when user is signed out' do
+        let(:group) { create(:group, :public) }
+
+        it 'renders template' do
+          sign_out(user)
+
+          board = create(:board, group: group)
+          create(:board_group_recent_visit, group: board.group, board: board, user: user)
+
+          list_boards
+
+          expect(response).to render_template :index
+          expect(response.content_type).to eq 'text/html'
+        end
+      end
     end
 
     context 'when format is JSON' do
       it 'return an array with one group board' do
         create(:board, group: group)
 
+        expect(Boards::Visits::LatestService).not_to receive(:new)
+
         list_boards format: :json
 
         parsed_response = JSON.parse(response.body)
@@ -74,7 +113,7 @@ describe Groups::BoardsController do
 
     context 'when format is HTML' do
       it 'renders template' do
-        read_board board: board
+        expect { read_board board: board }.to change(BoardGroupRecentVisit, :count).by(1)
 
         expect(response).to render_template :show
         expect(response.content_type).to eq 'text/html'
@@ -93,10 +132,25 @@ describe Groups::BoardsController do
           expect(response.content_type).to eq 'text/html'
         end
       end
+
+      context 'when user is signed out' do
+        let(:group) { create(:group, :public) }
+
+        it 'does not save visit' do
+          sign_out(user)
+
+          expect { read_board board: board }.to change(BoardGroupRecentVisit, :count).by(0)
+
+          expect(response).to render_template :show
+          expect(response.content_type).to eq 'text/html'
+        end
+      end
     end
 
     context 'when format is JSON' do
       it 'returns project board' do
+        expect(Boards::Visits::CreateService).not_to receive(:new)
+
         read_board board: board, format: :json
 
         expect(response).to match_response_schema('board')

spec/controllers/projects/boards_controller_spec.rb

@@ -28,6 +28,27 @@ describe Projects::BoardsController do
         expect(response.content_type).to eq 'text/html'
       end
 
+      it 'redirects to latest visited board' do
+        board = create(:board, project: project)
+        create(:board_project_recent_visit, project: board.project, board: board, user: user)
+
+        list_boards
+
+        expect(response).to redirect_to(namespace_project_board_path(id: board.id))
+      end
+
+      it 'renders template if visited board is not found' do
+        visited = double
+
+        allow(visited).to receive(:board_id).and_return(12)
+        allow_any_instance_of(Boards::Visits::LatestService).to receive(:execute).and_return(visited)
+
+        list_boards
+
+        expect(response).to render_template :index
+        expect(response.content_type).to eq 'text/html'
+      end
+
       context 'with unauthorized user' do
         before do
           allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(true)
@@ -41,12 +62,30 @@ describe Projects::BoardsController do
           expect(response.content_type).to eq 'text/html'
         end
       end
+
+      context 'when user is signed out' do
+        let(:project) { create(:project, :public) }
+
+        it 'renders template' do
+          sign_out(user)
+
+          board = create(:board, project: project)
+          create(:board_project_recent_visit, project: board.project, board: board, user: user)
+
+          list_boards
+
+          expect(response).to render_template :index
+          expect(response.content_type).to eq 'text/html'
+        end
+      end
     end
 
     context 'when format is JSON' do
       it 'returns a list of project boards' do
         create_list(:board, 2, project: project)
 
+        expect(Boards::Visits::LatestService).not_to receive(:new)
+
         list_boards format: :json
 
         parsed_response = JSON.parse(response.body)
@@ -98,7 +137,7 @@ describe Projects::BoardsController do
 
     context 'when format is HTML' do
       it 'renders template' do
-        read_board board: board
+        expect { read_board board: board }.to change(BoardProjectRecentVisit, :count).by(1)
 
         expect(response).to render_template :show
         expect(response.content_type).to eq 'text/html'
@@ -117,10 +156,25 @@ describe Projects::BoardsController do
           expect(response.content_type).to eq 'text/html'
         end
       end
+
+      context 'when user is signed out' do
+        let(:project) { create(:project, :public) }
+
+        it 'does not save visit' do
+          sign_out(user)
+
+          expect { read_board board: board }.to change(BoardProjectRecentVisit, :count).by(0)
+
+          expect(response).to render_template :show
+          expect(response.content_type).to eq 'text/html'
+        end
+      end
     end
 
     context 'when format is JSON' do
       it 'returns project board' do
+        expect(Boards::Visits::CreateService).not_to receive(:new)
+
         read_board board: board, format: :json
 
         expect(response).to match_response_schema('board')

spec/factories/board_group_recent_visit.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :board_group_recent_visit do
+    user
+    group
+    board
+  end
+end

spec/factories/board_project_recent_visit.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :board_project_recent_visit do
+    user
+    project
+    board
+  end
+end

spec/models/board_group_recent_visit_spec.rb

@@ -0,0 +1,64 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe BoardGroupRecentVisit do
+  let(:user)  { create(:user) }
+  let(:group) { create(:group) }
+  let(:board) { create(:board, group: group) }
+
+  describe 'relationships' do
+    it { is_expected.to belong_to(:user) }
+    it { is_expected.to belong_to(:group) }
+    it { is_expected.to belong_to(:board) }
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:user) }
+    it { is_expected.to validate_presence_of(:group) }
+    it { is_expected.to validate_presence_of(:board) }
+  end
+
+  describe '#visited' do
+    it 'creates a visit if one does not exists' do
+      expect { described_class.visited!(user, board) }.to change(described_class, :count).by(1)
+    end
+
+    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
+
+    it_behaves_like 'was visited previously'
+
+    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
+
+  describe '#latest' do
+    it 'returns the most recent visited' do
+      board2 = create(:board, group: group)
+      board3 = create(:board, group: group)
+
+      create :board_group_recent_visit, group: board.group, board: board, user: user, updated_at: 7.days.ago
+      create :board_group_recent_visit, group: board2.group, board: board2, user: user, updated_at: 5.days.ago
+      recent = create :board_group_recent_visit, group: board3.group, board: board3, user: user, updated_at: 1.day.ago
+
+      expect(described_class.latest(user, group)).to eq recent
+    end
+  end
+end

spec/models/board_project_recent_visit_spec.rb

@@ -0,0 +1,64 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe BoardProjectRecentVisit do
+  let(:user)    { create(:user) }
+  let(:project) { create(:project) }
+  let(:board)   { create(:board, project: project) }
+
+  describe 'relationships' do
+    it { is_expected.to belong_to(:user) }
+    it { is_expected.to belong_to(:project) }
+    it { is_expected.to belong_to(:board) }
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:user) }
+    it { is_expected.to validate_presence_of(:project) }
+    it { is_expected.to validate_presence_of(:board) }
+  end
+
+  describe '#visited' do
+    it 'creates a visit if one does not exists' do
+      expect { described_class.visited!(user, board) }.to change(described_class, :count).by(1)
+    end
+
+    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
+
+    it_behaves_like 'was visited previously'
+
+    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
+
+  describe '#latest' do
+    it 'returns the most recent visited' do
+      board2 = create(:board, project: project)
+      board3 = create(:board, project: project)
+
+      create :board_project_recent_visit, project: board.project, board: board, user: user, updated_at: 7.days.ago
+      create :board_project_recent_visit, project: board2.project, board: board2, user: user, updated_at: 5.days.ago
+      recent = create :board_project_recent_visit, project: board3.project, board: board3, user: user, updated_at: 1.day.ago
+
+      expect(described_class.latest(user, project)).to eq recent
+    end
+  end
+end

spec/services/boards/visits/create_service_spec.rb

@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Boards::Visits::CreateService do
+  describe '#execute' do
+    let(:user) { create(:user) }
+
+    context 'when a project board' do
+      let(:project)       { create(:project) }
+      let(:project_board) { create(:board, project: project) }
+
+      subject(:service) { described_class.new(project_board.parent, user) }
+
+      it 'returns nil when there is no user' do
+        service.current_user = nil
+
+        expect(service.execute(project_board)).to eq nil
+      end
+
+      it 'returns nil when database is read only' do
+        allow(Gitlab::Database).to receive(:read_only?) { true }
+
+        expect(service.execute(project_board)).to eq nil
+      end
+
+      it 'records the visit' do
+        expect(BoardProjectRecentVisit).to receive(:visited!).once
+
+        service.execute(project_board)
+      end
+    end
+
+    context 'when a group board' do
+      let(:group)       { create(:group) }
+      let(:group_board) { create(:board, group: group) }
+
+      subject(:service) { described_class.new(group_board.parent, user) }
+
+      it 'returns nil when there is no user' do
+        service.current_user = nil
+
+        expect(service.execute(group_board)).to eq nil
+      end
+
+      it 'records the visit' do
+        expect(BoardGroupRecentVisit).to receive(:visited!).once
+
+        service.execute(group_board)
+      end
+    end
+  end
+end

spec/services/boards/visits/latest_service_spec.rb

@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Boards::Visits::LatestService do
+  describe '#execute' do
+    let(:user) { create(:user) }
+
+    context 'when a project board' do
+      let(:project)       { create(:project) }
+      let(:project_board) { create(:board, project: project) }
+
+      subject(:service) { described_class.new(project_board.parent, user) }
+
+      it 'returns nil when there is no user' do
+        service.current_user = nil
+
+        expect(service.execute).to eq nil
+      end
+
+      it 'queries for most recent visit' do
+        expect(BoardProjectRecentVisit).to receive(:latest).once
+
+        service.execute
+      end
+    end
+
+    context 'when a group board' do
+      let(:group)       { create(:group) }
+      let(:group_board) { create(:board, group: group) }
+
+      subject(:service) { described_class.new(group_board.parent, user) }
+
+      it 'returns nil when there is no user' do
+        service.current_user = nil
+
+        expect(service.execute).to eq nil
+      end
+
+      it 'queries for most recent visit' do
+        expect(BoardGroupRecentVisit).to receive(:latest).once
+
+        service.execute
+      end
+    end
+  end
+end