Commit 72e22b5a authored by Allison Browne's avatar Allison Browne Committed by Rémy Coutable

Account for creating an issue with Zoom quick action

The issue was not persisted at the time of issue creation so a 500 was
thrown. Adjust the quick action to work on Issue create by accounting
for the un-persisted issue in the zoom link service.  Also, add the
zoom_url to the realtime endpoint for front end realtime consumption.
parent e555de12
...@@ -13,30 +13,29 @@ module Issues ...@@ -13,30 +13,29 @@ module Issues
if can_add_link? && (link = parse_link(link)) if can_add_link? && (link = parse_link(link))
begin begin
add_zoom_meeting(link) add_zoom_meeting(link)
success(_('Zoom meeting added'))
rescue ActiveRecord::RecordNotUnique rescue ActiveRecord::RecordNotUnique
error(_('Failed to add a Zoom meeting')) error(message: _('Failed to add a Zoom meeting'))
end end
else else
error(_('Failed to add a Zoom meeting')) error(message: _('Failed to add a Zoom meeting'))
end end
end end
def remove_link def remove_link
if can_remove_link? if can_remove_link?
remove_zoom_meeting remove_zoom_meeting
success(_('Zoom meeting removed')) success(message: _('Zoom meeting removed'))
else else
error(_('Failed to remove a Zoom meeting')) error(message: _('Failed to remove a Zoom meeting'))
end end
end end
def can_add_link? def can_add_link?
can_update_issue? && !@added_meeting can_change_link? && !@added_meeting
end end
def can_remove_link? def can_remove_link?
can_update_issue? && !!@added_meeting can_change_link? && @issue.persisted? && !!@added_meeting
end end
def parse_link(link) def parse_link(link)
...@@ -56,14 +55,29 @@ module Issues ...@@ -56,14 +55,29 @@ module Issues
end end
def add_zoom_meeting(link) def add_zoom_meeting(link)
ZoomMeeting.create( zoom_meeting = new_zoom_meeting(link)
response =
if @issue.persisted?
# Save the meeting directly since we only want to update one meeting, not all
zoom_meeting.save
success(message: _('Zoom meeting added'))
else
success(message: _('Zoom meeting added'), payload: { zoom_meetings: [zoom_meeting] })
end
track_meeting_added_event
SystemNoteService.zoom_link_added(@issue, @project, current_user)
response
end
def new_zoom_meeting(link)
ZoomMeeting.new(
issue: @issue, issue: @issue,
project: @issue.project, project: @project,
issue_status: :added, issue_status: :added,
url: link url: link
) )
track_meeting_added_event
SystemNoteService.zoom_link_added(@issue, @project, current_user)
end end
def remove_zoom_meeting def remove_zoom_meeting
...@@ -72,16 +86,20 @@ module Issues ...@@ -72,16 +86,20 @@ module Issues
SystemNoteService.zoom_link_removed(@issue, @project, current_user) SystemNoteService.zoom_link_removed(@issue, @project, current_user)
end end
def success(message) def success(message:, payload: nil)
ServiceResponse.success(message: message) ServiceResponse.success(message: message, payload: payload)
end end
def error(message) def error(message:)
ServiceResponse.error(message: message) ServiceResponse.error(message: message)
end end
def can_update_issue? def can_change_link?
can?(current_user, :update_issue, project) if @issue.persisted?
can?(current_user, :update_issue, @project)
else
can?(current_user, :create_issue, @project)
end
end end
end end
end end
---
title: Fix Zoom Quick Action server error when creating a GitLab Issue
merge_request: 21262
author:
type: fixed
...@@ -183,6 +183,7 @@ module Gitlab ...@@ -183,6 +183,7 @@ module Gitlab
command :zoom do |link| command :zoom do |link|
result = @zoom_service.add_link(link) result = @zoom_service.add_link(link)
@execution_message[:zoom] = result.message @execution_message[:zoom] = result.message
@updates.merge!(result.payload) if result.payload
end end
desc _('Remove Zoom meeting') desc _('Remove Zoom meeting')
......
...@@ -27,12 +27,18 @@ describe Issues::ZoomLinkService do ...@@ -27,12 +27,18 @@ describe Issues::ZoomLinkService do
end end
end end
shared_context 'insufficient permissions' do shared_context 'insufficient issue update permissions' do
before do before do
project.add_guest(user) project.add_guest(user)
end end
end end
shared_context 'insufficient issue create permissions' do
before do
expect(service).to receive(:can?).with(user, :create_issue, project).and_return(false)
end
end
describe '#add_link' do describe '#add_link' do
shared_examples 'can add meeting' do shared_examples 'can add meeting' do
it 'appends the new meeting to zoom_meetings' do it 'appends the new meeting to zoom_meetings' do
...@@ -69,16 +75,38 @@ describe Issues::ZoomLinkService do ...@@ -69,16 +75,38 @@ describe Issues::ZoomLinkService do
subject(:result) { service.add_link(zoom_link) } subject(:result) { service.add_link(zoom_link) }
context 'without existing Zoom meeting' do context 'without existing Zoom meeting' do
context 'when updating an issue' do
before do
allow(issue).to receive(:persisted?).and_return(true)
end
include_examples 'can add meeting' include_examples 'can add meeting'
context 'with invalid Zoom url' do context 'with insufficient issue update permissions' do
let(:zoom_link) { 'https://not-zoom.link' } include_context 'insufficient issue update permissions'
include_examples 'cannot add meeting'
end
end
context 'when creating an issue' do
before do
allow(issue).to receive(:persisted?).and_return(false)
end
it 'creates a new zoom meeting' do
expect(result).to be_success
expect(result.payload[:zoom_meetings][0].url).to eq(zoom_link)
end
context 'with insufficient issue create permissions' do
include_context 'insufficient issue create permissions'
include_examples 'cannot add meeting' include_examples 'cannot add meeting'
end end
end
context 'with invalid Zoom url' do
let(:zoom_link) { 'https://not-zoom.link' }
context 'with insufficient permissions' do
include_context 'insufficient permissions'
include_examples 'cannot add meeting' include_examples 'cannot add meeting'
end end
end end
...@@ -92,6 +120,7 @@ describe Issues::ZoomLinkService do ...@@ -92,6 +120,7 @@ describe Issues::ZoomLinkService do
include_context '"added" Zoom meeting' include_context '"added" Zoom meeting'
before do before do
allow(service).to receive(:can_add_link?).and_return(true) allow(service).to receive(:can_add_link?).and_return(true)
allow(issue).to receive(:persisted?).and_return(true)
end end
include_examples 'cannot add meeting' include_examples 'cannot add meeting'
...@@ -104,8 +133,8 @@ describe Issues::ZoomLinkService do ...@@ -104,8 +133,8 @@ describe Issues::ZoomLinkService do
context 'without "added" zoom meeting' do context 'without "added" zoom meeting' do
it { is_expected.to eq(true) } it { is_expected.to eq(true) }
context 'with insufficient permissions' do context 'with insufficient issue update permissions' do
include_context 'insufficient permissions' include_context 'insufficient issue update permissions'
it { is_expected.to eq(false) } it { is_expected.to eq(false) }
end end
...@@ -156,12 +185,24 @@ describe Issues::ZoomLinkService do ...@@ -156,12 +185,24 @@ describe Issues::ZoomLinkService do
context 'with Zoom meeting' do context 'with Zoom meeting' do
include_context '"added" Zoom meeting' include_context '"added" Zoom meeting'
context 'removes the link' do context 'with existing issue' do
before do
allow(issue).to receive(:persisted?).and_return(true)
end
include_examples 'can remove meeting' include_examples 'can remove meeting'
end end
context 'with insufficient permissions' do context 'without existing issue' do
include_context 'insufficient permissions' before do
allow(issue).to receive(:persisted?).and_return(false)
end
include_examples 'cannot remove meeting'
end
context 'with insufficient issue update permissions' do
include_context 'insufficient issue update permissions'
include_examples 'cannot remove meeting' include_examples 'cannot remove meeting'
end end
end end
...@@ -193,8 +234,8 @@ describe Issues::ZoomLinkService do ...@@ -193,8 +234,8 @@ describe Issues::ZoomLinkService do
it { is_expected.to eq(true) } it { is_expected.to eq(true) }
end end
context 'with insufficient permissions' do context 'with insufficient issue update permissions' do
include_context 'insufficient permissions' include_context 'insufficient issue update permissions'
it { is_expected.to eq(false) } it { is_expected.to eq(false) }
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment