Code Review Changes

- Do not return entire list of zoom links
- call the the SystemNoteService in the ZoomLinkService
= Use can? method from Gitlab::Allowable
- Add Zoom Meetings to project import spec

app/models/zoom_meeting.rb

@@ -17,7 +17,7 @@ class ZoomMeeting < ApplicationRecord
   scope :canonical, -> (issue) { where(issue: issue).added_to_issue }
 
   def self.canonical_meeting(issue)
-    canonical(issue)&.first
+    canonical(issue)&.take
   end
 
   def self.canonical_meeting_url(issue)

app/services/issues/update_service.rb

@@ -61,8 +61,6 @@ module Issues
       if added_mentions.present?
         notification_service.async.new_mentions_in_issue(issue, added_mentions, current_user)
       end
-
-      ZoomNotesService.new(issue, project, current_user, old_associations).execute
     end
 
     def handle_task_changes(issuable)

app/services/issues/zoom_link_service.rb

@@ -11,7 +11,12 @@ module Issues
 
     def add_link(link)
       if can_add_link? && (link = parse_link(link))
-        success(_('Zoom meeting added'), add_zoom_meeting(link))
+        begin
+          add_zoom_meeting(link)
+          success(_('Zoom meeting added'))
+        rescue ActiveRecord::RecordNotUnique
+          error(_('Failed to add a Zoom meeting'))
+        end
       else
         error(_('Failed to add a Zoom meeting'))
       end
@@ -19,18 +24,19 @@ module Issues
 
     def remove_link
       if can_remove_link?
-        success(_('Zoom meeting removed'), remove_zoom_meeting)
+        remove_zoom_meeting
+        success(_('Zoom meeting removed'))
       else
         error(_('Failed to remove a Zoom meeting'))
       end
     end
 
     def can_add_link?
-      can? && !@added_meeting
+      can_update_issue? && !@added_meeting
     end
 
     def can_remove_link?
-      can? && !!@added_meeting
+      can_update_issue? && !!@added_meeting
     end
 
     def parse_link(link)
@@ -57,28 +63,25 @@ module Issues
         url: link
       )
       track_meeting_added_event
-      issue.zoom_meetings
+      SystemNoteService.zoom_link_added(@issue, @project, current_user)
     end
 
     def remove_zoom_meeting
       @added_meeting.update(issue_status: :removed)
       track_meeting_removed_event
-      issue.zoom_meetings
+      SystemNoteService.zoom_link_removed(@issue, @project, current_user)
     end
 
-    def success(message, zoom_meetings)
-      ServiceResponse.success(
-        message: message,
-        payload: { zoom_meetings: zoom_meetings }
-      )
+    def success(message)
+      ServiceResponse.success(message: message)
     end
 
     def error(message)
       ServiceResponse.error(message: message)
     end
 
-    def can?
-      current_user.can?(:update_issue, project)
+    def can_update_issue?
+      can?(current_user, :update_issue, project)
     end
   end
 end

app/services/zoom_notes_service.rb

-# frozen_string_literal: true
-
-class ZoomNotesService
-  def initialize(issue, project, current_user, old_associations)
-    @issue = issue
-    @project = project
-    @current_user = current_user
-    @old_zoom_meetings = old_associations ? old_associations.fetch(:zoom_meetings, []) : []
-  end
-
-  def execute
-    return if @issue.zoom_meetings == @old_zoom_meetings
-
-    if zoom_link_added?
-      zoom_link_added_notification
-    else
-      zoom_link_removed_notification
-    end
-  end
-
-  private
-
-  def zoom_link_added?
-    @issue.zoom_meetings.any? { |z| z.issue_status == "added" }
-  end
-
-  def zoom_link_added_notification
-    SystemNoteService.zoom_link_added(@issue, @project, @current_user)
-  end
-
-  def zoom_link_removed_notification
-    SystemNoteService.zoom_link_removed(@issue, @project, @current_user)
-  end
-end

changelogs/unreleased/33042-persist-zoom-meetings-added-to-issues-in-the-database-2.yml

 ---
-title: Move zoom urls on issues added and removed by the zoom quick action commands
-  out of the issue discription and into a zoom meetings table
-merge_request:
+title: Store Zoom URLs in a table rather than in the issue description
+merge_request: 18620
 author:
 type: changed

lib/gitlab/import_export/import_export.yml

@@ -11,6 +11,7 @@ tree:
       - events:
         - :push_event_payload
     - issues:
+      - :zoom_meetings
       - events:
         - :push_event_payload
       - :timelogs

