diff options
42 files changed, 693 insertions, 29 deletions
@@ -100,7 +100,7 @@ gem 'seed-fu', '~> 2.3.5' # Markdown and HTML processing gem 'html-pipeline', '~> 1.11.0' -gem 'deckar01-task_list', '1.0.5', require: 'task_list/railtie' +gem 'deckar01-task_list', '1.0.6', require: 'task_list/railtie' gem 'gitlab-markup', '~> 1.5.0' gem 'redcarpet', '~> 3.3.3' gem 'RedCloth', '~> 4.3.2' diff --git a/Gemfile.lock b/Gemfile.lock index 3ecff5f6a68..6ea0578d9d2 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -159,7 +159,7 @@ GEM database_cleaner (1.5.3) debug_inspector (0.0.2) debugger-ruby_core_source (1.3.8) - deckar01-task_list (1.0.5) + deckar01-task_list (1.0.6) activesupport (~> 4.0) html-pipeline rack (~> 1.0) @@ -840,7 +840,7 @@ DEPENDENCIES creole (~> 0.5.0) d3_rails (~> 3.5.0) database_cleaner (~> 1.5.0) - deckar01-task_list (= 1.0.5) + deckar01-task_list (= 1.0.6) default_value_for (~> 3.0.0) devise (~> 4.2) devise-two-factor (~> 3.0.0) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index ac0a569d72a..9f104d903cc 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -599,12 +599,27 @@ class Projects::MergeRequestsController < Projects::ApplicationController end def merge_request_params - params.require(:merge_request).permit( - :title, :assignee_id, :source_project_id, :source_branch, - :target_project_id, :target_branch, :milestone_id, - :state_event, :description, :task_num, :force_remove_source_branch, - :lock_version, label_ids: [] - ) + params.require(:merge_request) + .permit(merge_request_params_ce) + end + + def merge_request_params_ce + [ + :assignee_id, + :description, + :force_remove_source_branch, + :lock_version, + :milestone_id, + :source_branch, + :source_project_id, + :state_event, + :target_branch, + :target_project_id, + :task_num, + :title, + + label_ids: [] + ] end def merge_params diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index bce5e29d8d8..6988527a3be 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -335,6 +335,7 @@ class ProjectsController < Projects::ApplicationController :visibility_level, :import_url, :last_activity_at, :namespace_id, :avatar, :build_allow_git_fetch, :build_timeout_in_minutes, :build_coverage_regex, :public_builds, :only_allow_merge_if_build_succeeds, :request_access_enabled, + :only_allow_merge_if_all_discussions_are_resolved, :lfs_enabled, project_feature_attributes ) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 6b8ac3fb48b..d76feb9680e 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -425,6 +425,7 @@ class MergeRequest < ActiveRecord::Base return false if work_in_progress? return false if broken? return false unless skip_ci_check || mergeable_ci_state? + return false unless mergeable_discussions_state? true end @@ -493,6 +494,12 @@ class MergeRequest < ActiveRecord::Base discussions_resolvable? && diff_discussions.none?(&:to_be_resolved?) end + def mergeable_discussions_state? + return true unless project.only_allow_merge_if_all_discussions_are_resolved? + + discussions_resolved? + end + def hook_attrs attrs = { source: source_project.try(:hook_attrs), diff --git a/app/models/project.rb b/app/models/project.rb index cf931f64c03..686d285410b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1067,10 +1067,6 @@ class Project < ActiveRecord::Base forks.count end - def find_label(name) - labels.find_by(name: name) - end - def origin_merge_requests merge_requests.where(source_project_id: self.id) end diff --git a/app/serializers/base_serializer.rb b/app/serializers/base_serializer.rb new file mode 100644 index 00000000000..de9a181db90 --- /dev/null +++ b/app/serializers/base_serializer.rb @@ -0,0 +1,18 @@ +class BaseSerializer + def initialize(parameters = {}) + @request = EntityRequest.new(parameters) + end + + def represent(resource, opts = {}) + self.class.entity_class + .represent(resource, opts.merge(request: @request)) + end + + def self.entity(entity_class) + @entity_class ||= entity_class + end + + def self.entity_class + @entity_class + end +end diff --git a/app/serializers/build_entity.rb b/app/serializers/build_entity.rb new file mode 100644 index 00000000000..3d9ac66de0e --- /dev/null +++ b/app/serializers/build_entity.rb @@ -0,0 +1,24 @@ +class BuildEntity < Grape::Entity + include RequestAwareEntity + + expose :id + expose :name + + expose :build_url do |build| + url_to(:namespace_project_build, build) + end + + expose :retry_url do |build| + url_to(:retry_namespace_project_build, build) + end + + expose :play_url, if: ->(build, _) { build.manual? } do |build| + url_to(:play_namespace_project_build, build) + end + + private + + def url_to(route, build) + send("#{route}_url", build.project.namespace, build.project, build) + end +end diff --git a/app/serializers/commit_entity.rb b/app/serializers/commit_entity.rb new file mode 100644 index 00000000000..f7eba6fc1e3 --- /dev/null +++ b/app/serializers/commit_entity.rb @@ -0,0 +1,12 @@ +class CommitEntity < API::Entities::RepoCommit + include RequestAwareEntity + + expose :author, using: UserEntity + + expose :commit_url do |commit| + namespace_project_tree_url( + request.project.namespace, + request.project, + id: commit.id) + end +end diff --git a/app/serializers/deployment_entity.rb b/app/serializers/deployment_entity.rb new file mode 100644 index 00000000000..ad6fc8d665b --- /dev/null +++ b/app/serializers/deployment_entity.rb @@ -0,0 +1,27 @@ +class DeploymentEntity < Grape::Entity + include RequestAwareEntity + + expose :id + expose :iid + expose :sha + + expose :ref do + expose :name do |deployment| + deployment.ref + end + + expose :ref_url do |deployment| + namespace_project_tree_url( + deployment.project.namespace, + deployment.project, + id: deployment.ref) + end + end + + expose :tag + expose :last? + expose :user, using: UserEntity + expose :commit, using: CommitEntity + expose :deployable, using: BuildEntity + expose :manual_actions, using: BuildEntity +end diff --git a/app/serializers/entity_request.rb b/app/serializers/entity_request.rb new file mode 100644 index 00000000000..456ba1174c0 --- /dev/null +++ b/app/serializers/entity_request.rb @@ -0,0 +1,12 @@ +class EntityRequest + # We use EntityRequest object to collect parameters and variables + # from the controller. Because options that are being passed to the entity + # do appear in each entity object in the chain, we need a way to pass data + # that is present in the controller (see #20045). + # + def initialize(parameters) + parameters.each do |key, value| + define_singleton_method(key) { value } + end + end +end diff --git a/app/serializers/environment_entity.rb b/app/serializers/environment_entity.rb new file mode 100644 index 00000000000..ee4392cc46d --- /dev/null +++ b/app/serializers/environment_entity.rb @@ -0,0 +1,20 @@ +class EnvironmentEntity < Grape::Entity + include RequestAwareEntity + + expose :id + expose :name + expose :state + expose :external_url + expose :environment_type + expose :last_deployment, using: DeploymentEntity + expose :stoppable? + + expose :environment_url do |environment| + namespace_project_environment_url( + environment.project.namespace, + environment.project, + environment) + end + + expose :created_at, :updated_at +end diff --git a/app/serializers/environment_serializer.rb b/app/serializers/environment_serializer.rb new file mode 100644 index 00000000000..91955542f25 --- /dev/null +++ b/app/serializers/environment_serializer.rb @@ -0,0 +1,3 @@ +class EnvironmentSerializer < BaseSerializer + entity EnvironmentEntity +end diff --git a/app/serializers/request_aware_entity.rb b/app/serializers/request_aware_entity.rb new file mode 100644 index 00000000000..ff8c1142abc --- /dev/null +++ b/app/serializers/request_aware_entity.rb @@ -0,0 +1,11 @@ +module RequestAwareEntity + extend ActiveSupport::Concern + + included do + include Gitlab::Routing.url_helpers + end + + def request + @options.fetch(:request) + end +end diff --git a/app/serializers/user_entity.rb b/app/serializers/user_entity.rb new file mode 100644 index 00000000000..43754ea94f7 --- /dev/null +++ b/app/serializers/user_entity.rb @@ -0,0 +1,2 @@ +class UserEntity < API::Entities::UserBasic +end diff --git a/app/views/projects/_merge_request_settings.html.haml b/app/views/projects/_merge_request_settings.html.haml index 80053dd501b..6e143c4b570 100644 --- a/app/views/projects/_merge_request_settings.html.haml +++ b/app/views/projects/_merge_request_settings.html.haml @@ -12,3 +12,7 @@ %span.descr Builds need to be configured to enable this feature. = link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_when_build_succeeds', anchor: 'only-allow-merge-requests-to-be-merged-if-the-build-succeeds') + .checkbox + = f.label :only_allow_merge_if_all_discussions_are_resolved do + = f.check_box :only_allow_merge_if_all_discussions_are_resolved + %strong Only allow merge requests to be merged if all discussions are resolved diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml index 842b6df310d..01314eb37d0 100644 --- a/app/views/projects/merge_requests/widget/_open.html.haml +++ b/app/views/projects/merge_requests/widget/_open.html.haml @@ -23,8 +23,10 @@ = render 'projects/merge_requests/widget/open/merge_when_build_succeeds' - elsif !@merge_request.can_be_merged_by?(current_user) = render 'projects/merge_requests/widget/open/not_allowed' - - elsif !@merge_request.mergeable_ci_state? && @pipeline && @pipeline.failed? + - elsif !@merge_request.mergeable_ci_state? = render 'projects/merge_requests/widget/open/build_failed' + - elsif !@merge_request.mergeable_discussions_state? + = render 'projects/merge_requests/widget/open/unresolved_discussions' - elsif @merge_request.can_be_merged? || resolved_conflicts = render 'projects/merge_requests/widget/open/accept' diff --git a/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml b/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml new file mode 100644 index 00000000000..35d5677ee37 --- /dev/null +++ b/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml @@ -0,0 +1,6 @@ +%h4 + = icon('exclamation-triangle') + This merge request has unresolved discussions + +%p + Please resolve these discussions to allow this merge request to be merged.
\ No newline at end of file diff --git a/changelogs/unreleased/20968-add-setting-to-check-unresolved-discussion.yml b/changelogs/unreleased/20968-add-setting-to-check-unresolved-discussion.yml new file mode 100644 index 00000000000..8f03746ff80 --- /dev/null +++ b/changelogs/unreleased/20968-add-setting-to-check-unresolved-discussion.yml @@ -0,0 +1,4 @@ +--- +title: Add setting to only allow merge requests to be merged when all discussions are resolved +merge_request: 7125 +author: Rodolfo Arruda diff --git a/db/migrate/20160914131004_only_allow_merge_if_all_discussions_are_resolved.rb b/db/migrate/20160914131004_only_allow_merge_if_all_discussions_are_resolved.rb new file mode 100644 index 00000000000..fad62d716b3 --- /dev/null +++ b/db/migrate/20160914131004_only_allow_merge_if_all_discussions_are_resolved.rb @@ -0,0 +1,17 @@ +class OnlyAllowMergeIfAllDiscussionsAreResolved < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + disable_ddl_transaction! + + def up + add_column_with_default(:projects, + :only_allow_merge_if_all_discussions_are_resolved, + :boolean, + default: false) + end + + def down + remove_column(:projects, :only_allow_merge_if_all_discussions_are_resolved) + end +end diff --git a/db/schema.rb b/db/schema.rb index dc088925d97..5476b0c93e5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -905,6 +905,7 @@ ActiveRecord::Schema.define(version: 20161103171205) do t.boolean "has_external_wiki" t.boolean "lfs_enabled" t.text "description_html" + t.boolean "only_allow_merge_if_all_discussions_are_resolved", default: false, null: false end add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree diff --git a/doc/api/projects.md b/doc/api/projects.md index 4f4b20a1874..bbb3bfb4995 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -89,6 +89,7 @@ Parameters: "public_builds": true, "shared_with_groups": [], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false }, { @@ -151,6 +152,7 @@ Parameters: "public_builds": true, "shared_with_groups": [], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ] @@ -429,6 +431,7 @@ Parameters: } ], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ``` @@ -602,6 +605,7 @@ Parameters: | `import_url` | string | no | URL to import repository from | | `public_builds` | boolean | no | If `true`, builds can be viewed by non-project-members | | `only_allow_merge_if_build_succeeds` | boolean | no | Set whether merge requests can only be merged with successful builds | +| `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved | | `lfs_enabled` | boolean | no | Enable LFS | | `request_access_enabled` | boolean | no | Allow users to request member access | @@ -634,6 +638,7 @@ Parameters: | `import_url` | string | no | URL to import repository from | | `public_builds` | boolean | no | If `true`, builds can be viewed by non-project-members | | `only_allow_merge_if_build_succeeds` | boolean | no | Set whether merge requests can only be merged with successful builds | +| `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved | | `lfs_enabled` | boolean | no | Enable LFS | | `request_access_enabled` | boolean | no | Allow users to request member access | @@ -665,6 +670,7 @@ Parameters: | `import_url` | string | no | URL to import repository from | | `public_builds` | boolean | no | If `true`, builds can be viewed by non-project-members | | `only_allow_merge_if_build_succeeds` | boolean | no | Set whether merge requests can only be merged with successful builds | +| `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved | | `lfs_enabled` | boolean | no | Enable LFS | | `request_access_enabled` | boolean | no | Allow users to request member access | @@ -752,6 +758,7 @@ Example response: "public_builds": true, "shared_with_groups": [], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ``` @@ -820,6 +827,7 @@ Example response: "public_builds": true, "shared_with_groups": [], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ``` @@ -908,6 +916,7 @@ Example response: "public_builds": true, "shared_with_groups": [], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ``` @@ -996,6 +1005,7 @@ Example response: "public_builds": true, "shared_with_groups": [], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ``` diff --git a/doc/user/project/merge_requests/img/only_allow_merge_if_all_discussions_are_resolved.png b/doc/user/project/merge_requests/img/only_allow_merge_if_all_discussions_are_resolved.png Binary files differnew file mode 100644 index 00000000000..52c8acf15e0 --- /dev/null +++ b/doc/user/project/merge_requests/img/only_allow_merge_if_all_discussions_are_resolved.png diff --git a/doc/user/project/merge_requests/img/only_allow_merge_if_all_discussions_are_resolved_msg.png b/doc/user/project/merge_requests/img/only_allow_merge_if_all_discussions_are_resolved_msg.png Binary files differnew file mode 100644 index 00000000000..79ba5c362c7 --- /dev/null +++ b/doc/user/project/merge_requests/img/only_allow_merge_if_all_discussions_are_resolved_msg.png diff --git a/doc/user/project/merge_requests/merge_request_discussion_resolution.md b/doc/user/project/merge_requests/merge_request_discussion_resolution.md index 2559f5f5250..285b1798ac5 100644 --- a/doc/user/project/merge_requests/merge_request_discussion_resolution.md +++ b/doc/user/project/merge_requests/merge_request_discussion_resolution.md @@ -33,7 +33,25 @@ resolved discussions tracker. !["3/4 discussions resolved"][discussions-resolved] +## Only allow merge requests to be merged if all discussions are resolved + +> [Introduced][ce-7125] in GitLab 8.14. + +You can prevent merge requests from being merged until all discussions are resolved. + +Navigate to your project's settings page, select the +**Only allow merge requests to be merged if all discussions are resolved** check +box and hit **Save** for the changes to take effect. + + + +From now on, you will not be able to merge from the UI until all discussions +are resolved. + + + [ce-5022]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5022 +[ce-7125]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7125 [resolve-discussion-button]: img/resolve_discussion_button.png [resolve-comment-button]: img/resolve_comment_button.png [discussion-view]: img/discussion_view.png diff --git a/doc/user/project/merge_requests/merge_when_build_succeeds.md b/doc/user/project/merge_requests/merge_when_build_succeeds.md index c138061fd40..d4e5b5de685 100644 --- a/doc/user/project/merge_requests/merge_when_build_succeeds.md +++ b/doc/user/project/merge_requests/merge_when_build_succeeds.md @@ -40,7 +40,7 @@ hit **Save** for the changes to take effect.  -From now on, every time the pipelinefails you will not be able to merge the +From now on, every time the pipeline fails you will not be able to merge the merge request from the UI, until you make all relevant builds pass. - + diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 1f378ba1635..01e31f6f7d1 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -100,6 +100,7 @@ module API end expose :only_allow_merge_if_build_succeeds expose :request_access_enabled + expose :only_allow_merge_if_all_discussions_are_resolved end class Member < UserBasic diff --git a/lib/api/labels.rb b/lib/api/labels.rb index 326e1e7ae00..238cea00fba 100644 --- a/lib/api/labels.rb +++ b/lib/api/labels.rb @@ -25,7 +25,7 @@ module API post ':id/labels' do authorize! :admin_label, user_project - label = user_project.find_label(params[:name]) + label = available_labels.find_by(title: params[:name]) conflict!('Label already exists') if label label = user_project.labels.create(declared(params, include_parent_namespaces: false).to_h) @@ -46,7 +46,7 @@ module API delete ':id/labels' do authorize! :admin_label, user_project - label = user_project.find_label(params[:name]) + label = user_project.labels.find_by(title: params[:name]) not_found!('Label') unless label present label.destroy, with: Entities::Label, current_user: current_user @@ -65,7 +65,7 @@ module API put ':id/labels' do authorize! :admin_label, user_project - label = user_project.find_label(params[:name]) + label = user_project.labels.find_by(title: params[:name]) not_found!('Label not found') unless label update_params = declared(params, diff --git a/lib/api/projects.rb b/lib/api/projects.rb index da16e24d7ea..6b856128c2e 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -139,7 +139,8 @@ module API :shared_runners_enabled, :snippets_enabled, :visibility_level, - :wiki_enabled] + :wiki_enabled, + :only_allow_merge_if_all_discussions_are_resolved] attrs = map_public_to_visibility_level(attrs) @project = ::Projects::CreateService.new(current_user, attrs).execute if @project.saved? @@ -193,7 +194,8 @@ module API :shared_runners_enabled, :snippets_enabled, :visibility_level, - :wiki_enabled] + :wiki_enabled, + :only_allow_merge_if_all_discussions_are_resolved] attrs = map_public_to_visibility_level(attrs) @project = ::Projects::CreateService.new(user, attrs).execute if @project.saved? @@ -275,7 +277,8 @@ module API :shared_runners_enabled, :snippets_enabled, :visibility_level, - :wiki_enabled] + :wiki_enabled, + :only_allow_merge_if_all_discussions_are_resolved] attrs = map_public_to_visibility_level(attrs) authorize_admin_project authorize! :rename_project, user_project if attrs[:name].present? diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 940d54f8686..49127aecc63 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -297,6 +297,72 @@ describe Projects::MergeRequestsController do end end end + + describe 'only_allow_merge_if_all_discussions_are_resolved? setting' do + let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project, author: user) } + + context 'when enabled' do + before do + project.update_column(:only_allow_merge_if_all_discussions_are_resolved, true) + end + + context 'with unresolved discussion' do + before do + expect(merge_request).not_to be_discussions_resolved + end + + it 'returns :failed' do + merge_with_sha + + expect(assigns(:status)).to eq(:failed) + end + end + + context 'with all discussions resolved' do + before do + merge_request.discussions.each { |d| d.resolve!(user) } + expect(merge_request).to be_discussions_resolved + end + + it 'returns :success' do + merge_with_sha + + expect(assigns(:status)).to eq(:success) + end + end + end + + context 'when disabled' do + before do + project.update_column(:only_allow_merge_if_all_discussions_are_resolved, false) + end + + context 'with unresolved discussion' do + before do + expect(merge_request).not_to be_discussions_resolved + end + + it 'returns :success' do + merge_with_sha + + expect(assigns(:status)).to eq(:success) + end + end + + context 'with all discussions resolved' do + before do + merge_request.discussions.each { |d| d.resolve!(user) } + expect(merge_request).to be_discussions_resolved + end + + it 'returns :success' do + merge_with_sha + + expect(assigns(:status)).to eq(:success) + end + end + end + end end end diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index f780e01253c..37eb49c94df 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -68,6 +68,11 @@ FactoryGirl.define do factory :closed_merge_request, traits: [:closed] factory :reopened_merge_request, traits: [:reopened] factory :merge_request_with_diffs, traits: [:with_diffs] + factory :merge_request_with_diff_notes do + after(:create) do |mr| + create(:diff_note_on_merge_request, noteable: mr, project: mr.source_project) + end + end factory :labeled_merge_request do transient do diff --git a/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb b/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb new file mode 100644 index 00000000000..7f11db3c417 --- /dev/null +++ b/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb @@ -0,0 +1,69 @@ +require 'spec_helper' + +feature 'Check if mergeable with unresolved discussions', js: true, feature: true do + let(:user) { create(:user) } + let(:project) { create(:project) } + let!(:merge_request) { create(:merge_request_with_diff_notes, source_project: project, author: user) } + + before do + login_as user + project.team << [user, :master] + end + + context 'when project.only_allow_merge_if_all_discussions_are_resolved == true' do + before do + project.update_column(:only_allow_merge_if_all_discussions_are_resolved, true) + end + + context 'with unresolved discussions' do + it 'does not allow to merge' do + visit_merge_request(merge_request) + + expect(page).not_to have_button 'Accept Merge Request' + expect(page).to have_content('This merge request has unresolved discussions') + end + end + + context 'with all discussions resolved' do + before do + merge_request.discussions.each { |d| d.resolve!(user) } + end + + it 'allows MR to be merged' do + visit_merge_request(merge_request) + + expect(page).to have_button 'Accept Merge Request' + end + end + end + + context 'when project.only_allow_merge_if_all_discussions_are_resolved == false' do + before do + project.update_column(:only_allow_merge_if_all_discussions_are_resolved, false) + end + + context 'with unresolved discussions' do + it 'does not allow to merge' do + visit_merge_request(merge_request) + + expect(page).to have_button 'Accept Merge Request' + end + end + + context 'with all discussions resolved' do + before do + merge_request.discussions.each { |d| d.resolve!(user) } + end + + it 'allows MR to be merged' do + visit_merge_request(merge_request) + + expect(page).to have_button 'Accept Merge Request' + end + end + end + + def visit_merge_request(merge_request) + visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request) + end +end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 1067ff7bb4d..fb032a89d50 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -825,11 +825,8 @@ describe MergeRequest, models: true do end context 'when failed' do - before { allow(subject).to receive(:broken?) { false } } - - context 'when project settings restrict to merge only if build succeeds and build failed' do + context 'when #mergeable_ci_state? is false' do before do - project.only_allow_merge_if_build_succeeds = true allow(subject).to receive(:mergeable_ci_state?) { false } end @@ -837,6 +834,16 @@ describe MergeRequest, models: true do expect(subject.mergeable_state?).to be_falsey end end + + context 'when #mergeable_discussions_state? is false' do + before do + allow(subject).to receive(:mergeable_discussions_state?) { false } + end + + it 'returns false' do + expect(subject.mergeable_state?).to be_falsey + end + end end end @@ -887,7 +894,49 @@ describe MergeRequest, models: true do end end - describe '#environments' do + describe '#mergeable_discussions_state?' do + let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project) } + + context 'when project.only_allow_merge_if_all_discussions_are_resolved == true' do + let(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: true) } + + context 'with all discussions resolved' do + before do + merge_request.discussions.each { |d| d.resolve!(merge_request.author) } + end + + it 'returns true' do + expect(merge_request.mergeable_discussions_state?).to be_truthy + end + end + + context 'with unresolved discussions' do + before do + merge_request.discussions.each(&:unresolve!) + end + + it 'returns false' do + expect(merge_request.mergeable_discussions_state?).to be_falsey + end + end + end + + context 'when project.only_allow_merge_if_all_discussions_are_resolved == false' do + let(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: false) } + + context 'with unresolved discussions' do + before do + merge_request.discussions.each(&:unresolve!) + end + + it 'returns true' do + expect(merge_request.mergeable_discussions_state?).to be_truthy + end + end + end + end + + describe "#environments" do let(:project) { create(:project) } let(:merge_request) { create(:merge_request, source_project: project) } diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb index 46641fcd846..f702dfaaf53 100644 --- a/spec/requests/api/labels_spec.rb +++ b/spec/requests/api/labels_spec.rb @@ -82,7 +82,20 @@ describe API::API, api: true do expect(json_response['message']['title']).to eq(['is invalid']) end - it 'returns 409 if label already exists' do + it 'returns 409 if label already exists in group' do + group = create(:group) + group_label = create(:group_label, group: group) + project.update(group: group) + + post api("/projects/#{project.id}/labels", user), + name: group_label.name, + color: '#FFAABB' + + expect(response).to have_http_status(409) + expect(json_response['message']).to eq('Label already exists') + end + + it 'returns 409 if label already exists in project' do post api("/projects/#{project.id}/labels", user), name: 'label1', color: '#FFAABB' diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 973928d007a..3c8f0ac531a 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -256,7 +256,8 @@ describe API::API, api: true do merge_requests_enabled: false, wiki_enabled: false, only_allow_merge_if_build_succeeds: false, - request_access_enabled: true + request_access_enabled: true, + only_allow_merge_if_all_discussions_are_resolved: false }) post api('/projects', user), project @@ -327,6 +328,22 @@ describe API::API, api: true do expect(json_response['only_allow_merge_if_build_succeeds']).to be_truthy end + it 'sets a project as allowing merge even if discussions are unresolved' do + project = attributes_for(:project, { only_allow_merge_if_all_discussions_are_resolved: false }) + + post api('/projects', user), project + + expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to be_falsey + end + + it 'sets a project as allowing merge only if all discussions are resolved' do + project = attributes_for(:project, { only_allow_merge_if_all_discussions_are_resolved: true }) + + post api('/projects', user), project + + expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to be_truthy + end + context 'when a visibility level is restricted' do before do @project = attributes_for(:project, { public: true }) @@ -448,6 +465,22 @@ describe API::API, api: true do post api("/projects/user/#{user.id}", admin), project expect(json_response['only_allow_merge_if_build_succeeds']).to be_truthy end + + it 'sets a project as allowing merge even if discussions are unresolved' do + project = attributes_for(:project, { only_allow_merge_if_all_discussions_are_resolved: false }) + + post api("/projects/user/#{user.id}", admin), project + + expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to be_falsey + end + + it 'sets a project as allowing merge only if all discussions are resolved' do + project = attributes_for(:project, { only_allow_merge_if_all_discussions_are_resolved: true }) + + post api("/projects/user/#{user.id}", admin), project + + expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to be_truthy + end end describe "POST /projects/:id/uploads" do @@ -509,6 +542,7 @@ describe API::API, api: true do expect(json_response['shared_with_groups'][0]['group_name']).to eq(group.name) expect(json_response['shared_with_groups'][0]['group_access_level']).to eq(link.group_access) expect(json_response['only_allow_merge_if_build_succeeds']).to eq(project.only_allow_merge_if_build_succeeds) + expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to eq(project.only_allow_merge_if_all_discussions_are_resolved) end it 'returns a project by path name' do diff --git a/spec/serializers/build_entity_spec.rb b/spec/serializers/build_entity_spec.rb new file mode 100644 index 00000000000..2734f5bedca --- /dev/null +++ b/spec/serializers/build_entity_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe BuildEntity do + let(:entity) do + described_class.new(build, request: double) + end + + subject { entity.as_json } + + context 'when build is a regular job' do + let(:build) { create(:ci_build) } + + it 'contains url to build page and retry action' do + expect(subject).to include(:build_url, :retry_url) + expect(subject).not_to include(:play_url) + end + + it 'does not contain sensitive information' do + expect(subject).not_to include(/token/) + expect(subject).not_to include(/variables/) + end + end + + context 'when build is a manual action' do + let(:build) { create(:ci_build, :manual) } + + it 'contains url to play action' do + expect(subject).to include(:play_url) + end + end +end diff --git a/spec/serializers/commit_entity_spec.rb b/spec/serializers/commit_entity_spec.rb new file mode 100644 index 00000000000..628e35c9a28 --- /dev/null +++ b/spec/serializers/commit_entity_spec.rb @@ -0,0 +1,44 @@ +require 'spec_helper' + +describe CommitEntity do + let(:entity) do + described_class.new(commit, request: request) + end + + let(:request) { double('request') } + let(:project) { create(:project) } + let(:commit) { project.commit } + + subject { entity.as_json } + + before do + allow(request).to receive(:project).and_return(project) + end + + context 'when commit author is a user' do + before do + create(:user, email: commit.author_email) + end + + it 'contains information about user' do + expect(subject.fetch(:author)).not_to be_nil + end + end + + context 'when commit author is not a user' do + it 'does not contain author details' do + expect(subject.fetch(:author)).to be_nil + end + end + + it 'contains commit URL' do + expect(subject).to include(:commit_url) + end + + it 'needs to receive project in the request' do + expect(request).to receive(:project) + .and_return(project) + + subject + end +end diff --git a/spec/serializers/deployment_entity_spec.rb b/spec/serializers/deployment_entity_spec.rb new file mode 100644 index 00000000000..51b6de91571 --- /dev/null +++ b/spec/serializers/deployment_entity_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' + +describe DeploymentEntity do + let(:entity) do + described_class.new(deployment, request: double) + end + + let(:deployment) { create(:deployment) } + + subject { entity.as_json } + + it 'exposes internal deployment id' do + expect(subject).to include(:iid) + end + + it 'exposes nested information about branch' do + expect(subject[:ref][:name]).to eq 'master' + expect(subject[:ref][:ref_url]).not_to be_empty + end +end diff --git a/spec/serializers/entity_request_spec.rb b/spec/serializers/entity_request_spec.rb new file mode 100644 index 00000000000..86654adfd54 --- /dev/null +++ b/spec/serializers/entity_request_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +describe EntityRequest do + subject do + described_class.new(user: 'user', project: 'some project') + end + + describe 'methods created' do + it 'defines accessible attributes' do + expect(subject.user).to eq 'user' + expect(subject.project).to eq 'some project' + end + + it 'raises error when attribute is not defined' do + expect { subject.some_method }.to raise_error NoMethodError + end + end +end diff --git a/spec/serializers/environment_entity_spec.rb b/spec/serializers/environment_entity_spec.rb new file mode 100644 index 00000000000..4ca8c299147 --- /dev/null +++ b/spec/serializers/environment_entity_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +describe EnvironmentEntity do + let(:entity) do + described_class.new(environment, request: double) + end + + let(:environment) { create(:environment) } + subject { entity.as_json } + + it 'exposes latest deployment' do + expect(subject).to include(:last_deployment) + end + + it 'exposes core elements of environment' do + expect(subject).to include(:id, :name, :state, :environment_url) + end +end diff --git a/spec/serializers/environment_serializer_spec.rb b/spec/serializers/environment_serializer_spec.rb new file mode 100644 index 00000000000..37bc086826c --- /dev/null +++ b/spec/serializers/environment_serializer_spec.rb @@ -0,0 +1,60 @@ +require 'spec_helper' + +describe EnvironmentSerializer do + let(:serializer) do + described_class + .new(user: user, project: project) + .represent(resource) + end + + let(:json) { serializer.as_json } + let(:user) { create(:user) } + let(:project) { create(:project) } + + context 'when there is a single object provided' do + before do + create(:ci_build, :manual, name: 'manual1', + pipeline: deployable.pipeline) + end + + let(:deployment) do + create(:deployment, deployable: deployable, + user: user, + project: project, + sha: project.commit.id) + end + + let(:deployable) { create(:ci_build) } + let(:resource) { deployment.environment } + + it 'it generates payload for single object' do + expect(json).to be_an_instance_of Hash + end + + it 'contains important elements of environment' do + expect(json) + .to include(:name, :external_url, :environment_url, :last_deployment) + end + + it 'contains relevant information about last deployment' do + last_deployment = json.fetch(:last_deployment) + + expect(last_deployment) + .to include(:ref, :user, :commit, :deployable, :manual_actions) + end + end + + context 'when there is a collection of objects provided' do + let(:project) { create(:empty_project) } + let(:resource) { create_list(:environment, 2) } + + it 'contains important elements of environment' do + expect(json.first) + .to include(:last_deployment, :name, :external_url) + end + + it 'generates payload for collection' do + expect(json).to be_an_instance_of Array + end + end +end diff --git a/spec/serializers/user_entity_spec.rb b/spec/serializers/user_entity_spec.rb new file mode 100644 index 00000000000..c5d11cbcf5e --- /dev/null +++ b/spec/serializers/user_entity_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe UserEntity do + let(:entity) { described_class.new(user) } + let(:user) { create(:user) } + subject { entity.as_json } + + it 'exposes user name and login' do + expect(subject).to include(:username, :name) + end + + it 'does not expose passwords' do + expect(subject).not_to include(/password/) + end + + it 'does not expose tokens' do + expect(subject).not_to include(/token/) + end + + it 'does not expose 2FA OTPs' do + expect(subject).not_to include(/otp/) + end +end |
