From 04a3e48c2a0e31e31f8ba0e9036597428ee3a373 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 14 Dec 2018 11:47:49 -0600 Subject: [PATCH 1/2] Use class methods for VersionCheck All of these methods are stateless, there was no point to have them as instance methods. Mostly this allows us to remove an `allow_any_instance_of` usage. --- app/helpers/version_check_helper.rb | 3 +-- lib/version_check.rb | 7 ++++--- spec/features/help_pages_spec.rb | 5 +++-- spec/helpers/version_check_helper_spec.rb | 12 ++++++------ 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/app/helpers/version_check_helper.rb b/app/helpers/version_check_helper.rb index ab77b149072..5e519cf5c19 100644 --- a/app/helpers/version_check_helper.rb +++ b/app/helpers/version_check_helper.rb @@ -6,8 +6,7 @@ module VersionCheckHelper return unless Gitlab::CurrentSettings.version_check_enabled return if User.single_user&.requires_usage_stats_consent? - image_url = VersionCheck.new.url - image_tag image_url, class: 'js-version-status-badge' + image_tag VersionCheck.url, class: 'js-version-status-badge' end def link_to_version diff --git a/lib/version_check.rb b/lib/version_check.rb index ccf7bb493db..c9f102f6b19 100644 --- a/lib/version_check.rb +++ b/lib/version_check.rb @@ -5,16 +5,17 @@ require "base64" # This class is used to build image URL to # check if it is a new version for update class VersionCheck - def data + def self.data { version: Gitlab::VERSION } end - def url + def self.url encoded_data = Base64.urlsafe_encode64(data.to_json) + "#{host}?gitlab_info=#{encoded_data}" end - def host + def self.host 'https://version.gitlab.com/check.svg' end end diff --git a/spec/features/help_pages_spec.rb b/spec/features/help_pages_spec.rb index c29dfb01381..8572a13055c 100644 --- a/spec/features/help_pages_spec.rb +++ b/spec/features/help_pages_spec.rb @@ -54,9 +54,10 @@ describe 'Help Pages' do context 'in a production environment with version check enabled', :js do before do - allow(Rails.env).to receive(:production?) { true } stub_application_setting(version_check_enabled: true) - allow_any_instance_of(VersionCheck).to receive(:url) { '/version-check-url' } + + allow(Rails.env).to receive(:production?).and_return(true) + allow(VersionCheck).to receive(:url).and_return('/version-check-url') sign_in(create(:user)) visit help_path diff --git a/spec/helpers/version_check_helper_spec.rb b/spec/helpers/version_check_helper_spec.rb index 9d4e34abef5..bfec7ad4bba 100644 --- a/spec/helpers/version_check_helper_spec.rb +++ b/spec/helpers/version_check_helper_spec.rb @@ -13,21 +13,21 @@ describe VersionCheckHelper do before do allow(Rails.env).to receive(:production?) { true } allow(Gitlab::CurrentSettings.current_application_settings).to receive(:version_check_enabled) { true } - allow_any_instance_of(VersionCheck).to receive(:url) { 'https://version.host.com/check.svg?gitlab_info=xxx' } - - @image_tag = helper.version_status_badge + allow(VersionCheck).to receive(:url) { 'https://version.host.com/check.svg?gitlab_info=xxx' } end it 'should return an image tag' do - expect(@image_tag).to match(/^ Date: Fri, 14 Dec 2018 11:49:58 -0600 Subject: [PATCH 2/2] Resolve transient failure in Help page spec Sometimes due to a slow request to load the version check image, the placeholder image data was still being seen, resulting in this failure: expected "data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==" to end with "/version-check-url" Now we check the `data-src` attribute, which allows us to remove the `:js` metadata. This commit also removes a redundant test, which was just ensuring that the selector we were already using in the other test is visible. If this test were failing, the other one would always fail too, so it was pointless. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/55372 --- spec/features/help_pages_spec.rb | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/spec/features/help_pages_spec.rb b/spec/features/help_pages_spec.rb index 8572a13055c..e24b1f4349d 100644 --- a/spec/features/help_pages_spec.rb +++ b/spec/features/help_pages_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe 'Help Pages' do @@ -52,7 +54,7 @@ describe 'Help Pages' do end end - context 'in a production environment with version check enabled', :js do + context 'in a production environment with version check enabled' do before do stub_application_setting(version_check_enabled: true) @@ -64,12 +66,9 @@ describe 'Help Pages' do end it 'has a version check image' do - expect(find('.js-version-status-badge', visible: false)['src']).to end_with('/version-check-url') - end - - it 'hides the version check image if the image request fails' do - # We use '--load-images=yes' with poltergeist so the image fails to load - expect(page).to have_selector('.js-version-status-badge', visible: false) + # Check `data-src` due to lazy image loading + expect(find('.js-version-status-badge', visible: false)['data-src']) + .to end_with('/version-check-url') end end