Skip to content

LG-9973: Implement new default Sign In page from A/B test results #8542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aduth merged 4 commits into from
Jun 9, 2023
Merged

LG-9973: Implement new default Sign In page from A/B test results #8542

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26a4dd9
LG-9973: Implement new default Sign In page from A/B test results
aduth Jun 6, 2023
d1f3066
Improve mobile appearance of tab navigation
aduth Jun 7, 2023
88d7f68
Add upstream reference
aduth Jun 7, 2023
dd5b7df
Incorporate upstream button styling improvements
aduth Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 0 additions & 25 deletions app/assets/stylesheets/utilities/_typography.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,28 +99,3 @@ h6,
.h6 {
@extend %h6;
}

.separator-text {
display: flex;
align-items: center;
text-align: center;
font-size: 1.125rem;
margin-bottom: 16px;

&::before,
&::after {
content: '';
display: block;
border-bottom: 1px solid color('primary-light');
flex-grow: 1;
min-width: 2rem;
}

&::before {
margin-right: 1rem;
}

&::after {
margin-left: 1rem;
}
}
5 changes: 0 additions & 5 deletions app/controllers/concerns/sign_in_a_b_test_concern.rb
View file
Open in desktop

This file was deleted.

6 changes: 1 addition & 5 deletions app/controllers/sign_up/completions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
module SignUp
class CompletionsController < ApplicationController
include SecureHeadersConcern
include SignInABTestConcern

before_action :confirm_two_factor_authenticated
before_action :verify_confirmed, if: :ial2?
Expand Down Expand Up @@ -93,10 +92,7 @@ def analytics_attributes(page_occurence)
end

def track_completion_event(last_page)
analytics.user_registration_complete(
**analytics_attributes(last_page),
sign_in_a_b_test_bucket:,
)
analytics.user_registration_complete(**analytics_attributes(last_page))
end

def pii
Expand Down
7 changes: 1 addition & 6 deletions app/controllers/sign_up/registrations_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ module SignUp
class RegistrationsController < ApplicationController
include PhoneConfirmation
include ApplicationHelper # for ial2_requested?
include SignInABTestConcern

before_action :confirm_two_factor_authenticated, only: [:destroy_confirm]
before_action :require_no_authentication
Expand All @@ -15,11 +14,7 @@ def new
analytics: analytics,
attempts_tracker: irs_attempts_api_tracker,
)
@sign_in_a_b_test_bucket = sign_in_a_b_test_bucket
analytics.user_registration_enter_email_visit(
sign_in_a_b_test_bucket: @sign_in_a_b_test_bucket,
from_sign_in: params[:source] == 'sign_in',
)
analytics.user_registration_enter_email_visit
render :new, formats: :html
end

Expand Down
4 changes: 0 additions & 4 deletions app/controllers/users/sessions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ class SessionsController < Devise::SessionsController
include RememberDeviceConcern
include Ial2ProfileConcern
include Api::CsrfTokenConcern
include SignInABTestConcern

rescue_from ActionController::InvalidAuthenticityToken, with: :redirect_to_signin

Expand All @@ -24,12 +23,9 @@ def new

@ial = sp_session_ial
@browser_is_ie11 = browser_is_ie11?
@is_on_home_page = true
@sign_in_a_b_test_bucket = sign_in_a_b_test_bucket
analytics.sign_in_page_visit(
flash: flash[:alert],
stored_location: session['user_return_to'],
sign_in_a_b_test_bucket: @sign_in_a_b_test_bucket,
)
super
end
Expand Down
18 changes: 3 additions & 15 deletions app/services/analytics_events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3366,14 +3366,12 @@ def show_password_button_clicked(path:, **extra)

