Replace cancel account creation link on email signup screen with cancel link #1314
Conversation
**Why**: Although the user is technically canceling their account creation, none of the actions in the modal make sense at this point, since they hacven't entered any information
| @@ -4,7 +4,7 @@ | |||
| before do | |||
| user = build_stubbed(:user) | |||
| allow(view).to receive(:current_user).and_return(nil) | |||
| allow(view).to receive(:session).and_return(sign_up_init: true) | |||
| allow(view).to receive(:params).and_return(confirmation_token: 123) | |||
There was a problem hiding this comment.
so are we also removing the cancel modal from the "enter password" page? That isn't mentioned in the PR description but this spec change would indicate that we are?
There was a problem hiding this comment.
Nope! The modal is still on this screen. We are just making use of the existing confirmation_token session key, rather than sign_up_init, which isn't needed anymore.
|
@el-mapache so this reverts part of #1255 ? |
| def user_signing_up? | ||
| sign_up_init? || current_user && !current_user.two_factor_enabled? | ||
| params[:confirmation_token] || (current_user && !current_user.two_factor_enabled?) |
There was a problem hiding this comment.
this seems to be the core logic change yes? everything else flows out of here.
|
@pkarman No, that PR specifically dealt with the |
|
|
||
| expect(rendered).to have_selector("input[value='#{link}']") | ||
| expect(rendered).to have_content(link) |
There was a problem hiding this comment.
What do you think about replacing these 2 last lines with this, to test both the link text and the actual destination?
expect(rendered).to have_link(t('links.cancel'), href: root_url)I would expect the cancel link to point to the root url, not api/saml/logout since the user is not signed in.
|
Also, the PR description doesn't seem to be accurate. It says that the cancel link will be removed entirely, but it's still there, only it points somewhere else. |
|
@el-mapache that's not how I read #1255. Many of the exact same files seem to be changing back. Don't mean to be pedantic, just trying to correlate with #1255 since that has been prioritized for the next RC fix release. |
|
@pkarman I see, now. Yes, I'm essentially just removing Re that other PR, there are two different 'password' screens affected by it, I'll update the title to reflect that. Many of the same files are changing back because we don't need to check for the presence of the |
|
Can we also update this test: https://github.com/18F/identity-idp/blob/master/spec/views/sign_up/passwords/new.html.slim_spec.rb#L26-L30 to test for the href as well as the link text since that's what we're really interested in testing? Doesn't have to be in this PR. Might be good to check for other similar tests that only test for the link text and not the href. |
|
@monfresh Sorry, I've been trying to make the titles as descriptive as possible given the character limit on the command line, but I'm not in the habit of updating them on github. I'll change it to make it clear that we are replacing one cancel link with another. |
el-mapache
changed the title
**Why**: It was still pointing at the `destroy_user_session_path`, which was a hold over from previous behavior.
Why: Although the user is technically canceling their account
creation, none of the actions in the modal make sense at this point,
since they haven't entered any information