lib/gitlab/quick_actions/issue_actions.rb

@@ -182,11 +182,6 @@ module Gitlab
         end
         command :zoom do |link|
           result = @zoom_service.add_link(link)
-
-          if result.success?
-            @updates[:zoom_meetings] = result.payload[:zoom_meetings]
-          end
-
           @execution_message[:zoom] = result.message
         end
 
@@ -200,11 +195,6 @@ module Gitlab
         end
         command :remove_zoom do
           result = @zoom_service.remove_link
-
-          if result.success?
-            @updates[:zoom_meetings] = result.payload[:zoom_meetings]
-          end
-
           @execution_message[:remove_zoom] = result.message
         end
 

spec/fixtures/lib/gitlab/import_export/project.json

@@ -80,6 +80,17 @@
           "issue_id": 40
         }
       ],
+      "zoom_meetings": [
+        {
+          "id": 1,
+          "project_id": 5,
+          "issue_id": 40,
+          "url": "https://zoom.us/j/123456789",
+          "issue_status": 1,
+          "created_at": "2016-06-14T15:02:04.418Z",
+          "updated_at": "2016-06-14T15:02:04.418Z"
+        }
+      ],
       "milestone": {
         "id": 1,
         "title": "test milestone",

spec/lib/gitlab/import_export/project_tree_restorer_spec.rb

@@ -211,6 +211,12 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
         expect(CustomIssueTrackerService.first).not_to be_nil
       end
 
+      it 'restores zoom meetings' do
+        meetings = @project.issues.first.zoom_meetings
+        expect(meetings.count).to eq(1)
+        expect(meetings.first.url).to eq("https://zoom.us/j/123456789")
+      end
+
       context 'Merge requests' do
         it 'always has the new project as a target' do
           expect(MergeRequest.find_by_title('MR1').target_project).to eq(@project)

spec/services/issues/update_service_spec.rb

@@ -222,15 +222,6 @@ describe Issues::UpdateService, :mailer do
       end
     end
 
-    context 'when zoom_meetings is changed' do
-      it 'creates zoom_link_added system note when a zoom link is added' do
-        update_issue(zoom_meetings: [create(:zoom_meeting, issue: issue)])
-        note = find_note('added a Zoom call')
-
-        expect(note.note).to eq('added a Zoom call to this issue')
-      end
-    end
-
     context 'when issue turns confidential' do
       let(:opts) do
         {

spec/services/issues/zoom_link_service_spec.rb

@@ -37,8 +37,7 @@ describe Issues::ZoomLinkService do
     shared_examples 'can add meeting' do
       it 'appends the new meeting to zoom_meetings' do
         expect(result).to be_success
-        expect(result.payload[:zoom_meetings].map(&:url))
-          .to include(zoom_link)
+        expect(ZoomMeeting.canonical_meeting_url(issue)).to eq(zoom_link)
       end
 
       it 'tracks the add event' do
@@ -46,6 +45,12 @@ describe Issues::ZoomLinkService do
           .with('IncidentManagement::ZoomIntegration', 'add_zoom_meeting', label: 'Issue ID', value: issue.id)
         result
       end
+
+      it 'creates a zoom_link_added notification' do
+        expect(SystemNoteService).to receive(:zoom_link_added).with(issue, project, user)
+        expect(SystemNoteService).not_to receive(:zoom_link_removed)
+        result
+      end
     end
 
     shared_examples 'cannot add meeting' do
@@ -53,6 +58,12 @@ describe Issues::ZoomLinkService do
         expect(result).to be_error
         expect(result.message).to eq('Failed to add a Zoom meeting')
       end
+
+      it 'creates no notification' do
+        expect(SystemNoteService).not_to receive(:zoom_link_added)
+        expect(SystemNoteService).not_to receive(:zoom_link_removed)
+        result
+      end
     end
 
     subject(:result) { service.add_link(zoom_link) }
@@ -76,6 +87,15 @@ describe Issues::ZoomLinkService do
       include_context '"added" Zoom meeting'
       include_examples 'cannot add meeting'
     end
+
+    context 'with "added" Zoom meeting and race condition' do
+      include_context '"added" Zoom meeting'
+      before do
+        allow(service).to receive(:can_add_link?).and_return(true)
+      end
+
+      include_examples 'cannot add meeting'
+    end
   end
 
   describe '#can_add_link?' do
@@ -104,13 +124,24 @@ describe Issues::ZoomLinkService do
         expect(result).to be_error
         expect(result.message).to eq('Failed to remove a Zoom meeting')
       end
+
+      it 'creates no notification' do
+        expect(SystemNoteService).not_to receive(:zoom_link_added)
+        expect(SystemNoteService).not_to receive(:zoom_link_removed)
+        result
+      end
     end
 
     shared_examples 'can remove meeting' do
+      it 'creates no notification' do
+        expect(SystemNoteService).not_to receive(:zoom_link_added).with(issue, project, user)
+        expect(SystemNoteService).to receive(:zoom_link_removed)
+        result
+      end
+
       it 'can remove the meeting' do
         expect(result).to be_success
-        expect(result.payload[:zoom_meetings].filter { |z| z.issue_status == "added" })
-        .to be_empty
+        expect(ZoomMeeting.canonical_meeting_url(issue)).to eq(nil)
       end
 
       it 'tracks the remove event' do

spec/services/zoom_notes_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe ZoomNotesService do
-  describe '#execute' do
-    let(:issue) { OpenStruct.new(zoom_meetings: zoom_meetings) }
-    let(:project) { Object.new }
-    let(:user) { Object.new }
-    let(:added_zoom_meeting) { OpenStruct.new(issue_status: "added") }
-    let(:removed_zoom_meeting) { OpenStruct.new(issue_status: "removed") }
-    let(:old_zoom_meetings) { [] }
-    let(:zoom_meetings) { [] }
-
-    subject { described_class.new(issue, project, user, zoom_meetings: old_zoom_meetings) }
-
-    shared_examples 'no notifications' do
-      it "doesn't create notifications" do
-        expect(SystemNoteService).not_to receive(:zoom_link_added)
-        expect(SystemNoteService).not_to receive(:zoom_link_removed)
-
-        subject.execute
-      end
-    end
-
-    shared_examples 'added notification' do
-      it 'creates a zoom_link_added notification' do
-        expect(SystemNoteService).to receive(:zoom_link_added).with(issue, project, user)
-        expect(SystemNoteService).not_to receive(:zoom_link_removed)
-
-        subject.execute
-      end
-    end
-
-    shared_examples 'removed notification' do
-      it 'creates a zoom_link_removed notification' do
-        expect(SystemNoteService).not_to receive(:zoom_link_added).with(issue, project, user)
-        expect(SystemNoteService).to receive(:zoom_link_removed)
-
-        subject.execute
-      end
-    end
-
-    context 'when zoom_meetings is not in old_associations' do
-      subject { described_class.new(issue, project, user, {}) }
-
-      context 'when zoom_meetings is empty' do
-        it_behaves_like 'no notifications'
-      end
-
-      context 'when zoom_meetings has "added" meeting' do
-        let(:zoom_meetings) { [added_zoom_meeting] }
-
-        it_behaves_like 'added notification'
-      end
-
-      context 'when zoom_meetings has "removed" meeting' do
-        let(:zoom_meetings) { [removed_zoom_meeting] }
-
-        it_behaves_like 'removed notification'
-      end
-    end
-
-    context 'when zoom_meetings == old_zoom_meetings' do
-      context 'when both are empty' do
-        it_behaves_like 'no notifications'
-      end
-
-      context 'when both are removed' do
-        let(:old_zoom_meetings) { [removed_zoom_meeting] }
-        let(:zoom_meetings) { old_zoom_meetings }
-
-        it_behaves_like 'no notifications'
-      end
-
-      context 'when both are added' do
-        let(:old_zoom_meetings) { [added_zoom_meeting] }
-        let(:zoom_meetings) { old_zoom_meetings }
-
-        it_behaves_like 'no notifications'
-      end
-    end
-
-    context 'when old_zoom_meetings is empty and zoom_meetings contains an "added" zoom_meeting' do
-      let(:old_zoom_meetings) { [] }
-      let(:zoom_meetings) { [added_zoom_meeting] }
-
-      it_behaves_like 'added notification'
-    end
-
-    context 'when a "added" meeting is added to a list of "removed" meetings' do
-      let(:old_zoom_meetings) { [removed_zoom_meeting] }
-      let(:zoom_meetings) { [removed_zoom_meeting, added_zoom_meeting] }
-
-      it_behaves_like 'added notification'
-    end
-
-    context 'when zoom_meetings no longer has an "added" zoom meeting' do
-      let(:old_zoom_meetings) { [added_zoom_meeting] }
-      let(:zoom_meetings) { [removed_zoom_meeting] }
-
-      it_behaves_like 'removed notification'
-    end
-  end
-end