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

Flash messages are not maintained with multiple redirects. #1162

Closed
wout opened this issue May 31, 2020 · 5 comments · Fixed by #1169
Closed

Flash messages are not maintained with multiple redirects. #1162

wout opened this issue May 31, 2020 · 5 comments · Fixed by #1169

Comments

@wout
Copy link
Contributor

wout commented May 31, 2020

In continuation of #1160.

@wout
Copy link
Contributor Author

wout commented May 31, 2020

Ok, I've located the issue and found an easy fix but before I make a PR, I would like to discuss the changes.

Let's say we have the following setup:

  • SignIns::Create redirects to Home::Index and sets a flash.success = "Yay!"
  • Home::Index redirects to Dashboard::Index and does not set a flash

The Home::Index action does not consume the flash, so when you arrive at Dashboard::Index, the flash has been lost. An easy fix is to recycle the flash on redirects. That's something we can add here:

module Lucky::Redirectable
  # ...
  def redirect(to path : String, status : Int32 = 302) : Lucky::TextResponse
    flash.recycle
    # ...
  end
  # ...
end

Its functionality would be:

class Lucky::FlashStore
  # ...
  def recycle : Void
    @next = @now
  end
  # ...
end

What do you think?

@confact
Copy link
Contributor

confact commented Jun 1, 2020

I had the same issue myself. Good catch @wout!

I think that sounds good, but maybe some setting/param to disable the flash on redirect if you want to for some reason.

@wout
Copy link
Contributor Author

wout commented Jun 1, 2020

@confact I've also been thinking about that. But if you don't want the flash messages to be recycled, you could always explicitly clear the flash:

class Home::Index < BrowserAction
  get "/home" do
    flash.clear
    redirect Dashboard::Index
  end
end

I found that to be convenient enough. :)

@confact
Copy link
Contributor

confact commented Jun 1, 2020

A yeah. That's even better :)

@paulcsmith
Copy link
Member

Love it! I think flash.recycle in the redirect helper is perfect, and clearing the flash should work great if you no longer want it.

The only suggestion I have (which shouldn't stop a PR) is to call it Flash#reuse. but maybe that's worse. Either way not a big deal since will likely only be used internally by Lucky 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants