Automatic merge of gitlab-org/gitlab-ce master

app/assets/javascripts/mr_popover/index.js

@@ -22,13 +22,10 @@ const handleUserPopoverMouseOut = ({ target }) => {
  * Adds a MergeRequestPopover component to the body, hands over as much data as the target element has in data attributes.
  * loads based on data-project-path and data-iid more data about an MR from the API and sets it on the popover
  */
-const handleMRPopoverMount = apolloProvider => ({ target }) => {
+const handleMRPopoverMount = ({ apolloProvider, projectPath, mrTitle, iid }) => ({ target }) => {
   // Add listener to actually remove it again
   target.addEventListener('mouseleave', handleUserPopoverMouseOut);
 
-  const { projectPath, mrTitle, iid } = target.dataset;
-  const mergeRequest = {};
-
   renderFn = setTimeout(() => {
     const MRPopoverComponent = Vue.extend(MRPopover);
     renderedPopover = new MRPopoverComponent({
@@ -36,7 +33,6 @@ const handleMRPopoverMount = apolloProvider => ({ target }) => {
         target,
         projectPath,
         mergeRequestIID: iid,
-        mergeRequest,
         mergeRequestTitle: mrTitle,
       },
       apolloProvider,
@@ -57,8 +53,13 @@ export default elements => {
     const listenerAddedAttr = 'data-mr-listener-added';
 
     mrLinks.forEach(el => {
-      if (!el.getAttribute(listenerAddedAttr)) {
-        el.addEventListener('mouseenter', handleMRPopoverMount(apolloProvider));
+      const { projectPath, mrTitle, iid } = el.dataset;
+
+      if (!el.getAttribute(listenerAddedAttr) && projectPath && mrTitle && iid) {
+        el.addEventListener(
+          'mouseenter',
+          handleMRPopoverMount({ apolloProvider, projectPath, mrTitle, iid }),
+        );
         el.setAttribute(listenerAddedAttr, true);
       }
     });

app/helpers/markup_helper.rb

@@ -83,7 +83,8 @@ module MarkupHelper
     text = sanitize(
       text,
       tags: tags,
-      attributes: Rails::Html::WhiteListSanitizer.allowed_attributes + ['style', 'data-src', 'data-name', 'data-unicode-version']
+      attributes: Rails::Html::WhiteListSanitizer.allowed_attributes +
+          %w(style data-src data-name data-unicode-version data-iid data-project-path data-mr-title)
     )
 
     # since <img> tags are stripped, this can leave empty <a> tags hanging around

app/models/internal_id.rb

@@ -87,12 +87,16 @@ class InternalId < ApplicationRecord
     end
 
     def available?
-      @available_flag ||= ActiveRecord::Migrator.current_version >= REQUIRED_SCHEMA_VERSION # rubocop:disable Gitlab/PredicateMemoization
+      return true unless Rails.env.test?
+
+      Gitlab::SafeRequestStore.fetch(:internal_ids_available_flag) do
+        ActiveRecord::Migrator.current_version >= REQUIRED_SCHEMA_VERSION
+      end
     end
 
     # Flushes cached information about schema
     def reset_column_information
-      @available_flag = nil
+      Gitlab::SafeRequestStore[:internal_ids_available_flag] = nil
       super
     end
   end

changelogs/unreleased/60540-merge-request-popover-is-not-working-on-the-to-do-page.yml

+---
+title: Fix MR popover on ToDos page
+merge_request: 27382
+author:
+type: fixed

changelogs/unreleased/sh-disable-internal-ids-available-check.yml

+---
+title: Always use internal ID tables in development and production
+merge_request: 27544
+author:
+type: fixed

spec/features/dashboard/todos/todos_spec.rb

@@ -17,6 +17,26 @@ describe 'Dashboard Todos' do
     end
   end
 
+  context 'when the todo references a merge request' do
+    let(:referenced_mr) { create(:merge_request, source_project: project) }
+    let(:note) { create(:note, project: project, note: "Check out #{referenced_mr.to_reference}") }
+    let!(:todo) { create(:todo, :mentioned, user: user, project: project, author: author, note: note) }
+
+    before do
+      sign_in(user)
+      visit dashboard_todos_path
+    end
+
+    it 'renders the mr link with the extra attributes' do
+      link = page.find_link(referenced_mr.to_reference)
+
+      expect(link).not_to be_nil
+      expect(link['data-iid']).to eq(referenced_mr.iid.to_s)
+      expect(link['data-project-path']).to eq(referenced_mr.project.full_path)
+      expect(link['data-mr-title']).to eq(referenced_mr.title)
+    end
+  end
+
   context 'User has a todo', :js do
     before do
       create(:todo, :mentioned, user: user, project: project, target: issue, author: author)

spec/frontend/mr_popover/index_spec.js

@@ -7,18 +7,28 @@ createDefaultClient.default = jest.fn();
 describe('initMRPopovers', () => {
   let mr1;
   let mr2;
+  let mr3;
 
   beforeEach(() => {
     setHTMLFixture(`
-      <div id="one" class="gfm-merge_request">MR1</div>
-      <div id="two" class="gfm-merge_request">MR2</div>
+      <div id="one" class="gfm-merge_request" data-mr-title="title" data-iid="1" data-project-path="group/project">
+        MR1
+      </div>
+      <div id="two" class="gfm-merge_request" data-mr-title="title" data-iid="1" data-project-path="group/project">
+        MR2
+      </div>
+      <div id="three" class="gfm-merge_request">
+        MR3
+      </div>
     `);
 
     mr1 = document.querySelector('#one');
     mr2 = document.querySelector('#two');
+    mr3 = document.querySelector('#three');
 
     mr1.addEventListener = jest.fn();
     mr2.addEventListener = jest.fn();
+    mr3.addEventListener = jest.fn();
   });
 
   it('does not add the same event listener twice', () => {
@@ -27,4 +37,10 @@ describe('initMRPopovers', () => {
     expect(mr1.addEventListener).toHaveBeenCalledTimes(1);
     expect(mr2.addEventListener).toHaveBeenCalledTimes(1);
   });
+
+  it('does not add listener if it does not have the necessary data attributes', () => {
+    initMRPopovers([mr1, mr2, mr3]);
+
+    expect(mr3.addEventListener).not.toHaveBeenCalled();
+  });
 });

spec/models/internal_id_spec.rb

@@ -93,7 +93,7 @@ describe InternalId do
       before do
         described_class.reset_column_information
         # Project factory will also call the current_version
-        expect(ActiveRecord::Migrator).to receive(:current_version).twice.and_return(InternalId::REQUIRED_SCHEMA_VERSION - 1)
+        expect(ActiveRecord::Migrator).to receive(:current_version).at_least(:once).and_return(InternalId::REQUIRED_SCHEMA_VERSION - 1)
       end
 
       let(:init) { double('block') }
@@ -104,6 +104,15 @@ describe InternalId do
         expect(init).to receive(:call).with(issue).and_return(val)
         expect(subject).to eq(val + 1)
       end
+
+      it 'always attempts to generate internal IDs in production mode' do
+        allow(Rails.env).to receive(:test?).and_return(false)
+        val = rand(1..100)
+        generator = double(generate: val)
+        expect(InternalId::InternalIdGenerator).to receive(:new).and_return(generator)
+
+        expect(subject).to eq(val)
+      end
     end
   end