Merge branch '300100_allow_guests_to_define_metadata_on_issue_create' into 'master'

Allow guest user to assign issue metadata on issue create

See merge request gitlab-org/gitlab!62816

app/policies/issue_policy.rb

@@ -15,6 +15,9 @@ class IssuePolicy < IssuablePolicy
   desc "Issue is confidential"
   condition(:confidential, scope: :subject) { @subject.confidential? }
 
+  desc "Issue is persisted"
+  condition(:persisted, scope: :subject) { @subject.persisted? }
+
   rule { confidential & ~can_read_confidential }.policy do
     prevent(*create_read_update_admin_destroy(:issue))
     prevent :read_issue_iid
@@ -40,6 +43,14 @@ class IssuePolicy < IssuablePolicy
     enable :create_todo
     enable :update_subscription
   end
+
+  rule { ~persisted & can?(:guest_access) }.policy do
+    enable :set_issue_metadata
+  end
+
+  rule { persisted & can?(:admin_issue) }.policy do
+    enable :set_issue_metadata
+  end
 end
 
 IssuePolicy.prepend_mod_with('IssuePolicy')

app/policies/merge_request_policy.rb

@@ -28,6 +28,10 @@ class MergeRequestPolicy < IssuablePolicy
   rule { can_merge }.policy do
     enable :accept_merge_request
   end
+
+  rule { can?(:admin_merge_request) }.policy do
+    enable :set_merge_request_metadata
+  end
 end
 
 MergeRequestPolicy.prepend_mod_with('MergeRequestPolicy')

app/services/issuable_base_service.rb

@@ -27,8 +27,14 @@ class IssuableBaseService < ::BaseProjectService
     can?(current_user, ability_name, issuable)
   end
 
+  def can_set_issuable_metadata?(issuable)
+    ability_name = :"set_#{issuable.to_ability_name}_metadata"
+
+    can?(current_user, ability_name, issuable)
+  end
+
   def filter_params(issuable)
-    unless can_admin_issuable?(issuable)
+    unless can_set_issuable_metadata?(issuable)
       params.delete(:milestone)
       params.delete(:milestone_id)
       params.delete(:labels)
@@ -45,6 +51,7 @@ class IssuableBaseService < ::BaseProjectService
       params.delete(:canonical_issue_id)
       params.delete(:project)
       params.delete(:discussion_locked)
+      params.delete(:confidential)
     end
 
     filter_assignees(issuable)

app/services/issues/update_service.rb

@@ -20,17 +20,6 @@ module Issues
       super
     end
 
-    override :filter_params
-    def filter_params(issue)
-      super
-
-      # filter confidential in `Issues::UpdateService` and not in `IssuableBaseService#filter_params`
-      # because we do allow users that cannot admin issues to set confidential flag when creating an issue
-      unless can_admin_issuable?(issue)
-        params.delete(:confidential)
-      end
-    end
-
     def before_update(issue, skip_spam_check: false)
       return if skip_spam_check
 

app/views/shared/issuable/_form.html.haml

@@ -24,14 +24,6 @@
 
 = render 'shared/form_elements/description', model: issuable, form: form, project: project
 
-- if issuable.respond_to?(:confidential)
-  .form-group.row
-    .offset-sm-2.col-sm-10
-      .form-check
-        = form.check_box :confidential, class: 'form-check-input'
-        = form.label :confidential, class: 'form-check-label' do
-          This issue is confidential and should only be visible to team members with at least Reporter access.
-
 = render 'shared/issuable/form/metadata', issuable: issuable, form: form, project: project, presenter: presenter
 
 = render_if_exists 'shared/issuable/approvals', issuable: issuable, presenter: presenter, form: form

app/views/shared/issuable/form/_metadata.html.haml

@@ -2,11 +2,19 @@
 - issuable = local_assigns.fetch(:issuable)
 - presenter = local_assigns.fetch(:presenter)
 
-- return unless can?(current_user, :"admin_#{issuable.to_ability_name}", issuable.project)
+- return unless can?(current_user, :"set_#{issuable.to_ability_name}_metadata", issuable)
 
 - has_due_date = issuable.has_attribute?(:due_date)
 - form = local_assigns.fetch(:form)
 
