Skip to content
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.

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

LG-15248: Account Management no change available #11701

Draft
wants to merge 29 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a189173
add requested attribute to accounts_show_presenter
jmdembe Jan 2, 2025
4020197
add logic for decorated_sp_session
jmdembe Jan 3, 2025
ee0db90
check for requested_attributes and ial2_requested
jmdembe Jan 3, 2025
e8be3a9
add tests for requested attribes and ial2 requested
jmdembe Jan 3, 2025
86d6926
changelog: User-Facing Improvements, account management, no change av…
jmdembe Jan 3, 2025
934f0de
add logic to show/hide change button
jmdembe Jan 3, 2025
74facb5
clean up controller and presenter
jmdembe Jan 3, 2025
591fc76
clean up requested_attributes in the rest of controllers and specs
jmdembe Jan 3, 2025
4664686
change logic for all emails requested as well as `nil` to `false` in …
jmdembe Jan 3, 2025
26e8be3
remove `ial2_requested`
jmdembe Jan 3, 2025
02d8719
lintfix
jmdembe Jan 6, 2025
5eefe8c
fix logic to show desired behavior when is false
jmdembe Jan 6, 2025
0134622
remove debugger
jmdembe Jan 6, 2025
997aeb8
fix logic
jmdembe Jan 6, 2025
b16e7da
add supporting test for view
jmdembe Jan 6, 2025
534f517
match `all_emails_requested` to be a boolean value
jmdembe Jan 6, 2025
761bfcc
add logic for if the partner does not have `email` as a consented att…
jmdembe Jan 6, 2025
f3f21d7
logic change to `all_emails_requested?`
jmdembe Jan 6, 2025
108fd92
refine check for no email (WIP), refine method name
jmdembe Jan 7, 2025
eb25134
fix all broken tests
jmdembe Jan 8, 2025
884b507
change service provider -> partner
jmdembe Jan 8, 2025
bc696dd
more logic correction
jmdembe Jan 8, 2025
5bc8280
rename and fix logic
jmdembe Jan 9, 2025
36b18ad
Update app/controllers/accounts/connected_accounts_controller.rb
jmdembe Jan 10, 2025
5986f27
change `all_emails requested` -> `change_email_available?`
jmdembe Jan 10, 2025
34a73c4
Merge branch 'main' into jd-LG-15248-hide-change-button
jmdembe Jan 10, 2025
5b8bd55
remove `change_email_available`
jmdembe Jan 14, 2025
d17800e
move logic around
jmdembe Jan 15, 2025
c7ca2fd
logic in presenter and view
jmdembe Jan 15, 2025
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
10 changes: 10 additions & 0 deletions app/controllers/accounts/connected_accounts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
module Accounts
class ConnectedAccountsController < ApplicationController
include RememberDeviceConcern
include ApplicationHelper

before_action :confirm_two_factor_authenticated

layout 'account_side_nav'
Expand All @@ -16,7 +18,15 @@ def show
sp_name: decorated_sp_session.sp_name,
user: current_user,
locked_for_session: pii_locked_for_session?(current_user),
all_emails_requested: all_emails_requested?,
)
end

def all_emails_requested?
jmdembe marked this conversation as resolved.
Show resolved Hide resolved
if decorated_sp_session.requested_attributes.present?
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be looking at the requested attribute for the current request, not for each connected application. We want the link to be available for any connection where the requested attribute includes email.

