Skip to content

Redirect to branded page after canceling#1471

Merged
monfresh merged 1 commit intomasterfrom
mb-cancel-link
Jun 3, 2017
Merged

Redirect to branded page after canceling#1471
monfresh merged 1 commit intomasterfrom
mb-cancel-link

Conversation

@monfresh
Copy link
Contributor

@monfresh monfresh commented Jun 2, 2017

Why: If you came from an SP, canceling account creation should
take you back to the branded start page.

@monfresh monfresh requested a review from zachmargolis June 2, 2017 18:48
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a meaningful change from root_url to the interpolated string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, the test now goes to the domain based on domain_name instead of test.host, so we can't use the URL helpers anymore which apparently only work with test.host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently this is due to returning a _url instead of _path in the decorated session. Switching to _path allows to use the URL helpers in the tests. I'll make the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be easier to just set properties on the underlying session hash than to stub this entire object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because the controller determines which URL to redirect to based on the decorated_session.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but we build a decorated session based on the values inside of the session? But it's not a huge deal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes. I see what you mean. I'll update.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@zachmargolis
Copy link
Contributor

Oh but looks like a spec is failing (possibly due to a copy change). LGTM % specs passing :)

**Why**: If you came from an SP, canceling account creation should
take you back to the branded start page.
@monfresh monfresh merged commit d39346c into master Jun 3, 2017
@monfresh monfresh deleted the mb-cancel-link branch June 3, 2017 01:20
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.

2 participants