Merge branch...

Merge branch '29483-no-feedback-when-checking-on-checklist-if-potential-spam-was-detected' into 'master'

Resolve "No feedback when checking on checklist if potential spam was detected"

Closes #29483

See merge request gitlab-org/gitlab-ce!15408

app/assets/javascripts/issue.js

@@ -8,18 +8,7 @@ import IssuablesHelper from './helpers/issuables_helper';
 
 export default class Issue {
   constructor() {
-    if ($('a.btn-close').length) {
-      this.taskList = new TaskList({
-        dataType: 'issue',
-        fieldName: 'description',
-        selector: '.detail-page-description',
-        onSuccess: (result) => {
-          document.querySelector('#task_status').innerText = result.task_status;
-          document.querySelector('#task_status_short').innerText = result.task_status_short;
-        }
-      });
-      this.initIssueBtnEventListeners();
-    }
+    if ($('a.btn-close').length) this.initIssueBtnEventListeners();
 
     Issue.$btnNewBranch = $('#new-branch');
     Issue.createMrDropdownWrap = document.querySelector('.create-mr-dropdown-wrap');

app/assets/javascripts/issue_show/components/app.vue

@@ -9,6 +9,7 @@ import descriptionComponent from './description.vue';
 import editedComponent from './edited.vue';
 import formComponent from './form.vue';
 import '../../lib/utils/url_utility';
+import RecaptchaDialogImplementor from '../../vue_shared/mixins/recaptcha_dialog_implementor';
 
 export default {
   props: {
@@ -149,6 +150,11 @@ export default {
     editedComponent,
     formComponent,
   },
+
+  mixins: [
+    RecaptchaDialogImplementor,
+  ],
+
   methods: {
     openForm() {
       if (!this.showForm) {
@@ -164,9 +170,11 @@ export default {
     closeForm() {
       this.showForm = false;
     },
+
     updateIssuable() {
       this.service.updateIssuable(this.store.formState)
         .then(res => res.json())
+        .then(data => this.checkForSpam(data))
         .then((data) => {
           if (location.pathname !== data.web_url) {
             gl.utils.visitUrl(data.web_url);
@@ -179,11 +187,24 @@ export default {
           this.store.updateState(data);
           eventHub.$emit('close.form');
         })
-        .catch(() => {
-          eventHub.$emit('close.form');
-          window.Flash(`Error updating ${this.issuableType}`);
+        .catch((error) => {
+          if (error && error.name === 'SpamError') {
+            this.openRecaptcha();
+          } else {
+            eventHub.$emit('close.form');
+            window.Flash(`Error updating ${this.issuableType}`);
+          }
         });
     },
+
+    closeRecaptchaDialog() {
+      this.store.setFormState({
+        updateLoading: false,
+      });
+
+      this.closeRecaptcha();
+    },
+
     deleteIssuable() {
       this.service.deleteIssuable()
         .then(res => res.json())
@@ -237,9 +258,9 @@ export default {
 </script>
 
 <template>
-  <div>
+<div>
+  <div v-if="canUpdate && showForm">
     <form-component
-      v-if="canUpdate && showForm"
       :form-state="formState"
       :can-destroy="canDestroy"
       :issuable-templates="issuableTemplates"
@@ -251,30 +272,37 @@ export default {
       :can-attach-file="canAttachFile"
       :enable-autocomplete="enableAutocomplete"
     />
-    <div v-else>
-      <title-component
-        :issuable-ref="issuableRef"
-        :can-update="canUpdate"
-        :title-html="state.titleHtml"
-        :title-text="state.titleText"
-        :show-inline-edit-button="showInlineEditButton"
-      />
-      <description-component
-        v-if="state.descriptionHtml"
-        :can-update="canUpdate"
-        :description-html="state.descriptionHtml"
-        :description-text="state.descriptionText"
-        :updated-at="state.updatedAt"
-        :task-status="state.taskStatus"
-        :issuable-type="issuableType"
-        :update-url="updateEndpoint"
-      />
-      <edited-component
-        v-if="hasUpdated"
-        :updated-at="state.updatedAt"
-        :updated-by-name="state.updatedByName"
-        :updated-by-path="state.updatedByPath"
-      />
-    </div>
+
+    <recaptcha-dialog
+      v-show="showRecaptcha"
+      :html="recaptchaHTML"
+      @close="closeRecaptchaDialog"
+    />
+  </div>
+  <div v-else>
+    <title-component
+      :issuable-ref="issuableRef"
+      :can-update="canUpdate"
+      :title-html="state.titleHtml"
+      :title-text="state.titleText"
+      :show-inline-edit-button="showInlineEditButton"
+    />
+    <description-component
+      v-if="state.descriptionHtml"
+      :can-update="canUpdate"
+      :description-html="state.descriptionHtml"
+      :description-text="state.descriptionText"
+      :updated-at="state.updatedAt"
+      :task-status="state.taskStatus"
+      :issuable-type="issuableType"
+      :update-url="updateEndpoint"
+    />
+    <edited-component
+      v-if="hasUpdated"
+      :updated-at="state.updatedAt"
+      :updated-by-name="state.updatedByName"
+      :updated-by-path="state.updatedByPath"
+    />
   </div>
+</div>
 </template>

app/assets/javascripts/issue_show/components/description.vue

 <script>
   import animateMixin from '../mixins/animate';
   import TaskList from '../../task_list';
+  import RecaptchaDialogImplementor from '../../vue_shared/mixins/recaptcha_dialog_implementor';
 
   export default {
-    mixins: [animateMixin],
+    mixins: [
+      animateMixin,
+      RecaptchaDialogImplementor,
+    ],
+
     props: {
       canUpdate: {
         type: Boolean,
@@ -51,6 +56,7 @@
         this.updateTaskStatusText();
       },
     },
+
     methods: {
       renderGFM() {
         $(this.$refs['gfm-content']).renderGFM();
@@ -61,9 +67,19 @@
             dataType: this.issuableType,
             fieldName: 'description',
             selector: '.detail-page-description',
+            onSuccess: this.taskListUpdateSuccess.bind(this),
           });
         }
       },
+
+      taskListUpdateSuccess(data) {
+        try {
+          this.checkForSpam(data);
+        } catch (error) {
+          if (error && error.name === 'SpamError') this.openRecaptcha();
+        }
+      },
+
       updateTaskStatusText() {
         const taskRegexMatches = this.taskStatus.match(/(\d+) of ((?!0)\d+)/);
         const $issuableHeader = $('.issuable-meta');
@@ -109,5 +125,11 @@
       :data-update-url="updateUrl"
     >
     </textarea>
+
+    <recaptcha-dialog
+      v-show="showRecaptcha"
+      :html="recaptchaHTML"
+      @close="closeRecaptcha"
+    />
   </div>
 </template>

app/assets/javascripts/vue_shared/components/popup_dialog.vue

@@ -38,7 +38,8 @@ export default {
     },
     primaryButtonLabel: {
       type: String,
-      required: true,
+      required: false,
+      default: '',
     },
     submitDisabled: {
       type: Boolean,
@@ -113,8 +114,9 @@ export default {
             {{ closeButtonLabel }}
           </button>
           <button
+            v-if="primaryButtonLabel"
             type="button"
-            class="btn pull-right"
+            class="btn pull-right js-primary-button"
             :disabled="submitDisabled"
             :class="btnKindClass"
             @click="emitSubmit(true)">

app/assets/javascripts/vue_shared/components/recaptcha_dialog.vue

+<script>
+import PopupDialog from './popup_dialog.vue';
+
+export default {
+  name: 'recaptcha-dialog',
+
+  props: {
+    html: {
+      type: String,
+      required: false,
+      default: '',
+    },
+  },
+
+  data() {
+    return {
+      script: {},
+      scriptSrc: 'https://www.google.com/recaptcha/api.js',
+    };
+  },
+
+  components: {
+    PopupDialog,
+  },
+
+  methods: {
+    appendRecaptchaScript() {
+      this.removeRecaptchaScript();
+
+      const script = document.createElement('script');
+      script.src = this.scriptSrc;
+      script.classList.add('js-recaptcha-script');
+      script.async = true;
+      script.defer = true;
+
+      this.script = script;
+
+      document.body.appendChild(script);
+    },
+
+    removeRecaptchaScript() {
+      if (this.script instanceof Element) this.script.remove();
+    },
+
+    close() {
+      this.removeRecaptchaScript();
+      this.$emit('close');
+    },
+
+    submit() {
+      this.$el.querySelector('form').submit();
+    },
+  },
+
+  watch: {
+    html() {
+      this.appendRecaptchaScript();
+    },
+  },
+
+  mounted() {
+    window.recaptchaDialogCallback = this.submit.bind(this);
+  },
+};
+</script>
+
+<template>
+<popup-dialog
+  kind="warning"
+  class="recaptcha-dialog js-recaptcha-dialog"
+  :hide-footer="true"
+  :title="__('Please solve the reCAPTCHA')"
+  @toggle="close"
+>
+  <div slot="body">
+    <p>
+      {{__('We want to be sure it is you, please confirm you are not a robot.')}}
+    </p>
+    <div
+      ref="recaptcha"
+      v-html="html"
+    ></div>
+  </div>
+</popup-dialog>
+</template>

app/assets/javascripts/vue_shared/mixins/recaptcha_dialog_implementor.js

+import RecaptchaDialog from '../components/recaptcha_dialog.vue';
+
+export default {
+  data() {
+    return {
+      showRecaptcha: false,
+      recaptchaHTML: '',
+    };
+  },
+
+  components: {
+    RecaptchaDialog,
+  },
+
+  methods: {
+    openRecaptcha() {
+      this.showRecaptcha = true;
+    },
+
+    closeRecaptcha() {
+      this.showRecaptcha = false;
+    },
+
+    checkForSpam(data) {
+      if (!data.recaptcha_html) return data;
+
+      this.recaptchaHTML = data.recaptcha_html;
+
+      const spamError = new Error(data.error_message);
+      spamError.name = 'SpamError';
+      spamError.message = 'SpamError';
+
+      throw spamError;
+    },
+  },
+};

app/assets/stylesheets/framework/modal.scss

@@ -48,3 +48,10 @@ body.modal-open {
   display: block;
 }
 
+.recaptcha-dialog .recaptcha-form {
+  display: inline-block;
+
+  .recaptcha {
+    margin: 0;
+  }
+}

app/controllers/concerns/issuable_actions.rb

@@ -25,7 +25,7 @@ module IssuableActions
       end
 
       format.json do
-        render_entity_json
+        recaptcha_check_with_fallback(false) { render_entity_json }
       end
     end
 

app/controllers/concerns/spammable_actions.rb

@@ -23,8 +23,8 @@ module SpammableActions
     @spam_config_loaded = Gitlab::Recaptcha.load_configurations!
   end
 
-  def recaptcha_check_with_fallback(&fallback)
-    if spammable.valid?
+  def recaptcha_check_with_fallback(should_redirect = true, &fallback)
+    if should_redirect && spammable.valid?
       redirect_to spammable_path
     elsif render_recaptcha?
       ensure_spam_config_loaded!
@@ -33,7 +33,18 @@ module SpammableActions
         flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.'
       end
 
-      render :verify
+      respond_to do |format|
+        format.html do
+          render :verify
+        end
+
+        format.json do
+          locals = { spammable: spammable, script: false, has_submit: false }
+          recaptcha_html = render_to_string(partial: 'shared/recaptcha_form', formats: :html, locals: locals)
+
+          render json: { recaptcha_html: recaptcha_html }
+        end
+      end
     else
       yield
     end

app/views/layouts/_recaptcha_verification.html.haml

 - humanized_resource_name = spammable.class.model_name.human.downcase
-- resource_name = spammable.class.model_name.singular
 
 %h3.page-title
   Anti-spam verification
@@ -8,16 +7,4 @@
 %p
   #{"We detected potential spam in the #{humanized_resource_name}. Please solve the reCAPTCHA to proceed."}
 
-= form_for form do |f|
-  .recaptcha
-    - params[resource_name].each do |field, value|
-      = hidden_field(resource_name, field, value: value)
-    = hidden_field_tag(:spam_log_id, spammable.spam_log.id)
-    = hidden_field_tag(:recaptcha_verification, true)
-    = recaptcha_tags
-
-    -# Yields a block with given extra params.
-    = yield
-
-  .row-content-block.footer-block
-    = f.submit "Submit #{humanized_resource_name}", class: 'btn btn-create'
+= render 'shared/recaptcha_form', spammable: spammable

app/views/shared/_recaptcha_form.html.haml

+- resource_name = spammable.class.model_name.singular
+- humanized_resource_name = spammable.class.model_name.human.downcase
+- script = local_assigns.fetch(:script, true)
+- has_submit = local_assigns.fetch(:has_submit, true)
+
+= form_for resource_name, method: :post, html: { class: 'recaptcha-form js-recaptcha-form' } do |f|
+  .recaptcha
+    - params[resource_name].each do |field, value|
+      = hidden_field(resource_name, field, value: value)
+    = hidden_field_tag(:spam_log_id, spammable.spam_log.id)
+    = hidden_field_tag(:recaptcha_verification, true)
+    = recaptcha_tags script: script, callback: 'recaptchaDialogCallback'
+
+    -# Yields a block with given extra params.
+    = yield
+
+  - if has_submit
+    .row-content-block.footer-block
+      = f.submit "Submit #{humanized_resource_name}", class: 'btn btn-create'

changelogs/unreleased/29483-no-feedback-when-checking-on-checklist-if-potential-spam-was-detected.yml

+---
+title: Add recaptcha modal to issue updates detected as spam
+merge_request: 15408
+author:
+type: fixed

spec/controllers/projects/issues_controller_spec.rb

@@ -272,6 +272,20 @@ describe Projects::IssuesController do
         expect(response).to have_http_status(:ok)
         expect(issue.reload.title).to eq('New title')
       end
+
+      context 'when Akismet is enabled and the issue is identified as spam' do
+        before do
+          stub_application_setting(recaptcha_enabled: true)
+          allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true)
+          allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true)
+        end
+
+        it 'renders json with recaptcha_html' do
+          subject
+
+          expect(JSON.parse(response.body)).to have_key('recaptcha_html')
+        end
+      end
     end
 
     context 'when user does not have access to update issue' do
@@ -504,17 +518,16 @@ describe Projects::IssuesController do
               expect(spam_logs.first.recaptcha_verified).to be_falsey
             end
 
-            it 'renders json errors' do
+            it 'renders recaptcha_html json response' do
               update_issue
 
-              expect(json_response)
-                .to eql("errors" => ["Your issue has been recognized as spam. Please, change the content or solve the reCAPTCHA to proceed."])
+              expect(json_response).to have_key('recaptcha_html')
             end
 
-            it 'returns 422 status' do
+            it 'returns 200 status' do
               update_issue
 
-              expect(response).to have_gitlab_http_status(422)
+              expect(response).to have_gitlab_http_status(200)
             end
           end
 

spec/javascripts/issue_show/components/app_spec.js

@@ -4,6 +4,7 @@ import '~/render_gfm';
 import issuableApp from '~/issue_show/components/app.vue';
 import eventHub from '~/issue_show/event_hub';
 import issueShowData from '../mock_data';
+import setTimeoutPromise from '../../helpers/set_timeout_promise_helper';
 
 function formatText(text) {
   return text.trim().replace(/\s\s+/g, ' ');
@@ -55,6 +56,8 @@ describe('Issuable output', () => {
     Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor);
 
     vm.poll.stop();
+
+    vm.$destroy();
   });
 
   it('should render a title/description/edited and update title/description/edited on update', (done) => {
@@ -268,6 +271,52 @@ describe('Issuable output', () => {
     });
   });
 
+  it('opens recaptcha dialog if update rejected as spam', (done) => {
+    function mockScriptSrc() {
+      const recaptchaChild = vm.$children
+        .find(child => child.$options._componentTag === 'recaptcha-dialog'); // eslint-disable-line no-underscore-dangle
+
+      recaptchaChild.scriptSrc = '//scriptsrc';
+    }
+
+    let modal;
+    const promise = new Promise((resolve) => {
+      resolve({
+        json() {
+          return {
+            recaptcha_html: '<div class="g-recaptcha">recaptcha_html</div>',
+          };
+        },
+      });
+    });
+
+    spyOn(vm.service, 'updateIssuable').and.returnValue(promise);
+
+    vm.canUpdate = true;
+    vm.showForm = true;
+
+    vm.$nextTick()
+      .then(() => mockScriptSrc())
+      .then(() => vm.updateIssuable())
+      .then(promise)
+      .then(() => setTimeoutPromise())
+      .then(() => {
+        modal = vm.$el.querySelector('.js-recaptcha-dialog');
+
+        expect(modal.style.display).not.toEqual('none');
+        expect(modal.querySelector('.g-recaptcha').textContent).toEqual('recaptcha_html');
+        expect(document.body.querySelector('.js-recaptcha-script').src).toMatch('//scriptsrc');
+      })
+      .then(() => modal.querySelector('.close').click())
+      .then(() => vm.$nextTick())
+      .then(() => {
+        expect(modal.style.display).toEqual('none');
+        expect(document.body.querySelector('.js-recaptcha-script')).toBeNull();
+      })
+      .then(done)
+      .catch(done.fail);
+  });
+
   describe('deleteIssuable', () => {
     it('changes URL when deleted', (done) => {
       spyOn(gl.utils, 'visitUrl');

spec/javascripts/issue_show/components/description_spec.js

@@ -51,6 +51,35 @@ describe('Description component', () => {
     });
   });
 
+  it('opens recaptcha dialog if update rejected as spam', (done) => {
+    let modal;
+    const recaptchaChild = vm.$children
+      .find(child => child.$options._componentTag === 'recaptcha-dialog'); // eslint-disable-line no-underscore-dangle
+
+    recaptchaChild.scriptSrc = '//scriptsrc';
+
+    vm.taskListUpdateSuccess({
+      recaptcha_html: '<div class="g-recaptcha">recaptcha_html</div>',
+    });
+
+    vm.$nextTick()
+      .then(() => {
+        modal = vm.$el.querySelector('.js-recaptcha-dialog');
+
+        expect(modal.style.display).not.toEqual('none');
+        expect(modal.querySelector('.g-recaptcha').textContent).toEqual('recaptcha_html');
+        expect(document.body.querySelector('.js-recaptcha-script').src).toMatch('//scriptsrc');
+      })
+      .then(() => modal.querySelector('.close').click())
+      .then(() => vm.$nextTick())
+      .then(() => {
+        expect(modal.style.display).toEqual('none');
+        expect(document.body.querySelector('.js-recaptcha-script')).toBeNull();
+      })
+      .then(done)
+      .catch(done.fail);
+  });
+
   describe('TaskList', () => {
     beforeEach(() => {
       vm = mountComponent(DescriptionComponent, Object.assign({}, props, {
@@ -86,6 +115,7 @@ describe('Description component', () => {
           dataType: 'issuableType',
           fieldName: 'description',
           selector: '.detail-page-description',
+          onSuccess: jasmine.any(Function),
         });
         done();
       });

spec/javascripts/vue_shared/components/popup_dialog_spec.js

+import Vue from 'vue';
+import PopupDialog from '~/vue_shared/components/popup_dialog.vue';
+import mountComponent from '../../helpers/vue_mount_component_helper';
+
+describe('PopupDialog', () => {
+  it('does not render a primary button if no primaryButtonLabel', () => {
+    const popupDialog = Vue.extend(PopupDialog);
+    const vm = mountComponent(popupDialog);
+
+    expect(vm.$el.querySelector('.js-primary-button')).toBeNull();
+  });
+});