decorated_sp_session.requested_attributes.map.include?('all_emails') ||
!decorated_sp_session.requested_attributes.map.include?('email')
jmdembe marked this conversation as resolved.
Show resolved Hide resolved
end
end
end
end
1 change: 1 addition & 0 deletions app/controllers/accounts/history_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ def show
sp_name: decorated_sp_session.sp_name,
user: current_user,
locked_for_session: pii_locked_for_session?(current_user),
all_emails_requested: false,
)
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ def show
sp_name: decorated_sp_session.sp_name,
user: current_user,
locked_for_session: pii_locked_for_session?(current_user),
all_emails_requested: false,
)
end
end
Expand Down
1 change: 1 addition & 0 deletions app/controllers/accounts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ def show
sp_name: decorated_sp_session.sp_name,
user: current_user,
locked_for_session: pii_locked_for_session?(current_user),
all_emails_requested: false,
)
if session.delete(:from_select_email_flow)
flash.now[:success] = t(
Expand Down
1 change: 1 addition & 0 deletions app/controllers/events_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ def show
sp_name: decorated_sp_session.sp_name,
user: current_user,
locked_for_session: pii_locked_for_session?(current_user),
all_emails_requested: false,
)
device_and_events
rescue ActiveRecord::RecordNotFound, ActiveModel::RangeError
Expand Down
8 changes: 7 additions & 1 deletion app/presenters/account_show_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,16 @@ def initialize(
authn_context:,
sp_name:,
user:,
locked_for_session:
locked_for_session:,
all_emails_requested: false
)
@decrypted_pii = decrypted_pii
@user = user
@sp_name = sp_name
@sp_session_request_url = sp_session_request_url
@authn_context = authn_context
@locked_for_session = locked_for_session
@all_emails_requested = all_emails_requested
@pii = determine_pii
end

Expand Down Expand Up @@ -135,6 +137,10 @@ def connected_apps
user.connected_apps.includes([:service_provider_record, :email_address])
end

def hide_change_option
@all_emails_requested
end

delegate :recent_events, :recent_devices, to: :user

private
Expand Down
10 changes: 6 additions & 4 deletions app/views/accounts/_connected_app.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@
<strong>
<%= identity.email_address&.email || t('account.connected_apps.email_not_selected') %>
</strong>
Copy link
Member

Choose a reason for hiding this comment

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

We also don't want to show the "selected" email if the connected application didn't request email.

From the ticket:

"No email selected" / selected email [...] are not shown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making sure I understand the changes to be made--so it should look like this:

image

Copy link
Member

Choose a reason for hiding this comment

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

I think we'd want to fix up the stray "with:" there. I think this changed in the pull request which originally added the feature, so we might be able to get some strings from the previous implementation to use.

It might also be worth double-checking with the UX folks, but other than that, I think it's what I'd expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense.

<%= link_to(
t('help_text.requested_attributes.change_email_link'),
edit_connected_account_selected_email_path(identity_id: identity.id),
) %>
<% if [email protected]_change_option %>
<%= link_to(
t('help_text.requested_attributes.change_email_link'),
edit_connected_account_selected_email_path(identity_id: identity.id),
) %>
<% end %>
<% else %>
<%= t(
'account.connected_apps.associated_html',
Expand Down
2 changes: 2 additions & 0 deletions spec/controllers/accounts_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@
sp_name: nil,
user: user,
locked_for_session: false,
all_emails_requested: false,
)
allow(subject).to receive(:presenter).and_return(presenter)

Expand Down Expand Up @@ -172,6 +173,7 @@
sp_name: nil,
user: user,
locked_for_session: false,
all_emails_requested: false,
)
allow(subject).to receive(:presenter).and_return(presenter)

Expand Down
12 changes: 12 additions & 0 deletions spec/presenters/account_show_presenter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
sp_name:,
user:,
locked_for_session:,
all_emails_requested: false,
)
end

Expand Down Expand Up @@ -294,6 +295,7 @@
sp_name: nil,
user: user,
locked_for_session: false,
all_emails_requested: false,
)

expect(account_show.pending_ipp?).to be(false)
Expand Down Expand Up @@ -327,6 +329,7 @@
sp_name: nil,
user: user,
locked_for_session: false,
all_emails_requested: false,
)

expect(account_show.pending_ipp?).to be(false)
Expand Down Expand Up @@ -454,6 +457,7 @@
authn_context: nil,
sp_name: nil,
locked_for_session: false,
all_emails_requested: false,
)