# @param [String] flash
# @param [String] stored_location
# @param [String] sign_in_a_b_test_bucket
# tracks when a user visits the sign in page
def sign_in_page_visit(flash:, stored_location:, sign_in_a_b_test_bucket:, **extra)
def sign_in_page_visit(flash:, stored_location:, **extra)
track_event(
'Sign in page visited',
flash: flash,
stored_location: stored_location,
sign_in_a_b_test_bucket:,
**extra,
)
end
Expand Down Expand Up @@ -3716,15 +3714,13 @@ def user_registration_cancellation(request_came_from:, **extra)
# @param [String] service_provider_name
# @param [String] page_occurence
# @param [String] needs_completion_screen_reason
# @param [String] sign_in_a_b_test_bucket
# @param [Array] sp_request_requested_attributes
# @param [Array] sp_session_requested_attributes
def user_registration_complete(
ial2:,
service_provider_name:,
page_occurence:,
needs_completion_screen_reason:,
sign_in_a_b_test_bucket:,
sp_session_requested_attributes:,
sp_request_requested_attributes: nil,
ialmax: nil,
Expand All @@ -3737,7 +3733,6 @@ def user_registration_complete(
service_provider_name: service_provider_name,
page_occurence: page_occurence,
needs_completion_screen_reason: needs_completion_screen_reason,
sign_in_a_b_test_bucket:,
sp_request_requested_attributes: sp_request_requested_attributes,
sp_session_requested_attributes: sp_session_requested_attributes,
**extra,
Expand Down Expand Up @@ -3797,15 +3792,8 @@ def user_registration_email_confirmation(
end

# Tracks when user visits enter email page
# @param [String] sign_in_a_b_test_bucket
# @param [Boolean] from_sign_in
def user_registration_enter_email_visit(sign_in_a_b_test_bucket:, from_sign_in:, **extra)
track_event(
'User Registration: enter email visited',
sign_in_a_b_test_bucket:,
from_sign_in:,

@zachmargolis zachmargolis Jun 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a useful param to keep at all?

@aduth aduth Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be, but my YAGNI hardliner instincts tell me we don't have an immediate use for it detached from the A/B test.

Generally, I do find that we're increasingly wanting to track "clicks" and correlating how a user arrives at a particular place, but maybe that's something we'd want to have some generic support for (e.g. a referrer base property on analytics events, or using ClickObserverComponent now that its implemented with sendBeacon as of @matthinz's improvements in #8496†).

† Previously, I would have avoided ClickObserverComponent for links which navigate the user away, since you couldn't trust that the analytics would be logged unless adding some async await + spinner button, often more trouble than it was worth.

@matthinz matthinz Jun 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I think a referer property on events could be useful (perhaps normalized to just the path, could even leave it nil for links from other hosts if we wanted).

@matthinz matthinz Jun 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(obviously we'd keep the misspelling from HTTP 😄 )

**extra,
)
def user_registration_enter_email_visit
track_event('User Registration: enter email visited')
end

# @param [Boolean] success
Expand Down
27 changes: 9 additions & 18 deletions app/views/devise/sessions/new.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,18 @@

<% if decorated_session.sp_name %>
<%= render 'sign_up/registrations/sp_registration_heading' %>
<% elsif @sign_in_a_b_test_bucket != :tabbed %>
<%= render PageHeadingComponent.new.with_content(decorated_session.new_session_heading) %>
<% end %>

<% if @sign_in_a_b_test_bucket == :tabbed %>
<%= render TabNavigationComponent.new(
label: t('account.login.tab_navigation'),
routes: [
{ text: t('links.next'), path: new_user_session_url },
{ text: t('links.create_account'), path: sign_up_email_url(source: :sign_in) },
],
class: 'margin-bottom-4',
) %>
<%= render TabNavigationComponent.new(
label: t('account.login.tab_navigation'),
routes: [
{ text: t('links.next'), path: new_user_session_url },
{ text: t('links.create_account'), path: sign_up_email_url },
],
class: 'margin-bottom-4',
) %>

<%= render PageHeadingComponent.new.with_content(t('headings.sign_in_existing_users')) %>
<% end %>
<%= render PageHeadingComponent.new.with_content(t('headings.sign_in_existing_users')) %>

<%= render 'shared/sp_alert', section: 'sign_in' %>

Expand Down Expand Up @@ -54,11 +50,6 @@
},
) %>
<%= f.submit t('links.next'), full_width: true, wide: false %>
<% if @sign_in_a_b_test_bucket == :default %>
<div class="margin-top-4 margin-bottom-2">
<%= render 'shared/create_account_banner' %>
</div>
<% end %>
<% end %>
<% if @ial && desktop_device? %>
<div class='margin-x-neg-1 margin-top-205'>
Expand Down
6 changes: 0 additions & 6 deletions app/views/layouts/base.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,6 @@
class: 'site-wrap bg-primary-lighter',
id: local_assigns[:user_main_tag] == false ? nil : 'main-content',
) do %>
<% if @is_on_home_page && @sign_in_a_b_test_bucket == :banner %>
<div class="grid-container padding-y-2 card tablet:margin-bottom-1">
<%= render 'shared/create_account_banner' %>
</div>
<% end %>

<div class="grid-container padding-y-4 tablet:padding-y-8 <%= local_assigns[:disable_card].present? ? '' : 'card' %>">
<%= yield(:pre_flash_content) if content_for?(:pre_flash_content) %>
<%= render FlashComponent.new(flash: flash) %>
Expand Down
10 changes: 0 additions & 10 deletions app/views/shared/_create_account_banner.html.erb
View file
Open in desktop

This file was deleted.

28 changes: 12 additions & 16 deletions app/views/sign_up/registrations/new.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,20 @@

<%= render 'shared/sp_alert', section: 'sign_up' %>

<% if @sign_in_a_b_test_bucket == :tabbed %>
<% if decorated_session.sp_name %>
<%= render 'sign_up/registrations/sp_registration_heading' %>
<% end %>
<% if decorated_session.sp_name %>
<%= render 'sign_up/registrations/sp_registration_heading' %>
<% end %>

<%= render TabNavigationComponent.new(
label: t('account.login.tab_navigation'),
routes: [
{ text: t('links.next'), path: new_user_session_url },
{ text: t('links.create_account'), path: sign_up_email_path },
],
class: 'margin-bottom-4',
) %>
<%= render TabNavigationComponent.new(
label: t('account.login.tab_navigation'),
routes: [
{ text: t('links.next'), path: new_user_session_url },
{ text: t('links.create_account'), path: sign_up_email_path },
],
class: 'margin-bottom-4',
) %>

<%= render PageHeadingComponent.new.with_content(t('headings.create_account_new_users')) %>
<% else %>
<%= render PageHeadingComponent.new.with_content(t('titles.registrations.new')) %>
<% end %>
<%= render PageHeadingComponent.new.with_content(t('headings.create_account_new_users')) %>

<%= simple_form_for(
@register_user_email_form,
Expand Down
1 change: 0 additions & 1 deletion config/application.yml.default
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,6 @@ session_total_duration_timeout_in_minutes: 720
ses_configuration_set_name: ''
set_remember_device_session_expiration: false
sign_up_mfa_selection_order_testing: '{ "default": 100, "authentication_app_priority": 0, "usability_priority": 0 }'
sign_in_a_b_testing: '{ "default": 100,"tabbed" :0, "banner": 0 } '

@zachmargolis zachmargolis Jun 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱 can't believe we had the extra space inside that string there, glad we're removing it

@aduth aduth Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱 can't believe we had the extra space inside that string there, glad we're removing it

😅 Luckily it seemed to tolerate pretty well regardless!

sp_handoff_bounce_max_seconds: 2
show_user_attribute_deprecation_warnings: false
otp_min_attempts_remaining_warning_count: 3
Expand Down
5 changes: 0 additions & 5 deletions config/initializers/ab_tests.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,6 @@ module AbTests
},
)

