Merge branch 'remove_hyperlink_from_close_reopen_button' into 'master'

Remove hyperlink from close reopen button

Closes #15498

See merge request gitlab-org/gitlab!22220

app/assets/javascripts/close_reopen_report_toggle.js

@@ -80,12 +80,7 @@ class CloseReopenReportToggle {
         {
           input: this.button,
           valueAttribute: 'data-url',
-          inputAttribute: 'href',
-        },
-        {
-          input: this.button,
-          valueAttribute: 'data-method',
-          inputAttribute: 'data-method',
+          inputAttribute: 'data-endpoint',
         },
       ],
     };

app/assets/javascripts/issue.js

@@ -11,7 +11,7 @@ import { __ } from './locale';
 
 export default class Issue {
   constructor() {
-    if ($('a.btn-close').length) this.initIssueBtnEventListeners();
+    if ($('.btn-close, .btn-reopen').length) this.initIssueBtnEventListeners();
 
     if ($('.js-close-blocked-issue-warning').length) this.initIssueWarningBtnEventListener();
 
@@ -32,8 +32,8 @@ export default class Issue {
       Issue.initRelatedBranches();
     }
 
-    this.closeButtons = $('a.btn-close');
-    this.reopenButtons = $('a.btn-reopen');
+    this.closeButtons = $('.btn-close');
+    this.reopenButtons = $('.btn-reopen');
 
     this.initCloseReopenReport();
 
@@ -103,7 +103,7 @@ export default class Issue {
     // NOTE: data attribute seems unnecessary but is actually necessary
     return $('.js-issuable-buttons[data-action="close-reopen"]').on(
       'click',
-      'a.btn-close, a.btn-reopen, a.btn-close-anyway',
+      '.btn-close, .btn-reopen, .btn-close-anyway',
       e => {
         e.preventDefault();
         e.stopImmediatePropagation();
@@ -120,7 +120,7 @@ export default class Issue {
         } else {
           this.disableCloseReopenButton($button);
 
-          const url = $button.attr('href');
+          const url = $button.data('endpoint');
 
           return axios
             .put(url)

app/assets/javascripts/merge_request.js

 /* eslint-disable func-names, no-underscore-dangle, consistent-return */
 
 import $ from 'jquery';
+import axios from './lib/utils/axios_utils';
 import { __ } from '~/locale';
 import createFlash from '~/flash';
 import TaskList from './task_list';
@@ -65,9 +66,17 @@ MergeRequest.prototype.showAllCommits = function() {
 
 MergeRequest.prototype.initMRBtnListeners = function() {
   const _this = this;
-  return $('a.btn-close, a.btn-reopen').on('click', function(e) {
+  return $('.btn-close, .btn-reopen').on('click', function(e) {
     const $this = $(this);
     const shouldSubmit = $this.hasClass('btn-comment');
+    if ($this.hasClass('js-btn-issue-action')) {
+      const url = $this.data('endpoint');
+      return axios
+        .put(url)
+        .then(() => window.location.reload())
+        .catch(() => createFlash(__('Something went wrong.')));
+    }
+
     if (shouldSubmit && $this.data('submitted')) {
       return;
     }

app/assets/javascripts/notes/components/comment_form.vue

@@ -125,9 +125,13 @@ export default {
     canToggleIssueState() {
       return (
         this.getNoteableData.current_user.can_update &&
-        this.getNoteableData.state !== constants.MERGED
+        this.getNoteableData.state !== constants.MERGED &&
+        !this.closedAndLocked
       );
     },
+    closedAndLocked() {
+      return !this.isOpen && this.isLocked(this.getNoteableData);
+    },
     endpoint() {
       return this.getNoteableData.create_note_path;
     },

app/assets/javascripts/notes/stores/actions.js

@@ -205,7 +205,6 @@ export const closeIssue = ({ commit, dispatch, state }) => {
     commit(types.CLOSE_ISSUE);
     dispatch('emitStateChangedEvent', data);
     dispatch('toggleStateButtonLoading', false);
-    dispatch('toggleBlockedIssueWarning', false);
   });
 };
 

app/helpers/issuables_helper.rb

@@ -367,15 +367,6 @@ module IssuablesHelper
     end
   end
 
-  def issuable_close_reopen_button_method(issuable)
-    case issuable
-    when Issue
-      ''
-    when MergeRequest
-      'put'
-    end
-  end
-
   def issuable_author_is_current_user(issuable)
     issuable.author == current_user
   end

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

 - is_current_user = issuable_author_is_current_user(issuable)
 - display_issuable_type = issuable_display_type(issuable)
-- button_method = issuable_close_reopen_button_method(issuable)
 - are_close_and_open_buttons_hidden = issuable_button_hidden?(issuable, true) && issuable_button_hidden?(issuable, false)
 - add_blocked_class = false
 - if defined? warn_before_close
@@ -8,11 +7,13 @@
 
 - if is_current_user
   - if can_update
-    = link_to _("Close %{display_issuable_type}") % { display_issuable_type: display_issuable_type }, close_issuable_path(issuable), method: button_method,
-              class: "d-none d-sm-none d-md-block btn btn-grouped btn-close js-btn-issue-action #{issuable_button_visibility(issuable, true)} #{(add_blocked_class ? 'btn-issue-blocked' : '')}", title: _("Close %{display_issuable_type}") % { display_issuable_type: display_issuable_type }, data: { qa_selector: 'close_issue_button' }
+    %button{ class: "d-none d-sm-none d-md-block btn btn-grouped btn-close js-btn-issue-action #{issuable_button_visibility(issuable, true)} #{(add_blocked_class ? 'btn-issue-blocked' : '')}",
+             data: { remote: 'true', endpoint: close_issuable_path(issuable), qa_selector: 'close_issue_button' } }
+      = _("Close %{display_issuable_type}") % { display_issuable_type: display_issuable_type }
   - if can_reopen
-    = link_to _("Reopen %{display_issuable_type}") % { display_issuable_type: display_issuable_type }, reopen_issuable_path(issuable), method: button_method,
-              class: "d-none d-sm-none d-md-block btn btn-grouped btn-reopen js-btn-issue-action #{issuable_button_visibility(issuable, false)}", title: _("Reopen %{display_issuable_type}") % { display_issuable_type: display_issuable_type }, data: { qa_selector: 'reopen_issue_button' }
+    %button{ class: "d-none d-sm-none d-md-block btn btn-grouped btn-reopen js-btn-issue-action #{issuable_button_visibility(issuable, false)}",
+             data: { remote: 'true', endpoint: reopen_issuable_path(issuable), qa_selector: 'reopen_issue_button' } }
+      = _("Reopen %{display_issuable_type}") % { display_issuable_type: display_issuable_type }
 - else
   - if can_update && !are_close_and_open_buttons_hidden
     = render 'shared/issuable/close_reopen_report_toggle', issuable: issuable, warn_before_close: add_blocked_class

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

@@ -4,14 +4,13 @@
 - button_responsive_class = 'd-none d-sm-none d-md-block'
 - button_class = "#{button_responsive_class} btn btn-grouped js-issuable-close-button js-btn-issue-action issuable-close-button"
 - toggle_class = "#{button_responsive_class} btn btn-nr dropdown-toggle js-issuable-close-toggle"
-- button_method = issuable_close_reopen_button_method(issuable)
 - add_blocked_class = false
 - if defined? warn_before_close
   - add_blocked_class = !issuable.closed? && warn_before_close
 
 .float-left.btn-group.prepend-left-10.issuable-close-dropdown.droplab-dropdown.js-issuable-close-dropdown
-  = link_to "#{display_button_action} #{display_issuable_type}", close_reopen_issuable_path(issuable),
-            method: button_method, class: "#{button_class} btn-#{button_action} #{(add_blocked_class ? 'btn-issue-blocked' : '')}", title: "#{display_button_action} #{display_issuable_type}", data: { qa_selector: 'close_issue_button' }
+  %button{ class: "#{button_class} btn-#{button_action} #{(add_blocked_class ? 'btn-issue-blocked' : '')}", data: { qa_selector: 'close_issue_button', endpoint: close_reopen_issuable_path(issuable) } }
+    #{display_button_action} #{display_issuable_type}
 
   = button_tag type: 'button', class: "#{toggle_class} btn-#{button_action}-color",
                data: { 'dropdown-trigger' => '#issuable-close-menu' }, 'aria-label' => _('Toggle dropdown') do
@@ -20,7 +19,7 @@
   %ul#issuable-close-menu.js-issuable-close-menu.dropdown-menu{ data: { dropdown: true } }
     %li.close-item{ class: "#{issuable_button_visibility(issuable, true) || 'droplab-item-selected'}",
                     data: { text: _("Close %{display_issuable_type}") % { display_issuable_type: display_issuable_type }, url: close_issuable_path(issuable),
-                    button_class: "#{button_class} btn-close", toggle_class: "#{toggle_class} btn-close-color", method: button_method } }
+                    button_class: "#{button_class} btn-close", toggle_class: "#{toggle_class} btn-close-color" } }
       %button.btn.btn-transparent
         = icon('check', class: 'icon')
         .description
@@ -30,7 +29,7 @@
 
     %li.reopen-item{ class: "#{issuable_button_visibility(issuable, false) || 'droplab-item-selected'}",
                      data: { text: _("Reopen %{display_issuable_type}") % { display_issuable_type: display_issuable_type }, url: reopen_issuable_path(issuable),
-                     button_class: "#{button_class} btn-reopen", toggle_class: "#{toggle_class} btn-reopen-color", method: button_method } }
+                     button_class: "#{button_class} btn-reopen", toggle_class: "#{toggle_class} btn-reopen-color" } }
       %button.btn.btn-transparent
         = icon('check', class: 'icon')
         .description

changelogs/unreleased/remove_hyperlink_from_close_reopen_button.yml

+---
+title: Remove broken hyperlink from close and reopen button
+merge_request: 22220
+author: Lee Tickett
+type: fixed

ee/app/views/projects/issues/_alert_blocked.html.haml

@@ -9,7 +9,7 @@
     .gl-alert-body
       = _('This issue is currently blocked by the following issues: %{issues}.').html_safe % { issues: blocked_by_issues_links }
     .gl-alert-actions
-      = link_to _("Yes, close issue"), close_issuable_path(issue), rel: 'nofollow', method: '',
-                class: "btn btn-close-anyway gl-alert-action btn-warning btn-md gl-button", title: _("Yes, close issue")
+      %button{ class: "btn btn-close-anyway gl-alert-action btn-warning btn-md gl-button", data: { endpoint: close_issuable_path(@issue) } }
+        = _("Yes, close issue")
       %button.btn.gl-alert-action.btn-warning.btn-md.gl-button.btn-secondary
         = s_('Cancel')

ee/spec/features/merge_request/user_selects_branches_for_new_mr_spec.rb

@@ -27,7 +27,7 @@ RSpec.describe 'Merge request > User selects branches for new MR', :js do
         fill_in 'merge_request_title', with: 'Test'
         click_button 'Submit merge request'
 
-        expect(page).to have_link('Close merge request')
+        expect(page).to have_button('Close merge request')
       end
     end
   end

ee/spec/frontend/issue_spec.js

@@ -98,7 +98,7 @@ describe('Issue', () => {
 
       testContext.$triggeredButton = $btn;
 
-      mockCloseButtonResponseSuccess(testContext.$triggeredButton.attr('href'), {
+      mockCloseButtonResponseSuccess(testContext.$triggeredButton.data('endpoint'), {
         id: 34,
       });
 

locale/gitlab.pot

@@ -21196,6 +21196,9 @@ msgstr ""
 msgid "Something went wrong, unable to search projects"
 msgstr ""
 
+msgid "Something went wrong."
+msgstr ""
+
 msgid "Something went wrong. Please try again."
 msgstr ""
 

spec/features/issuables/close_reopen_report_toggle_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe 'Issuables Close/Reopen/Report toggle' do
     let(:human_model_name) { issuable.model_name.human.downcase }
 
     it 'shows toggle' do
-      expect(page).to have_link("Close #{human_model_name}")
+      expect(page).to have_button("Close #{human_model_name}")
       expect(page).to have_selector('.issuable-close-dropdown')
     end
 
@@ -63,7 +63,7 @@ RSpec.describe 'Issuables Close/Reopen/Report toggle' do
         let(:issuable) { create(:issue, :closed, :locked, project: project) }
 
         it 'hides the reopen button' do
-          expect(page).not_to have_link('Reopen issue')
+          expect(page).not_to have_button('Reopen issue')
         end
 
         context 'when the issue author is the current user' do
@@ -72,7 +72,7 @@ RSpec.describe 'Issuables Close/Reopen/Report toggle' do
           end
 
           it 'hides the reopen button' do
-            expect(page).not_to have_link('Reopen issue')
+            expect(page).not_to have_button('Reopen issue')
           end
         end
       end
@@ -91,8 +91,8 @@ RSpec.describe 'Issuables Close/Reopen/Report toggle' do
       it 'only shows the `Report abuse` and `New issue` buttons' do
         expect(page).to have_link('Report abuse')
         expect(page).to have_link('New issue')
-        expect(page).not_to have_link('Close issue')
-        expect(page).not_to have_link('Reopen issue')
+        expect(page).not_to have_button('Close issue')
+        expect(page).not_to have_button('Reopen issue')
         expect(page).not_to have_link('Edit')
       end
     end
@@ -120,8 +120,8 @@ RSpec.describe 'Issuables Close/Reopen/Report toggle' do
         it 'shows only the `Report abuse` and `Edit` button' do
           expect(page).to have_link('Report abuse')
           expect(page).to have_link('Edit')
-          expect(page).not_to have_link('Close merge request')
-          expect(page).not_to have_link('Reopen merge request')
+          expect(page).not_to have_button('Close merge request')
+          expect(page).not_to have_button('Reopen merge request')
         end
 
         context 'when the merge request author is the current user' do
@@ -130,8 +130,8 @@ RSpec.describe 'Issuables Close/Reopen/Report toggle' do
           it 'shows only the `Edit` button' do
             expect(page).to have_link('Edit')
             expect(page).not_to have_link('Report abuse')
-            expect(page).not_to have_link('Close merge request')
-            expect(page).not_to have_link('Reopen merge request')
+            expect(page).not_to have_button('Close merge request')
+            expect(page).not_to have_button('Reopen merge request')
           end
         end
       end
@@ -149,8 +149,8 @@ RSpec.describe 'Issuables Close/Reopen/Report toggle' do
 
       it 'only shows a `Report abuse` button' do
         expect(page).to have_link('Report abuse')
-        expect(page).not_to have_link('Close merge request')
-        expect(page).not_to have_link('Reopen merge request')
+        expect(page).not_to have_button('Close merge request')
+        expect(page).not_to have_button('Reopen merge request')
         expect(page).not_to have_link('Edit')
       end
     end

spec/features/merge_request/user_closes_merge_request_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe 'User closes a merge requests', :js do
   end
 
   it 'closes a merge request' do
-    click_link('Close merge request', match: :first)
+    click_button('Close merge request', match: :first)
 
     expect(page).to have_content(merge_request.title)
     expect(page).to have_content('Closed by')

spec/features/merge_request/user_reopens_merge_request_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe 'User reopens a merge requests', :js do
   end
 
   it 'reopens a merge request' do
-    click_link('Reopen merge request', match: :first)
+    click_button('Reopen merge request', match: :first)
 
     page.within('.status-box') do
       expect(page).to have_content('Open')

spec/features/task_lists_spec.rb

@@ -103,7 +103,7 @@ RSpec.describe 'Task Lists' do
         wait_for_requests
 
         expect(page).to have_selector(".md .task-list .task-list-item .task-list-item-checkbox")
-        expect(page).to have_selector('a.btn-close')
+        expect(page).to have_selector('.btn-close')
       end
 
       it 'is only editable by author' do

spec/frontend/close_reopen_report_toggle_spec.js

@@ -274,12 +274,7 @@ describe('CloseReopenReportToggle', () => {
           {
             input: button,
             valueAttribute: 'data-url',
-            inputAttribute: 'href',
-          },
-          {
-            input: button,
-            valueAttribute: 'data-method',
-            inputAttribute: 'data-method',
+            inputAttribute: 'data-endpoint',
           },
         ],
       });