Merge branch '346171-add-url-text-to-issuable-metric-images' into 'master'

Add url_text column to issuable metric images, and allow updates to url and url_text

See merge request gitlab-org/gitlab!78430

db/migrate/20220118015633_add_url_text_to_issuable_metric_images.rb

+# frozen_string_literal: true
+
+class AddUrlTextToIssuableMetricImages < Gitlab::Database::Migration[1.0]
+  # rubocop:disable Migration/AddLimitToTextColumns
+  # limit is added in 20220118020026_add_url_text_limit_to_issuable_metric_images
+  def change
+    add_column :issuable_metric_images, :url_text, :text
+  end
+  # rubocop:enable Migration/AddLimitToTextColumns
+end

db/migrate/20220118020026_add_url_text_limit_to_issuable_metric_images.rb

+# frozen_string_literal: true
+
+class AddUrlTextLimitToIssuableMetricImages < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    add_text_limit :issuable_metric_images, :url_text, 128
+  end
+
+  def down
+    remove_text_limit :issuable_metric_images, :url_text
+  end
+end

db/schema_migrations/20220118015633

+6a73f49306de7c799a39afa3ac1f761840860833a96f1a91cf992c9a3ebfef9b
\ No newline at end of file

db/schema_migrations/20220118020026

+77374c81456f235d3afeb45cdda14552e1ef8047de5aaa3f5bb0a82e4aebe849
\ No newline at end of file

db/structure.sql

@@ -15353,6 +15353,8 @@ CREATE TABLE issuable_metric_images (
     file_store smallint,
     file text NOT NULL,
     url text,
+    url_text text,
+    CONSTRAINT check_3bc6d47661 CHECK ((char_length(url_text) <= 128)),
     CONSTRAINT check_5b3011e234 CHECK ((char_length(url) <= 255)),
     CONSTRAINT check_7ed527062f CHECK ((char_length(file) <= 255))
 );

doc/api/issues.md

@@ -2428,10 +2428,11 @@ POST /projects/:id/issues/:issue_iid/metric_images
 | `issue_iid` | integer | yes      | The internal ID of a project's issue |
 | `file` | file | yes      | The image file to be uploaded |
 | `url` | string | no      | The URL to view more metric information |
+| `url_text` | string | no      | A description of the image or URL |
 
 ```shell
 curl --header "PRIVATE-TOKEN: <your_access_token>" --form 'file=@/path/to/file.png' \
---form 'url=http://example.com' "https://gitlab.example.com/api/v4/projects/5/issues/93/metric_images"
+--form 'url=http://example.com' --form 'url_text=Example website' "https://gitlab.example.com/api/v4/projects/5/issues/93/metric_images"
 ```
 
 Example response:
@@ -2442,7 +2443,8 @@ Example response:
     "created_at": "2020-11-13T00:06:18.084Z",
     "filename": "file.png",
     "file_path": "/uploads/-/system/issuable_metric_image/file/23/file.png",
-    "url": "http://example.com"
+    "url": "http://example.com",
+    "url_text": "Example website"
 }
 ```
 
@@ -2484,6 +2486,39 @@ Example response:
 ]
 ```
 
