Skip to content

LG-15248: Account Management no change available#11701

Merged
jmdembe merged 54 commits intomainfrom
jd-LG-15248-hide-change-button
Jan 28, 2025
Merged

LG-15248: Account Management no change available#11701
jmdembe merged 54 commits intomainfrom
jd-LG-15248-hide-change-button

Conversation

@jmdembe
Copy link
Copy Markdown
Contributor

@jmdembe jmdembe commented Jan 3, 2025

🎫 Ticket

Link to the relevant ticket:
LG-15248

🛠 Summary of changes

On the connected accounts screen, the "change" option is not shown when

  • all_emails is a requested_attribute; or
  • email does not exist on requested attribute

📜 Testing Plan

Before testing: Clone, setup, and start the identity-oidc-sinatra app and set feature_select_email_to_share_enabled to true

Default behavior:

  • In the identity-oidc-sinatra app, leave all options checked in requested_attributes
  • Once on the identity-idp app, follow all steps to create a new account
  • Once your email is confirmed, follow all steps to set up 2FA
  • After setup, you will be on the /account page. Click Your connected accounts on the left side of the page
  • Once on the connected accounts page, you will see "Example Sinatra App" as a connected account. Observe that the "change" link appears

Simulating missing email or present all_emails attribute

  • In the identity-oidc-sinatra app, either uncheck email or uncheck email and check all_emails
  • Once on the identity-idp app, follow all steps to create a new account
  • Once your email is confirmed, follow all steps to set up 2FA
  • After setup, you will be on the /account page. Click Your connected accounts on the left side of the page
  • Once on the connected accounts page, you will see "Example Sinatra App" as a connected account. Observe that the "change" does not appear

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

When `email` is requested attribute
Screen.Recording.2025-01-08.at.1.46.46.PM.mov
When `all_emails` is a requested attribute
all.emails.no.change.link.mov
When `email` are not a requested attributehttps://github.com/user-attachments/assets/0f3b9cfe-ed76-48ca-8bc9-23e650bec4ea

@aduth aduth changed the title DRAFT: Account Management no change available DRAFT: LG-15248: Account Management no change available Jan 6, 2025
@jmdembe jmdembe force-pushed the jd-LG-15248-hide-change-button branch from 85db79e to 884b507 Compare January 8, 2025 17:33
@jmdembe jmdembe marked this pull request as ready for review January 8, 2025 19:32
@jmdembe jmdembe marked this pull request as draft January 8, 2025 19:38
@jmdembe jmdembe marked this pull request as ready for review January 9, 2025 21:25
@jmdembe jmdembe changed the title DRAFT: LG-15248: Account Management no change available LG-15248: Account Management no change available Jan 9, 2025
@jmdembe jmdembe requested a review from a team January 9, 2025 22:23
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.

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 or decorated_sp_session) and do the logic in the presenter.

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.

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.

Comment on lines 20 to 22
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.

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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.

@jmdembe jmdembe marked this pull request as draft January 10, 2025 19:07
@jmdembe jmdembe force-pushed the jd-LG-15248-hide-change-button branch 3 times, most recently from 05e9816 to 586055d Compare January 16, 2025 20:51
@jmdembe jmdembe marked this pull request as ready for review January 16, 2025 21:31
@jmdembe jmdembe marked this pull request as draft January 16, 2025 21:31
@jmdembe jmdembe force-pushed the jd-LG-15248-hide-change-button branch from 8ed728f to 0a8ad18 Compare January 17, 2025 18:02
@jmdembe jmdembe marked this pull request as ready for review January 24, 2025 20:04
Copy link
Copy Markdown
Contributor

@aduth aduth Jan 27, 2025

Choose a reason for hiding this comment

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

cc @mdiarra3 I think we should align between this pull request and #11717 and have a single method, since this is being used for the same purpose.

Since this lives in the model, we should also avoid any UI-specific naming, so something like verified_single_email_attribute? similar to #11717 would be my vote.

Copy link
Copy Markdown
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Approving with caveat that a handful of the recent commits are my own, so please review whether those make sense to you before merging.

@aduth
Copy link
Copy Markdown
Contributor

aduth commented Jan 27, 2025

Can you also make sure to rebase this branch before merging

@jmdembe jmdembe force-pushed the jd-LG-15248-hide-change-button branch from 100d31a to 979db37 Compare January 28, 2025 13:53
@jmdembe jmdembe merged commit 32b1f7f into main Jan 28, 2025
@jmdembe jmdembe deleted the jd-LG-15248-hide-change-button branch January 28, 2025 20:56
n1zyy pushed a commit that referenced this pull request Jan 31, 2025
* add requested attribute to accounts_show_presenter

* add logic for decorated_sp_session

* check for requested_attributes and ial2_requested

* add tests for requested attribes and ial2 requested

* changelog: User-Facing Improvements, account management, no change available if partner shares all emails

* add logic to show/hide change button

* clean up controller and presenter

* clean up requested_attributes in the rest of controllers and specs

* change logic for all emails requested as well as `nil` to `false` in specs

* remove `ial2_requested`

* lintfix

* fix logic to show desired behavior when  is false

* remove debugger

* fix logic

* add supporting test for view

* match `all_emails_requested` to be a boolean value

* add logic for if the partner does not have `email` as a consented attribute

* logic change to `all_emails_requested?`

* refine check for no email (WIP), refine method name

* fix all broken tests

* change service provider -> partner

* more logic correction

* rename and fix logic

* Update app/controllers/accounts/connected_accounts_controller.rb

Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>

* change `all_emails requested` -> `change_email_available?`

* remove `change_email_available`

* move logic around

* logic in presenter and view

* change to `change_option_available`, fix test

* edit test block

* change name to be more intuitive

* add tests to view

* fix account show presenter tests

* method name cleanup

* more test fixes

* more changes

* fix eager loading problem

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

* lintfix

* add test

* test 1 for connected apps

* check against `identities` and pass value

* place logic in model, remove logic from presenter and controller, add check to presenter

* no longer doing logic in controller, so return connected apps to previous state

* remove `requested_attributes`

* fix logic in model and view

* method name change

* remove unneeded keyword

* fix tests

* Repurpose ServiceProviderIdentity#hide_change_email? as verified_single_email_attribute?

* Revert changes to ApplicationController, AccountShowPresenter spec

* Handle nil verified_attributes

* Update tests for connected accounts view

* Exempt eager loading error validation for connected accounts

* Update stubbed identity in connected accounts feature spec to include email

---------

Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants