Resolve Flaky test: spec/features/invites_spec.rb
What does this MR do and why?
Resolve the flaky test covered in #414971 (closed).
The root cause of the flaky example is believed to be out of order requests. The correct order of request should be:
- On sign up page,
username_validator
does GET tousers/user5/exists
- Submit the form, POST to
/users
, redirect to/welcome
- GET
/welcom
, fill out the form and submit - PATCH
/welcome
, redirect topath_for_signed_in_user
which should be group activity path.
However, due to debouncing the username validation, GET users/user5/exists
can be out of order and in one of the failure logs, it happened after GET /welcom
. And also due to a before method in ApplicationController called requried_signup_info
which was removed just recently, the app will redirect the user back to users/user5/exists
after submitting the welcome form:
- post to
/users
(the actual sign-up click) - get to
/welcom
- get to
users/user5/exists
(this is whererequired_signup_info
store request full path to session, then redirect back to welcome) - patch to
/welcom
, redirect topath_for_signed_in_user
which callsstored_location_for(user)
first, hence back touser_exists_path
.
So to conclude, an expect
is added in the spec to ensure only submit signup form after username validation is done. And also since required_signup_info
is removed, these two examples should be good to unquarantine.
How to set up and validate locally
Reproduce the failure locally
- Apply this diff and run the two examples
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index ef489a65575c..7e55978b4932 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -38,6 +38,7 @@ class ApplicationController < ActionController::Base
before_action :active_user_check, unless: :devise_controller?
before_action :set_usage_stats_consent_flag
before_action :check_impersonation_availability
+ before_action :required_signup_info
# Make sure the `auth_user` is memoized so it can be logged, we do this after
# all other before filters that could have set the user.
@@ -561,6 +562,15 @@ def disable_usage_stats
.execute
end
+ def required_signup_info
+ return unless current_user
+ return unless current_user.role_required?
+
+ store_location_for :user, request.fullpath
+
+ redirect_to users_sign_up_welcome_path
+ end
+
def allow_gitaly_ref_name_caching
::Gitlab::GitalyClient.allow_ref_name_caching do
yield
diff --git a/app/controllers/registrations/welcome_controller.rb b/app/controllers/registrations/welcome_controller.rb
index 6401b1070c07..4e15dc3417d6 100644
--- a/app/controllers/registrations/welcome_controller.rb
+++ b/app/controllers/registrations/welcome_controller.rb
@@ -8,7 +8,7 @@ class WelcomeController < ApplicationController
include ::Gitlab::Utils::StrongMemoize
layout 'minimal'
- skip_before_action :check_two_factor_requirement
+ skip_before_action :check_two_factor_requirement, :required_signup_info
helper_method :welcome_update_params
helper_method :onboarding_status
diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb
index a8b5ca81f492..81f53958822c 100644
--- a/app/controllers/registrations_controller.rb
+++ b/app/controllers/registrations_controller.rb
@@ -48,6 +48,7 @@ def create
accept_pending_invitations if new_user.persisted?
persist_accepted_terms_if_required(new_user)
+ set_role_required(new_user)
send_custom_confirmation_instructions
track_weak_password_error(new_user, self.class.name, 'create')
@@ -90,6 +91,10 @@ def persist_accepted_terms_if_required(new_user)
Users::RespondToTermsService.new(new_user, terms).execute(accepted: true)
end
+ def set_role_required(new_user)
+ new_user.set_role_required! if new_user.persisted?
+ end
+
def destroy_confirmation_valid?
if current_user.confirm_deletion_with_password?
current_user.valid_password?(params[:password])
diff --git a/app/models/user.rb b/app/models/user.rb
index 507a58dfd490..05bde40040db 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -2175,6 +2175,14 @@ def account_age_in_days
(Date.current - created_at.to_date).to_i
end
+ def role_required?
+ role_before_type_cast == 99
+ end
+
+ def set_role_required!
+ update_column(:role, 99)
+ end
+
def webhook_email
public_email.presence || _('[REDACTED]')
end
diff --git a/spec/features/invites_spec.rb b/spec/features/invites_spec.rb
index 939de930df8d..c0d3b07f3a3a 100644
--- a/spec/features/invites_spec.rb
+++ b/spec/features/invites_spec.rb
@@ -199,6 +199,7 @@ def fill_in_welcome_form
it 'signs up and redirects to the activity page' do
fill_in_sign_up_form(new_user)
+ visit user_exists_path(new_user)
fill_in_welcome_form
expect(page).to have_current_path(activity_group_path(group), ignore_query: true)
@@ -269,6 +270,7 @@ def fill_in_welcome_form
it 'signs up and redirects to the group activity page' do
fill_in_sign_up_form(new_user)
+ visit user_exists_path(new_user)
fill_in_welcome_form
expect(page).to have_current_path(activity_group_path(group), ignore_query: true)
Edited by Roy Liu