Merge branch '11159-move-approval-user-password-input-from-inline-to-a-modal' into 'master'

Resolve "Move approval user password input from inline to a modal"

Closes #11159

See merge request gitlab-org/gitlab-ee!14123

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue

@@ -41,8 +41,8 @@ export default {
       isApproving: false,
       isExpanded: false,
       isLoadingRules: false,
-      showApprovalAuth: false,
       hasApprovalAuthError: false,
+      modalId: 'approvals-auth',
     };
   },
   computed: {
@@ -151,7 +151,7 @@ export default {
     },
     approve() {
       if (this.requirePasswordToApprove) {
-        this.showApprovalAuth = true;
+        this.$root.$emit('bv::show::modal', this.modalId);
         return;
       }
       this.updateApproval(
@@ -184,7 +184,7 @@ export default {
         .then(data => {
           this.mr.setApprovals(data);
           eventHub.$emit('MRWidgetUpdateRequested');
-          this.showApprovalAuth = false;
+          this.$root.$emit('bv::hide::modal', this.modalId);
         })
         .catch(errFn)
         .then(() => {
@@ -192,10 +192,6 @@ export default {
           this.refreshRules();
         });
     },
-    onApprovalAuthCancel() {
-      this.showApprovalAuth = false;
-      this.clearError();
-    },
   },
   FETCH_LOADING,
 };
@@ -207,37 +203,35 @@ export default {
       <div v-if="fetchingApprovals">{{ $options.FETCH_LOADING }}</div>
       <template v-else>
         <approvals-auth
-          v-if="showApprovalAuth"
           :is-approving="isApproving"
           :has-error="hasApprovalAuthError"
+          :modal-id="modalId"
           @approve="approveWithAuth"
-          @cancel="onApprovalAuthCancel"
+          @hide="clearError"
+        />
+        <gl-button
+          v-if="action"
+          :variant="action.variant"
+          :class="{ 'btn-inverted': action.inverted }"
+          size="sm"
+          class="mr-3"
+          @click="action.action"
+        >
+          <gl-loading-icon v-if="isApproving" inline />
+          {{ action.text }}
+        </gl-button>
+        <approvals-summary-optional
+          v-if="isOptional"
+          :can-approve="hasAction"
+          :help-path="mr.approvalsHelpPath"
+        />
+        <approvals-summary
+          v-else
+          :approved="isApproved"
+          :approvals-left="approvals.approvals_left"
+          :rules-left="approvals.approvalRuleNamesLeft"
+          :approvers="approvedBy"
         />
-        <template v-else>
-          <gl-button
-            v-if="action"
-            :variant="action.variant"
-            :class="{ 'btn-inverted': action.inverted }"
-            size="sm"
-            class="mr-3"
-            @click="action.action"
-          >
-            <gl-loading-icon v-if="isApproving" inline />
-            {{ action.text }}
-          </gl-button>
-          <approvals-summary-optional
-            v-if="isOptional"
-            :can-approve="hasAction"
-            :help-path="mr.approvalsHelpPath"
-          />
-          <approvals-summary
-            v-else
-            :approved="isApproved"
-            :approvals-left="approvals.approvals_left"
-            :rules-left="approvals.approvalRuleNamesLeft"
-            :approvers="approvedBy"
-          />
-        </template>
       </template>
     </div>
     <approvals-footer

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_auth.vue

 <script>
-import { GlButton, GlLoadingIcon } from '@gitlab/ui';
+import { GlButton, GlLoadingIcon, GlModal } from '@gitlab/ui';
 
 export default {
   components: {
     GlButton,
     GlLoadingIcon,
+    GlModal,
   },
   props: {
     isApproving: {
@@ -17,63 +18,77 @@ export default {
       default: false,
       required: false,
     },
+    modalId: {
+      type: String,
+      required: true,
+    },
   },
   data() {
     return {
       approvalPassword: '',
     };
   },
-  mounted() {
-    this.$nextTick(() => this.$refs.approvalPassword.focus());
-  },
   methods: {
-    approve() {
+    approve(event) {
+      event.preventDefault();
       this.$emit('approve', this.approvalPassword);
     },
-    cancel() {
-      this.$emit('cancel');
+    onHide() {
+      this.approvalPassword = '';
+      this.$emit('hide');
+    },
+    onShow() {
+      setTimeout(() => {
+        this.$refs.approvalPasswordInput.focus();
+      }, 0);
     },
   },
 };
 </script>
 <template>
-  <form class="form-inline align-items-center" @submit.prevent="approve">
-    <div class="form-group mb-2 mr-2 mb-sm-0">
-      <input
-        ref="approvalPassword"
-        v-model="approvalPassword"
-        type="password"
-        class="form-control"
-        :class="{ 'is-invalid': hasError }"
-        autocomplete="new-password"
-        :placeholder="s__('Password')"
-      />
-    </div>
-    <div class="form-group mb-2 mr-2 mb-sm-0">
-      <gl-button
-        variant="primary"
-        :disabled="isApproving"
-        size="sm"
-        class="mr-1 js-confirm"
-        @click="approve"
-      >
-        <gl-loading-icon v-if="isApproving" inline />
-        {{ s__('Confirm') }}
-      </gl-button>
-      <gl-button
-        variant="default"
-        :disabled="isApproving"
-        size="sm"
-        class="js-cancel"
-        @click="cancel"
-      >
-        {{ s__('Cancel') }}
-      </gl-button>
-    </div>
-    <div v-if="hasError">
-      <span class="gl-field-error">
-        {{ s__('mrWidget|Approval password is invalid.') }}
-      </span>
-    </div>
-  </form>
+  <gl-modal
+    :modal-id="modalId"
+    :ok-disabled="isApproving"
+    :title="s__('Enter your password to approve')"
+    ok-variant="success"
+    modal-class="js-mr-approvals-modal"
+    @ok="approve"
+    @hide="onHide"
+    @show="onShow"
+  >
+    <form @submit.prevent="approve">
+      <p>
+        {{
+          s__(
+            'mrWidget|To approve this merge request, please enter your password. This project requires all approvals to be authenticated.',
+          )
+        }}
+      </p>
+      <div class="form-group mb-0">
+        <label class="mb-1" for="approvalPasswordInput">{{ s__('mrWidget|Your password') }}</label>
+        <div>
+          <input
+            id="approvalPasswordInput"
+            ref="approvalPasswordInput"
+            v-model="approvalPassword"
+            type="password"
+            class="form-control"
+            :class="{ 'is-invalid': hasError }"
+            autocomplete="new-password"
+            :placeholder="s__('Password')"
+          />
+        </div>
+      </div>
+      <div v-if="hasError">
+        <span class="gl-field-error">{{ s__('mrWidget|Approval password is invalid.') }}</span>
+      </div>
+    </form>
+
+    <template slot="modal-cancel">{{ s__('Cancel') }}</template>
+
+    <template slot="modal-ok">
+      <gl-loading-icon v-if="isApproving" inline />
+      {{ s__('Approve') }}
+    </template>
+  </gl-modal>
 </template>

ee/changelogs/unreleased/11159-move-approval-user-password-input-from-inline-to-a-modal.yml

+---
+title: Resolve Move approval user password input from inline to a modal
+merge_request: 14123
+author:
+type: changed

ee/spec/features/merge_request/user_approves_with_password_spec.rb

@@ -16,9 +16,9 @@ describe 'Merge request > User approves with password', :js do
   end
 
   it 'works, when user approves and enters correct password' do
-    page.within('.js-mr-approvals') do
-      approve_with_password '12345678'
+    approve_with_password '12345678'
 
+    page.within('.js-mr-approvals') do
       expect(page).not_to have_button('Approve')
       expect(page).to have_text('Approved by')
     end
@@ -33,9 +33,9 @@ describe 'Merge request > User approves with password', :js do
   end
 
   it 'shows error, when user approves and enters incorrect password' do
-    page.within('.js-mr-approvals') do
-      approve_with_password 'nottherightpassword'
+    approve_with_password 'nottherightpassword'
 
+    page.within('.js-mr-approvals-modal') do
       expect(page).to have_text('Approval password is invalid.')
       click_button 'Cancel'
       expect(page).not_to have_text('Approved by')
@@ -44,10 +44,15 @@ describe 'Merge request > User approves with password', :js do
 end
 
 def approve_with_password(password)
-  click_button('Approve')
-  fill_in(type: 'password', with: password)
-  click_button('Confirm')
-  wait_for_requests
+  page.within('.js-mr-approvals') do
+    click_button('Approve')
+  end
+
+  page.within('#approvals-auth') do
+    fill_in(type: 'password', with: password)
+    click_button('Approve')
+    wait_for_requests
+  end
 end
 
 def unapprove

ee/spec/javascripts/vue_mr_widget/components/approvals/approvals_auth_spec.js

 import { createLocalVue, shallowMount } from '@vue/test-utils';
-import { GlLoadingIcon } from '@gitlab/ui';
+import { GlModal } from '@gitlab/ui';
 import ApprovalsAuth from 'ee/vue_merge_request_widget/components/approvals/approvals_auth.vue';
 
 const TEST_PASSWORD = 'password';
@@ -21,9 +21,9 @@ describe('Approval auth component', () => {
     wrapper = shallowMount(localVue.extend(ApprovalsAuth), {
       propsData: {
         ...props,
+        modalId: 'testid',
       },
       localVue,
-      sync: false,
     });
   };
 
@@ -32,9 +32,6 @@ describe('Approval auth component', () => {
     wrapper = null;
   });
 
-  const findConfirm = () => wrapper.find('.js-confirm');
-  const findCancel = () => wrapper.find('.js-cancel');
-  const findLoading = () => findConfirm().find(GlLoadingIcon);
   const findInput = () => wrapper.find('input[type=password]');
   const findErrorMessage = () => wrapper.find('.gl-field-error');
 
@@ -44,14 +41,14 @@ describe('Approval auth component', () => {
       waitForTick(done);
     });
 
-    it('approve button, cancel button, and password input controls are rendered', () => {
-      expect(findConfirm().exists()).toBe(true);
-      expect(findCancel().exists()).toBe(true);
+    it('password input control is rendered', () => {
       expect(wrapper.find('input').exists()).toBe(true);
     });
 
-    it('does not show loading icon', () => {
-      expect(findLoading().exists()).toBe(false);
+    it('does not disable approve button', () => {
+      const attrs = wrapper.attributes();
+
+      expect(attrs['ok-disabled']).toBeUndefined();
     });
 
     it('does not show error message', () => {
@@ -66,25 +63,15 @@ describe('Approval auth component', () => {
   describe('when approve clicked', () => {
     beforeEach(done => {
       createComponent();
-      findInput().setValue(TEST_PASSWORD);
-      findConfirm().vm.$emit('click');
       waitForTick(done);
     });
 
-    it('emits the approve event', () => {
-      expect(wrapper.emittedByOrder()).toEqual([{ name: 'approve', args: [TEST_PASSWORD] }]);
-    });
-  });
-
-  describe('when cancel is clicked', () => {
-    beforeEach(done => {
-      createComponent();
-      findCancel().vm.$emit('click');
+    it('emits the approve event', done => {
+      findInput().setValue(TEST_PASSWORD);
+      wrapper.find(GlModal).vm.$emit('ok', { preventDefault: () => null });
       waitForTick(done);
-    });
 
-    it('emits the cancel event', () => {
-      expect(wrapper.emittedByOrder()).toEqual([{ name: 'cancel', args: [] }]);
+      expect(wrapper.emittedByOrder()).toEqual([{ name: 'approve', args: [TEST_PASSWORD] }]);
     });
   });
 
@@ -94,8 +81,10 @@ describe('Approval auth component', () => {
       waitForTick(done);
     });
 
-    it('shows loading icon when isApproving is true', () => {
-      expect(findLoading().exists()).toBe(true);
+    it('disables the approve button', () => {
+      const attrs = wrapper.attributes();
+
+      expect(attrs['ok-disabled']).toEqual('true');
     });
   });
 

ee/spec/javascripts/vue_mr_widget/components/approvals/approvals_spec.js

@@ -296,20 +296,12 @@ describe('EE MRWidget approvals', () => {
           waitForTick(done);
         });
 
-        it('does not initially show approvals auth component', () => {
-          expect(wrapper.find(ApprovalsAuth).exists()).toBe(false);
-        });
-
         describe('when approve is clicked', () => {
           beforeEach(done => {
             findAction().vm.$emit('click');
             waitForTick(done);
           });
 
-          it('shows approvals auth component', () => {
-            expect(wrapper.find(ApprovalsAuth).exists()).toBe(true);
-          });
-
           describe('when emits approve', () => {
             let authReject;
 

locale/gitlab.pot

@@ -1548,6 +1548,9 @@ msgstr ""
 msgid "Approvals"
 msgstr ""
 
+msgid "Approve"
+msgstr ""
+
 msgid "Apr"
 msgstr ""
 
@@ -5035,6 +5038,9 @@ msgstr ""
 msgid "Enter the merge request title"
 msgstr ""
 
+msgid "Enter your password to approve"
+msgstr ""
+
 msgid "Enter zen mode"
 msgstr ""
 
@@ -17276,6 +17282,9 @@ msgstr ""
 msgid "mrWidget|This project is archived, write access has been disabled"
 msgstr ""
 
+msgid "mrWidget|To approve this merge request, please enter your password. This project requires all approvals to be authenticated."
+msgstr ""
+
 msgid "mrWidget|When this merge request is ready, remove the WIP: prefix from the title to allow it to be merged"
 msgstr ""
 
@@ -17288,6 +17297,9 @@ msgstr ""
 msgid "mrWidget|You can merge this merge request manually using the"
 msgstr ""
 
+msgid "mrWidget|Your password"
+msgstr ""
+
 msgid "mrWidget|branch does not exist."
 msgstr ""