Conversation
el-mapache
force-pushed
the
add-otp-choice-presenter
branch
2 times, most recently
from
29c1b26 to
c2973c6
Compare
el-mapache
force-pushed
the
add-otp-choice-presenter
branch
from
c2973c6 to
b362cb5
Compare
el-mapache
force-pushed
the
add-otp-choice-presenter
branch
from
e7bd26e to
8b746d5
Compare
el-mapache
added
status - ready for review
and removed
status - work in progress
labels
There was a problem hiding this comment.
If we add a unit test for OtpDeliveryPresenter that verifies the actual expected text for header, then checking @presenter.header here is fine. Otherwise, without the unit test, this expectation would pass even if the class modifies the header method to something unexpected.
There was a problem hiding this comment.
Definitely, I'll change this to be consistent with the other header method tests tests. I must have changed/missed this when I was rebasing. Thanks!
monfresh
left a comment
monfresh
left a comment
There was a problem hiding this comment.
LGTM other than the missing spec for OtpDeliveryPresenter.
el-mapache
force-pushed
the
add-otp-choice-presenter
branch
from
8b746d5 to
6948c61
Compare
|
I noticed a bug related to the changes you've been making, but it also happens on the master branch, so I'm not sure when it was introduced: If you attempt to change your phone after the 60-minute window has passed, such that you need to enter your current password + 2FA, once you are prompted to enter the OTP that was sent to your current phone, if you click the link to "get another text message", it takes you to the Profile page instead of refreshing the page with a new OTP. |
|
ok, I'll take a look. Just for clarification, the re auth window expires after 60 minutes? not 60 seconds? |
|
I think this bug existed before any of these changes were merged, so I'll take a look at it, and we can merge this PR when it's ready. |
|
And yes, I meant 60 seconds. |
|
I see that you've addressed the |
|
LGTM. Please squash and merge yourself. Thanks! |
el-mapache
force-pushed
the
add-otp-choice-presenter
branch
2 times, most recently
from
9b6b542 to
9aea107
Compare
|
Still looks good. Please squash, and then I'll merge because Hakiri is acting up. |
el-mapache
force-pushed
the
add-otp-choice-presenter
branch
2 times, most recently
from
702fb8a to
fa9bc56
Compare
|
@monfresh done, thanks! |
|
Nm got a bit hasty, will fix these tonight. 😬 |
**Why**: Presenters make the views a little cleaner Don't show recovery code, unconfirmed phone links **Why**: The user shouldn't be able to use a recovery code when confirming a changed phone number, nor should they be able to select the 'use another phone' choice when reauthenticating after choosing a new phone number Moves session context + related methods to module **Why**: Methods were getting duplicated across modules Adds new method to check for auth context only **Why**: The `authentication_context?` needs to handle two types of auth contexts, and we don't want to keep cluttering up multiple files with hardcoded string references Adds presenters for phone view **Why**: To consolidate logic of how we show fallback otp links and associated help text to the user Adds authenticator presenter **Why**: To consolidate logic for displaying otp fallback links and help text
el-mapache
force-pushed
the
add-otp-choice-presenter
branch
from
fa9bc56 to
d65d0fc
Compare
2 participants
Depends on previous two presenter PRs (#885, #901)