Automatic merge of gitlab-org/gitlab-ce master

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

@@ -8,12 +8,14 @@ import SystemNote from '~/vue_shared/components/notes/system_note.vue';
 import NoteableNote from './noteable_note.vue';
 import ToggleRepliesWidget from './toggle_replies_widget.vue';
 import NoteEditedText from './note_edited_text.vue';
+import DiscussionNotesRepliesWrapper from './discussion_notes_replies_wrapper.vue';
 
 export default {
   name: 'DiscussionNotes',
   components: {
     ToggleRepliesWidget,
     NoteEditedText,
+    DiscussionNotesRepliesWrapper,
   },
   props: {
     discussion: {
@@ -119,9 +121,7 @@ export default {
           />
           <slot slot="avatar-badge" name="avatar-badge"></slot>
         </component>
-        <div
-          :class="discussion.diff_discussion ? 'discussion-collapsible bordered-box clearfix' : ''"
-        >
+        <discussion-notes-replies-wrapper :is-diff-discussion="discussion.diff_discussion">
           <toggle-replies-widget
             v-if="hasReplies"
             :collapsed="!isExpanded"
@@ -141,7 +141,7 @@ export default {
             />
           </template>
           <slot :show-replies="isExpanded || !hasReplies" name="footer"></slot>
-        </div>
+        </discussion-notes-replies-wrapper>
       </template>
       <template v-else>
         <component

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

+<script>
+/**
+ * Wrapper for discussion notes replies section.
+ *
+ * This is a functional component using the render method because in some cases
+ * the wrapper is not needed and we want to simply render along the children.
+ */
+export default {
+  functional: true,
+  props: {
+    isDiffDiscussion: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+  },
+  render(h, { props, children }) {
+    if (props.isDiffDiscussion) {
+      return h('li', { class: 'discussion-collapsible bordered-box clearfix' }, [
+        h('ul', { class: 'notes' }, children),
+      ]);
+    }
+
+    return children;
+  },
+};
+</script>

app/assets/stylesheets/pages/diff.scss

@@ -1095,6 +1095,10 @@ table.code {
 
 .discussion-collapsible {
   margin: 0 $gl-padding $gl-padding 71px;
+
+  .notes {
+    border-radius: $border-radius-default;
+  }
 }
 
 .parallel {

app/assets/stylesheets/pages/notes.scss

@@ -139,7 +139,6 @@ $note-form-margin-left: 72px;
     border-radius: 4px 4px 0 0;
 
     &.collapsed {
-      border: 0;
       border-radius: 4px;
     }
   }

app/models/concerns/reactive_caching.rb

@@ -173,8 +173,12 @@ module ReactiveCaching
     end
 
     def within_reactive_cache_lifetime?(*args)
+      if Feature.enabled?(:reactive_caching_check_key_exists, default_enabled: true)
+        Rails.cache.exist?(alive_reactive_cache_key(*args))
+      else
         !!Rails.cache.read(alive_reactive_cache_key(*args))
       end
+    end
 
     def enqueuing_update(*args)
       yield

changelogs/unreleased/allow-reactive-caching-of-nil.yml

+---
+title: Allow ReactiveCaching to support nil value
+merge_request: 30456
+author:
+type: performance

spec/frontend/notes/components/discussion_notes_replies_wrapper_spec.js

+import { mount } from '@vue/test-utils';
+import DiscussionNotesRepliesWrapper from '~/notes/components/discussion_notes_replies_wrapper.vue';
+
+const TEST_CHILDREN = '<li>Hello!</li><li>World!</li>';
+
+// We have to wrap our SUT with a TestComponent because multiple roots are possible
+// because it's a functional component.
+const TestComponent = {
+  components: { DiscussionNotesRepliesWrapper },
+  template: `<ul><discussion-notes-replies-wrapper v-bind="$attrs">${TEST_CHILDREN}</discussion-notes-replies-wrapper></ul>`,
+};
+
+describe('DiscussionNotesRepliesWrapper', () => {
+  let wrapper;
+
+  const createComponent = (props = {}) => {
+    wrapper = mount(TestComponent, {
+      propsData: props,
+      sync: false,
+    });
+  };
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  describe('when normal discussion', () => {
+    beforeEach(() => {
+      createComponent();
+    });
+
+    it('renders children directly', () => {
+      expect(wrapper.html()).toEqual(`<ul>${TEST_CHILDREN}</ul>`);
+    });
+  });
+
+  describe('when diff discussion', () => {
+    beforeEach(() => {
+      createComponent({
+        isDiffDiscussion: true,
+      });
+    });
+
+    it('wraps children with notes', () => {
+      const notes = wrapper.find('li.discussion-collapsible ul.notes');
+
+      expect(notes.exists()).toBe(true);
+      expect(notes.html()).toEqual(`<ul class="notes">${TEST_CHILDREN}</ul>`);
+    });
+  });
+});

spec/javascripts/diffs/components/inline_diff_view_spec.js

@@ -10,6 +10,7 @@ describe('InlineDiffView', () => {
   let component;
   const getDiffFileMock = () => Object.assign({}, diffFileMockData);
   const getDiscussionsMockData = () => [Object.assign({}, discussionsMockData)];
+  const notesLength = getDiscussionsMockData()[0].notes.length;
 
   beforeEach(done => {
     const diffFile = getDiffFileMock();
@@ -40,7 +41,7 @@ describe('InlineDiffView', () => {
 
       Vue.nextTick(() => {
         expect(el.querySelectorAll('.notes_holder').length).toEqual(1);
-        expect(el.querySelectorAll('.notes_holder .note-discussion li').length).toEqual(6);
+        expect(el.querySelectorAll('.notes_holder .note').length).toEqual(notesLength + 1);
         expect(el.innerText.indexOf('comment 5')).toBeGreaterThan(-1);
         component.$store.dispatch('setInitialNotes', []);
 

spec/models/concerns/reactive_caching_spec.rb

@@ -47,30 +47,12 @@ describe ReactiveCaching, :use_clean_rails_memory_store_caching do
 
     subject(:go!) { instance.result }
 
-    context 'when cache is empty' do
-      it { is_expected.to be_nil }
-
-      it 'enqueues a background worker to bootstrap the cache' do
-        expect(ReactiveCachingWorker).to receive(:perform_async).with(CacheTest, 666)
-
-        go!
-      end
-
-      it 'updates the cache lifespan' do
-        expect(reactive_cache_alive?(instance)).to be_falsy
-
-        go!
-
-        expect(reactive_cache_alive?(instance)).to be_truthy
-      end
-    end
-
-    context 'when the cache is full' do
+    shared_examples 'a cacheable value' do |cached_value|
       before do
-        stub_reactive_cache(instance, 4)
+        stub_reactive_cache(instance, cached_value)
       end
 
-      it { is_expected.to eq(4) }
+      it { is_expected.to eq(cached_value) }
 
       it 'does not enqueue a background worker' do
         expect(ReactiveCachingWorker).not_to receive(:perform_async)
@@ -90,9 +72,7 @@ describe ReactiveCaching, :use_clean_rails_memory_store_caching do
         end
 
         it { is_expected.to be_nil }
-      end
 
-      context 'when cache was invalidated' do
         it 'refreshes cache' do
           expect(ReactiveCachingWorker).to receive(:perform_async).with(CacheTest, 666)
 
@@ -101,12 +81,34 @@ describe ReactiveCaching, :use_clean_rails_memory_store_caching do
       end
     end
 
-    context 'when cache contains non-nil but blank value' do
-      before do
-        stub_reactive_cache(instance, false)
+    context 'when cache is empty' do
+      it { is_expected.to be_nil }
+
+      it 'enqueues a background worker to bootstrap the cache' do
+        expect(ReactiveCachingWorker).to receive(:perform_async).with(CacheTest, 666)
+
+        go!
+      end
+
+      it 'updates the cache lifespan' do
+        expect(reactive_cache_alive?(instance)).to be_falsy
+
+        go!
+
+        expect(reactive_cache_alive?(instance)).to be_truthy
+      end
+    end
+
+    context 'when the cache is full' do
+      it_behaves_like 'a cacheable value', 4
+    end
+
+    context 'when the cache contains non-nil but blank value' do
+      it_behaves_like 'a cacheable value', false
     end
 
-      it { is_expected.to eq(false) }
+    context 'when the cache contains nil value' do
+      it_behaves_like 'a cacheable value', nil
     end
   end
 

spec/support/helpers/reactive_caching_helpers.rb

@@ -10,7 +10,7 @@ module ReactiveCachingHelpers
   def stub_reactive_cache(subject = nil, data = nil, *qualifiers)
     allow(ReactiveCachingWorker).to receive(:perform_async)
     allow(ReactiveCachingWorker).to receive(:perform_in)
-    write_reactive_cache(subject, data, *qualifiers) unless data.nil?
+    write_reactive_cache(subject, data, *qualifiers) unless subject.nil?
   end
 
   def synchronous_reactive_cache(subject)