+- if issuable.respond_to?(:confidential)
+  .form-group.row
+    .offset-sm-2.col-sm-10
+      .form-check
+        = form.check_box :confidential, class: 'form-check-input'
+        = form.label :confidential, class: 'form-check-label' do
+          This issue is confidential and should only be visible to team members with at least Reporter access.
+
 %hr
 .row
   %div{ class: (has_due_date ? "col-lg-6" : "col-12") }

ee/app/policies/epic_policy.rb

@@ -35,4 +35,8 @@ class EpicPolicy < BasePolicy
   rule { ~anonymous & can?(:read_epic) }.policy do
     enable :create_todo
   end
+
+  rule { can?(:admin_epic) }.policy do
+    enable :set_epic_metadata
+  end
 end

ee/spec/services/epics/create_service_spec.rb

@@ -12,6 +12,11 @@ RSpec.describe Epics::CreateService do
   subject { described_class.new(group: group, current_user: user, params: params).execute }
 
   describe '#execute' do
+    before do
+      group.add_reporter(user)
+      stub_licensed_features(epics: true, subepics: true)
+    end
+
     it 'creates one epic correctly' do
       allow(NewEpicWorker).to receive(:perform_async)
 
@@ -26,116 +31,111 @@ RSpec.describe Epics::CreateService do
       expect(epic.confidential).to be_truthy
       expect(NewEpicWorker).to have_received(:perform_async).with(epic.id, user.id)
     end
-  end
 
-  context 'handling fixed dates' do
-    it 'sets the fixed date correctly' do
-      date = Date.new(2019, 10, 10)
-      params[:start_date_fixed] = date
-      params[:start_date_is_fixed] = true
+    context 'handling fixed dates' do
+      it 'sets the fixed date correctly' do
+        date = Date.new(2019, 10, 10)
+        params[:start_date_fixed] = date
+        params[:start_date_is_fixed] = true
 
-      subject
+        subject
 
-      epic = Epic.last
-      expect(epic.start_date).to eq(date)
-      expect(epic.start_date_fixed).to eq(date)
-      expect(epic.start_date_is_fixed).to be_truthy
+        epic = Epic.last
+        expect(epic.start_date).to eq(date)
+        expect(epic.start_date_fixed).to eq(date)
+        expect(epic.start_date_is_fixed).to be_truthy
+      end
     end
-  end
 
-  context 'after_save callback to store_mentions' do
-    let(:labels) { create_pair(:group_label, group: group) }
+    context 'after_save callback to store_mentions' do
+      let(:labels) { create_pair(:group_label, group: group) }
 
-    context 'when mentionable attributes change' do
-      context 'when content has no mentions' do
-        let(:params) { { title: 'Title', description: "Description with no mentions" } }
-
-        it 'calls store_mentions! and saves no mentions' do
-          expect_next_instance_of(Epic) do |instance|
-            expect(instance).to receive(:store_mentions!).and_call_original
-          end
+      context 'when mentionable attributes change' do
+        context 'when content has no mentions' do
+          let(:params) { { title: 'Title', description: "Description with no mentions" } }
 
-          expect { subject }.not_to change { EpicUserMention.count }
-        end
-      end
-
-      context 'when content has mentions' do
-        let(:params) { { title: 'Title', description: "Description with #{user.to_reference}" } }
+          it 'calls store_mentions! and saves no mentions' do
+            expect_next_instance_of(Epic) do |instance|
+              expect(instance).to receive(:store_mentions!).and_call_original
+            end
 
-        it 'calls store_mentions! and saves mentions' do
-          expect_next_instance_of(Epic) do |instance|
-            expect(instance).to receive(:store_mentions!).and_call_original
+            expect { subject }.not_to change { EpicUserMention.count }
           end
-
-          expect { subject }.to change { EpicUserMention.count }.by(1)
         end
-      end
 
