Merge branch 'jswain_whats_new_platform_specific_results' into 'master'

Refactor whats_new with new ReleaseHighlight model

See merge request gitlab-org/gitlab!45697

app/controllers/whats_new_controller.rb

 # frozen_string_literal: true
 
 class WhatsNewController < ApplicationController
-  include Gitlab::WhatsNew
-
   skip_before_action :authenticate_user!
 
   before_action :check_feature_flag, :check_valid_page_param, :set_pagination_headers
@@ -12,7 +10,7 @@ class WhatsNewController < ApplicationController
   def index
     respond_to do |format|
       format.js do
-        render json: whats_new_release_items(page: current_page)
+        render json: most_recent_items
       end
     end
   end
@@ -27,18 +25,19 @@ class WhatsNewController < ApplicationController
     render_404 if current_page < 1
   end
 
-  def set_pagination_headers
-    response.set_header('X-Next-Page', next_page)
-  end
-
   def current_page
     params[:page]&.to_i || 1
   end
 
-  def next_page
-    next_page = current_page + 1
-    next_index = next_page - 1
+  def most_recent
+    @most_recent ||= ReleaseHighlight.paginated(page: current_page)
+  end
+
+  def most_recent_items
+    most_recent[:items].map {|item| Gitlab::WhatsNew::ItemPresenter.present(item) }
+  end
 
-    next_page if whats_new_file_paths[next_index]
+  def set_pagination_headers
+    response.set_header('X-Next-Page', most_recent[:next_page])
   end
 end

app/helpers/whats_new_helper.rb

 # frozen_string_literal: true
 
 module WhatsNewHelper
-  include Gitlab::WhatsNew
-
   def whats_new_most_recent_release_items_count
-    Gitlab::ProcessMemoryCache.cache_backend.fetch('whats_new:release_items_count', expires_in: CACHE_DURATION) do
-      whats_new_release_items&.count
-    end
+    ReleaseHighlight.most_recent_item_count
   end
 
   def whats_new_storage_key
-    return unless whats_new_most_recent_version
-
-    ['display-whats-new-notification', whats_new_most_recent_version].join('-')
-  end
+    most_recent_version = ReleaseHighlight.most_recent_version
 
-  private
+    return unless most_recent_version
 
-  def whats_new_most_recent_version
-    Gitlab::ProcessMemoryCache.cache_backend.fetch('whats_new:release_version', expires_in: CACHE_DURATION) do
-      whats_new_release_items&.first&.[]('release')
-    end
+    ['display-whats-new-notification', most_recent_version].join('-')
   end
 end

app/models/release_highlight.rb

+# frozen_string_literal: true
+
+class ReleaseHighlight
+  CACHE_DURATION = 1.hour
+  FILES_PATH = Rails.root.join('data', 'whats_new', '*.yml')
+
+  def self.paginated(page: 1)
+    Rails.cache.fetch(cache_key(page), expires_in: CACHE_DURATION) do
+      items = self.load_items(page: page)
+
+      next if items.nil?
+
+      {
+        items: items,
+        next_page: next_page(current_page: page)
+      }
+    end
+  end
+
+  def self.load_items(page:)
+    index = page - 1
+    file_path = file_paths[index]
+
+    return if file_path.nil?
+
+    file = File.read(file_path)
+
+    items = YAML.safe_load(file, permitted_classes: [Date])
+
+    platform = Gitlab.com? ? 'gitlab-com' : 'self-managed'
+    items&.select {|item| item[platform] }
+  rescue Psych::Exception => e
+    Gitlab::ErrorTracking.track_exception(e, file_path: file_path)
+
+    nil
+  end
+
+  def self.file_paths
+    @file_paths ||= Rails.cache.fetch('release_highlight:file_paths', expires_in: CACHE_DURATION) do
+      Dir.glob(FILES_PATH).sort.reverse
+    end
+  end
+
+  def self.cache_key(page)
+    filename = /\d*\_\d*\_\d*/.match(self.file_paths&.first)
+    "release_highlight:items:file-#{filename}:page-#{page}"
+  end
+
+  def self.next_page(current_page: 1)
+    next_page = current_page + 1
+    next_index = next_page - 1
+
+    next_page if self.file_paths[next_index]
+  end
+
+  def self.most_recent_version
+    Gitlab::ProcessMemoryCache.cache_backend.fetch('release_highlight:release_version', expires_in: CACHE_DURATION) do
+      self.paginated&.[](:items)&.first&.[]('release')
+    end
+  end
+
+  def self.most_recent_item_count
+    Gitlab::ProcessMemoryCache.cache_backend.fetch('release_highlight:recent_item_count', expires_in: CACHE_DURATION) do
+      self.paginated&.[](:items)&.count
+    end
+  end
+end

