seanpdoyle
commented
2 years ago If you call redirect_to with status: :see_other, does the second (unwanted) DELETE post occur?
Closed salex closed 2 years ago
seanpdoyle
commented
2 years ago If you call redirect_to with status: :see_other, does the second (unwanted) DELETE post occur?
seanpdoyle
commented
2 years ago There are a few contributing factors at play here, but unless you're nesting the link_to within a <turbo-frame> element or marking it with a [data-turbo-frame], I'm skeptical that this is a Turbo Frame issue.
Similarly, if you're not declaring a format.turbo_stream block or an app/views/rounds/destroy.turbo_stream.erb template, I'm skeptical that it's a Turbo Stream issue (in spite of the Processing by PlayersController#destroy as TURBO_STREAM log output).
I believe the root of this issue is the [data-turbo-method="delete"] (the new Turbo version of [data-method="delete"]).
With a call to button_to method: :delete, the resulting <form> will resemble something like:
<form method="post" action="...">
<input type="hidden" name="_method" value="delete">
<!-- ... -->
</form>Note the form[method="post"]. This will submit a POST request with a _method that Rails will interpret as an HTTP verb. This is a POST request, and not a DELETE request.
With a call to link_to data: { turbo_method: :delete }, Turbo will synthesize a <form> with [method="delete"]:
<form method="delete" action="...">
<!-- --->
</form>Browser's don't know how to submit a <form method="delete">, since it isn't a POST or GET. Turbo intervenes, stops browsers from submitting forms, and submits forms as fetch(...) calls, so that isn't an issue.
These two requests, while similar in intent, are made with different verbs: POST in the case of button_to and DELETE in the case of the link_to.
When Rails is handling the response, it will redirect with a 302 by default.
This is potentially destructive, because the subsequent request will be made with the same verb as the original request (in the case of the link_to data: { turbo_method: "delete" }, that's a DELETE request.
If you override the status: to :see_other (a 303), the subsequent request is a GET, and is not destructive.
There have been discussions about doing this by default in Rails (https://github.com/rails/rails/issues/4144) and has been discussed before in hotwired/turbo.
https://github.com/hotwired/turbo-rails/pull/239 aims to incorporate a submitter's [formmethod] into the _method parameter, but could be expanded to transform <form method="..."> attributes that are not GET or POST into _method arguments.
salex
commented
2 years ago It works as expect by setting status: :see_other, only deletes the one record.
But not very intuitive!
Thank for the more details explanation. At least I know it's a possible problem and my go back to my helper.
On Jan 25, 2022, at 10:22 AM, Sean Doyle @.***> wrote:
If you call redirect_to with status: :see_other, does the second (unwanted) DELETE post occur?
— Reply to this email directly, view it on GitHub https://github.com/hotwired/turbo-rails/issues/301#issuecomment-1021364968, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA6WFFHBEYCY6WIGGRF2SDUX3E57ANCNFSM5MYT6HFA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you authored the thread.
WriterZephos
commented
2 years ago I think redirecting inside a turbo_stream is a bit of an anti-pattern, isn't it?
All you really need to do is have turbo_frame containing all the show content that you want to swap out with your next page, and target that turbo_frame with the content you are currently redirecting to.
salex
commented
2 years ago @WriterZephos
I'll plead guilty to not knowing what I was doing when I added the Turbo destroy link_to tag. It created the turbo-frame, not Me. I just copied it out of the getting started page.
What I didn't see (if it was there at the time) was:
def destroy
@article = Article.find(params[:article_id])
@comment = @article.comments.find(params[:id])
@comment.destroy
redirect_to article_path(@article), status: 303
endThe status: 303 thing.
My whole point on this semi-issue that if you've been working with Rail for a long time, this a big and unintuitive change - unless you go back and read the getting started guide. There was nothing in the upgrade guide about this.
Maybe there should be and upgraded guide "upgrade from @rails/ujs to turbo".
WriterZephos
commented
2 years ago @salex in my hotwire apps the only time I ever need to set the status is when a form submits but doesn't succeed, in which case I render the form back with a 422.
I have implemented destroy actions and simply render back whatever I want the user to see next using a turbo_stream. I can replace the entire page or only a small part of it that way.
Note that I always respond to the turbo_stream format for form submissions and destroys, as that is the format for those requests. Your above examples don't appear to be doing that.
If you are set on redirecting, you should opt out of using Hotwire for the destroy action. You can do this by setting data-turbo="false" on the link or button. More here: https://turbo.hotwired.dev/handbook/drive#disabling-turbo-drive-on-specific-links-or-forms
But I would venture to say that redirecting inside a turbo_stream is not the intended use of turbo_streams and I wouldn't expect it to be supported.
seanpdoyle
commented
2 years ago But I would venture to say that redirecting inside a turbo_stream is not the intended use of turbo_streams and I wouldn't expect it to be supported.
@salex please correct me if I'm missing something, but I don't see any <turbo-stream> or <turbo-frame> elements in the example code you've shared.
Without either of those, the presence of Processing by PlayersController#destroy as TURBO_STREAM in the logs indicates that the form submission is being handled by Turbo, and Turbo is appending the Stream MIME type to the Accept header.
Since you're not declaring a format.turbo_stream block, the client should handle the redirect in the response as a full-page redirect.
My whole point on this semi-issue that if you've been working with Rail for a long time, this a big and unintuitive change - unless you go back and read the getting started guide. There was nothing in the upgrade guide about this.
I agree with you, it can be surprising and can lead to unintended side effects. With that being said, I'm curious if replacing the link_to call with a button_to call (see https://github.com/hotwired/turbo-rails/issues/301#issuecomment-1021381863) would make a status: :see_other or status: 303 in the controller unnecessary. Have you tried that combination yet?
WriterZephos
commented
2 years ago @seanpdoyle ah you make a good point about link_to vs button_to. One is a form submission and the other is not, so that could make a big difference.
As for the MIME type, I bet Turbo automatically does that for any links with turbo_method set to something that normally would be a form submission. If that's true, I don't think using button_to would make a difference since both will be turbo_stream requests, and should be handle accordingly.
Since you're not declaring a format.turbo_stream block, the client should handle the redirect in the response as a full-page redirect.
I am not sure it works that way. Turbo is going to intercept the response and try to parse it as a turbo_stream, so if you aren't sending a turbo_stream back it will break. The way to opt out of this behavior is to set data-turbo="false" on the link or form.
EDIT: I realized I might be wrong about it breaking, but it may not render with the layout as you would want for a normal redirect. Opting out is still probably a good idea.
seanpdoyle
commented
2 years ago If that's true, I don't think using button_to would make a difference since both will be turbo_stream requests, and should be handle accordingly.
My hunch is that the way that button_to- and link_to- generated elements will be handled differently.
A button_to-generated <form> element will look something like:
<form action="articles/123" method="post">
<input type="hidden" name="_method" value="delete">
<button>Destroy</button>
</form>Note the _method hidden field, and the [method="post"] on the form itself.
Turbo will transform a link_to-generated <a> element with turbo_method: :delete into a <form> element that it submits programmatically. That form's [method] attribute is assigned the value of the turbo_method: :delete option instead of [method="post"].
When handling that submission, Turbo's FormSubmission passes through the value of the [method] attribute from the <form> element to the fetch options behind the scenes.
At the HTTP level, the button_to-generated request is sent to the server as a POST with a _method=delete parameter. Rails translates that POST-_method pairing into a ArticlesController#destroy request. At the HTTP level, the link_to-generated request is submitted as a DELETE, without the _method. Rails still handles the request as a ArticlesController#destroy action, but the browser considers the two as distinctly different HTTP requests, so the way it handles the 302 might differ from how it handles the 303.
I am not sure it works that way. Turbo is going to intercept the response and try to parse it as a turbo_stream, so if you aren't sending a turbo_stream back it will break.
EDIT: I realized I might be wrong about it breaking, but it may not render with the layout as you would want for a normal redirect. Opting out is still probably a good idea.
@WriterZephos That should not be the case. If you're experiencing this, could you open a separate issue with some code to reliably reproduce that behavior?
sethherr
commented
2 years ago This tripped me up as well, from a migration perspective. I understand that button_to is preferable to link_to with a data-turbo-method, but it would make migration much easier if you didn't have to switch every link_to over and deal with the styling changes and the removal of confirmation popups.
Both status: :see_other and status: 303 fix the issue for me, so maybe I'll just go around and add status: :see_other to all the pertinent redirects.
seanpdoyle
commented
2 years ago There's an open pull request to hotwired/turbo-rails (https://github.com/hotwired/turbo-rails/pull/239) that transforms [formmethod] declarations into _method to avoid these types of surprises. A similar change might make sense for handling a fetch with a method: other than GET or POST in a similar way.
That type of fix would only benefit Rails-powered applications, but that's probably the case for many applications migrating away from @rails/ujs.
salex
commented
2 years ago @seanpdoyle @WriterZephos
I assume my use of the link_to:
= link_to icon('fas fa-trash','Destroy Round'), @round, title: 'Destroy it', class: 'btn btn-danger mr-2',
method: :delete, data: { turbo_method:'delete',turbo_confirm: 'Are you sure?'}created the turbo stream. but I didn't respond to a turbo_stream.
I initially tried to use button_to, but it doesn't have a confirm tag. My whole purpose was to have a confirm tag since my users have been confused by so many buttons,
I ended up using button_to, wrapped in a helper that uses a stimulus controller to display the confirm alert.
def destroyConfirmTag(model,confirm_msg:"",klass:"",prompt:"")
klass= "#{btnDanger} inline-block" if klass.blank?
confirm_msg = "Are You Sure?" if confirm_msg.blank?
prompt = "Delete #{model.class.name}" if prompt.blank?
node = content_tag(:div, class: klass,
data:{
controller:"destroyConfirm",
action:"click->destroyConfirm#confirm",
destroyConfirm_cmsg_value:confirm_msg
}){
concat(tag.span(prompt))
concat(button_to( '',model, method: :delete,class:"hidden",data:{destroyConfirm_target:"submit"}))
}
node
endimport { Controller } from "@hotwired/stimulus"
export default class extends Controller {
static targets = [ "submit"]
static values = { cmsg: String}
connect() {
if (this.hasCmsgValue) {
this.confirm_msg = this.cmsgValue
}else{
this.confirm_msg = "Are you sure?"
}
}
confirm(){
let ans = confirm(`${this.confirm_msg}`)
if (ans == true) {
this.submitTarget.closest('form').submit()
}
}
}In view I just do
= destroyConfirmTag(@round,
prompt: icon("fas fa-trash","Delete Unfinished Round"))Seems like a long way around doing something that was fairly easy. Again the problem didn't show up until I had a delete_tag on a show page with a redirect. That caused the unintended consequences of not knowing what I was doing!
seanpdoyle
commented
2 years ago I initially tried to use button_to, but it doesn't have a confirm tag.
I believe the Turbo implementation of UJS' [data-confirm] requires that the data-turbo-confirm attribute is declared on the <form> element. Have you tried using button_to with the form: option:
button_to icon('fas fa-trash','Destroy Round'), @round, title: 'Destroy it', class: 'btn btn-danger mr-2',
method: :delete, form: { data: { turbo_confirm: 'Are you sure?' } }@seanpdoyle
Have you tried using button_to with the form: option:
Well it seems to work! But like most Rails documentation you have to know how to read in between the lines - like where did you find data: { turbo_confirm: 'Are you sure?' } was an option for a form?
I think it's been mentioned before , button_to should be the default way to delete something.
Thank you for your help, and putting up with me,
seanpdoyle
commented
2 years ago where did you find data: { turbo_confirm: 'Are you sure?' } was an option for a form?
The only place in the Turbo documentation that mentions the Rails UJS analogs is Performing Visits with a Different Method. There's certainly a gap in documenting those newer features.
dhh
commented
2 years ago Feel free to open a PR on this repo with better explanations on this integration with Rails. Feels like it needs to live here, not in Rails.
Rails 7.0.1 Ruby 3.0.1
This is probably a Rails issue, but it originates from turbo-rails
In converting to Rails 7, one of my biggest problems was to overcome the loss of the confirm message when deleting a record. This was apparently also a problem with the Rails team in that there were many pull issues/discussions dealing with this.
Most leaned toward using
button_towith a delete method, which works fine - but there is no confirm tag. I used this in Rail 7.0.0 and added adelete_button_tohelper that relied on a stimulus controller to display the confirm message.Until a week or so, the
Getting started with Rail guidestill had the Rails.ajax link decribing how to delete a record. It has now been changed toThis works fine - if your calling it from a normal rails scaffold link.
If you use this in a show page, you are in for BIG problems. I have done this for years in some models where a `belongs_to" model was rarely edited or deleted, unless there was a problem with the child record.
Take for instance the below app/models. This is a Golf Group management application - where group player show up to play a Golf game. Player are added to the game by adding an unscored rounds. Teams may be formed, score cards printed, and they go out to play the game. After the game is completed, the unscored rounds are scored. What happens if one of the player become ill or has the leave the game before its completed? You need to delete the unscored round before you enter the scores.
Not knowing the concequeces, I decided to try the approch used in the guide and get away from my helper.
Round show view.
Round controller
BOOM
Not being a Rails expert, the `turbo_method:'delete' appear to assume a turbo_frame on the show page and tries to replace it after the delete, but there is no longer a matching round.
It then decides that not only is it going the delete the requested round, it going to delete all the players rounds, after it deletes the Player!
The development log (condensed)
Nowhere in the Guide did it say I should of put in a respond to turbo_frame or take any other kind of action. I just wanted to delete a specific record.
I think this is dangerous!!!