-      context 'when mentionable.save fails' do
-        let(:params) { { title: '', label_ids: labels.map(&:id) } }
+        context 'when content has mentions' do
+          let(:params) { { title: 'Title', description: "Description with #{user.to_reference}" } }
 
-        it 'does not call store_mentions and saves no mentions' do
-          expect_next_instance_of(Epic) do |instance|
-            expect(instance).not_to receive(:store_mentions!).and_call_original
-          end
-
-          expect { subject }.not_to change { EpicUserMention.count }
-          expect(subject.valid?).to be false
-        end
-      end
+          it 'calls store_mentions! and saves mentions' do
+            expect_next_instance_of(Epic) do |instance|
+              expect(instance).to receive(:store_mentions!).and_call_original
+            end
 
-      context 'when description param has quick action' do
-        before do
-          stub_licensed_features(epics: true, subepics: true)
-          group.add_developer(user)
+            expect { subject }.to change { EpicUserMention.count }.by(1)
+          end
         end
 
-        context 'for /parent_epic' do
-          it 'assigns parent epic' do
-            parent_epic = create(:epic, group: group)
-            description = "/parent_epic #{parent_epic.to_reference}"
-            params = { title: 'New epic with parent', description: description }
+        context 'when mentionable.save fails' do
+          let(:params) { { title: '', label_ids: labels.map(&:id) } }
 
-            epic = described_class.new(group: group, current_user: user, params: params).execute
+          it 'does not call store_mentions and saves no mentions' do
+            expect_next_instance_of(Epic) do |instance|
+              expect(instance).not_to receive(:store_mentions!).and_call_original
+            end
 
-            expect(epic.parent).to eq(parent_epic)
+            expect { subject }.not_to change { EpicUserMention.count }
+            expect(subject.valid?).to be false
           end
+        end
 
-          context 'when parent epic cannot be assigned' do
-            it 'does not assign parent epic' do
-              other_group = create(:group, :private)
-              parent_epic = create(:epic, group: other_group)
-              description = "/parent_epic #{parent_epic.to_reference(group)}"
+        context 'when description param has quick action' do
+          context 'for /parent_epic' do
+            it 'assigns parent epic' do
+              parent_epic = create(:epic, group: group)
+              description = "/parent_epic #{parent_epic.to_reference}"
               params = { title: 'New epic with parent', description: description }
 
               epic = described_class.new(group: group, current_user: user, params: params).execute
 
-              expect(epic.parent).to eq(nil)
+              expect(epic.parent).to eq(parent_epic)
             end
-          end
-        end
 
-        context 'for /child_epic' do
-          it 'sets a child epic' do
-            child_epic = create(:epic, group: group)
-            description = "/child_epic #{child_epic.to_reference}"
-            params = { title: 'New epic with child', description: description }
+            context 'when parent epic cannot be assigned' do
+              it 'does not assign parent epic' do
+                other_group = create(:group, :private)
+                parent_epic = create(:epic, group: other_group)
+                description = "/parent_epic #{parent_epic.to_reference(group)}"
+                params = { title: 'New epic with parent', description: description }
 
-            epic = described_class.new(group: group, current_user: user, params: params).execute
+                epic = described_class.new(group: group, current_user: user, params: params).execute
 
-            expect(epic.reload.children).to include(child_epic)
+                expect(epic.parent).to eq(nil)
+              end
+            end
           end
 
-          context 'when child epic cannot be assigned' do
-            it 'does not set child epic' do
-              other_group = create(:group, :private)
-              child_epic = create(:epic, group: other_group)
-              description = "/child_epic #{child_epic.to_reference(group)}"
+          context 'for /child_epic' do
+            it 'sets a child epic' do
+              child_epic = create(:epic, group: group)
+              description = "/child_epic #{child_epic.to_reference}"
               params = { title: 'New epic with child', description: description }
 
               epic = described_class.new(group: group, current_user: user, params: params).execute
 
