Skip to content

Move USPS verification specs to shared example#1581

Merged
monfresh merged 1 commit intomasterfrom
mb-fix-oidc-controller
Aug 2, 2017
Merged

Move USPS verification specs to shared example#1581
monfresh merged 1 commit intomasterfrom
mb-fix-oidc-controller

Conversation

@monfresh
Copy link
Copy Markdown
Contributor

@monfresh monfresh commented Aug 1, 2017

Why: To continue consolidating SAML and OIDC specs

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.

I just realized this does not involve account creation. I will move to a different folder.

@monfresh monfresh force-pushed the mb-fix-oidc-controller branch from cc6bfd7 to 0092303 Compare August 1, 2017 18:14
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.

should we make this an elsif? might get rid of the rubocop warning above?

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.

Will do.

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.

should this be .to eq(1) instead of be ?

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.

Sure. It was like that before but I can change it.

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 is pretty similar to the code with have in the account creation shared example. It'd be nice to move all of this into an idv helper

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.

Agreed.

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.

Done here: 8a99b8a

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 thought we were planning on having these live under the account creation feature? I guess this is kind of a gray area since the account is already created, but it seems like it is still a part of that flow?

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.

I think for this case, it makes sense to have a separate USPS verification folder since the user will need to wait several days to receive the code before being able to finish, and within this feature, there are several scenarios to consider, such as entering the correct vs wrong code, asking for a new letter, exceeding the max allowed amount of letters, etc.

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.

Also, I added a new scenario for the context where the user is signing back in directly (i.e. not coming from an SP). PTAL.

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.

Thoughts on finding a way to rename this so it is clear it's specific to OIDC? I'd also think visit_idp_from_sp_with_loa3 fits better on the method that is currently named perform_loa3_request_from_sp.

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.

Good points. I renamed the methods here: abcd42c

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.

👍👍

Copy link
Copy Markdown
Contributor

@jmhooper jmhooper left a comment

Choose a reason for hiding this comment

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

I had a final comment about a method name, but I'm fine either way on that

@monfresh monfresh force-pushed the mb-fix-oidc-controller branch from abcd42c to 0728a00 Compare August 2, 2017 14:58
**Why**: To continue consolidating SAML and OIDC specs
@monfresh monfresh merged commit 2258bd7 into master Aug 2, 2017
@monfresh monfresh deleted the mb-fix-oidc-controller branch August 2, 2017 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants