Skip to content

LG-11758: Add inline device nickname editing for authenticator apps#9845

Merged
jmdembe merged 38 commits intomainfrom
jd-LG-11758-edit-auth-apps
Jan 18, 2024
Merged

LG-11758: Add inline device nickname editing for authenticator apps#9845
jmdembe merged 38 commits intomainfrom
jd-LG-11758-edit-auth-apps

Conversation

@jmdembe
Copy link
Contributor

@jmdembe jmdembe commented Jan 3, 2024

🎫 Ticket

LG-11758: Add inline device nickname editing for authenticator apps

🛠 Summary of changes

This PR makes it so that users can edit their device nickname or delete an app authenticator in one line.

📜 Testing Plan

Steps require that users have an account with two auth methods set up. This is so that you can easily test the delete and nickname functionality

Edit name

  • Next to the name of the app auth method, click "Manage"
  • Click "Rename"
  • Change device name
  • See a banner that says "Successfully renamed your authentication app method"

Delete method

  • Next to the name of the app auth method, click "Manage"
  • Click "Delete"
  • When prompt for deleting authentication method appears, click "OK"
  • See a banner that says "Successfully deleted an authentication app method"

👀 Screenshots

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

Edit feature
Screen.Recording.2024-01-09.at.10.11.34.AM.mov
Delete feature
Screen.Recording.2024-01-09.at.10.12.22.AM.mov

Comment on lines +53 to +60
def form_class
case action_name
when 'edit', 'update'
TwoFactorAuthentication::AuthAppUpdateForm
when 'destroy'
TwoFactorAuthentication::AuthAppDeleteForm
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this abstraction is more difficult to follow and it would be preferable to explicitly instantiate the form in each of the controllers. It would also allow dropping the set_form alias and before_action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With form_class, we use that method to define the form in each controller and the validate_configuration_exists before_action. Wouldn't dropping set_form break the validation before_action?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see what you mean. It would break that, but I think it may be worth splitting it up still and being explicit about that too since this makes it difficult to respond with something other than render_not_found. The existing controller redirects, and I think that might be preferable (perhaps with an error message?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some thought, I will stick to what I am proposing--we are using one view to rename and delete an app method and the form that we need in the view is toggled based on the desired action.

@jmdembe jmdembe changed the title LG-11758 Edit Auth app renaming/deletion functionality LG-11758 Inline editing for auth apps Jan 4, 2024
@jmdembe jmdembe force-pushed the jd-LG-11758-edit-auth-apps branch from 37f4254 to 4ffb39f Compare January 8, 2024 17:49
@jmdembe jmdembe marked this pull request as ready for review January 9, 2024 15:41
@mdiarra3
Copy link
Contributor

mdiarra3 commented Jan 9, 2024

failing due to content freeze

@jmdembe jmdembe requested a review from a team January 9, 2024 17:52
Copy link
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.

LGTM 👍 We'll need to wait until after the content freeze to merge.

@jmdembe jmdembe changed the title LG-11758 Inline editing for auth apps LG-11758: Add inline device nickname editing for authenticator apps Jan 10, 2024
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.

4 participants