Automatic merge of gitlab-org/gitlab master

app/assets/javascripts/issues/show/index.js

@@ -77,9 +77,7 @@ export function initIssueApp(issueData, store) {
 
   const { fullPath } = el.dataset;
 
-  if (gon?.features?.fixCommentScroll) {
-    scrollToTargetOnResize();
-  }
+  scrollToTargetOnResize();
 
   bootstrapApollo({ ...issueState, issueType: el.dataset.issueType });
 

app/assets/javascripts/lib/utils/ignore_while_pending.js

+/**
+ * This will wrap the given function to make sure that it is only triggered once
+ * while executing asynchronously
+ *
+ * @param {Function} fn some function that returns a promise
+ * @returns A function that will only be triggered *once* while the promise is executing
+ */
+export const ignoreWhilePending = (fn) => {
+  const isPendingMap = new WeakMap();
+  const defaultContext = {};
+
+  // We need this to be a function so we get the `this`
+  return function ignoreWhilePendingInner(...args) {
+    const context = this || defaultContext;
+
+    if (isPendingMap.get(context)) {
+      return Promise.resolve();
+    }
+
+    isPendingMap.set(context, true);
+
+    return fn.apply(this, args).finally(() => {
+      isPendingMap.delete(context);
+    });
+  };
+};

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

@@ -6,6 +6,7 @@ import createFlash from '~/flash';
 import { clearDraft, getDiscussionReplyKey } from '~/lib/utils/autosave';
 import { isLoggedIn } from '~/lib/utils/common_utils';
 import { confirmAction } from '~/lib/utils/confirm_via_gl_modal/confirm_via_gl_modal';
+import { ignoreWhilePending } from '~/lib/utils/ignore_while_pending';
 import { s__, __ } from '~/locale';
 import diffLineNoteFormMixin from '~/notes/mixins/diff_line_note_form';
 import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue';
@@ -171,7 +172,7 @@ export default {
         this.expandDiscussion({ discussionId: this.discussion.id });
       }
     },