app/presenters/gitlab/whats_new/item_presenter.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module WhatsNew
+    class ItemPresenter
+      DICTIONARY = {
+        free: 'Free',
+        starter: 'Bronze',
+        premium: 'Silver',
+        ultimate: 'Gold'
+      }.freeze
+
+      def self.present(item)
+        if Gitlab.com?
+          item['packages'] = item['packages'].map { |p| DICTIONARY[p.downcase.to_sym] }
+        end
+
+        item
+      end
+    end
+  end
+end

lib/gitlab/whats_new.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module WhatsNew
-    CACHE_DURATION = 1.hour
-    WHATS_NEW_FILES_PATH = Rails.root.join('data', 'whats_new', '*.yml')
-
-    private
-
-    def whats_new_release_items(page: 1)
-      Rails.cache.fetch(whats_new_items_cache_key(page), expires_in: CACHE_DURATION) do
-        index = page - 1
-        file_path = whats_new_file_paths[index]
-
-        next if file_path.nil?
-
-        file = File.read(file_path)
-
-        items = YAML.safe_load(file, permitted_classes: [Date])
-
-        items if items.is_a?(Array)
-      end
-    rescue => e
-      Gitlab::ErrorTracking.track_exception(e, page: page)
-
-      nil
-    end
-
-    def whats_new_file_paths
-      @whats_new_file_paths ||= Rails.cache.fetch('whats_new:file_paths', expires_in: CACHE_DURATION) do
-        Dir.glob(WHATS_NEW_FILES_PATH).sort.reverse
-      end
-    end
-
-    def whats_new_items_cache_key(page)
-      filename = /\d*\_\d*\_\d*/.match(whats_new_file_paths&.first)
-      "whats_new:release_items:file-#{filename}:page-#{page}"
-    end
-  end
-end

spec/fixtures/whats_new/20201225_01_01.yml

 ---
 - title: It's gonna be a bright
+  self-managed: true
+  gitlab-com: false
+  packages: ["Premium", "Ultimate"]

spec/fixtures/whats_new/20201225_01_02.yml

 ---
 - title: bright
+  self-managed: true
+  gitlab-com: false
+  packages: ["Premium", "Ultimate"]

spec/fixtures/whats_new/20201225_01_05.yml

 ---
 - title: bright and sunshinin' day
+  self-managed: true
+  gitlab-com: false
+  packages: ["Premium", "Ultimate"]
   release: '01.05'
+- title: I think I can make it now the pain is gone
+  self-managed: false
+  gitlab-com: true
+  packages: ["Premium", "Ultimate"]

spec/helpers/whats_new_helper_spec.rb

@@ -3,22 +3,22 @@
 require 'spec_helper'
 
 RSpec.describe WhatsNewHelper do
-  let(:fixture_dir_glob) { Dir.glob(File.join('spec', 'fixtures', 'whats_new', '*.yml')) }
-
   describe '#whats_new_storage_key' do
     subject { helper.whats_new_storage_key }
 
     context 'when version exist' do
+      let(:release_item) { double(:item) }
+
       before do
-        allow(Dir).to receive(:glob).with(Rails.root.join('data', 'whats_new', '*.yml')).and_return(fixture_dir_glob)
+        allow(ReleaseHighlight).to receive(:most_recent_version).and_return(84.0)
       end
 
-      it { is_expected.to eq('display-whats-new-notification-01.05') }
+      it { is_expected.to eq('display-whats-new-notification-84.0') }
     end
 
-    context 'when recent release items do NOT exist' do
+    context 'when most recent release highlights do NOT exist' do
       before do
-        allow(helper).to receive(:whats_new_release_items).and_return(nil)
+        allow(ReleaseHighlight).to receive(:most_recent_version).and_return(nil)
       end
 
       it { is_expected.to be_nil }
@@ -30,31 +30,18 @@ RSpec.describe WhatsNewHelper do
 
     context 'when recent release items exist' do
       it 'returns the count from the most recent file' do
