Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Flash message not set on destroy failure #61

Closed
michaelrigart opened this Issue · 14 comments

3 participants

@michaelrigart

This issue has come up before, but none of the solutions seem to work for me.

When I destroy a resource, I need to get redirected back to the index action ( on success and on fail ) so I use the location option.

   respond_with(@resource, location: resources_url)

On success, the notice flash message is set correctly.

When the destroy failed, it redirects to the index action and shows the notice flash message.
So as stated in other tickets, I added a base error on the resource through a before_destroy callback

This time I get redirected to the index action, but no flash message has been set.
Any advice?

@nashby
Collaborator

Can you please provide a sample application that reproduces the error?

@michaelrigart

Hi,

I've set up a small test: https://github.com/michaelrigart/responder_test
Just clone the the test en run db:migrate . Run the server and go directly to the /posts resource . Create test post.

To simulate my error, I've overwritten the destroy model method to return false.

You will notice that when you now try to destroy the object, you will get redirected back to the index. That is thanks to the location option I've set in the controller, otherwise I would get redirected to the show action.

The only thing that doesn't seem right, is the fact that I get the notice flash instead of the alert .

Hope this helps?

@nashby
Collaborator

Hey @michaelrigart. Thanks for the sample application!
Well, as you can see here responder checks for errors with has_errors? method which returns true only if your resource(in our case it's post) has errors. So it returns false even if destroy method returned false.

But you said you tried to add a base error to the resource and it didn't help, right? In this case I'm not sure what's wrong.

@michaelrigart

Hi,

thanks for pointing that out. Indeed, I've tried adding a base error ( updated the example by adding a error in the destroy method. But still the no flash message is set.

@michaelrigart

Hey @nashby

I think I've located the problem to here . I think the flash message get overwritten by flash_now. The @flash_now variable is set to :on_failure

@michaelrigart

In combination with the location option, the flash message gets lost

@nashby
Collaborator

Right. As you can see this commit made it using flash_now by default on failure. Here is an explanation:

Since the usual pattern is that a page gets rerendered after failure,
the proper behavior would be to set the flash directly, so that it
doesn't stick around if you give up and leave the page.

Since this option is configurable you can easily skip it with

respond_with(@post, location: posts_url, flash_now: false)
@michaelrigart

Hi @nashby

seems like setting the flash_now option to false did the trick.

Thank you for pointing that out.

@josevalim
Owner

However, if the default behaviour is to redirect, we should not set flash now, right?

@josevalim josevalim reopened this
@michaelrigart

I got a bit confused by the quote

Since the usual pattern is that a page gets rerendered after failure,
the proper behavior would be to set the flash directly, so that it
doesn't stick around if you give up and leave the page.

I don't know what the author ment by saying it is "the usual pattern" so I can't really comment on that, but I always thought that on a normal destroy action ( no ajax), the default rails behaviour is, you get either redirected to the index action on success or redirected to show on failure.

So, if a redirect occurs, it seems to the flash shouldn't be set now. On the other hand, I can't see what made the author of the pull request change this.

@nashby
Collaborator

well, we can always ask @iain about this :)

@josevalim
Owner

I think we can simply look into default_action to know if it is going to redirect or render something (i.e. if we are rendering something, default action will exist) and set the flash message accordingly.

@michaelrigart

That might indeed be a cleaner way of solving this issue

@nashby nashby referenced this issue from a commit in nashby/responders
@nashby nashby do not set flash.now with redirect action
closes #61
4e036da
@nashby nashby referenced this issue from a commit in nashby/responders
@nashby nashby do not set flash.now with redirect action
closes #61
0a07b54
@michaelrigart

Problem seems solved. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.