-    async cancelReplyForm(shouldConfirm, isDirty) {
+    cancelReplyForm: ignoreWhilePending(async function cancelReplyForm(shouldConfirm, isDirty) {
       if (shouldConfirm && isDirty) {
         const msg = s__('Notes|Are you sure you want to cancel creating this comment?');
 
@@ -188,7 +189,7 @@ export default {
 
       this.isReplying = false;
       clearDraft(this.autosaveKey);
-    },
+    }),
     saveReply(noteText, form, callback) {
       if (!noteText) {
         this.cancelReplyForm();

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

@@ -7,6 +7,7 @@ import { confirmAction } from '~/lib/utils/confirm_via_gl_modal/confirm_via_gl_m
 import { INLINE_DIFF_LINES_KEY } from '~/diffs/constants';
 import createFlash from '~/flash';
 import httpStatusCodes from '~/lib/utils/http_status';
+import { ignoreWhilePending } from '~/lib/utils/ignore_while_pending';
 import { truncateSha } from '~/lib/utils/text_utility';
 import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue';
 import { __, s__, sprintf } from '../../locale';
@@ -350,7 +351,10 @@ export default {
         parent: this.$el,
       });
     },
-    async formCancelHandler({ shouldConfirm, isDirty }) {
+    formCancelHandler: ignoreWhilePending(async function formCancelHandler({
+      shouldConfirm,
+      isDirty,
+    }) {
       if (shouldConfirm && isDirty) {
         const msg = __('Are you sure you want to cancel editing this comment?');
         const confirmed = await confirmAction(msg);
@@ -364,7 +368,7 @@ export default {
       }
       this.isEditing = false;
       this.$emit('cancelForm');
-    },
+    }),
     recoverNoteContent(noteText) {
       // we need to do this to prevent noteForm inconsistent content warning
       // this is something we intentionally do so we need to recover the content

app/assets/javascripts/pipeline_wizard/components/input.vue

+<script>
+import { isNode, isDocument, isSeq, visit } from 'yaml';
+import { capitalize } from 'lodash';
+import TextWidget from '~/pipeline_wizard/components/widgets/text.vue';
+import ListWidget from '~/pipeline_wizard/components/widgets/list.vue';
+
+const widgets = {
+  TextWidget,
+  ListWidget,
+};
+
+function isNullOrUndefined(v) {
+  return [undefined, null].includes(v);
+}
+
+export default {
+  components: {
+    ...widgets,
+  },
+  props: {
+    template: {
+      type: Object,
+      required: true,
+      validator: (v) => isNode(v),
+    },
+    compiled: {
+      type: Object,
+      required: true,
+      validator: (v) => isDocument(v) || isNode(v),
+    },
+    target: {
+      type: String,
+      required: true,
+      validator: (v) => /^\$.*/g.test(v),
+    },
+    widget: {
+      type: String,
+      required: true,
+      validator: (v) => {
+        return Object.keys(widgets).includes(`${capitalize(v)}Widget`);
+      },
+    },
+    validate: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+  },
+  computed: {
+    path() {
+      let res;
+      visit(this.template, (seqKey, node, path) => {
+        if (node && node.value === this.target) {
+          // `path` is an array of objects (all the node's parents)
+          // So this reducer will reduce it to an array of the path's keys,
+          // e.g. `[ 'foo', 'bar', '0' ]`
+          res = path.reduce((p, { key }) => (key ? [...p, `${key}`] : p), []);
+          const parent = path[path.length - 1];
+          if (isSeq(parent)) {
+            res.push(seqKey);
+          }
+        }
+      });
+      return res;
+    },
+  },
+  methods: {
+    compile(v) {
+      if (!this.path) return;
+      if (isNullOrUndefined(v)) {
+        this.compiled.deleteIn(this.path);
+      }
+      this.compiled.setIn(this.path, v);
+    },
+    onModelChange(v) {
+      this.$emit('beforeUpdate:compiled');
+      this.compile(v);
+      this.$emit('update:compiled', this.compiled);
+      this.$emit('highlight', this.path);
+    },
+    onValidationStateChange(v) {
+      this.$emit('update:valid', v);
+    },
+  },
+};
+</script>
+
+<template>
+  <div>
+    <component
+      :is="`${widget}-widget`"
+      ref="widget"
+      :validate="validate"
+      v-bind="$attrs"
+      @input="onModelChange"
+      @update:valid="onValidationStateChange"
+    />
+  </div>
+</template>

app/controllers/projects/issues_controller.rb

@@ -49,7 +49,6 @@ class Projects::IssuesController < Projects::ApplicationController
     push_frontend_feature_flag(:confidential_notes, project&.group, default_enabled: :yaml)
     push_frontend_feature_flag(:issue_assignees_widget, project, default_enabled: :yaml)
     push_frontend_feature_flag(:paginated_issue_discussions, project, default_enabled: :yaml)
-    push_frontend_feature_flag(:fix_comment_scroll, project, default_enabled: :yaml)
     push_frontend_feature_flag(:work_items, project&.group, default_enabled: :yaml)
   end
 

config/feature_flags/development/fix_comment_scroll.yml

----
-name: fix_comment_scroll
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76340
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/349638
-milestone: '14.7'
-type: development
-group: group::project management
-default_enabled: false

ee/app/models/ee/group.rb

@@ -582,10 +582,10 @@ module EE
 
     def billed_user_ids_excluding_guests
       strong_memoize(:billed_user_ids_excluding_guests) do
-        group_member_user_ids = billed_group_users(non_guests: true).distinct.pluck(:id)
-        project_member_user_ids = billed_project_users(non_guests: true).distinct.pluck(:id)
-        shared_group_user_ids = billed_shared_non_guests_group_users.distinct.pluck(:id)
-        shared_project_user_ids = billed_invited_non_guests_group_to_project_users.distinct.pluck(:id)
+        group_member_user_ids = billed_group_users(exclude_guests: true).distinct.pluck(:id)
+        project_member_user_ids = billed_project_users(exclude_guests: true).distinct.pluck(:id)
+        shared_group_user_ids = billed_shared_group_users(exclude_guests: true).distinct.pluck(:id)
+        shared_project_user_ids = billed_invited_group_to_project_users(exclude_guests: true).distinct.pluck(:id)
 
         {
           user_ids: (group_member_user_ids + project_member_user_ids + shared_group_user_ids + shared_project_user_ids).to_set,
@@ -624,21 +624,21 @@ module EE
     end
 
     # Members belonging directly to Group or its subgroups
-    def billed_group_users(non_guests: false)
+    def billed_group_users(exclude_guests: false)
       members = ::GroupMember.active_without_invites_and_requests.where(
         source_id: self_and_descendants
       )
 
-      members = members.non_guests if non_guests
+      members = members.non_guests if exclude_guests
 
       users_without_project_bots(members)
     end
 
     # Members belonging directly to Projects within Group or Projects within subgroups
-    def billed_project_users(non_guests: false)
+    def billed_project_users(exclude_guests: false)
       members = ::ProjectMember.without_invites_and_requests
 
-      members = members.non_guests if non_guests
+      members = members.non_guests if exclude_guests
 
       members = members.where(
         source_id: ::Project.joins(:group).where(namespace: self_and_descendants)
@@ -648,13 +648,11 @@ module EE
     end
 
     # Members belonging to Groups invited to collaborate with Projects
-    def billed_invited_group_to_project_users
-      members = invited_or_shared_group_members(invited_groups_in_projects)
-      users_without_project_bots(members)
-    end
+    def billed_invited_group_to_project_users(exclude_guests: false)
+      groups = (exclude_guests ? invited_group_as_non_guests_in_projects : invited_groups_in_projects)
+      members = invited_or_shared_group_members(groups)
+      members = members.non_guests if exclude_guests
 
-    def billed_invited_non_guests_group_to_project_users
-      members = invited_or_shared_group_members(invited_group_as_non_guests_in_projects).non_guests
       users_without_project_bots(members)
     end
 
@@ -668,13 +666,11 @@ module EE
     end
 
     # Members belonging to Groups invited to collaborate with Groups and Subgroups
-    def billed_shared_group_users
-      members = invited_or_shared_group_members(invited_group_in_groups)
-      users_without_project_bots(members)
-    end
+    def billed_shared_group_users(exclude_guests: false)
+      groups = (exclude_guests ? invited_non_guest_group_in_groups : invited_group_in_groups)
+      members = invited_or_shared_group_members(groups)
+      members = members.non_guests if exclude_guests
 
-    def billed_shared_non_guests_group_users
-      members = invited_or_shared_group_members(invited_non_guest_group_in_groups).non_guests
       users_without_project_bots(members)
     end
 

spec/frontend/lib/utils/ignore_while_pending_spec.js

+import waitForPromises from 'helpers/wait_for_promises';
+import { ignoreWhilePending } from '~/lib/utils/ignore_while_pending';
+
+const TEST_ARGS = [123, { foo: 'bar' }];
+
+describe('~/lib/utils/ignore_while_pending', () => {
+  let spyResolve;
+  let spyReject;
+  let spy;
+  let subject;
+
+  beforeEach(() => {
+    spy = jest.fn().mockImplementation(
+      // NOTE: We can't pass an arrow function here...
+      function foo() {
+        return new Promise((resolve, reject) => {
+          spyResolve = resolve;
+          spyReject = reject;
+        });
+      },
+    );
+  });
+
+  describe('with non-instance method', () => {
+    beforeEach(() => {
+      subject = ignoreWhilePending(spy);
+    });
+
+    it('while pending, will ignore subsequent calls', () => {
+      subject(...TEST_ARGS);
+      subject();
+      subject();
+      subject();
+
+      expect(spy).toHaveBeenCalledTimes(1);
+      expect(spy).toHaveBeenCalledWith(...TEST_ARGS);
+    });
+
+    it.each`
+      desc               | act
+      ${'when resolved'} | ${() => spyResolve()}
+      ${'when rejected'} | ${() => spyReject(new Error('foo'))}
+    `('$desc, can be triggered again', async ({ act }) => {
+      // We need the empty catch(), since we are testing rejecting the promise,
+      // which would otherwise cause the test to fail.
+      subject(...TEST_ARGS).catch(() => {});
+      subject();
+      subject();
+      subject();
+
+      act();
+      // We need waitForPromises, so that the underlying finally() runs.
+      await waitForPromises();
+
+      subject({ again: 'foo' });
+
+      expect(spy).toHaveBeenCalledTimes(2);
+      expect(spy).toHaveBeenCalledWith(...TEST_ARGS);
+      expect(spy).toHaveBeenCalledWith({ again: 'foo' });
+    });
+
+    it('while pending, returns empty resolutions for ignored calls', async () => {
+      subject(...TEST_ARGS);
+
+      await expect(subject(...TEST_ARGS)).resolves.toBeUndefined();
+      await expect(subject(...TEST_ARGS)).resolves.toBeUndefined();
+    });
+
+    it('when resolved, returns resolution for origin call', async () => {
+      const resolveValue = { original: 1 };
+      const result = subject(...TEST_ARGS);
+
+      spyResolve(resolveValue);
+
+      await expect(result).resolves.toEqual(resolveValue);
+    });
+
+    it('when rejected, returns rejection for original call', async () => {
+      const rejectedErr = new Error('original');
+      const result = subject(...TEST_ARGS);
+
+      spyReject(rejectedErr);
+
+      await expect(result).rejects.toEqual(rejectedErr);
+    });
+  });
+
+  describe('with instance method', () => {
+    let instance1;
+    let instance2;
+
+    beforeEach(() => {
+      // Let's capture the "this" for tests
+      subject = ignoreWhilePending(function instanceMethod(...args) {
+        return spy(this, ...args);
+      });
+
+      instance1 = {};
+      instance2 = {};
+    });
+
+    it('will not ignore calls across instances', () => {
+      subject.call(instance1, { context: 1 });
+      subject.call(instance1, {});
+      subject.call(instance1, {});
+      subject.call(instance2, { context: 2 });
+      subject.call(instance2, {});
+
+      expect(spy.mock.calls).toEqual([
+        [instance1, { context: 1 }],
+        [instance2, { context: 2 }],
+      ]);
+    });
+
+    it('resolving one instance does not resolve other instances', async () => {
+      subject.call(instance1, { context: 1 });
+
+      // We need to save off spyResolve so it's not overwritten by next call
+      const instance1Resolve = spyResolve;
+
+      subject.call(instance2, { context: 2 });
+
+      instance1Resolve();
+      await waitForPromises();
+
+      subject.call(instance1, { context: 1 });
+      subject.call(instance2, { context: 2 });
+
+      expect(spy.mock.calls).toEqual([
+        [instance1, { context: 1 }],
+        [instance2, { context: 2 }],
+        [instance1, { context: 1 }],
+      ]);
+    });
+  });
+});

spec/frontend/pipeline_wizard/components/input_spec.js

+import { mount, shallowMount } from '@vue/test-utils';
+import { Document } from 'yaml';
+import InputWrapper from '~/pipeline_wizard/components/input.vue';
+import TextWidget from '~/pipeline_wizard/components/widgets/text.vue';
+
+describe('Pipeline Wizard -- Input Wrapper', () => {
+  let wrapper;
+
+  const createComponent = (props = {}, mountFunc = mount) => {
+    wrapper = mountFunc(InputWrapper, {
+      propsData: {
+        template: new Document({
+          template: {
+            bar: 'baz',
+            foo: { some: '$TARGET' },
+          },
+        }).get('template'),
+        compiled: new Document({ bar: 'baz', foo: { some: '$TARGET' } }),
+        target: '$TARGET',
+        widget: 'text',
+        label: 'some label (required by the text widget)',
+        ...props,
+      },
+    });
+  };
+
+  describe('API', () => {
+    const inputValue = 'dslkfjsdlkfjlskdjfn';
+    let inputChild;
+
+    beforeEach(() => {
+      createComponent({});
+      inputChild = wrapper.find(TextWidget);
+    });
+
+    afterEach(() => {
+      wrapper.destroy();
+    });
+
+    it('will replace its value in compiled', async () => {
+      await inputChild.vm.$emit('input', inputValue);
+      const expected = new Document({
+        bar: 'baz',
+        foo: { some: inputValue },
+      });
+      expect(wrapper.emitted()['update:compiled']).toEqual([[expected]]);
+    });
+
+    it('will emit a highlight event with the correct path if child emits an input event', async () => {
+      await inputChild.vm.$emit('input', inputValue);
+      const expected = ['foo', 'some'];
+      expect(wrapper.emitted().highlight).toEqual([[expected]]);
+    });
+  });
+
+  describe('Target Path Discovery', () => {
+    afterEach(() => {
+      wrapper.destroy();
+    });
+
+    it.each`
+      scenario                  | template                             | target     | expected
+      ${'simple nested object'} | ${{ foo: { bar: { baz: '$BOO' } } }} | ${'$BOO'}  | ${['foo', 'bar', 'baz']}
+      ${'list, first pos.'}     | ${{ foo: ['$BOO'] }}                 | ${'$BOO'}  | ${['foo', 0]}
+      ${'list, second pos.'}    | ${{ foo: ['bar', '$BOO'] }}          | ${'$BOO'}  | ${['foo', 1]}
+      ${'lowercase target'}     | ${{ foo: { bar: '$jupp' } }}         | ${'$jupp'} | ${['foo', 'bar']}
+      ${'root list'}            | ${['$BOO']}                          | ${'$BOO'}  | ${[0]}
+    `('$scenario', ({ template, target, expected }) => {
+      createComponent(
+        {
+          template: new Document({ template }).get('template'),
+          target,
+        },
+        shallowMount,
+      );
+      expect(wrapper.vm.path).toEqual(expected);
+    });
+  });
+});

spec/frontend/pipeline_wizard/components/widgets_spec.js

+import fs from 'fs';
+import { mount } from '@vue/test-utils';
+import { Document } from 'yaml';
+import InputWrapper from '~/pipeline_wizard/components/input.vue';
+
+describe('Test all widgets in ./widgets/* whether they provide a minimal api', () => {
+  const createComponent = (props = {}, mountFunc = mount) => {
+    mountFunc(InputWrapper, {
+      propsData: {
+        template: new Document({
+          template: {
+            bar: 'baz',
+            foo: { some: '$TARGET' },
+          },
+        }).get('template'),
+        compiled: new Document({ bar: 'baz', foo: { some: '$TARGET' } }),
+        target: '$TARGET',
+        widget: 'text',
+        label: 'some label (required by the text widget)',
+        ...props,
+      },
+    });
+  };
+
+  const widgets = fs
+    .readdirSync('./app/assets/javascripts/pipeline_wizard/components/widgets')
+    .map((filename) => [filename.match(/^(.*).vue$/)[1]]);
+  let consoleErrorSpy;
+
+  beforeAll(() => {
+    consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
+  });
+
+  afterAll(() => {
+    consoleErrorSpy.mockRestore();
+  });
+
+  describe.each(widgets)('`%s` Widget', (name) => {
+    it('passes the input validator', () => {
+      const validatorFunc = InputWrapper.props.widget.validator;
+      expect(validatorFunc(name)).toBe(true);
+    });
+
+    it('mounts without error', () => {
+      createComponent({ widget: name });
+      expect(consoleErrorSpy).not.toHaveBeenCalled();
+    });
+  });
+});