SIGN_IN = AbTestBucket.new(
experiment_name: 'Sign In Experience',
buckets: IdentityConfig.store.sign_in_a_b_testing,
)

def self.in_person_cta_variant_testing_buckets
buckets = Hash.new
percents = IdentityConfig.store.in_person_cta_variant_testing_percents
Expand Down
1 change: 0 additions & 1 deletion config/locales/headings/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ en:
new: Send another confirmation email
create_account_new_users: Create an account for new users
create_account_with_sp:
cta: First time using %{app_name}?
sp_text: is using %{app_name} to allow you to sign in to your account safely and
securely.
edit_info:
Expand Down
1 change: 0 additions & 1 deletion config/locales/headings/es.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ es:
new: Enviar otro email de confirmación
create_account_new_users: Crear una cuenta para usuarios nuevos
create_account_with_sp:
cta: '¿Es la primera vez que utiliza %{app_name}?'
sp_text: está utilizando %{app_name} para permitirle iniciar sesión en su cuenta
de forma segura.
edit_info:
Expand Down
1 change: 0 additions & 1 deletion config/locales/headings/fr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ fr:
new: Envoyer un autre courriel de confirmation
create_account_new_users: Créer un compte pour les nouveaux utilisateurs
create_account_with_sp:
cta: Première fois que vous utilisez %{app_name}?
sp_text: utilise %{app_name} pour vous permettre de vous connecter à votre
compte de façon sûre et sécurisée.
edit_info:
Expand Down
1 change: 0 additions & 1 deletion lib/identity_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,6 @@ def self.build_store(config_map)
:sign_up_mfa_selection_order_testing, type: :json,
options: { symbolize_names: true }
)
config.add(:sign_in_a_b_testing, type: :json, options: { symbolize_names: true })
config.add(:skip_encryption_allowed_list, type: :json)
config.add(:sp_handoff_bounce_max_seconds, type: :integer)
config.add(:state_tracking_enabled, type: :boolean)
Expand Down
38 changes: 0 additions & 38 deletions spec/controllers/concerns/sign_in_a_b_test_concern_spec.rb
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions spec/controllers/sign_up/completions_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,6 @@
before do
stub_analytics
allow(@analytics).to receive(:track_event)
allow(controller).to receive(:sign_in_a_b_test_bucket).and_return(:default)
@linker = instance_double(IdentityLinker)
allow(@linker).to receive(:link_identity).and_return(true)
allow(IdentityLinker).to receive(:new).and_return(@linker)
Expand All @@ -214,7 +213,6 @@
service_provider_name: subject.decorated_session.sp_name,
page_occurence: 'agency-page',
needs_completion_screen_reason: :new_sp,
sign_in_a_b_test_bucket: :default,
sp_request_requested_attributes: nil,
sp_session_requested_attributes: nil,
)
Expand Down Expand Up @@ -274,7 +272,6 @@
service_provider_name: subject.decorated_session.sp_name,
page_occurence: 'agency-page',
needs_completion_screen_reason: :new_sp,
sign_in_a_b_test_bucket: :default,
sp_request_requested_attributes: nil,
sp_session_requested_attributes: ['email'],
)
Expand Down
Loading