-        expect(Dir).to receive(:glob).with(Rails.root.join('data', 'whats_new', '*.yml')).and_return(fixture_dir_glob)
+        allow(ReleaseHighlight).to receive(:most_recent_item_count).and_return(1)
 
         expect(subject).to eq(1)
       end
     end
 
     context 'when recent release items do NOT exist' do
-      before do
-        allow(YAML).to receive(:safe_load).and_raise
-
-        expect(Gitlab::ErrorTracking).to receive(:track_exception)
-      end
+      it 'returns nil' do
+        allow(ReleaseHighlight).to receive(:most_recent_item_count).and_return(nil)
 
-      it 'fails gracefully and logs an error' do
         expect(subject).to be_nil
       end
     end
   end
-
-  # Testing this important private method here because the request spec required multiple confusing mocks and felt wrong and overcomplicated
-  describe '#whats_new_items_cache_key' do
-    it 'returns a key containing the most recent file name and page parameter' do
-      allow(Dir).to receive(:glob).with(Rails.root.join('data', 'whats_new', '*.yml')).and_return(fixture_dir_glob)
-
-      expect(helper.send(:whats_new_items_cache_key, 2)).to eq('whats_new:release_items:file-20201225_01_05:page-2')
-    end
-  end
 end

spec/models/release_highlight_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ReleaseHighlight do
+  describe '#paginated' do
+    let(:fixture_dir_glob) { Dir.glob(File.join('spec', 'fixtures', 'whats_new', '*.yml')) }
+    let(:cache_mock) { double(:cache_mock) }
+    let(:dot_com) { false }
+
+    before do
+      allow(Gitlab).to receive(:com?).and_return(dot_com)
+      allow(Dir).to receive(:glob).with(Rails.root.join('data', 'whats_new', '*.yml')).and_return(fixture_dir_glob)
+
+      expect(Rails).to receive(:cache).twice.and_return(cache_mock)
+      expect(cache_mock).to receive(:fetch).with('release_highlight:file_paths', expires_in: 1.hour).and_yield
+    end
+
+    after do
+      ReleaseHighlight.instance_variable_set(:@file_paths, nil)
+    end
+
+    context 'with page param' do
+      subject { ReleaseHighlight.paginated(page: page) }
+
+      before do
+        allow(cache_mock).to receive(:fetch).and_yield
+      end
+
+      context 'when there is another page of results' do
+        let(:page) { 2 }
+
+        it 'responds with paginated results' do
+          expect(subject[:items].first['title']).to eq('bright')
+          expect(subject[:next_page]).to eq(3)
+        end
+      end
+
+      context 'when there is NOT another page of results' do
+        let(:page) { 3 }
+
+        it 'responds with paginated results and no next_page' do
+          expect(subject[:items].first['title']).to eq("It's gonna be a bright")
+          expect(subject[:next_page]).to eq(nil)
+        end
+      end
+
+      context 'when that specific page does not exist' do
+        let(:page) { 84 }
+
+        it 'returns nil' do
+          expect(subject).to be_nil
+        end
+      end
+    end
+
+    context 'with no page param' do
+      subject { ReleaseHighlight.paginated }
+
+      before do
+        expect(cache_mock).to receive(:fetch).with('release_highlight:items:file-20201225_01_05:page-1', expires_in: 1.hour).and_yield
+      end
+
+      it 'returns platform specific items and uses a cache key' do
+        expect(subject[:items].count).to eq(1)
+        expect(subject[:items].first['title']).to eq("bright and sunshinin' day")
+        expect(subject[:next_page]).to eq(2)
+      end
+
+      context 'when Gitlab.com' do
+        let(:dot_com) { true }
+
+        it 'responds with a different set of data' do
+          expect(subject[:items].count).to eq(1)
+          expect(subject[:items].first['title']).to eq("I think I can make it now the pain is gone")
+        end
+      end
+
+      context 'when recent release items do NOT exist' do
+        before do
+          allow(YAML).to receive(:safe_load).and_raise(Psych::Exception)
+
+          expect(Gitlab::ErrorTracking).to receive(:track_exception)
+        end
+
+        it 'fails gracefully and logs an error' do
+          expect(subject).to be_nil
+        end
+      end
+    end
+  end
+
+  describe '.most_recent_version' do
+    subject { ReleaseHighlight.most_recent_version }
+
+    context 'when version exist' do
+      let(:release_item) { double(:item) }
+
+      before do
+        allow(ReleaseHighlight).to receive(:paginated).and_return({ items: [release_item] })
+        allow(release_item).to receive(:[]).with('release').and_return(84.0)
+      end
+
+      it { is_expected.to eq(84.0) }
+    end
+
+    context 'when most recent release highlights do NOT exist' do
+      before do
+        allow(ReleaseHighlight).to receive(:paginated).and_return(nil)
+      end
+
+      it { is_expected.to be_nil }
+    end
+  end
+
+  describe '#most_recent_item_count' do
+    subject { ReleaseHighlight.most_recent_item_count }
+
+    context 'when recent release items exist' do
+      it 'returns the count from the most recent file' do
+        allow(ReleaseHighlight).to receive(:paginated).and_return({ items: [double(:item)] })
+
+        expect(subject).to eq(1)
+      end
+    end
+
+    context 'when recent release items do NOT exist' do
+      it 'returns nil' do
+        allow(ReleaseHighlight).to receive(:paginated).and_return(nil)
+
+        expect(subject).to be_nil
+      end
+    end
+  end
+end