+## Update metric image
+
+Available only for Incident issues.
+
+```plaintext
+PUT /projects/:id/issues/:issue_iid/metric_images/:image_id
+```
+
+| Attribute   | Type    | Required | Description                          |
+|-------------|---------|----------|--------------------------------------|
+| `id`        | integer/string | yes      | The ID or [URL-encoded path of the project](index.md#namespaced-path-encoding) owned by the authenticated user  |
+| `issue_iid` | integer | yes      | The internal ID of a project's issue |
+| `image_id` | integer | yes      | The ID of the image |
+| `url` | string | no      | The URL to view more metric information |
+| `url_text` | string | no      | A description of the image or URL |
+
+```shell
+curl --header "PRIVATE-TOKEN: <your_access_token>" --request PUT  --form 'url=http://example.com' --form 'url_text=Example website' "https://gitlab.example.com/api/v4/projects/5/issues/93/metric_images/1"
+```
+
+Example response:
+
+```json
+{
+    "id": 23,
+    "created_at": "2020-11-13T00:06:18.084Z",
+    "filename": "file.png",
+    "file_path": "/uploads/-/system/issuable_metric_image/file/23/file.png",
+    "url": "http://example.com",
+    "url_text": "Example website"
+}
+```
+
 ## Delete metric image
 
 Available only for Incident issues.

ee/app/models/issuable_metric_image.rb

@@ -15,6 +15,7 @@ class IssuableMetricImage < ApplicationRecord
   validates :file, presence: true
   validate :validate_file_is_image
   validates :url, length: { maximum: 255 }, public_url: { allow_blank: true }
+  validates :url_text, length: { maximum: 128 }
 
   scope :order_created_at_asc, -> { order(created_at: :asc) }
 

ee/app/policies/ee/issuable_policy.rb

@@ -31,11 +31,13 @@ module EE
       end
 
       rule { is_author | can?(:create_issue) & can?(:update_issue) }.policy do
+        enable :update_issuable_metric_image
         enable :destroy_issuable_metric_image
       end
 
       rule { ~is_project_member }.policy do
         prevent :upload_issuable_metric_image
+        prevent :update_issuable_metric_image
         prevent :destroy_issuable_metric_image
       end
     end

ee/app/services/incident_management/incidents/upload_metric_service.rb

@@ -10,6 +10,7 @@ module IncidentManagement
         @project = issuable&.project
         @file = params.fetch(:file)
         @url = params.fetch(:url, nil)
+        @url_text = params.fetch(:url_text, nil)
       end
 
       def execute
@@ -22,7 +23,7 @@ module IncidentManagement
         ServiceResponse.error(message: e.message)
       end
 
-      attr_reader :issuable, :project, :file, :url, :metric
+      attr_reader :issuable, :project, :file, :url, :url_text, :metric
 
       private
 
@@ -30,7 +31,8 @@ module IncidentManagement
         @metric = IssuableMetricImage.create!(
           issue: issuable,
           file: file,
-          url: url
+          url: url,
+          url_text: url_text
         )
       end
 

ee/lib/ee/api/entities/issuable_metric_image.rb

@@ -5,7 +5,7 @@ module EE
     module Entities
       class IssuableMetricImage < Grape::Entity
         expose :id, :created_at
-        expose :filename, :file_path, :url
+        expose :filename, :file_path, :url, :url_text
       end
     end
   end

ee/lib/ee/api/issues.rb

@@ -34,6 +34,7 @@ module EE
             params do
               requires :file, type: ::API::Validations::Types::WorkhorseFile, desc: 'The image file to be uploaded'
               optional :url, type: String, desc: 'The url to view more metric info'
+              optional :url_text, type: String, desc: 'A description of the image or URL'
             end
             post do
               require_gitlab_workhorse!
@@ -44,7 +45,7 @@ module EE
               upload = ::IncidentManagement::Incidents::UploadMetricService.new(
                 issue,
                 current_user,
-                params.slice(:file, :url)
+                params.slice(:file, :url, :url_text)
               ).execute
 
               if upload.success?
@@ -71,6 +72,30 @@ module EE
               end
             end
 
+            desc 'Update a metric image for an issue' do
+              success Entities::IssuableMetricImage
+            end
+            params do
+              requires :metric_image_id, type: Integer, desc: 'The ID of metric image'
+              optional :url, type: String, desc: 'The url to view more metric info'
+              optional :url_text, type: String, desc: 'A description of the image or URL'
+            end
+            put ':metric_image_id' do
+              issue = find_project_issue(params[:issue_iid])
+
+              authorize!(:update_issuable_metric_image, issue)
+
+              metric_image = issue.metric_images.find_by_id(params[:metric_image_id])
+
+              render_api_error!('Metric image not found', 404) unless metric_image
+
+              if metric_image&.update(params.slice(:url, :url_text))
+                present metric_image, with: Entities::IssuableMetricImage, current_user: current_user, project: user_project
+              else
+                render_api_error!('Metric image could not be updated', 400)
+              end
+            end
+
             desc 'Remove a metric image for an issue' do
               success Entities::IssuableMetricImage
             end

ee/spec/models/issuable_metric_image_spec.rb

@@ -15,6 +15,7 @@ RSpec.describe IssuableMetricImage do
 
     it { is_expected.not_to allow_value(txt_file).for(:file) }
     it { is_expected.to allow_value(img_file).for(:file) }
+    it { is_expected.to validate_length_of(:url_text).is_at_most(128) }
 
     describe 'url' do
       it { is_expected.not_to allow_value('test').for(:url) }

ee/spec/policies/issuable_policy_spec.rb

@@ -30,19 +30,19 @@ RSpec.describe IssuablePolicy, models: true do
 
       it 'disallows non-members from creating and deleting metric images' do
         expect(permissions(non_member, issue)).to be_allowed(:read_issuable_metric_image)
-        expect(permissions(non_member, issue)).to be_disallowed(:upload_issuable_metric_image, :destroy_issuable_metric_image)
+        expect(permissions(non_member, issue)).to be_disallowed(:upload_issuable_metric_image, :update_issuable_metric_image, :destroy_issuable_metric_image)
       end
 
       it 'allows guests to read, create metric images, and delete them in their own issues' do
         expect(permissions(guest, issue)).to be_allowed(:read_issuable_metric_image)
         expect(permissions(guest, issue)).to be_disallowed(:upload_issuable_metric_image, :destroy_issuable_metric_image)
 
-        expect(permissions(guest, guest_issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
+        expect(permissions(guest, guest_issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :update_issuable_metric_image, :destroy_issuable_metric_image)
       end
 
       it 'allows reporters to create and delete metric images' do
-        expect(permissions(reporter, issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
-        expect(permissions(reporter, reporter_issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
+        expect(permissions(reporter, issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :update_issuable_metric_image, :destroy_issuable_metric_image)
+        expect(permissions(reporter, reporter_issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :update_issuable_metric_image, :destroy_issuable_metric_image)
       end
 
       context 'Timeline events' do
@@ -79,19 +79,19 @@ RSpec.describe IssuablePolicy, models: true do
       let_it_be(:issue) { create(:issue, project: project) }
 
       it 'disallows non-members from creating and deleting metric images' do
-        expect(permissions(non_member, issue)).to be_disallowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
+        expect(permissions(non_member, issue)).to be_disallowed(:read_issuable_metric_image, :upload_issuable_metric_image, :update_issuable_metric_image, :destroy_issuable_metric_image)
       end
 
       it 'allows guests to read metric images, and create + delete in their own issues' do
         expect(permissions(guest, issue)).to be_allowed(:read_issuable_metric_image)
-        expect(permissions(guest, issue)).to be_disallowed(:upload_issuable_metric_image, :destroy_issuable_metric_image)
+        expect(permissions(guest, issue)).to be_disallowed(:upload_issuable_metric_image, :update_issuable_metric_image, :destroy_issuable_metric_image)
 
-        expect(permissions(guest, guest_issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
+        expect(permissions(guest, guest_issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :update_issuable_metric_image, :destroy_issuable_metric_image)
       end
 
       it 'allows reporters to create and delete metric images' do
-        expect(permissions(reporter, issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
-        expect(permissions(reporter, reporter_issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
+        expect(permissions(reporter, issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :update_issuable_metric_image, :destroy_issuable_metric_image)
+        expect(permissions(reporter, reporter_issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :update_issuable_metric_image, :destroy_issuable_metric_image)
       end
 
       context 'Timeline events' do

ee/spec/requests/api/issues_spec.rb

@@ -583,8 +583,9 @@ RSpec.describe API::Issues, :mailer do
     let(:file) { fixture_file_upload('spec/fixtures/rails_sample.jpg', 'image/jpg') }
     let(:file_name) { 'rails_sample.jpg' }
     let(:url) { 'http://gitlab.com' }
+    let(:url_text) { 'GitLab' }
 
-    let(:params) { { url: url } }
+    let(:params) { { url: url, url_text: url_text } }
 
     subject do
       workhorse_finalize(
@@ -604,6 +605,7 @@ RSpec.describe API::Issues, :mailer do
         expect(response).to have_gitlab_http_status(:created)
         expect(json_response['filename']).to eq(file_name)
         expect(json_response['url']).to eq(url)
+        expect(json_response['url_text']).to eq(url_text)
         expect(json_response['file_path']).to match(%r{/uploads/-/system/issuable_metric_image/file/[\d+]/#{file_name}})
         expect(json_response['created_at']).not_to be_nil
         expect(json_response['id']).not_to be_nil
@@ -701,7 +703,8 @@ RSpec.describe API::Issues, :mailer do
             created_at: image.created_at.strftime('%Y-%m-%dT%H:%M:%S.%LZ'),
             filename: image.filename,
             file_path: image.file_path,
-            url: image.url
+            url: image.url,
+            url_text: nil
           }.with_indifferent_access
         )
       end
@@ -736,6 +739,104 @@ RSpec.describe API::Issues, :mailer do
     end
   end
 
+  describe 'PUT /projects/:id/issues/:issue_iid/metric_images/:metric_image_id' do
+    using RSpec::Parameterized::TableSyntax
+
+    let_it_be(:project) do
+      create(:project, :public, creator_id: user.id, namespace: user.namespace)
+    end
+
+    let!(:image) { create(:issuable_metric_image, issue: issue) }
+    let(:params) { { url: 'http://test.example.com', url_text: 'Example website 123' } }
+
+    subject { put api("/projects/#{project.id}/issues/#{issue.iid}/metric_images/#{image.id}", user2), params: params }
+
+    shared_examples 'can_update_metric_image' do
+      it 'can update the metric images' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:success)
+        expect(json_response['url']).to eq(params[:url])
+        expect(json_response['url_text']).to eq(params[:url_text])
+      end
+    end
+
+    shared_examples 'unauthorized_update' do
+      it 'cannot delete the metric image' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+        expect(image.reload).to eq(image)
+      end
+    end
+
+    shared_examples 'not_found' do
+      it 'cannot delete the metric image' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+        expect(image.reload).to eq(image)
+      end
+    end
+
+    where(:user_role, :own_issue, :issue_confidential, :expected_status) do
+      :not_member | false | false | :unauthorized_update
+      :not_member | true  | false | :unauthorized_update
+      :not_member | true  | true  | :unauthorized_update
+      :guest      | false | true  | :not_found
+      :guest      | false | false | :unauthorized_update
+      :guest      | true  | false | :can_update_metric_image
+      :reporter   | true  | false | :can_update_metric_image
+      :reporter   | false | false | :can_update_metric_image
+    end
+
+    with_them do
+      before do
+        stub_licensed_features(incident_metric_upload: true)
+        project.send("add_#{user_role}", user2) unless user_role == :not_member
+      end
+
+      let!(:issue) do
+        author = own_issue ? user2 : user
+        confidential = issue_confidential
+
+        create(:incident, project: project, confidential: confidential, author: author)
+      end
+
+      it_behaves_like "#{params[:expected_status]}"
+    end
+
+    context 'user has access' do
+      let(:issue) { create(:incident, project: project) }
+
+      before do
+        project.add_reporter(user2)
+      end
+
+      context 'metric image not found' do
+        subject { delete api("/projects/#{project.id}/issues/#{issue.iid}/metric_images/#{non_existing_record_id}", user2) }
+
+        it 'returns an error' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:not_found)
+          expect(json_response['message']).to eq('Metric image not found')
+        end
+      end
+
+      context 'metric image cannot be updated' do
+        let(:params) { { url_text: 'something_long' * 100 } }
+
+        it 'returns an error' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['message']).to eq('Metric image could not be updated')
+        end
+      end
+    end
+  end
+
   describe 'DELETE /projects/:id/issues/:issue_iid/metric_images/:metric_image_id' do
     using RSpec::Parameterized::TableSyntax