Skip to content

Improve flash message when page is refreshed#1160

Merged
hursey013 merged 4 commits intomasterfrom
bh-timeout-messages
Mar 2, 2017
Merged

Improve flash message when page is refreshed#1160
hursey013 merged 4 commits intomasterfrom
bh-timeout-messages

Conversation

@hursey013
Copy link
Copy Markdown
Contributor

To give users more context on why we refresh the page and clear the form inputs.

When accessing app directly:
screen shot 2017-03-01 at 7 01 14 pm

When accessing from service provider:
screen shot 2017-03-01 at 7 03 49 pm

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 duplicated this method a bit in openid_connect/authorization_controller.rb -- can you rebase and add this there too? 😀 I can try to refactor it after too so we can share more code

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.

the description for this is a lie, it looks like issuer is added to the url and not sp_name

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 would escape the param here for consistency: issuer=http%3A%2F%2Flocalhost%3A3000

hursey013 and others added 3 commits March 2, 2017 09:44
**Why**: To give users more context on why we refresh the page and clear the form inputs.
**Why**: Pass tests
@hursey013 hursey013 force-pushed the bh-timeout-messages branch 2 times, most recently from e7be003 to f96af43 Compare March 2, 2017 15:51
@hursey013
Copy link
Copy Markdown
Contributor Author

@zachmargolis updated!

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 personally am happy to disable rubocop everywhere, but I know that @monfresh and @pkarman sometimes try to appease it rather than disable. Any strong opinions here?

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 personally don't think there's ever a good reason to disable AbcSize. It's a sign the method is too complex. A method can always be refactored to eliminate the offense. I'd be happy to take a look to see how we can improve this code.

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.

Yeah we have the max size for a method set to 15 in rubocop and this particular method was at 15.07... couldn't think of a way to restructure it any further but definitely open to suggestions.

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'm fine with this for now. I'll refactor after this gets merged. There are a lot of things that need to be cleaned up now that we are storing Service Providers in the database.

Copy link
Copy Markdown
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!

**Why**: Making it better
@hursey013 hursey013 force-pushed the bh-timeout-messages branch from f96af43 to d4564e9 Compare March 2, 2017 19:50
@hursey013 hursey013 merged commit fbb2b3c into master Mar 2, 2017
@hursey013 hursey013 deleted the bh-timeout-messages branch March 2, 2017 20:14
amoose pushed a commit that referenced this pull request Mar 7, 2017
* Improve flash message when page is refreshed

**Why**: To give users more context on why we refresh the page and clear the form inputs.
amoose pushed a commit that referenced this pull request Mar 8, 2017
* Improve flash message when page is refreshed

**Why**: To give users more context on why we refresh the page and clear the form inputs.
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