Merge branch '29067-vrt-no-auth' into 'master'

Remove visual review app token step

Closes #30132 and #29067

See merge request gitlab-org/gitlab!18141

app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline_container.vue

@@ -4,6 +4,7 @@ import ArtifactsApp from './artifacts_list_app.vue';
 import Deployment from './deployment.vue';
 import MrWidgetContainer from './mr_widget_container.vue';
 import MrWidgetPipeline from './mr_widget_pipeline.vue';
+import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 
 /**
  * Renders the pipeline and related deployments from the store.
@@ -23,6 +24,7 @@ export default {
     MergeTrainPositionIndicator: () =>
       import('ee_component/vue_merge_request_widget/components/merge_train_position_indicator.vue'),
   },
+  mixins: [glFeatureFlagsMixin()],
   props: {
     mr: {
       type: Object,
@@ -62,7 +64,7 @@ export default {
       return this.isPostMerge ? this.mr.mergePipeline : this.mr.pipeline;
     },
     showVisualReviewAppLink() {
-      return this.mr.visualReviewAppAvailable;
+      return this.mr.visualReviewAppAvailable && this.glFeatures.anonymousVisualReviewFeedback;
     },
     showMergeTrainPositionIndicator() {
       return _.isNumber(this.mr.mergeTrainIndex);

app/assets/javascripts/vue_merge_request_widget/components/review_app_link.vue

@@ -18,7 +18,14 @@ export default {
 };
 </script>
 <template>
-  <a :href="link" target="_blank" rel="noopener noreferrer nofollow" :class="cssClass">
+  <a
+    :href="link"
+    target="_blank"
+    rel="noopener noreferrer nofollow"
+    :class="cssClass"
+    data-track-event="open_review_app"
+    data-track-label="review_app"
+  >
     {{ __('View app') }} <icon class="fgray" name="external-link" />
   </a>
 </template>

changelogs/unreleased/29067-vrt-no-auth.yml

+---
+title: Remove authentication step from visual review tools instructions
+merge_request:
+author:
+type: changed

doc/ci/review_apps/index.md

@@ -163,6 +163,13 @@ that spawned the Review App.
 
 ### Configuring Visual Reviews
 
+Ensure that the `anonymous_visual_review_feedback` feature flag is enabled.
+Administrators can enable with a Rails console as follows:
+
+```ruby
+Feature.enabled(:anonymous_visual_review_feedback)
+```
+
 The feedback form is served through a script you add to pages in your Review App.
 If you have [Developer permissions](../../user/permissions.md) to the project,
 you can access it by clicking the **Review** button in the **Pipeline** section
@@ -221,6 +228,19 @@ NOTE: **Note:**
 Future enhancements [are planned](https://gitlab.com/gitlab-org/gitlab/issues/11322)
 to make this process even easier.
 
+### Determining merge request ID
+
+The visual review tools retrieve the merge request ID from the `data-merge-request-id`
+data attribute included in the `script` HTML tag used to add the visual review tools
+to your review app.
+
+​After determining the ID for the merge request to link to a visual review app, you
+can supply the ID by either:​​
+
+- Hardcoding it in the script tag via the data attribute `data-merge-request-id` of the app.
+- Dynamically adding the `data-merge-request-id` value during the build of the app.
+- Supplying it manually through the visual review form in the app.
+
 ### Using Visual Reviews
 
 After Visual Reviews has been [enabled](#configuring-visual-reviews) for the
@@ -231,25 +251,15 @@ the bottom-right corner.
 
 To use the feedback form:
 
-1. Create a [personal access token](../../user/profile/personal_access_tokens.md)
-   with the API scope selected.
-1. Paste the token into the feedback box when prompted. If you select **Remember me**,
-   your browser stores the token so that future visits to Review Apps at the same URL
-   will not require you to re-enter the token. To clear the token, click **Log out**.
 1. Make a comment on the visual review. You can make use of all the
    [Markdown annotations](../../user/markdown.md) that are also available in
    merge request comments.
+1. Submit your feedback anonymously or add your name.
 1. Finally, click **Send feedback**.
 
 After you make and submit a comment in the visual review box, it will appear
 automatically in the respective merge request.
 
-TIP: **Tip:**
-Because tokens must be entered on a per-domain basis and they can only be accessed
-once, different review apps will not remember your token. You can save the token
-to your password manager specifically for the purpose of Visual Reviews. This way,
-you will not need to create additional tokens for each merge request.
-
 ## Limitations
 
 Review App limitations are the same as [environments limitations](../environments.md#limitations).

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

@@ -86,20 +86,7 @@ export default {
         ),
         step3: sprintf(
           s__(
-            'VisualReviewApp|%{stepStart}Step 3%{stepEnd}. Open the Review App and provide a %{linkStart}personal access token%{linkEnd}.',
-          ),
-          {
-            stepStart: '<strong>',
-            stepEnd: '</strong>',
-            linkStart:
-              '<a href="https://docs.gitlab.com/ee/user/profile/personal_access_tokens.html">',
-            linkEnd: '</a>',
-          },
-          false,
-        ),
-        step4: sprintf(
-          s__(
-            `VisualReviewApp|%{stepStart}Step 4%{stepEnd}. If not previously %{linkStart}configured%{linkEnd} by a developer, enter the merge request ID for the review when prompted. The ID of this merge request is %{stepStart}%{mrId}%{stepStart}.`,
+            `VisualReviewApp|%{stepStart}Step 3%{stepEnd}. If not previously %{linkStart}configured%{linkEnd} by a developer, enter the merge request ID for the review when prompted. The ID of this merge request is %{stepStart}%{mrId}%{stepStart}.`,
           ),
           {
             stepStart: '<strong>',
@@ -111,8 +98,8 @@ export default {
           },
           false,
         ),
-        step5: sprintf(
-          s__('VisualReviewApp|%{stepStart}Step 5%{stepEnd}. Leave feedback in the Review App.'),
+        step4: sprintf(
+          s__('VisualReviewApp|%{stepStart}Step 4%{stepEnd}. Leave feedback in the Review App.'),
           {
             stepStart: '<strong>',
             stepEnd: '</strong>',
@@ -145,7 +132,14 @@ export default {
       ok-variant="success"
     >
       <template slot="modal-ok">
-        <a :href="link" target="_blank" rel="noopener noreferrer nofollow" class="text-white">
+        <a
+          :href="link"
+          target="_blank"
+          rel="noopener noreferrer nofollow"
+          class="text-white js-review-app-link"
+          data-track-event="open_review_app"
+          data-track-label="review_app"
+        >
           {{ s__('VisualReviewApp|Open review app') }}
           <icon class="fwhite" name="external-link" />
         </a>
@@ -165,9 +159,8 @@ export default {
         </div>
       </div>
       <p v-html="instructionText.step2"></p>
-      <p v-html="instructionText.step3"></p>
       <p>
-        <span v-html="instructionText.step4"></span>
+        <span v-html="instructionText.step3"></span>
         <modal-copy-button
           :title="copyToClipboard.mrId"
           :text="appMetadata.mergeRequestId.toString()"
@@ -175,7 +168,7 @@ export default {
           css-classes="border-0 gl-pt-0 gl-pr-0 gl-pl-1 gl-pb-0"
         />
       </p>
-      <p v-html="instructionText.step5"></p>
+      <p v-html="instructionText.step4"></p>
     </gl-modal>
   </div>
 </template>

ee/app/controllers/ee/projects/merge_requests_controller.rb

@@ -16,6 +16,7 @@ module EE
           push_frontend_feature_flag(:container_scanning_merge_request_report_api, default_enabled: true)
           push_frontend_feature_flag(:dependency_scanning_merge_request_report_api, default_enabled: true)
           push_frontend_feature_flag(:parsed_license_report)
+          push_frontend_feature_flag(:anonymous_visual_review_feedback)
         end
 
         before_action :whitelist_query_limiting_ee_merge, only: [:merge]

ee/spec/frontend/vue_mr_widget/components/visual_review_app_spec.js

@@ -2,6 +2,7 @@ import { shallowMount, mount, createLocalVue } from '@vue/test-utils';
 import VisualReviewAppLink from 'ee/vue_merge_request_widget/components/visual_review_app_link.vue';
 import { GlButton, GlModal } from '@gitlab/ui';
 import ModalCopyButton from '~/vue_shared/components/modal_copy_button.vue';
+import { mockTracking, triggerEvent } from 'helpers/tracking_helper';
 
 const localVue = createLocalVue();
 
@@ -81,10 +82,20 @@ describe('Visual Review App Link', () => {
       expect(
         wrapper
           .find(GlModal)
-          .find('a')
+          .find('a.js-review-app-link')
           .attributes('href'),
       ).toEqual(propsData.link);
     });
+
+    it('tracks an event when review app link is clicked', () => {
+      const spy = mockTracking('_category_', wrapper.element, jest.spyOn);
+      const appLink = wrapper.find(GlModal).find('a.js-review-app-link');
+      triggerEvent(appLink.element);
+
+      expect(spy).toHaveBeenCalledWith('_category_', 'open_review_app', {
+        label: 'review_app',
+      });
+    });
   });
 
   describe('renders the copyToClipboard button', () => {

ee/spec/javascripts/vue_mr_widget/components/mr_widget_pipeline_container_spec.js

@@ -2,19 +2,25 @@ import { mount, createLocalVue } from '@vue/test-utils';
 import MrWidgetPipelineContainer from '~/vue_merge_request_widget/components/mr_widget_pipeline_container.vue';
 import { MT_MERGE_STRATEGY, MWPS_MERGE_STRATEGY } from '~/vue_merge_request_widget/constants';
 import MergeTrainPositionIndicator from 'ee/vue_merge_request_widget/components/merge_train_position_indicator.vue';
+import VisualReviewAppLink from 'ee/vue_merge_request_widget/components/visual_review_app_link.vue';
 import { mockStore } from 'spec/vue_mr_widget/mock_data';
 
 describe('MrWidgetPipelineContainer', () => {
   let wrapper;
 
-  const factory = (mrUpdates = {}) => {
+  const factory = (mrUpdates = {}, provide = {}) => {
     const localVue = createLocalVue();
 
     wrapper = mount(localVue.extend(MrWidgetPipelineContainer), {
       propsData: {
         mr: Object.assign({}, mockStore, mrUpdates),
       },
+      provide: {
+        ...provide,
+      },
       localVue,
+      sync: false,
+      attachToDocument: true,
     });
   };
 
@@ -50,4 +56,76 @@ describe('MrWidgetPipelineContainer', () => {
       expect(wrapper.find(MergeTrainPositionIndicator).exists()).toBe(false);
     });
   });
+
+  describe('with anonymous visual review feedback feature flag enabled', () => {
+    beforeEach(() => {
+      factory(
+        {
+          visualReviewAppAvailable: true,
+          appUrl: 'http://gitlab.example.com',
+          iid: 1,
+          sourceProjectId: 20,
+          sourceProjectFullPath: 'source/project',
+        },
+        {
+          glFeatures: {
+            anonymousVisualReviewFeedback: true,
+          },
+        },
+      );
+
+      // the visual review app link component is lazy loaded
+      // so we need to re-render the component
+      return wrapper.vm.$nextTick();
+    });
+
+    it('renders the visual review app link', done => {
+      // the visual review app link component is lazy loaded
+      // so we need to re-render the component again, as once
+      // apparently isn't enough.
+      wrapper.vm
+        .$nextTick()
+        .then(() => {
+          expect(wrapper.find(VisualReviewAppLink).exists()).toEqual(true);
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
+
+  describe('with anonymous visual review feedback feature flag disabled', () => {
+    beforeEach(() => {
+      factory(
+        {
+          visualReviewAppAvailable: true,
+          appUrl: 'http://gitlab.example.com',
+          iid: 1,
+          sourceProjectId: 20,
+          sourceProjectFullPath: 'source/project',
+        },
+        {
+          glFeatures: {
+            anonymousVisualReviewFeedback: false,
+          },
+        },
+      );
+
+      // the visual review app link component is lazy loaded
+      // so we need to re-render the component
+      return wrapper.vm.$nextTick();
+    });
+
+    it('does not render the visual review app link', done => {
+      // the visual review app link component is lazy loaded
+      // so we need to re-render the component again, as once
+      // apparently isn't enough.
+      wrapper.vm
+        .$nextTick()
+        .then(() => {
+          expect(wrapper.find(VisualReviewAppLink).exists()).toEqual(false);
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
 });

locale/gitlab.pot

@@ -19330,13 +19330,10 @@ msgstr ""
 msgid "VisualReviewApp|%{stepStart}Step 2%{stepEnd}. Add it to the %{headTags} tags of every page of your application, ensuring the merge request ID is set or not set as required. "
 msgstr ""
 
-msgid "VisualReviewApp|%{stepStart}Step 3%{stepEnd}. Open the Review App and provide a %{linkStart}personal access token%{linkEnd}."
+msgid "VisualReviewApp|%{stepStart}Step 3%{stepEnd}. If not previously %{linkStart}configured%{linkEnd} by a developer, enter the merge request ID for the review when prompted. The ID of this merge request is %{stepStart}%{mrId}%{stepStart}."
 msgstr ""
 
-msgid "VisualReviewApp|%{stepStart}Step 4%{stepEnd}. If not previously %{linkStart}configured%{linkEnd} by a developer, enter the merge request ID for the review when prompted. The ID of this merge request is %{stepStart}%{mrId}%{stepStart}."
-msgstr ""
-
-msgid "VisualReviewApp|%{stepStart}Step 5%{stepEnd}. Leave feedback in the Review App."
+msgid "VisualReviewApp|%{stepStart}Step 4%{stepEnd}. Leave feedback in the Review App."
 msgstr ""
 
 msgid "VisualReviewApp|Copy merge request ID"

spec/javascripts/vue_mr_widget/components/review_app_link_spec.js

 import Vue from 'vue';
 import component from '~/vue_merge_request_widget/components/review_app_link.vue';
+import { mockTracking, triggerEvent } from 'spec/helpers/tracking_helper';
 import mountComponent from '../../helpers/vue_mount_component_helper';
 
 describe('review app link', () => {
@@ -35,4 +36,13 @@ describe('review app link', () => {
   it('renders svg icon', () => {
     expect(el.querySelector('svg')).not.toBeNull();
   });
+
+  it('tracks an event when clicked', () => {
+    const spy = mockTracking('_category_', el, spyOn);
+    triggerEvent(el);
+
+    expect(spy).toHaveBeenCalledWith('_category_', 'open_review_app', {
+      label: 'review_app',
+    });
+  });
 });

spec/javascripts/vue_mr_widget/mock_data.js

@@ -284,7 +284,20 @@ export const mockStore = {
   targetBranch: 'target-branch',
   sourceBranch: 'source-branch',
   sourceBranchLink: 'source-branch-link',
-  deployments: [{ id: 0, name: 'bogus' }, { id: 1, name: 'bogus-docs' }],
+  deployments: [
+    {
+      id: 0,
+      name: 'bogus',
+      external_url: 'https://fake.com',
+      external_url_formatted: 'https://fake.com',
+    },
+    {
+      id: 1,
+      name: 'bogus-docs',
+      external_url: 'https://fake.com',
+      external_url_formatted: 'https://fake.com',
+    },
+  ],
   postMergeDeployments: [{ id: 0, name: 'prod' }, { id: 1, name: 'prod-docs' }],
   troubleshootingDocsPath: 'troubleshooting-docs-path',
   ciStatus: 'ci-status',