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

Include the content of the flash in the auto-generated etag #26250

Merged
merged 5 commits into from Aug 22, 2016
Merged

Conversation

@dhh
Copy link
Member

@dhh dhh commented Aug 22, 2016

When you're using the flash, it's generally used as a conditional on the view. This means the content of the view depends on the flash. Which in turn means that the etag for a response should be computed with the content of the flash in mind. This does that by including the content of the flash as a component in the etag that's generated for a response.

@matthewd
Copy link
Member

@matthewd matthewd commented Aug 22, 2016

Could we just make it uncacheable? (Does the etag method have the ability to do that? Maybe it should.)

Doesn't seem worth keeping, vs likelihood of seeing a same-tagged request again.

@dhh
Copy link
Member Author

@dhh dhh commented Aug 22, 2016

I think it's fair that you can cache a flashed response. Lots of flashes are static confirmations that are likely to repeat.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Aug 22, 2016

Idea and implementation is fine to me. Tests are broken though, and the string quotes are not consistent. rucobop -a actionpack/test/controller/flash_test.rb to fix the style problem.

@claudiob
Copy link
Member

@claudiob claudiob commented Aug 22, 2016

I'm trying to understand the result of this commit.
Is the following interpretation correct?

If I see a page with a flash message, and I reload the page, then:

  • if the page was cached, I still see the flash message
  • if the page was not cached, I don't see the flash message
@matthewd
Copy link
Member

@matthewd matthewd commented Aug 22, 2016

@claudiob the flash will go away on refresh... the cached version (with the flash visible) will not be used for your second request (because the etag has changed)

@dhh
Copy link
Member Author

@dhh dhh commented Aug 22, 2016

@claudiob Here's the common issue this solves:

  1. POST /messages
  2. MessagesController#create adds redirect_to messages_url, notice: 'Message was added!'
  3. User visits /messages/1
  4. User goes back to /messages, and since the etag for that didn't consider the flash, it'll say that the page is "the same", even though on this visit we do not wish to display the flash. But the flash will be displayed because it was present in the version of /messages that was cached after step 2.
@claudiob
Copy link
Member

@claudiob claudiob commented Aug 22, 2016

@dhh Then that sounds great.

What do you think about adding that example to the CHANGELOG?
I think it might be helpful when writing the Release Notes for 5.1. Thanks!

@dhh
Copy link
Member Author

@dhh dhh commented Aug 22, 2016

Sure!

On Mon, Aug 22, 2016 at 1:24 PM, Claudio B. notifications@github.com
wrote:

@dhh https://github.com/dhh Then that sounds great.

What do you think about adding that example should be added to the
CHANGELOG?
I think it might be helpful in writing the Release Notes for 5.1. Thanks!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26250 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAKtSyh1nqXbmmkVFfqZW6SFvHwCVdVks5qigV1gaJpZM4JqJaa
.

@dhh dhh merged commit debd774 into master Aug 22, 2016
0 of 2 checks passed
0 of 2 checks passed
codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@dhh dhh deleted the etag-with-flash branch Aug 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.