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 message not set on destroy failure #61

Closed
michaelrigart opened this issue Mar 25, 2013 · 14 comments · Fixed by #63
Closed

Flash message not set on destroy failure #61

michaelrigart opened this issue Mar 25, 2013 · 14 comments · Fixed by #63

Comments

@michaelrigart
Copy link

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
Copy link
Collaborator

nashby commented Mar 29, 2013

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

@michaelrigart
Copy link
Author

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
Copy link
Collaborator

nashby commented Mar 29, 2013

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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

@nashby
Copy link
Collaborator

nashby commented Mar 30, 2013

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
Copy link
Author

Hi @nashby

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

Thank you for pointing that out.

@josevalim
Copy link
Contributor

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

@josevalim josevalim reopened this Mar 30, 2013
@michaelrigart
Copy link
Author

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
Copy link
Collaborator

nashby commented Mar 30, 2013

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

@josevalim
Copy link
Contributor

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
Copy link
Author

That might indeed be a cleaner way of solving this issue

nashby added a commit to nashby/responders that referenced this issue Apr 2, 2013
nashby added a commit to nashby/responders that referenced this issue Apr 2, 2013
@michaelrigart
Copy link
Author

Problem seems solved. Thanks!

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

Successfully merging a pull request may close this issue.

3 participants