expect(profile_index.header_personalization).to eq first_name
Expand All @@ -472,6 +476,7 @@
authn_context: nil,
sp_name: nil,
locked_for_session: false,
all_emails_requested: false,
)

expect(profile_index.header_personalization).to eq email_address.email
Expand All @@ -494,6 +499,7 @@
authn_context: nil,
sp_name: nil,
locked_for_session: false,
all_emails_requested: false,
)

expect(profile_index.totp_content).to eq t('account.index.auth_app_enabled')
Expand All @@ -513,6 +519,7 @@
authn_context: nil,
sp_name: nil,
locked_for_session: false,
all_emails_requested: false,
)

expect(profile_index.totp_content).to eq t('account.index.auth_app_disabled')
Expand Down Expand Up @@ -550,6 +557,7 @@
sp_name: nil,
user: user.reload,
locked_for_session: false,
all_emails_requested: false,
)

expect(account_show.backup_codes_generated_at).to be_within(
Expand All @@ -569,6 +577,7 @@
sp_name: nil,
user: user.reload,
locked_for_session: false,
all_emails_requested: false,
)

expect(account_show.backup_codes_generated_at).to be_nil
Expand Down Expand Up @@ -617,6 +626,7 @@
authn_context: nil,
sp_name: nil,
locked_for_session: false,
all_emails_requested: false,
)

expect(
Expand All @@ -641,6 +651,7 @@
authn_context: nil,
sp_name: nil,
locked_for_session: false,
all_emails_requested: false,
)

expect(
Expand All @@ -660,6 +671,7 @@
authn_context: nil,
sp_name: nil,
locked_for_session: false,
all_emails_requested: false,
)

expect(profile_index.personal_key_generated_at).to be_nil
Expand Down
1 change: 1 addition & 0 deletions spec/views/accounts/_badges.html.erb_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
sp_name: nil,
user:,
locked_for_session: false,
all_emails_requested: false,
)
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
sp_name:,
user:,
locked_for_session: false,
all_emails_requested: false,
)
end

Expand Down
28 changes: 28 additions & 0 deletions spec/views/accounts/connected_accounts/show.html.erb_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
authn_context: nil,
sp_name: nil,
locked_for_session: false,
all_emails_requested: false,
),
)
end
Expand Down Expand Up @@ -107,4 +108,31 @@
expect(rendered).to_not include('&lt;')
end
end

context 'when the partner requests all emails' do
before do
assign(
:presenter,
AccountShowPresenter.new(
decrypted_pii: nil,
user: user,
sp_session_request_url: nil,
authn_context: nil,
sp_name: nil,
locked_for_session: false,
all_emails_requested: true,
),
)
end
let!(:identity) { create(:service_provider_identity, user:) }

it 'does not show the change link' do
render

expect(rendered).not_to have_link(
t('help_text.requested_attributes.change_email_link'),
href: edit_connected_account_selected_email_path(identity_id: identity.id),
)
end
end
end
1 change: 1 addition & 0 deletions spec/views/accounts/history/show.html.erb_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
authn_context: nil,
sp_name: nil,
locked_for_session: false,
all_emails_requested: false,
),
)
end
Expand Down
2 changes: 2 additions & 0 deletions spec/views/accounts/show.html.erb_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
authn_context:,
sp_name: nil,
locked_for_session: false,
all_emails_requested: false,
),
)
end
Expand Down Expand Up @@ -255,6 +256,7 @@
authn_context:,
sp_name: sp.friendly_name,
locked_for_session: false,
all_emails_requested: false,
),
)
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
authn_context: nil,
sp_name: nil,
locked_for_session: false,
all_emails_requested: false,
),
)
end
Expand All @@ -40,6 +41,7 @@
authn_context: nil,
sp_name: nil,
locked_for_session: false,
all_emails_requested: false,
),
)
end
Expand Down