From f680eca912f854373fc538ae1e9d0dcb60fcd310 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 1 Jun 2016 13:25:44 +0100 Subject: Don't allow merges with new commits Set a `sha` parameter on the MR form. If this doesn't match the HEAD of the source branch when the form is submitted, show a warning (like with a merge conflict) and don't merge the branch. --- app/controllers/projects/merge_requests_controller.rb | 5 +++++ app/views/projects/merge_requests/merge.js.haml | 3 +++ app/views/projects/merge_requests/widget/open/_accept.html.haml | 1 + .../merge_requests/widget/open/_merge_when_build_succeeds.html.haml | 2 +- .../projects/merge_requests/widget/open/_sha_mismatch.html.haml | 6 ++++++ 5 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml (limited to 'app') diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index d54284d7b20..3142fe5c767 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -190,6 +190,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController return end + if params[:sha] != @merge_request.source_sha + @status = :sha_mismatch + return + end + TodoService.new.merge_merge_request(merge_request, current_user) @merge_request.update(merge_error: nil) diff --git a/app/views/projects/merge_requests/merge.js.haml b/app/views/projects/merge_requests/merge.js.haml index 92ce479d463..84b6c9ebc5c 100644 --- a/app/views/projects/merge_requests/merge.js.haml +++ b/app/views/projects/merge_requests/merge.js.haml @@ -5,6 +5,9 @@ - when :merge_when_build_succeeds :plain $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/merge_when_build_succeeds'))}"); +- when :sha_mismatch + :plain + $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/sha_mismatch'))}"); - else :plain $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/reload'))}"); diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml index cfdf4edac37..0d49b6471a9 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -2,6 +2,7 @@ = form_for [:merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-quick-submit js-requires-input' } do |f| = hidden_field_tag :authenticity_token, form_authenticity_token + = hidden_field_tag :sha, @merge_request.source_sha .accept-merge-holder.clearfix.js-toggle-container .clearfix .accept-action diff --git a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml index b83ddcab3a4..ad898ff153b 100644 --- a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml +++ b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml @@ -16,7 +16,7 @@ - if remove_source_branch_button || user_can_cancel_automatic_merge .clearfix.prepend-top-10 - if remove_source_branch_button - = link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do + = link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true, sha: @merge_request.source_sha), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do = icon('times') Remove Source Branch When Merged diff --git a/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml b/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml new file mode 100644 index 00000000000..a78583b7c61 --- /dev/null +++ b/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml @@ -0,0 +1,6 @@ +%h4 + = icon("exclamation-triangle") + This merge request has new commits since the page was loaded. + +%p + Please review the new commits before merging. -- cgit v1.2.3 From 4f726683cb59da54f47302880d5c0c447638402a Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 2 Jun 2016 17:00:42 +0100 Subject: fixup! Don't allow merges with new commits --- app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml b/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml index a78583b7c61..499624f8dd8 100644 --- a/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml +++ b/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml @@ -1,6 +1,6 @@ %h4 = icon("exclamation-triangle") - This merge request has new commits since the page was loaded. + This merge request has received new commits since the page was loaded. %p - Please review the new commits before merging. + Please reload the page to review the new commits before merging. -- cgit v1.2.3 From 905e8b6b545dfb6bc408824889b66d47aa02eda2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 2 Jun 2016 18:14:33 +0200 Subject: Remove unused Issuable#is_assigned? method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/concerns/issuable.rb | 4 ---- 1 file changed, 4 deletions(-) (limited to 'app') diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 50f5b749e38..37124379c18 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -171,10 +171,6 @@ module Issuable today? && created_at == updated_at end - def is_assigned? - !!assignee_id - end - def is_being_reassigned? assignee_id_changed? end -- cgit v1.2.3 From c3e923c496b7d1c344a5fa68cef4a80ce23c90d0 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Wed, 25 May 2016 18:52:10 -0700 Subject: Ensure we don't show TODOS for projects pending delete By joining the Todos on the project table. --- app/finders/todos_finder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app') diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 4bd46a76087..f638b5bf91f 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -23,7 +23,7 @@ class TodosFinder end def execute - items = current_user.todos + items = current_user.todos.joins(:project).where(projects: { pending_delete: false }) items = by_action_id(items) items = by_author(items) items = by_project(items) -- cgit v1.2.3 From 4ecc10fade61a1b45cd45ea4189e95a2acbea353 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Wed, 25 May 2016 21:31:36 -0700 Subject: Reduce the filters on the todos joins project query by being explicit about the join --- app/finders/todos_finder.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'app') diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index f638b5bf91f..3243d62fc95 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -23,7 +23,13 @@ class TodosFinder end def execute - items = current_user.todos.joins(:project).where(projects: { pending_delete: false }) + items = current_user.todos + + # Filter out todos linked to project pending deletion + items = items.joins( + 'INNER JOIN projects ON projects.id = todos.project_id AND projects.pending_delete = false' + ) + items = by_action_id(items) items = by_author(items) items = by_project(items) -- cgit v1.2.3 From 4280575fc0888632196cf4483dcd777618c13390 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Mon, 30 May 2016 10:59:14 -0700 Subject: Move filtering todos by projects not pending deletion into a scope on the todo model --- app/finders/todos_finder.rb | 8 +------- app/models/todo.rb | 1 + 2 files changed, 2 insertions(+), 7 deletions(-) (limited to 'app') diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 3243d62fc95..5d7d55180e1 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -23,13 +23,7 @@ class TodosFinder end def execute - items = current_user.todos - - # Filter out todos linked to project pending deletion - items = items.joins( - 'INNER JOIN projects ON projects.id = todos.project_id AND projects.pending_delete = false' - ) - + items = current_user.todos.not_pending_delete items = by_action_id(items) items = by_author(items) items = by_project(items) diff --git a/app/models/todo.rb b/app/models/todo.rb index 3a091373329..f66755436ea 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -19,6 +19,7 @@ class Todo < ActiveRecord::Base scope :pending, -> { with_state(:pending) } scope :done, -> { with_state(:done) } + scope :not_pending_delete, -> { joins('INNER JOIN projects ON projects.id = todos.project_id AND projects.pending_delete = false') } state_machine :state, initial: :pending do event :done do -- cgit v1.2.3 From b173ea2bd4bbc65529b827f9afa5999f6f04579e Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Wed, 1 Jun 2016 16:44:35 -0700 Subject: Use the project finder in the todos finder to limit todos to just ones within projects you have access to. --- app/finders/todos_finder.rb | 14 +++++++++++++- app/models/todo.rb | 1 - 2 files changed, 13 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 5d7d55180e1..6fbe68a720d 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -23,7 +23,7 @@ class TodosFinder end def execute - items = current_user.todos.not_pending_delete + items = current_user.todos items = by_action_id(items) items = by_author(items) items = by_project(items) @@ -78,6 +78,16 @@ class TodosFinder @project end + def projects + return @projects if defined?(@projects) + + if project? + @projects = project + else + @projects = ProjectsFinder.new.execute(current_user).reorder(nil) + end + end + def type? type.present? && ['Issue', 'MergeRequest'].include?(type) end @@ -105,6 +115,8 @@ class TodosFinder def by_project(items) if project? items = items.where(project: project) + elsif projects + items = items.merge(projects).joins(:project) end items diff --git a/app/models/todo.rb b/app/models/todo.rb index f66755436ea..3a091373329 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -19,7 +19,6 @@ class Todo < ActiveRecord::Base scope :pending, -> { with_state(:pending) } scope :done, -> { with_state(:done) } - scope :not_pending_delete, -> { joins('INNER JOIN projects ON projects.id = todos.project_id AND projects.pending_delete = false') } state_machine :state, initial: :pending do event :done do -- cgit v1.2.3 From f0ca487cd513d50c807e4226a1ee459586f16b08 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Thu, 2 Jun 2016 14:17:46 -0700 Subject: Reorder the todos because the use of the project finder attempts to order them differently --- app/finders/todos_finder.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 6fbe68a720d..1d88116d7d2 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -30,7 +30,7 @@ class TodosFinder items = by_state(items) items = by_type(items) - items + items.reorder(id: :desc) end private @@ -84,7 +84,7 @@ class TodosFinder if project? @projects = project else - @projects = ProjectsFinder.new.execute(current_user).reorder(nil) + @projects = ProjectsFinder.new.execute(current_user) end end -- cgit v1.2.3