spec/presenters/gitlab/whats_new/item_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::WhatsNew::ItemPresenter do
+  let(:present) { Gitlab::WhatsNew::ItemPresenter.present(item) }
+  let(:item) { { "packages" => %w(Premium Ultimate) } }
+  let(:gitlab_com) { true }
+
+  before do
+    allow(Gitlab).to receive(:com?).and_return(gitlab_com)
+  end
+
+  describe '.present' do
+    context 'when on Gitlab.com' do
+      it 'transforms package names to gitlab.com friendly package names' do
+        expect(present).to eq({ "packages" => %w(Silver Gold) })
+      end
+    end
+
+    context 'when not on Gitlab.com' do
+      let(:gitlab_com) { false }
+
+      it 'does not transform package names' do
+        expect(present).to eq({ "packages" => %w(Premium Ultimate) })
+      end
+    end
+  end
+end

spec/requests/whats_new_controller_spec.rb

@@ -5,28 +5,30 @@ require 'spec_helper'
 RSpec.describe WhatsNewController do
   describe 'whats_new_path' do
     context 'with whats_new_drawer feature enabled' do
-      let(:fixture_dir_glob) { Dir.glob(File.join('spec', 'fixtures', 'whats_new', '*.yml')) }
-
       before do
         stub_feature_flags(whats_new_drawer: true)
-        allow(Dir).to receive(:glob).with(Rails.root.join('data', 'whats_new', '*.yml')).and_return(fixture_dir_glob)
       end
 
       context 'with no page param' do
+        let(:most_recent) { { items: [item], next_page: 2 } }
+        let(:item) { double(:item) }
+
         it 'responds with paginated data and headers' do
+          allow(ReleaseHighlight).to receive(:paginated).with(page: 1).and_return(most_recent)
+          allow(Gitlab::WhatsNew::ItemPresenter).to receive(:present).with(item).and_return(item)
+
           get whats_new_path, xhr: true
 
-          expect(response.body).to eq([{ title: "bright and sunshinin' day", release: "01.05" }].to_json)
+          expect(response.body).to eq(most_recent[:items].to_json)
           expect(response.headers['X-Next-Page']).to eq(2)
         end
       end
 
       context 'with page param' do
-        it 'responds with paginated data and headers' do
-          get whats_new_path(page: 2), xhr: true
+        it 'passes the page parameter' do
+          expect(ReleaseHighlight).to receive(:paginated).with(page: 2).and_call_original
 
-          expect(response.body).to eq([{ title: 'bright' }].to_json)
-          expect(response.headers['X-Next-Page']).to eq(3)
+          get whats_new_path(page: 2), xhr: true
         end
 
         it 'returns a 404 if page param is negative' do
@@ -34,14 +36,6 @@ RSpec.describe WhatsNewController do
 
           expect(response).to have_gitlab_http_status(:not_found)
         end
-
-        context 'when there are no more paginated results' do
-          it 'responds with nil X-Next-Page header' do
-            get whats_new_path(page: 3), xhr: true
-            expect(response.body).to eq([{ title: "It's gonna be a bright" }].to_json)
-            expect(response.headers['X-Next-Page']).to be nil
-          end
-        end
       end
     end