-
Notifications
You must be signed in to change notification settings - Fork 120
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
base: main
Are you sure you want to change the base?
Changes from 26 commits
a189173
4020197
ee0db90
e8be3a9
86d6926
934f0de
74facb5
591fc76
4664686
26e8be3
02d8719
5eefe8c
0134622
997aeb8
b16e7da
534f517
761bfcc
f3f21d7
108fd92
eb25134
884b507
bc696dd
5bc8280
36b18ad
5986f27
34a73c4
5b8bd55
d17800e
c7ca2fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ | |
module Accounts | ||
class ConnectedAccountsController < ApplicationController | ||
include RememberDeviceConcern | ||
include ApplicationHelper | ||
|
||
before_action :confirm_two_factor_authenticated | ||
|
||
layout 'account_side_nav' | ||
|
@@ -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), | ||
change_email_available: change_email_available?, | ||
) | ||
end | ||
|
||
def change_email_available? | ||
if decorated_sp_session.requested_attributes.present? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.include?('all_emails') || | ||
!decorated_sp_session.requested_attributes.include?('email') | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,10 +20,12 @@ | |
<strong> | ||
<%= identity.email_address&.email || t('account.connected_apps.email_not_selected') %> | ||
</strong> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
sp_name: nil, | ||
user:, | ||
locked_for_session: false, | ||
change_email_available: false, | ||
) | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ | |
sp_name:, | ||
user:, | ||
locked_for_session: false, | ||
change_email_available: false, | ||
) | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, part of the benefit of having a presenter like this is to help keep controllers lean by moving the logic into the presenter class, but in this case we're using the presenter as a pass-through and having the logic in the controller.
Instead, I might have expected that we could pass a value which is more readily available (
decorated_sp_session.requested_attributes
ordecorated_sp_session
) and do the logic in the presenter.