diff options
29 files changed, 347 insertions, 52 deletions
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index ed17b3b4689..b13034d3333 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -323,6 +323,7 @@ class ProjectsController < Projects::ApplicationController :build_allow_git_fetch, :build_coverage_regex, :build_timeout_in_minutes, + :resolve_outdated_diff_discussions, :container_registry_enabled, :default_branch, :description, diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 8c5e258f519..083ace65b09 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -146,4 +146,8 @@ module NotesHelper autocomplete: autocomplete } end + + def discussion_resolved_intro(discussion) + discussion.resolved_by_push? ? 'Automatically resolved' : 'Resolved' + end end diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb index dd979e7bb17..f006a271327 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/app/models/concerns/resolvable_discussion.rb @@ -24,6 +24,7 @@ module ResolvableDiscussion delegate :resolved_at, :resolved_by, + :resolved_by_push?, to: :last_resolved_note, allow_nil: true diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb index 05eb6f86704..668c5a079e3 100644 --- a/app/models/concerns/resolvable_note.rb +++ b/app/models/concerns/resolvable_note.rb @@ -51,22 +51,34 @@ module ResolvableNote end # If you update this method remember to also update `.resolve!` - def resolve!(current_user) - return unless resolvable? - return if resolved? + def resolve_without_save(current_user, resolved_by_push: false) + return false unless resolvable? + return false if resolved? self.resolved_at = Time.now self.resolved_by = current_user - save! + self.resolved_by_push = resolved_by_push + + true end # If you update this method remember to also update `.unresolve!` - def unresolve! - return unless resolvable? - return unless resolved? + def unresolve_without_save + return false unless resolvable? + return false unless resolved? self.resolved_at = nil self.resolved_by = nil - save! + + true + end + + def resolve!(current_user, resolved_by_push: false) + resolve_without_save(current_user, resolved_by_push: resolved_by_push) && + save! + end + + def unresolve! + unresolve_without_save && save! end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 724fb4ccef1..b82f49d7073 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -918,6 +918,12 @@ class MergeRequest < ActiveRecord::Base active_diff_discussions.each do |discussion| service.execute(discussion) end + + if project.resolve_outdated_diff_discussions? + MergeRequests::ResolvedDiscussionNotificationService + .new(project, current_user) + .execute(self) + end end def keep_around_commit diff --git a/app/models/project.rb b/app/models/project.rb index 3d89dabd96f..fdd516ec2ae 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -37,6 +37,7 @@ class Project < ActiveRecord::Base default_value_for :archived, false default_value_for :visibility_level, gitlab_config_features.visibility_level + default_value_for :resolve_outdated_diff_discussions, false default_value_for :container_registry_enabled, gitlab_config_features.container_registry default_value_for(:repository_storage) { current_application_settings.pick_repository_storage } default_value_for(:shared_runners_enabled) { current_application_settings.shared_runners_enabled } diff --git a/app/services/discussions/update_diff_position_service.rb b/app/services/discussions/update_diff_position_service.rb index 1ef8d9edbe1..746f209e20f 100644 --- a/app/services/discussions/update_diff_position_service.rb +++ b/app/services/discussions/update_diff_position_service.rb @@ -10,6 +10,10 @@ module Discussions discussion.notes.each do |note| if outdated note.change_position = position + + if project.resolve_outdated_diff_discussions? + note.resolve_without_save(current_user, resolved_by_push: true) + end else note.position = position note.change_position = nil diff --git a/app/views/discussions/_headline.html.haml b/app/views/discussions/_headline.html.haml index 25e90924413..b865eb815f0 100644 --- a/app/views/discussions/_headline.html.haml +++ b/app/views/discussions/_headline.html.haml @@ -1,9 +1,11 @@ - if discussion.resolved? .discussion-headline-light.js-discussion-headline - Resolved + = discussion_resolved_intro(discussion) - if discussion.resolved_by by = link_to_member(@project, discussion.resolved_by, avatar: false) + - if discussion.resolved_by_push? + with a push = time_ago_with_tooltip(discussion.resolved_at, placement: "bottom") - elsif discussion.updated? .discussion-headline-light.js-discussion-headline diff --git a/app/views/projects/_merge_request_merge_settings.html.haml b/app/views/projects/_merge_request_merge_settings.html.haml index 61420fd0fb6..e3effe45d6c 100644 --- a/app/views/projects/_merge_request_merge_settings.html.haml +++ b/app/views/projects/_merge_request_merge_settings.html.haml @@ -14,6 +14,10 @@ = form.check_box :only_allow_merge_if_all_discussions_are_resolved %strong Only allow merge requests to be merged if all discussions are resolved .checkbox + = form.label :resolve_outdated_diff_discussions do + = form.check_box :resolve_outdated_diff_discussions + %strong Automatically resolve merge request diff discussions when they become outdated + .checkbox = form.label :printing_merge_request_link_enabled do = form.check_box :printing_merge_request_link_enabled %strong Show link to create/view merge request when pushing from the command line diff --git a/changelogs/unreleased/36994-toggle-for-automatically-collapsing-outdated-diff-comments.yml b/changelogs/unreleased/36994-toggle-for-automatically-collapsing-outdated-diff-comments.yml new file mode 100644 index 00000000000..83f6b2d21e1 --- /dev/null +++ b/changelogs/unreleased/36994-toggle-for-automatically-collapsing-outdated-diff-comments.yml @@ -0,0 +1,5 @@ +--- +title: Add repository toggle for automatically resolving outdated diff discussions +merge_request: 14053 +author: AshleyDumaine +type: added diff --git a/db/migrate/20170825154015_resolve_outdated_diff_discussions.rb b/db/migrate/20170825154015_resolve_outdated_diff_discussions.rb new file mode 100644 index 00000000000..235530bb1e6 --- /dev/null +++ b/db/migrate/20170825154015_resolve_outdated_diff_discussions.rb @@ -0,0 +1,9 @@ +class ResolveOutdatedDiffDiscussions < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column(:projects, :resolve_outdated_diff_discussions, :boolean) + end +end diff --git a/db/migrate/20170905112933_add_resolved_by_push_to_notes.rb b/db/migrate/20170905112933_add_resolved_by_push_to_notes.rb new file mode 100644 index 00000000000..ceb31ffb08a --- /dev/null +++ b/db/migrate/20170905112933_add_resolved_by_push_to_notes.rb @@ -0,0 +1,9 @@ +class AddResolvedByPushToNotes < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :notes, :resolved_by_push, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index f980667a38f..69911ce7107 100644 --- a/db/schema.rb +++ b/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: 20170901071411) do +ActiveRecord::Schema.define(version: 20170905112933) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1002,6 +1002,7 @@ ActiveRecord::Schema.define(version: 20170901071411) do t.text "note_html" t.integer "cached_markdown_version" t.text "change_position" + t.boolean "resolved_by_push" end add_index "notes", ["author_id"], name: "index_notes_on_author_id", using: :btree @@ -1219,6 +1220,7 @@ ActiveRecord::Schema.define(version: 20170901071411) do t.string "ci_config_path" t.text "delete_error" t.integer "storage_version", limit: 2 + t.boolean "resolve_outdated_diff_discussions" 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 d3f8e509612..2d973547d14 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -70,6 +70,7 @@ Parameters: "jobs_enabled": true, "wiki_enabled": true, "snippets_enabled": false, + "resolve_outdated_diff_discussions": false, "container_registry_enabled": false, "created_at": "2013-09-30T13:46:02Z", "last_activity_at": "2013-09-30T13:46:02Z", @@ -137,6 +138,7 @@ Parameters: "jobs_enabled": true, "wiki_enabled": true, "snippets_enabled": false, + "resolve_outdated_diff_discussions": false, "container_registry_enabled": false, "created_at": "2013-09-30T13:46:02Z", "last_activity_at": "2013-09-30T13:46:02Z", @@ -246,6 +248,7 @@ Parameters: "jobs_enabled": true, "wiki_enabled": true, "snippets_enabled": false, + "resolve_outdated_diff_discussions": false, "container_registry_enabled": false, "created_at": "2013-09-30T13:46:02Z", "last_activity_at": "2013-09-30T13:46:02Z", @@ -313,6 +316,7 @@ Parameters: "jobs_enabled": true, "wiki_enabled": true, "snippets_enabled": false, + "resolve_outdated_diff_discussions": false, "container_registry_enabled": false, "created_at": "2013-09-30T13:46:02Z", "last_activity_at": "2013-09-30T13:46:02Z", @@ -411,6 +415,7 @@ Parameters: "jobs_enabled": true, "wiki_enabled": true, "snippets_enabled": false, + "resolve_outdated_diff_discussions": false, "container_registry_enabled": false, "created_at": "2013-09-30T13:46:02Z", "last_activity_at": "2013-09-30T13:46:02Z", @@ -537,6 +542,7 @@ Parameters: | `jobs_enabled` | boolean | no | Enable jobs for this project | | `wiki_enabled` | boolean | no | Enable wiki for this project | | `snippets_enabled` | boolean | no | Enable snippets for this project | +| `resolve_outdated_diff_discussions` | boolean | no | Automatically resolve merge request diffs discussions on lines changed with a push | | `container_registry_enabled` | boolean | no | Enable container registry for this project | | `shared_runners_enabled` | boolean | no | Enable shared runners for this project | | `visibility` | string | no | See [project visibility level](#project-visibility-level) | @@ -574,6 +580,7 @@ Parameters: | `jobs_enabled` | boolean | no | Enable jobs for this project | | `wiki_enabled` | boolean | no | Enable wiki for this project | | `snippets_enabled` | boolean | no | Enable snippets for this project | +| `resolve_outdated_diff_discussions` | boolean | no | Automatically resolve merge request diffs discussions on lines changed with a push | | `container_registry_enabled` | boolean | no | Enable container registry for this project | | `shared_runners_enabled` | boolean | no | Enable shared runners for this project | | `visibility` | string | no | See [project visibility level](#project-visibility-level) | @@ -610,6 +617,7 @@ Parameters: | `jobs_enabled` | boolean | no | Enable jobs for this project | | `wiki_enabled` | boolean | no | Enable wiki for this project | | `snippets_enabled` | boolean | no | Enable snippets for this project | +| `resolve_outdated_diff_discussions` | boolean | no | Automatically resolve merge request diffs discussions on lines changed with a push | | `container_registry_enabled` | boolean | no | Enable container registry for this project | | `shared_runners_enabled` | boolean | no | Enable shared runners for this project | | `visibility` | string | no | See [project visibility level](#project-visibility-level) | @@ -683,6 +691,7 @@ Example response: "jobs_enabled": true, "wiki_enabled": true, "snippets_enabled": false, + "resolve_outdated_diff_discussions": false, "container_registry_enabled": false, "created_at": "2013-09-30T13:46:02Z", "last_activity_at": "2013-09-30T13:46:02Z", @@ -758,6 +767,7 @@ Example response: "jobs_enabled": true, "wiki_enabled": true, "snippets_enabled": false, + "resolve_outdated_diff_discussions": false, "container_registry_enabled": false, "created_at": "2013-09-30T13:46:02Z", "last_activity_at": "2013-09-30T13:46:02Z", @@ -839,6 +849,7 @@ Example response: "jobs_enabled": true, "wiki_enabled": true, "snippets_enabled": false, + "resolve_outdated_diff_discussions": false, "container_registry_enabled": false, "created_at": "2013-09-30T13:46:02Z", "last_activity_at": "2013-09-30T13:46:02Z", @@ -932,6 +943,7 @@ Example response: "jobs_enabled": true, "wiki_enabled": true, "snippets_enabled": false, + "resolve_outdated_diff_discussions": false, "container_registry_enabled": false, "created_at": "2013-09-30T13:46:02Z", "last_activity_at": "2013-09-30T13:46:02Z", diff --git a/doc/user/discussions/img/automatically_resolve_outdated_discussions.png b/doc/user/discussions/img/automatically_resolve_outdated_discussions.png Binary files differnew file mode 100644 index 00000000000..9a798ddd178 --- /dev/null +++ b/doc/user/discussions/img/automatically_resolve_outdated_discussions.png diff --git a/doc/user/discussions/index.md b/doc/user/discussions/index.md index 8b1d299484c..efea99eb120 100644 --- a/doc/user/discussions/index.md +++ b/doc/user/discussions/index.md @@ -116,6 +116,23 @@ are resolved.  +### Automatically resolve merge request diff discussions when they become outdated + +> [Introduced][ce-14053] in GitLab 10.0. + +You can automatically resolve merge request diff discussions on lines modified +with a new push. + +Navigate to your project's settings page, select the **Automatically resolve +merge request diffs discussions on lines changed with a push** check box and hit +**Save** for the changes to take effect. + + + +From now on, any discussions on a diff will be resolved by default if a push +makes that diff section outdated. Discussions on lines that don't change and +top-level resolvable discussions are not automatically resolved. + ## Threaded discussions > [Introduced][ce-7527] in GitLab 9.1. @@ -141,6 +158,7 @@ comments in greater detail. [ce-7527]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7527 [ce-7180]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7180 [ce-8266]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8266 +[ce-14053]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14053 [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/lib/api/entities.rb b/lib/api/entities.rb index 031dd02c6eb..9114b69606b 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -119,6 +119,7 @@ module API expose :archived?, as: :archived expose :visibility expose :owner, using: Entities::UserBasic, unless: ->(project, options) { project.group } + expose :resolve_outdated_diff_discussions expose :container_registry_enabled # Expose old field names with the new permissions methods to keep API compatible diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 4845242a173..7dc19788462 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -16,6 +16,7 @@ module API optional :jobs_enabled, type: Boolean, desc: 'Flag indication if jobs are enabled' optional :snippets_enabled, type: Boolean, desc: 'Flag indication if snippets are enabled' optional :shared_runners_enabled, type: Boolean, desc: 'Flag indication if shared runners are enabled for that project' + optional :resolve_outdated_diff_discussions, type: Boolean, desc: 'Automatically resolve merge request diffs discussions on lines changed with a push' optional :container_registry_enabled, type: Boolean, desc: 'Flag indication if the container registry is enabled for that project' optional :lfs_enabled, type: Boolean, desc: 'Flag indication if Git LFS is enabled for that project' optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The visibility of the project.' @@ -236,6 +237,7 @@ module API at_least_one_of_ce = [ :jobs_enabled, + :resolve_outdated_diff_discussions, :container_registry_enabled, :default_branch, :description, diff --git a/lib/api/v3/entities.rb b/lib/api/v3/entities.rb index a9a35f2a4bd..ac47a713966 100644 --- a/lib/api/v3/entities.rb +++ b/lib/api/v3/entities.rb @@ -64,6 +64,7 @@ module API expose :owner, using: ::API::Entities::UserBasic, unless: ->(project, options) { project.group } expose :name, :name_with_namespace expose :path, :path_with_namespace + expose :resolve_outdated_diff_discussions expose :container_registry_enabled # Expose old field names with the new permissions methods to keep API compatible diff --git a/lib/api/v3/projects.rb b/lib/api/v3/projects.rb index 449876c10d9..74df246bdfe 100644 --- a/lib/api/v3/projects.rb +++ b/lib/api/v3/projects.rb @@ -18,6 +18,7 @@ module API optional :builds_enabled, type: Boolean, desc: 'Flag indication if builds are enabled' optional :snippets_enabled, type: Boolean, desc: 'Flag indication if snippets are enabled' optional :shared_runners_enabled, type: Boolean, desc: 'Flag indication if shared runners are enabled for that project' + optional :resolve_outdated_diff_discussions, type: Boolean, desc: 'Automatically resolve merge request diffs discussions on lines changed with a push' optional :container_registry_enabled, type: Boolean, desc: 'Flag indication if the container registry is enabled for that project' optional :lfs_enabled, type: Boolean, desc: 'Flag indication if Git LFS is enabled for that project' optional :public, type: Boolean, desc: 'Create a public project. The same as visibility_level = 20.' @@ -296,9 +297,9 @@ module API use :optional_params at_least_one_of :name, :description, :issues_enabled, :merge_requests_enabled, :wiki_enabled, :builds_enabled, :snippets_enabled, - :shared_runners_enabled, :container_registry_enabled, - :lfs_enabled, :public, :visibility_level, :public_builds, - :request_access_enabled, :only_allow_merge_if_build_succeeds, + :shared_runners_enabled, :resolve_outdated_diff_discussions, + :container_registry_enabled, :lfs_enabled, :public, :visibility_level, + :public_builds, :request_access_enabled, :only_allow_merge_if_build_succeeds, :only_allow_merge_if_all_discussions_are_resolved, :path, :default_branch end diff --git a/spec/features/merge_requests/resolve_outdated_diff_discussions.rb b/spec/features/merge_requests/resolve_outdated_diff_discussions.rb new file mode 100644 index 00000000000..55a82bdf2b9 --- /dev/null +++ b/spec/features/merge_requests/resolve_outdated_diff_discussions.rb @@ -0,0 +1,78 @@ +require 'spec_helper' + +feature 'Resolve outdated diff discussions', js: true do + let(:project) { create(:project, :repository, :public) } + + let(:merge_request) do + create(:merge_request, source_project: project, source_branch: 'csv', target_branch: 'master') + end + + let(:outdated_diff_refs) { project.commit('926c6595b263b2a40da6b17f3e3b7ea08344fad6').diff_refs } + let(:current_diff_refs) { merge_request.diff_refs } + + let(:outdated_position) do + Gitlab::Diff::Position.new( + old_path: 'files/csv/Book1.csv', + new_path: 'files/csv/Book1.csv', + old_line: nil, + new_line: 9, + diff_refs: outdated_diff_refs + ) + end + + let(:current_position) do + Gitlab::Diff::Position.new( + old_path: 'files/csv/Book1.csv', + new_path: 'files/csv/Book1.csv', + old_line: nil, + new_line: 1, + diff_refs: current_diff_refs + ) + end + + let!(:outdated_discussion) do + create(:diff_note_on_merge_request, + project: project, + noteable: merge_request, + position: outdated_position).to_discussion + end + + let!(:current_discussion) do + create(:diff_note_on_merge_request, + noteable: merge_request, + project: project, + position: current_position).to_discussion + end + + before do + sign_in(merge_request.author) + end + + context 'when a discussion was resolved by a push' do + before do + project.update!(resolve_outdated_diff_discussions: true) + + merge_request.update_diff_discussion_positions( + old_diff_refs: outdated_diff_refs, + new_diff_refs: current_diff_refs, + current_user: merge_request.author + ) + + visit project_merge_request_path(project, merge_request) + end + + it 'shows that as automatically resolved' do + within(".discussion[data-discussion-id='#{outdated_discussion.id}']") do + expect(page).to have_css('.discussion-body', visible: false) + expect(page).to have_content('Automatically resolved') + end + end + + it 'does not show that for active discussions' do + within(".discussion[data-discussion-id='#{current_discussion.id}']") do + expect(page).to have_css('.discussion-body', visible: true) + expect(page).not_to have_content('Automatically resolved') + end + end + end +end diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb index 9921ca1af33..68540dd4e59 100644 --- a/spec/helpers/notes_helper_spec.rb +++ b/spec/helpers/notes_helper_spec.rb @@ -231,7 +231,7 @@ describe NotesHelper do end end - describe '#form_resurces' do + describe '#form_resources' do it 'returns note for personal snippet' do @snippet = create(:personal_snippet) @note = create(:note_on_personal_snippet) @@ -266,4 +266,22 @@ describe NotesHelper do expect(noteable_note_url(note)).to match("/#{project.namespace.path}/#{project.path}/issues/#{issue.iid}##{dom_id(note)}") end end + + describe '#discussion_resolved_intro' do + context 'when the discussion was resolved by a push' do + let(:discussion) { double(:discussion, resolved_by_push?: true) } + + it 'returns "Automatically resolved"' do + expect(discussion_resolved_intro(discussion)).to eq('Automatically resolved') + end + end + + context 'when the discussion was not resolved by a push' do + let(:discussion) { double(:discussion, resolved_by_push?: false) } + + it 'returns "Resolved"' do + expect(discussion_resolved_intro(discussion)).to eq('Resolved') + end + end + end end diff --git a/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb b/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb index 0d5fffa38ff..c56b08b18a2 100644 --- a/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb +++ b/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb @@ -214,7 +214,7 @@ end # The background migration relies on a temporary table, hence we're migrating # to a specific version of the database where said table is still present. # -describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads, :migration, schema: 20170608152748 do +describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads, :migration, schema: 20170825154015 do let(:migration) { described_class.new } let(:project) { create(:project_empty_repo) } let(:author) { create(:user) } diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index b852ac570a3..122b8ee0314 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -65,6 +65,7 @@ Note: - change_position - resolved_at - resolved_by_id +- resolved_by_push - discussion_id - original_discussion_id LabelLink: @@ -398,6 +399,7 @@ Project: - public_builds - last_repository_check_failed - last_repository_check_at +- collapse_outdated_diff_comments - container_registry_enabled - only_allow_merge_if_pipeline_succeeds - has_external_issue_tracker @@ -406,6 +408,7 @@ Project: - only_allow_merge_if_all_discussions_are_resolved - auto_cancel_pending_pipelines - printing_merge_request_link_enabled +- resolve_outdated_diff_discussions - build_allow_git_fetch - last_repository_updated_at - ci_config_path diff --git a/spec/models/concerns/resolvable_note_spec.rb b/spec/models/concerns/resolvable_note_spec.rb index d00faa4f8be..91591017587 100644 --- a/spec/models/concerns/resolvable_note_spec.rb +++ b/spec/models/concerns/resolvable_note_spec.rb @@ -189,8 +189,8 @@ describe Note, ResolvableNote do allow(subject).to receive(:resolvable?).and_return(false) end - it "returns nil" do - expect(subject.resolve!(current_user)).to be_nil + it "returns false" do + expect(subject.resolve!(current_user)).to be_falsey end it "doesn't set resolved_at" do @@ -224,8 +224,8 @@ describe Note, ResolvableNote do subject.resolve!(user) end - it "returns nil" do - expect(subject.resolve!(current_user)).to be_nil + it "returns false" do + expect(subject.resolve!(current_user)).to be_falsey end it "doesn't change resolved_at" do @@ -279,8 +279,8 @@ describe Note, ResolvableNote do allow(subject).to receive(:resolvable?).and_return(false) end - it "returns nil" do - expect(subject.unresolve!).to be_nil + it "returns false" do + expect(subject.unresolve!).to be_falsey end end @@ -320,8 +320,8 @@ describe Note, ResolvableNote do end context "when not resolved" do - it "returns nil" do - expect(subject.unresolve!).to be_nil + it "returns false" do + expect(subject.unresolve!).to be_falsey end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index f5d079c27c4..d80d5657c42 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1262,7 +1262,6 @@ describe MergeRequest do describe "#reload_diff" do let(:discussion) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject).to_discussion } - let(:commit) { subject.project.commit(sample_commit.id) } it "does not change existing merge request diff" do @@ -1280,9 +1279,19 @@ describe MergeRequest do subject.reload_diff end - it "updates diff discussion positions" do - old_diff_refs = subject.diff_refs + it "calls update_diff_discussion_positions" do + expect(subject).to receive(:update_diff_discussion_positions) + + subject.reload_diff + end + end + describe '#update_diff_discussion_positions' do + let(:discussion) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject).to_discussion } + let(:commit) { subject.project.commit(sample_commit.id) } + let(:old_diff_refs) { subject.diff_refs } + + before do # Update merge_request_diff so that #diff_refs will return commit.diff_refs allow(subject).to receive(:create_merge_request_diff) do subject.merge_request_diffs.create( @@ -1293,7 +1302,9 @@ describe MergeRequest do subject.merge_request_diff(true) end + end + it "updates diff discussion positions" do expect(Discussions::UpdateDiffPositionService).to receive(:new).with( subject.project, subject.author, @@ -1305,7 +1316,26 @@ describe MergeRequest do expect_any_instance_of(Discussions::UpdateDiffPositionService).to receive(:execute).with(discussion).and_call_original expect_any_instance_of(DiffNote).to receive(:save).once - subject.reload_diff(subject.author) + subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs, + new_diff_refs: commit.diff_refs, + current_user: subject.author) + end + + context 'when resolve_outdated_diff_discussions is set' do + before do + discussion + + subject.project.update!(resolve_outdated_diff_discussions: true) + end + + it 'calls MergeRequests::ResolvedDiscussionNotificationService' do + expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService) + .to receive(:execute).with(subject) + + subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs, + new_diff_refs: commit.diff_refs, + current_user: subject.author) + end end end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 4490e50702b..f771e4fa4ff 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -414,6 +414,7 @@ describe API::Projects do jobs_enabled: false, merge_requests_enabled: false, wiki_enabled: false, + resolve_outdated_diff_discussions: false, only_allow_merge_if_pipeline_succeeds: false, request_access_enabled: true, only_allow_merge_if_all_discussions_are_resolved: false, @@ -477,20 +478,40 @@ describe API::Projects do expect(json_response['avatar_url']).to eq("http://localhost/uploads/-/system/project/avatar/#{project_id}/banana_sample.gif") end + it 'sets a project as allowing outdated diff discussions to automatically resolve' do + project = attributes_for(:project, resolve_outdated_diff_discussions: false) + + post api('/projects', user), project + + expect(json_response['resolve_outdated_diff_discussions']).to be_falsey + end + + it 'sets a project as allowing outdated diff discussions to automatically resolve if resolve_outdated_diff_discussions' do + project = attributes_for(:project, resolve_outdated_diff_discussions: true) + + post api('/projects', user), project + + expect(json_response['resolve_outdated_diff_discussions']).to be_truthy + end + it 'sets a project as allowing merge even if build fails' do - project = attributes_for(:project, { only_allow_merge_if_pipeline_succeeds: false }) + project = attributes_for(:project, only_allow_merge_if_pipeline_succeeds: false) + post api('/projects', user), project + expect(json_response['only_allow_merge_if_pipeline_succeeds']).to be_falsey end it 'sets a project as allowing merge only if merge_when_pipeline_succeeds' do - project = attributes_for(:project, { only_allow_merge_if_pipeline_succeeds: true }) + project = attributes_for(:project, only_allow_merge_if_pipeline_succeeds: true) + post api('/projects', user), project + expect(json_response['only_allow_merge_if_pipeline_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 }) + project = attributes_for(:project, only_allow_merge_if_all_discussions_are_resolved: false) post api('/projects', user), project @@ -506,7 +527,7 @@ describe API::Projects do 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 }) + project = attributes_for(:project, only_allow_merge_if_all_discussions_are_resolved: true) post api('/projects', user), project @@ -514,7 +535,7 @@ describe API::Projects do end it 'ignores import_url when it is nil' do - project = attributes_for(:project, { import_url: nil }) + project = attributes_for(:project, import_url: nil) post api('/projects', user), project @@ -642,20 +663,36 @@ describe API::Projects do expect(json_response['visibility']).to eq('private') end + it 'sets a project as allowing outdated diff discussions to automatically resolve' do + project = attributes_for(:project, resolve_outdated_diff_discussions: false) + + post api("/projects/user/#{user.id}", admin), project + + expect(json_response['resolve_outdated_diff_discussions']).to be_falsey + end + + it 'sets a project as allowing outdated diff discussions to automatically resolve' do + project = attributes_for(:project, resolve_outdated_diff_discussions: true) + + post api("/projects/user/#{user.id}", admin), project + + expect(json_response['resolve_outdated_diff_discussions']).to be_truthy + end + it 'sets a project as allowing merge even if build fails' do - project = attributes_for(:project, { only_allow_merge_if_pipeline_succeeds: false }) + project = attributes_for(:project, only_allow_merge_if_pipeline_succeeds: false) post api("/projects/user/#{user.id}", admin), project expect(json_response['only_allow_merge_if_pipeline_succeeds']).to be_falsey end - it 'sets a project as allowing merge only if merge_when_pipeline_succeeds' do - project = attributes_for(:project, { only_allow_merge_if_pipeline_succeeds: true }) + it 'sets a project as allowing merge only if pipeline succeeds' do + project = attributes_for(:project, only_allow_merge_if_pipeline_succeeds: true) post api("/projects/user/#{user.id}", admin), project expect(json_response['only_allow_merge_if_pipeline_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 }) + project = attributes_for(:project, only_allow_merge_if_all_discussions_are_resolved: false) post api("/projects/user/#{user.id}", admin), project @@ -663,7 +700,7 @@ describe API::Projects do 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 }) + project = attributes_for(:project, only_allow_merge_if_all_discussions_are_resolved: true) post api("/projects/user/#{user.id}", admin), project @@ -732,6 +769,7 @@ describe API::Projects do expect(json_response['wiki_enabled']).to be_present expect(json_response['jobs_enabled']).to be_present expect(json_response['snippets_enabled']).to be_present + expect(json_response['resolve_outdated_diff_discussions']).to eq(project.resolve_outdated_diff_discussions) expect(json_response['container_registry_enabled']).to be_present expect(json_response['created_at']).to be_present expect(json_response['last_activity_at']).to be_present diff --git a/spec/requests/api/v3/projects_spec.rb b/spec/requests/api/v3/projects_spec.rb index a514166274a..cae2c3118da 100644 --- a/spec/requests/api/v3/projects_spec.rb +++ b/spec/requests/api/v3/projects_spec.rb @@ -687,6 +687,7 @@ describe API::V3::Projects do expect(json_response['wiki_enabled']).to be_present expect(json_response['builds_enabled']).to be_present expect(json_response['snippets_enabled']).to be_present + expect(json_response['resolve_outdated_diff_discussions']).to eq(project.resolve_outdated_diff_discussions) expect(json_response['container_registry_enabled']).to be_present expect(json_response['created_at']).to be_present expect(json_response['last_activity_at']).to be_present diff --git a/spec/services/discussions/update_diff_position_service_spec.rb b/spec/services/discussions/update_diff_position_service_spec.rb index c239494298b..82b156f5ebe 100644 --- a/spec/services/discussions/update_diff_position_service_spec.rb +++ b/spec/services/discussions/update_diff_position_service_spec.rb @@ -150,21 +150,7 @@ describe Discussions::UpdateDiffPositionService do ) end - context "when the diff line is the same" do - let(:line) { 16 } - - it "updates the position" do - subject.execute(discussion) - - expect(discussion.original_position).to eq(old_position) - expect(discussion.position).not_to eq(old_position) - expect(discussion.position.new_line).to eq(22) - end - end - - context "when the diff line has changed" do - let(:line) { 9 } - + shared_examples 'outdated diff note' do it "doesn't update the position" do subject.execute(discussion) @@ -189,5 +175,51 @@ describe Discussions::UpdateDiffPositionService do subject.execute(discussion) end end + + context "when the diff line is the same" do + let(:line) { 16 } + + it "updates the position" do + subject.execute(discussion) + + expect(discussion.original_position).to eq(old_position) + expect(discussion.position).not_to eq(old_position) + expect(discussion.position.new_line).to eq(22) + end + + context 'when the resolve_outdated_diff_discussions setting is set' do + before do + project.update!(resolve_outdated_diff_discussions: true) + end + + it 'does not resolve the discussion' do + subject.execute(discussion) + + expect(discussion).not_to be_resolved + expect(discussion).not_to be_resolved_by_push + end + end + end + + context "when the diff line has changed" do + let(:line) { 9 } + + include_examples 'outdated diff note' + + context 'when the resolve_outdated_diff_discussions setting is set' do + before do + project.update!(resolve_outdated_diff_discussions: true) + end + + it 'sets resolves the discussion and sets resolved_by_push' do + subject.execute(discussion) + + expect(discussion).to be_resolved + expect(discussion).to be_resolved_by_push + end + + include_examples 'outdated diff note' + end + end end end |
