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 0000000000000000000000000000000000000000..f804da8f6a52b40ff92529ed8598d02b7a2aadf2 --- /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/schema_migrations/20250717203537 b/db/schema_migrations/20250717203537 new file mode 100644 index 0000000000000000000000000000000000000000..7152aa36b58f6ccb32a3b1870f59f5c25a73de2e --- /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 42f730a4f5122b67b028a7ebe1808289a8000e05..b3e431b3e2613f7d54a789b9ce619131add3f0bc 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -24883,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/controllers/projects/get_started_controller.rb b/ee/app/controllers/projects/get_started_controller.rb index 5c24f4f21caddb477116b1caf51d19c157ec3ecc..4f8fce6a5758395d56150f469ce22df37a614c9d 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 0bab359bf73e8258543457b99b08ec6df64eba5c..7113665f5e6ad983b4f72ef35396074086de4506 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/models/ee/user.rb b/ee/app/models/ee/user.rb index e7243a29cc498a39858c6853a3dfeb96b340a417..0dbe9afc7f2e8a2a3f0efb989003e2ae40795619 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/app/services/onboarding/finish_user_service.rb b/ee/app/services/onboarding/finish_user_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..0ff0b49e15e8f210086c29c11768dd901597c5b9 --- /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 89a970a97cfca96d8e1378afd83a7b3bce724000..07af9743bf61ad18becb027897c4e15a7e0333bd 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 cf2dd9eafcc2f00755ea458e2229cca2bceba461..bed8d363821fa4e477667c214716813645233baa 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/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index f63ca956854729853ec99e1732fbcc5ed8c05f23..212f38fa5870cc06174ddd7044b189386c525811 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 } diff --git a/ee/spec/requests/projects/get_started_controller_spec.rb b/ee/spec/requests/projects/get_started_controller_spec.rb index 5600fb296b3f8ba21ea154d5fd179570241f0002..38d64a31e6a0e2be2c6b73e047ac40d76806bbe3 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 0000000000000000000000000000000000000000..bea836028e6069707c4202d3176d6f8d2c1e437c --- /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 9ece3772ad170ddc4c58b84203f8224525a9c27e..fa8d2414b14a9aea07750c4ff819419b581dbc19 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({})