From 2d729b43e11382f1eac343562cc0597cc19fc26f Mon Sep 17 00:00:00 2001 From: Doug Stull Date: Thu, 17 Jul 2025 16:42:07 -0400 Subject: [PATCH 1/2] Add onboarding for new user based onboarding to users - add db column - wire up the starting/completion logic Changelog: other --- ...3537_add_onboarding_experience_to_users.rb | 12 ++ db/schema_migrations/20250717203537 | 1 + db/structure.sql | 1 + .../projects/get_started_controller.rb | 4 +- .../projects/learn_gitlab_controller.rb | 4 +- .../onboarding/finish_user_service.rb | 27 ++++ .../onboarding/status_create_service.rb | 1 + .../projects/learn_gitlab_controller_spec.rb | 10 +- .../projects/get_started_controller_spec.rb | 26 ++-- .../onboarding/finish_user_service_spec.rb | 120 ++++++++++++++++++ .../onboarding/status_create_service_spec.rb | 16 +++ 11 files changed, 202 insertions(+), 20 deletions(-) create mode 100644 db/migrate/20250717203537_add_onboarding_experience_to_users.rb create mode 100644 db/schema_migrations/20250717203537 create mode 100644 ee/app/services/onboarding/finish_user_service.rb create mode 100644 ee/spec/services/onboarding/finish_user_service_spec.rb diff --git a/db/migrate/20250717203537_add_onboarding_experience_to_users.rb b/db/migrate/20250717203537_add_onboarding_experience_to_users.rb new file mode 100644 index 00000000000000..5665370385684f --- /dev/null +++ b/db/migrate/20250717203537_add_onboarding_experience_to_users.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class AddOnboardingExperienceToUsers < Gitlab::Database::Migration[2.3] + milestone '18.3' + + def change + # rubocop:disable Migration/PreventAddingColumns -- this will be hit on almost every page load for a logged-in user and having here will reduce db queries + # See more info in https://gitlab.com/gitlab-org/gitlab/-/issues/547922 + add_column :users, :onboarding_experience, :boolean, default: false, null: false + # rubocop:enable Migration/PreventAddingColumns + end +end diff --git a/db/schema_migrations/20250717203537 b/db/schema_migrations/20250717203537 new file mode 100644 index 00000000000000..7152aa36b58f6c --- /dev/null +++ b/db/schema_migrations/20250717203537 @@ -0,0 +1 @@ +653f22ab9571a989162327d3133e32c05b01176864b7bfc502d3bff907162c5e \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 42f730a4f5122b..bd727ffb265447 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -438,6 +438,7 @@ CREATE TABLE users ( color_mode_id smallint DEFAULT 1 NOT NULL, composite_identity_enforced boolean DEFAULT false NOT NULL, organization_id bigint DEFAULT 1 NOT NULL, + onboarding_experience boolean DEFAULT false NOT NULL, CONSTRAINT check_061f6f1c91 CHECK ((project_view IS NOT NULL)), CONSTRAINT check_0dd5948e38 CHECK ((user_type IS NOT NULL)), CONSTRAINT check_3a60c18afc CHECK ((hide_no_password IS NOT NULL)), diff --git a/ee/app/controllers/projects/get_started_controller.rb b/ee/app/controllers/projects/get_started_controller.rb index 5c24f4f21caddb..4f8fce6a575839 100644 --- a/ee/app/controllers/projects/get_started_controller.rb +++ b/ee/app/controllers/projects/get_started_controller.rb @@ -20,7 +20,9 @@ def show end def end_tutorial - if onboarding_progress.update(ended_at: Time.current) + result = Onboarding::FinishUserService.new(current_user, onboarding_progress).execute + + if result.success? redirect_to project_path(project) flash[:success] = s_("GetStarted|You've ended the tutorial.") else diff --git a/ee/app/controllers/projects/learn_gitlab_controller.rb b/ee/app/controllers/projects/learn_gitlab_controller.rb index 0bab359bf73e82..7113665f5e6ad9 100644 --- a/ee/app/controllers/projects/learn_gitlab_controller.rb +++ b/ee/app/controllers/projects/learn_gitlab_controller.rb @@ -18,7 +18,9 @@ def show end def end_tutorial - if onboarding_progress.update(ended_at: Time.current) + result = Onboarding::FinishUserService.new(current_user, onboarding_progress).execute + + if result.success? redirect_to project_path(@project) flash[:success] = s_("LearnGitlab|You've ended the Learn GitLab tutorial.") else diff --git a/ee/app/services/onboarding/finish_user_service.rb b/ee/app/services/onboarding/finish_user_service.rb new file mode 100644 index 00000000000000..0ff0b49e15e8f2 --- /dev/null +++ b/ee/app/services/onboarding/finish_user_service.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Onboarding + class FinishUserService + def initialize(user, onboarding_progress) + @user = user + @onboarding_progress = onboarding_progress + end + + def execute + User.transaction do + onboarding_progress.update!(ended_at: Time.current) + user.update!(onboarding_experience: false) + + ServiceResponse.success + end + rescue StandardError => error + Gitlab::ErrorTracking.track_exception(error) + + ServiceResponse.error(message: 'An error occurred while finishing user onboarding') + end + + private + + attr_reader :user, :onboarding_progress + end +end diff --git a/ee/app/services/onboarding/status_create_service.rb b/ee/app/services/onboarding/status_create_service.rb index 89a970a97cfca9..07af9743bf61ad 100644 --- a/ee/app/services/onboarding/status_create_service.rb +++ b/ee/app/services/onboarding/status_create_service.rb @@ -41,6 +41,7 @@ def payload def user_attributes attrs = { onboarding_in_progress: true, + onboarding_experience: true, onboarding_status_step_url: step_url, onboarding_status_initial_registration_type: registration_type, onboarding_status_registration_type: registration_type, diff --git a/ee/spec/controllers/projects/learn_gitlab_controller_spec.rb b/ee/spec/controllers/projects/learn_gitlab_controller_spec.rb index cf2dd9eafcc2f0..bed8d363821fa4 100644 --- a/ee/spec/controllers/projects/learn_gitlab_controller_spec.rb +++ b/ee/spec/controllers/projects/learn_gitlab_controller_spec.rb @@ -61,7 +61,7 @@ sign_in(user) end - context 'when update is successful' do + context 'when it is successful' do it 'updates the onboarding progress ended value to be set' do get_end_tutorial @@ -71,14 +71,14 @@ end end - context 'when update has an error' do + context 'when there is an error' do before do allow(controller).to receive(:onboarding_progress).and_return(onboarding_progress) - allow(onboarding_progress).to receive(:update).and_return(false) + allow(onboarding_progress).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) end - it 'does not update the onboarding progress and shows an error message' do - expect { get_end_tutorial }.not_to change { onboarding_progress.reload.ended_at } + it 'shows an error message' do + get_end_tutorial expect(response).not_to redirect_to(project_path(project)) expect(flash[:danger]) diff --git a/ee/spec/requests/projects/get_started_controller_spec.rb b/ee/spec/requests/projects/get_started_controller_spec.rb index 5600fb296b3f8b..38d64a31e6a0e2 100644 --- a/ee/spec/requests/projects/get_started_controller_spec.rb +++ b/ee/spec/requests/projects/get_started_controller_spec.rb @@ -93,7 +93,7 @@ it { is_expected.to have_gitlab_http_status(:not_found) } end - context 'when update is successful' do + context 'when it is successful' do it 'sets onboarding progress ended value' do get_end_tutorial @@ -101,22 +101,22 @@ expect(flash[:success]).to eql("You've ended the tutorial.") expect(onboarding_progress.ended_at).to be_present end + end - context 'when update has an error' do - before do - allow(onboarding_progress).to receive(:update).and_return(false) - allow_next_instance_of(described_class) do |instance| - allow(instance).to receive(:onboarding_progress).and_return(onboarding_progress) - end + context 'when there is an error' do + before do + allow(onboarding_progress).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) + allow_next_instance_of(described_class) do |instance| + allow(instance).to receive(:onboarding_progress).and_return(onboarding_progress) end + end - it 'does not update the onboarding progress and shows an error message' do - expect { get_end_tutorial }.not_to change { onboarding_progress.reload.ended_at } + it 'shows an error message' do + get_end_tutorial - expect(response).not_to redirect_to(project_path(project)) - expect(flash[:danger]) - .to eql("There was a problem trying to end the tutorial. Please try again.") - end + expect(response).not_to redirect_to(project_path(project)) + expect(flash[:danger]) + .to eql("There was a problem trying to end the tutorial. Please try again.") end end end diff --git a/ee/spec/services/onboarding/finish_user_service_spec.rb b/ee/spec/services/onboarding/finish_user_service_spec.rb new file mode 100644 index 00000000000000..bea836028e6069 --- /dev/null +++ b/ee/spec/services/onboarding/finish_user_service_spec.rb @@ -0,0 +1,120 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Onboarding::FinishUserService, feature_category: :onboarding do + let_it_be(:user) { create(:user, onboarding_experience: true) } + let_it_be(:onboarding_progress) { create(:onboarding_progress) } + + subject(:finish_onboarding) { described_class.new(user, onboarding_progress).execute } + + describe '#execute' do + context 'when the operation is successful' do + it 'updates onboarding_progress ended_at timestamp' do + freeze_time do + expect { finish_onboarding }.to change { onboarding_progress.reload.ended_at }.from(nil).to(Time.current) + end + end + + it 'sets user onboarding_experience to false' do + expect { finish_onboarding }.to change { user.reload.onboarding_experience }.from(true).to(false) + end + + it 'returns a success service response' do + result = finish_onboarding + + expect(result).to be_success + end + + it 'performs both updates within a transaction' do + expect(User).to receive(:transaction).and_call_original + + finish_onboarding + end + end + + context 'when onboarding_progress update fails' do + before do + allow(onboarding_progress).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) + end + + it 'does not update the user' do + expect { finish_onboarding }.not_to change { user.reload.onboarding_experience } + end + + it 'tracks the exception' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with(an_instance_of(ActiveRecord::RecordInvalid)) + + finish_onboarding + end + + it 'returns an error service response' do + result = finish_onboarding + + expect(result).to be_error + expect(result.message).to eq('An error occurred while finishing user onboarding') + end + end + + context 'when user update fails' do + before do + allow(user).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) + end + + it 'does not update the onboarding_progress due to transaction rollback' do + expect { finish_onboarding }.not_to change { onboarding_progress.reload.ended_at } + end + + it 'tracks the exception' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with(an_instance_of(ActiveRecord::RecordInvalid)) + + finish_onboarding + end + + it 'returns an error service response' do + result = finish_onboarding + + expect(result).to be_error + expect(result.message).to eq('An error occurred while finishing user onboarding') + end + end + + context 'when an unexpected error occurs' do + before do + allow(onboarding_progress).to receive(:update!).and_raise(StandardError, 'Unexpected error') + end + + it 'tracks the exception' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with(an_instance_of(StandardError)) + + finish_onboarding + end + + it 'returns an error service response with generic message' do + result = finish_onboarding + + expect(result).to be_error + expect(result.message).to eq('An error occurred while finishing user onboarding') + end + end + end + + context 'with transaction behavior' do + it 'rolls back all changes when an error occurs' do + allow(user).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) + + expect do + finish_onboarding + end.to not_change { onboarding_progress.reload.ended_at }.and not_change { user.reload.onboarding_experience } + end + + it 'commits all changes when successful' do + freeze_time do + finish_onboarding + + expect(onboarding_progress.reload.ended_at).to eq(Time.current) + expect(user.reload.onboarding_experience).to be_falsey + end + end + end +end diff --git a/ee/spec/services/onboarding/status_create_service_spec.rb b/ee/spec/services/onboarding/status_create_service_spec.rb index 9ece3772ad170d..fa8d2414b14a9a 100644 --- a/ee/spec/services/onboarding/status_create_service_spec.rb +++ b/ee/spec/services/onboarding/status_create_service_spec.rb @@ -31,6 +31,7 @@ it 'places the user into onboarding' do expect(execute[:user]).to be_onboarding_in_progress + expect(execute[:user]).to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_success end @@ -89,6 +90,7 @@ it 'updates onboarding_status_step_url' do expect(execute[:user].onboarding_status.symbolize_keys).to eq(expected_onboarding_status) expect(execute[:user]).to be_onboarding_in_progress + expect(execute[:user]).to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_success end @@ -111,6 +113,7 @@ it 'merges new data into onboarding_status and does not delete it' do expect(execute[:user]).to be_onboarding_in_progress + expect(execute[:user]).to be_onboarding_experience expect(execute[:user].onboarding_status.symbolize_keys).to eq(onboarding_status.merge(email_opt_in: true)) end end @@ -166,6 +169,7 @@ it 'does not update the onboarding_status_step_url' do expect(execute[:user]).not_to be_onboarding_in_progress + expect(execute[:user]).not_to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_error expect(execute[:user].onboarding_status).to eq({}) @@ -178,6 +182,7 @@ it 'does not enter the user into onboarding' do expect(execute[:user]).not_to be_onboarding_in_progress + expect(execute[:user]).not_to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_error end @@ -196,6 +201,7 @@ context 'with a verified domain' do it 'does not enter the user into onboarding' do expect(execute[:user]).not_to be_onboarding_in_progress + expect(execute[:user]).not_to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_error end @@ -208,6 +214,7 @@ it 'places the user into onboarding' do expect(execute[:user]).to be_onboarding_in_progress + expect(execute[:user]).to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_success end @@ -220,6 +227,7 @@ it 'places the user into onboarding' do expect(execute[:user]).to be_onboarding_in_progress + expect(execute[:user]).to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_success end @@ -232,6 +240,7 @@ it 'places the user into onboarding' do expect(execute[:user]).to be_onboarding_in_progress + expect(execute[:user]).to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_success end @@ -244,6 +253,7 @@ it 'places the user into onboarding' do expect(execute[:user]).to be_onboarding_in_progress + expect(execute[:user]).to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_success end @@ -256,6 +266,7 @@ it 'places the user into onboarding' do expect(execute[:user]).to be_onboarding_in_progress + expect(execute[:user]).to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_success end @@ -268,6 +279,7 @@ it 'does not enter the user into onboarding' do expect(execute[:user]).not_to be_onboarding_in_progress + expect(execute[:user]).not_to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_error end @@ -280,6 +292,7 @@ it 'does not enter the user into onboarding' do expect(execute[:user]).not_to be_onboarding_in_progress + expect(execute[:user]).not_to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_error end @@ -292,6 +305,7 @@ it 'does not enter the user into onboarding' do expect(execute[:user]).not_to be_onboarding_in_progress + expect(execute[:user]).not_to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_error end @@ -305,6 +319,7 @@ it 'does not enter the user into onboarding' do expect(execute[:user]).not_to be_onboarding_in_progress + expect(execute[:user]).not_to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_error end @@ -321,6 +336,7 @@ it 'does not update onboarding_in_progress' do expect(execute[:user]).not_to be_onboarding_in_progress + expect(execute[:user]).not_to be_onboarding_experience expect(execute).to be_a(ServiceResponse) expect(execute).to be_error expect(execute[:user].onboarding_status).to eq({}) -- GitLab From a7692a09f9e107a9516bdd5053066745034a1fb4 Mon Sep 17 00:00:00 2001 From: Doug Stull Date: Fri, 18 Jul 2025 12:47:52 -0400 Subject: [PATCH 2/2] Pivot to use user_details --- ...3537_add_onboarding_experience_to_user_details.rb | 9 +++++++++ ...50717203537_add_onboarding_experience_to_users.rb | 12 ------------ db/structure.sql | 2 +- ee/app/models/ee/user.rb | 5 ++++- ee/spec/models/ee/user_spec.rb | 4 ++++ 5 files changed, 18 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20250717203537_add_onboarding_experience_to_user_details.rb delete mode 100644 db/migrate/20250717203537_add_onboarding_experience_to_users.rb diff --git a/db/migrate/20250717203537_add_onboarding_experience_to_user_details.rb b/db/migrate/20250717203537_add_onboarding_experience_to_user_details.rb new file mode 100644 index 00000000000000..f804da8f6a52b4 --- /dev/null +++ b/db/migrate/20250717203537_add_onboarding_experience_to_user_details.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddOnboardingExperienceToUserDetails < Gitlab::Database::Migration[2.3] + milestone '18.3' + + def change + add_column :user_details, :onboarding_experience, :boolean, default: false, null: false + end +end diff --git a/db/migrate/20250717203537_add_onboarding_experience_to_users.rb b/db/migrate/20250717203537_add_onboarding_experience_to_users.rb deleted file mode 100644 index 5665370385684f..00000000000000 --- a/db/migrate/20250717203537_add_onboarding_experience_to_users.rb +++ /dev/null @@ -1,12 +0,0 @@ -# frozen_string_literal: true - -class AddOnboardingExperienceToUsers < Gitlab::Database::Migration[2.3] - milestone '18.3' - - def change - # rubocop:disable Migration/PreventAddingColumns -- this will be hit on almost every page load for a logged-in user and having here will reduce db queries - # See more info in https://gitlab.com/gitlab-org/gitlab/-/issues/547922 - add_column :users, :onboarding_experience, :boolean, default: false, null: false - # rubocop:enable Migration/PreventAddingColumns - end -end diff --git a/db/structure.sql b/db/structure.sql index bd727ffb265447..b3e431b3e2613f 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -438,7 +438,6 @@ CREATE TABLE users ( color_mode_id smallint DEFAULT 1 NOT NULL, composite_identity_enforced boolean DEFAULT false NOT NULL, organization_id bigint DEFAULT 1 NOT NULL, - onboarding_experience boolean DEFAULT false NOT NULL, CONSTRAINT check_061f6f1c91 CHECK ((project_view IS NOT NULL)), CONSTRAINT check_0dd5948e38 CHECK ((user_type IS NOT NULL)), CONSTRAINT check_3a60c18afc CHECK ((hide_no_password IS NOT NULL)), @@ -24884,6 +24883,7 @@ CREATE TABLE user_details ( email_otp_last_sent_to text, email_otp_last_sent_at timestamp with time zone, email_otp_required_after timestamp with time zone, + onboarding_experience boolean DEFAULT false NOT NULL, CONSTRAINT check_18a53381cd CHECK ((char_length(bluesky) <= 256)), CONSTRAINT check_245664af82 CHECK ((char_length(webauthn_xid) <= 100)), CONSTRAINT check_444573ee52 CHECK ((char_length(skype) <= 500)), diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index e7243a29cc498a..0dbe9afc7f2e8a 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -60,11 +60,14 @@ module User :onboarding_status_glm_content, :onboarding_status_glm_content=, :onboarding_status_glm_source, :onboarding_status_glm_source=, :onboarding_status_version, :onboarding_status_version=, - :onboarding_status_joining_project, :onboarding_status_joining_project=, :onboarding_status_role, :onboarding_status_role=, :onboarding_status_role_name, + :onboarding_status_joining_project, :onboarding_status_joining_project=, :onboarding_status_role, + :onboarding_status_role=, :onboarding_status_role_name, :enterprise_group, :enterprise_group=, :enterprise_group_id, :enterprise_group_id=, :enterprise_group_associated_at, :enterprise_group_associated_at=, to: :user_detail, allow_nil: true + delegate :onboarding_experience, :onboarding_experience=, :onboarding_experience?, to: :user_detail + delegate :enabled_zoekt?, :enabled_zoekt, :enabled_zoekt=, to: :user_preference diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index f63ca956854729..212f38fa5870cc 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -34,6 +34,10 @@ is_expected.to delegate_method(:enterprise_group_associated_at=).to(:user_detail).with_arguments(Time.current) end + it { is_expected.to delegate_method(:onboarding_experience).to(:user_detail) } + it { is_expected.to delegate_method(:onboarding_experience?).to(:user_detail) } + it { is_expected.to delegate_method(:onboarding_experience=).to(:user_detail).with_arguments(true) } + it { is_expected.to delegate_method(:onboarding_status_step_url=).to(:user_detail).with_arguments('url').allow_nil } it { is_expected.to delegate_method(:onboarding_status_step_url).to(:user_detail).allow_nil } -- GitLab