Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Timeoutable timeouts flash hash keys #4464

Closed
wants to merge 1 commit into from
Closed

Fix Timeoutable timeouts flash hash keys #4464

wants to merge 1 commit into from

Conversation

janraasch
Copy link

@janraasch janraasch commented Mar 10, 2017

See #1777 for an in-depth discussion of the unexpected behaviour.

See #4038 for the fix, but awaiting tests.

The behaviour still works as expected for redirects, see

test 'error message with i18n'
test 'error message with i18n with double redirect'

in https://github.com/plataformatec/devise/blob/master/test/integration/timeoutable_test.rb#L141-L166.

See #1777 for an in-depth discussion of the unexpected behaviour.

See #4038 for the fix, but awaiting tests.

The behaviour still works as expected for redirects, see

```
test 'error message with i18n'
test 'error message with i18n with double redirect'
```

in `integration/timeoutable_test.rb`.
@tegon
Copy link
Member

tegon commented Dec 3, 2018

@janraasch Thanks for tackling this.

It seems really odd to me that the tests still pass after the code was deleted. It makes me wonder whether the tests were actually covering the intended scenarios in the first place.
I'll try to simulate the issue this code was trying to solve in the past, but I'm also worried about what @josevalim said about the multi-thread/process environments.

Did you have a chance to manually test this?

@janraasch
Copy link
Author

Did you have a chance to manually test this?

Long time no see 😉. If I remember correctly we monkey-patched this on our app, but to be honest I no longer am affiliated with that application so I cannot look into the source code. So \_o_/ I guess 🤓.

It seems really odd to me that the tests still pass after the code was deleted. It makes me wonder whether the tests were actually covering the intended scenarios in the first place.

That does seem strange, I absolutely agree.

@tegon
Copy link
Member

tegon commented Dec 11, 2018

Ok, get it. Thanks for replying.

I'm going to close this then, as I don't have the context of why this change was needed or what side effects this pull request might cause. I hope you understand my reasoning.

Thanks again!

@tegon tegon closed this Dec 11, 2018
@janraasch janraasch deleted the fix-timeoutable-flash-keys branch December 11, 2018 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants