From a29826241cabf5522fade68fb0dd32f92800c010 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Tue, 7 Apr 2020 17:01:41 +0800 Subject: [PATCH 1/5] Fix trial callout to use the correct plan namespaces.plan_id is unused and we should use the plan in the gitlab_subscriptions table --- ee/app/views/shared/billings/_trial_status.html.haml | 2 +- ee/spec/helpers/ee/user_callouts_helper_spec.rb | 5 ++++- .../views/shared/billings/_trial_status.html.haml_spec.rb | 8 +------- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/ee/app/views/shared/billings/_trial_status.html.haml b/ee/app/views/shared/billings/_trial_status.html.haml index a9b9b67b12b776..8384806d62c4a6 100644 --- a/ee/app/views/shared/billings/_trial_status.html.haml +++ b/ee/app/views/shared/billings/_trial_status.html.haml @@ -5,7 +5,7 @@ - if namespace.eligible_for_trial? = s_("BillingPlans|Learn more about each plan by reading our %{faq_link}, or start a free 30-day trial of GitLab.com Gold.").html_safe % { faq_link: faq_link } - elsif namespace.trial_active? - = s_("BillingPlans|Your GitLab.com %{plan} trial will expire after %{expiration_date}. You can retain access to the %{plan} features by upgrading below.").html_safe % { plan: namespace.plan&.title, expiration_date: namespace.trial_ends_on } + = s_("BillingPlans|Your GitLab.com %{plan} trial will expire after %{expiration_date}. You can retain access to the %{plan} features by upgrading below.").html_safe % { plan: namespace.gitlab_subscription&.plan_title, expiration_date: namespace.trial_ends_on } - elsif namespace.trial_expired? = s_("BillingPlans|Your GitLab.com trial expired on %{expiration_date}. You can restore access to the features at any time by upgrading below.").html_safe % { expiration_date: namespace.trial_ends_on } - else diff --git a/ee/spec/helpers/ee/user_callouts_helper_spec.rb b/ee/spec/helpers/ee/user_callouts_helper_spec.rb index dea61ce3e6b582..66b761055f5308 100644 --- a/ee/spec/helpers/ee/user_callouts_helper_spec.rb +++ b/ee/spec/helpers/ee/user_callouts_helper_spec.rb @@ -201,7 +201,10 @@ allow(helper).to receive(:show_gold_trial?) { show_gold_trial? } allow(helper).to receive(:user_default_dashboard?) { user_default_dashboard? } allow(helper).to receive(:has_some_namespaces_with_no_trials?) { has_some_namespaces_with_no_trials? } - namespace.update(plan: gold_plan) unless has_no_trial_or_gold_plan? + + unless has_no_trial_or_gold_plan? + create(:gitlab_subscription, hosted_plan: gold_plan, namespace: namespace) + end end it do diff --git a/ee/spec/views/shared/billings/_trial_status.html.haml_spec.rb b/ee/spec/views/shared/billings/_trial_status.html.haml_spec.rb index d042600cf1acc7..ac8d4227ffb5d5 100644 --- a/ee/spec/views/shared/billings/_trial_status.html.haml_spec.rb +++ b/ee/spec/views/shared/billings/_trial_status.html.haml_spec.rb @@ -6,18 +6,12 @@ include ApplicationHelper let_it_be(:group) { create(:group) } - let_it_be(:gitlab_subscription) { create(:gitlab_subscription, namespace: group) } let(:plan) { nil } let(:trial_ends_on) { nil } let(:trial) { false } before do - gitlab_subscription.update( - hosted_plan: plan, - trial_ends_on: trial_ends_on, - trial: trial - ) - group.update(plan: plan) + create(:gitlab_subscription, namespace: group, hosted_plan: plan, trial_ends_on: trial_ends_on, trial: trial) end context 'when not eligible for trial' do -- GitLab From d580d4e145a6460557787e3f9b49e3e979c5cb43 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 26 Mar 2020 16:42:27 +0000 Subject: [PATCH 2/5] Fix EE::User#has_paid_namespace? plan check --- ee/app/models/ee/user.rb | 5 +++-- ee/spec/models/user_spec.rb | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 588fc5afe3f71a..e242f60218b3c4 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -231,8 +231,9 @@ def any_namespace_without_trial? def has_paid_namespace? ::Namespace - .from("(#{namespace_union_for_reporter_developer_maintainer_owned(:plan_id)}) #{::Namespace.table_name}") - .where(plan_id: Plan.where(name: Plan::PAID_HOSTED_PLANS).select(:id)) + .from("(#{namespace_union_for_reporter_developer_maintainer_owned}) #{::Namespace.table_name}") + .include_gitlab_subscription + .where(gitlab_subscriptions: { hosted_plan_id: Plan.where(name: Plan::PAID_HOSTED_PLANS) }) .any? end diff --git a/ee/spec/models/user_spec.rb b/ee/spec/models/user_spec.rb index 2740d0bc5412cb..029ab095b390de 100644 --- a/ee/spec/models/user_spec.rb +++ b/ee/spec/models/user_spec.rb @@ -1029,4 +1029,39 @@ end end end + + describe '#has_paid_namespace?' do + let_it_be(:user) { create(:user) } + let_it_be(:gold_group) { create(:group_with_plan, plan: :gold_plan) } + let_it_be(:bronze_group) { create(:group_with_plan, plan: :bronze_plan) } + let_it_be(:free_group) { create(:group_with_plan, plan: :free_plan) } + let_it_be(:group_without_plan) { create(:group) } + + context 'when the user has Reporter or higher on at least one paid group' do + it 'returns true' do + gold_group.add_reporter(user) + bronze_group.add_guest(user) + + expect(user.has_paid_namespace?).to eq(true) + end + end + + context 'when the user is only a Guest on paid groups' do + it 'returns false' do + gold_group.add_guest(user) + bronze_group.add_guest(user) + free_group.add_owner(user) + + expect(user.has_paid_namespace?).to eq(false) + end + end + + context 'when the user is not a member of any groups with plans' do + it 'returns false' do + group_without_plan.add_owner(user) + + expect(user.has_paid_namespace?).to eq(false) + end + end + end end -- GitLab From d0effe700a115f6e3b647b4b949f6e55fdc9556f Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 30 Mar 2020 14:38:27 +0100 Subject: [PATCH 3/5] Replace any_namespace_with_gold? with has_paid_namespace? --- ee/app/helpers/ee/user_callouts_helper.rb | 2 +- ee/app/models/ee/user.rb | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/ee/app/helpers/ee/user_callouts_helper.rb b/ee/app/helpers/ee/user_callouts_helper.rb index b67adc6403ca4e..229e51f5c45d55 100644 --- a/ee/app/helpers/ee/user_callouts_helper.rb +++ b/ee/app/helpers/ee/user_callouts_helper.rb @@ -119,7 +119,7 @@ def show_gold_trial_suitable_env? end def has_no_trial_or_gold_plan?(user) - return false if user.any_namespace_with_gold? + return false if user.has_paid_namespace? !user.any_namespace_with_trial? end diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index e242f60218b3c4..eedab196af2fa3 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -237,14 +237,6 @@ def has_paid_namespace? .any? end - def any_namespace_with_gold? - ::Namespace - .includes(:plan) - .where("namespaces.id IN (#{namespace_union})") # rubocop:disable GitlabSecurity/SqlInjection - .where.not(plans: { id: nil }) - .any? - end - def managed_free_namespaces manageable_groups .left_joins(:gitlab_subscription) -- GitLab From 1a9fda7ae1d5e2c27cac8a94207325dab90e99d5 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 7 Apr 2020 11:52:15 +0100 Subject: [PATCH 4/5] Put dashboard gold trial behind feature flag This is behind a flag because it's a change in behaviour from the previous callout. We've fixed when it displays, but it might surprise some people. So to be cautious, we allow it to be disabled entirely. --- ee/app/helpers/ee/user_callouts_helper.rb | 5 +++-- .../helpers/ee/user_callouts_helper_spec.rb | 20 +++++++++++++++++-- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/ee/app/helpers/ee/user_callouts_helper.rb b/ee/app/helpers/ee/user_callouts_helper.rb index 229e51f5c45d55..f721d5191dcb79 100644 --- a/ee/app/helpers/ee/user_callouts_helper.rb +++ b/ee/app/helpers/ee/user_callouts_helper.rb @@ -52,7 +52,8 @@ def show_migrate_hashed_storage_warning? def render_dashboard_gold_trial(user) return unless show_gold_trial?(user, GOLD_TRIAL) && user_default_dashboard?(user) && - has_no_trial_or_gold_plan?(user) && + ::Feature.enabled?(:render_dashboard_gold_trial, default_enabled: true) && + has_no_trial_or_paid_plan?(user) && has_some_namespaces_with_no_trials?(user) render 'shared/gold_trial_callout_content' @@ -118,7 +119,7 @@ def show_gold_trial_suitable_env? ::Gitlab.com? && !::Gitlab::Database.read_only? end - def has_no_trial_or_gold_plan?(user) + def has_no_trial_or_paid_plan?(user) return false if user.has_paid_namespace? !user.any_namespace_with_trial? diff --git a/ee/spec/helpers/ee/user_callouts_helper_spec.rb b/ee/spec/helpers/ee/user_callouts_helper_spec.rb index 66b761055f5308..cbd8564dbf3053 100644 --- a/ee/spec/helpers/ee/user_callouts_helper_spec.rb +++ b/ee/spec/helpers/ee/user_callouts_helper_spec.rb @@ -177,7 +177,7 @@ let_it_be(:gold_plan) { create(:gold_plan) } let(:user) { namespace.owner } - where(:has_some_namespaces_with_no_trials?, :show_gold_trial?, :user_default_dashboard?, :has_no_trial_or_gold_plan?, :should_render?) do + where(:has_some_namespaces_with_no_trials?, :show_gold_trial?, :user_default_dashboard?, :has_no_trial_or_paid_plan?, :should_render?) do true | true | true | true | true true | true | true | false | false true | true | false | true | false @@ -202,7 +202,7 @@ allow(helper).to receive(:user_default_dashboard?) { user_default_dashboard? } allow(helper).to receive(:has_some_namespaces_with_no_trials?) { has_some_namespaces_with_no_trials? } - unless has_no_trial_or_gold_plan? + unless has_no_trial_or_paid_plan? create(:gitlab_subscription, hosted_plan: gold_plan, namespace: namespace) end end @@ -217,6 +217,22 @@ helper.render_dashboard_gold_trial(user) end end + + context 'when render_dashboard_gold_trial feature is disabled' do + before do + stub_feature_flags(render_dashboard_gold_trial: false) + + allow(helper).to receive(:show_gold_trial?).and_return(true) + allow(helper).to receive(:user_default_dashboard?).and_return(true) + allow(helper).to receive(:has_some_namespaces_with_no_trials?).and_return(true) + end + + it 'does not render' do + expect(helper).not_to receive(:render) + + helper.render_dashboard_gold_trial(user) + end + end end describe '#render_billings_gold_trial' do -- GitLab From 7f48e84d384ab8e776605027ef97c5f287d0f224 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Wed, 8 Apr 2020 18:21:37 +0800 Subject: [PATCH 5/5] Fix logic for displaying gold trial callout This makes it the same with previous behavior where a user that is a Maintainer / Developer / Reporter of a paid plan should still see the Gold trial banner. --- ee/app/helpers/ee/user_callouts_helper.rb | 2 +- ee/app/models/ee/user.rb | 16 ++++-- ee/spec/models/user_spec.rb | 65 +++++++++++++++++------ 3 files changed, 61 insertions(+), 22 deletions(-) diff --git a/ee/app/helpers/ee/user_callouts_helper.rb b/ee/app/helpers/ee/user_callouts_helper.rb index f721d5191dcb79..ff8c704d9edf25 100644 --- a/ee/app/helpers/ee/user_callouts_helper.rb +++ b/ee/app/helpers/ee/user_callouts_helper.rb @@ -120,7 +120,7 @@ def show_gold_trial_suitable_env? end def has_no_trial_or_paid_plan?(user) - return false if user.has_paid_namespace? + return false if user.owns_paid_namespace? !user.any_namespace_with_trial? end diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index eedab196af2fa3..9b0cfb7c325f28 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -217,14 +217,14 @@ def group_view def any_namespace_with_trial? ::Namespace - .from("(#{namespace_union(:trial_ends_on)}) #{::Namespace.table_name}") + .from("(#{namespace_union_for_owned(:trial_ends_on)}) #{::Namespace.table_name}") .where('trial_ends_on > ?', Time.now.utc) .any? end def any_namespace_without_trial? ::Namespace - .from("(#{namespace_union(:trial_ends_on)}) #{::Namespace.table_name}") + .from("(#{namespace_union_for_owned(:trial_ends_on)}) #{::Namespace.table_name}") .where(trial_ends_on: nil) .any? end @@ -233,7 +233,15 @@ def has_paid_namespace? ::Namespace .from("(#{namespace_union_for_reporter_developer_maintainer_owned}) #{::Namespace.table_name}") .include_gitlab_subscription - .where(gitlab_subscriptions: { hosted_plan_id: Plan.where(name: Plan::PAID_HOSTED_PLANS) }) + .where(gitlab_subscriptions: { hosted_plan: Plan.where(name: Plan::PAID_HOSTED_PLANS) }) + .any? + end + + def owns_paid_namespace? + ::Namespace + .from("(#{namespace_union_for_owned}) #{::Namespace.table_name}") + .include_gitlab_subscription + .where(gitlab_subscriptions: { hosted_plan: Plan.where(name: Plan::PAID_HOSTED_PLANS) }) .any? end @@ -335,7 +343,7 @@ def password_required?(*) private - def namespace_union(select = :id) + def namespace_union_for_owned(select = :id) ::Gitlab::SQL::Union.new([ ::Namespace.select(select).where(type: nil, owner: self), owned_groups.select(select).where(parent_id: nil) diff --git a/ee/spec/models/user_spec.rb b/ee/spec/models/user_spec.rb index 029ab095b390de..56f7aff1e88361 100644 --- a/ee/spec/models/user_spec.rb +++ b/ee/spec/models/user_spec.rb @@ -1030,37 +1030,68 @@ end end - describe '#has_paid_namespace?' do + context 'paid namespaces' do let_it_be(:user) { create(:user) } let_it_be(:gold_group) { create(:group_with_plan, plan: :gold_plan) } let_it_be(:bronze_group) { create(:group_with_plan, plan: :bronze_plan) } let_it_be(:free_group) { create(:group_with_plan, plan: :free_plan) } let_it_be(:group_without_plan) { create(:group) } - context 'when the user has Reporter or higher on at least one paid group' do - it 'returns true' do - gold_group.add_reporter(user) - bronze_group.add_guest(user) + describe '#has_paid_namespace?' do + context 'when the user has Reporter or higher on at least one paid group' do + it 'returns true' do + gold_group.add_reporter(user) + bronze_group.add_guest(user) - expect(user.has_paid_namespace?).to eq(true) + expect(user.has_paid_namespace?).to eq(true) + end end - end - context 'when the user is only a Guest on paid groups' do - it 'returns false' do - gold_group.add_guest(user) - bronze_group.add_guest(user) - free_group.add_owner(user) + context 'when the user is only a Guest on paid groups' do + it 'returns false' do + gold_group.add_guest(user) + bronze_group.add_guest(user) + free_group.add_owner(user) + + expect(user.has_paid_namespace?).to eq(false) + end + end + + context 'when the user is not a member of any groups with plans' do + it 'returns false' do + group_without_plan.add_owner(user) - expect(user.has_paid_namespace?).to eq(false) + expect(user.has_paid_namespace?).to eq(false) + end end end - context 'when the user is not a member of any groups with plans' do - it 'returns false' do - group_without_plan.add_owner(user) + describe '#owns_paid_namespace?' do + context 'when the user is an owner of at least one paid group' do + it 'returns true' do + gold_group.add_owner(user) + bronze_group.add_owner(user) + + expect(user.owns_paid_namespace?).to eq(true) + end + end + + context 'when the user is only a Maintainer on paid groups' do + it 'returns false' do + gold_group.add_maintainer(user) + bronze_group.add_maintainer(user) + free_group.add_owner(user) - expect(user.has_paid_namespace?).to eq(false) + expect(user.owns_paid_namespace?).to eq(false) + end + end + + context 'when the user is not a member of any groups with plans' do + it 'returns false' do + group_without_plan.add_owner(user) + + expect(user.owns_paid_namespace?).to eq(false) + end end end end -- GitLab