-              expect(epic.reload.children).to be_empty
+              expect(epic.reload.children).to include(child_epic)
+            end
+
+            context 'when child epic cannot be assigned' do
+              it 'does not set child epic' do
+                other_group = create(:group, :private)
+                child_epic = create(:epic, group: other_group)
+                description = "/child_epic #{child_epic.to_reference(group)}"
+                params = { title: 'New epic with child', description: description }
+
+                epic = described_class.new(group: group, current_user: user, params: params).execute
+
+                expect(epic.reload.children).to be_empty
+              end
             end
           end
         end

spec/features/issues/user_creates_issue_spec.rb

@@ -30,7 +30,7 @@ RSpec.describe "User creates issue" do
     end
   end
 
-  context "when signed in as guest" do
+  context "when signed in as guest", :js do
     before do
       project.add_guest(user)
       sign_in(user)
@@ -38,41 +38,19 @@ RSpec.describe "User creates issue" do
       visit(new_project_issue_path(project))
     end
 
-    it "creates issue", :js do
-      page.within(".issue-form") do
-        expect(page).to have_no_content("Assign to")
-        .and have_no_content("Labels")
-        .and have_no_content("Milestone")
-
-        expect(page.find('#issue_title')['placeholder']).to eq 'Title'
-        expect(page.find('#issue_description')['placeholder']).to eq 'Write a description or drag your files here…'
+    context 'available metadata' do
+      it 'allows guest to set issue metadata' do
+        page.within(".issue-form") do
+          expect(page).to have_content("Title")
+            .and have_content("Description")
+            .and have_content("Type")
+            .and have_content("Assignee")
+            .and have_content("Milestone")
+            .and have_content("Labels")
+            .and have_content("Due date")
+            .and have_content("This issue is confidential and should only be visible to team members with at least Reporter access.")
+        end
       end
-
-      issue_title = "500 error on profile"
-
-      fill_in("Title", with: issue_title)
-      first('.js-md').click
-      first('.rspec-issuable-form-description').native.send_keys('Description')
-
-      click_button("Create issue")
-
-      expect(page).to have_content(issue_title)
-        .and have_content(user.name)
-        .and have_content(project.name)
-      expect(page).to have_selector('strong', text: 'Description')
-    end
-
-    it 'does not render the issue type dropdown' do
-      expect(page).not_to have_selector('.s-issuable-type-filter-dropdown-wrap')
-    end
-  end
-
-  context "when signed in as developer", :js do
-    before do
-      project.add_developer(user)
-      sign_in(user)
-
-      visit(new_project_issue_path(project))
     end
 
     context "when previewing" do

spec/services/issues/create_service_spec.rb

@@ -18,8 +18,8 @@ RSpec.describe Issues::CreateService do
       let_it_be(:labels) { create_pair(:label, project: project) }
 
       before_all do
-        project.add_maintainer(user)
-        project.add_maintainer(assignee)
+        project.add_guest(user)
+        project.add_guest(assignee)
       end
 
       let(:opts) do
@@ -88,15 +88,11 @@ RSpec.describe Issues::CreateService do
         expect { issue }.to change { project.open_issues_count }.from(0).to(1)
       end
 
-      context 'when current user cannot admin issues in the project' do
-        let_it_be(:guest) { create(:user) }
-
-        before_all do
-          project.add_guest(guest)
-        end
+      context 'when current user cannot set issue metadata in the project' do
+        let_it_be(:non_member) { create(:user) }
 
-        it 'filters out params that cannot be set without the :admin_issue permission' do
-          issue = described_class.new(project: project, current_user: guest, params: opts).execute
+        it 'filters out params that cannot be set without the :set_issue_metadata permission' do
+          issue = described_class.new(project: project, current_user: non_member, params: opts).execute
 
           expect(issue).to be_persisted
           expect(issue.title).to eq('Awesome issue')
@@ -107,8 +103,8 @@ RSpec.describe Issues::CreateService do
           expect(issue.due_date).to be_nil
         end
 
-        it 'creates confidential issues' do
-          issue = described_class.new(project: project, current_user: guest, params: { confidential: true }).execute
+        it 'can create confidential issues' do
+          issue = described_class.new(project: project, current_user: non_member, params: { confidential: true }).execute
 
           expect(issue.confidential).to be_truthy
         end