Change return codes to new Responders defaults

[tagliala]

Change default error and redirect status codes
Update responder class to the current defaults for responder gem

Close: #862

Ref: heartcombo/responders#240

Supersedes #863

---

I'm doing some tests with this branch and $ bin/rake local server with Active Admin main

Action	Method	Status Before	Status After	Match Rails
Create category	POST	302 Found	303 See Other	❌ (302, but 303 is better)
Create category (fail validations)	POST	200 OK	422 Unprocessable Entity	✅
Update category	POST	302 Found	303 See Other	❌ (302, but 303 is better)
Update category (fail validations)	POST	200 OK	422 Unprocessable Entity	✅
Destroy category	POST	302 Found	303 See Other	✅

Rails by default returns 302 on successful create and update, and 303 only on destroy. However, according to HTTP/1.1 standard, it appears that 303 is the correct status code to redirect from a POST/PATCH/PUT/DELETE to a GET and clients do treat 302 like a 303, by using GET instead of using the original method

References that motivate the decision:

• Add redirect_code_for_unsafe_http_methods config rails/rails#45393

• Clarification on redirect status code (303) hotwired/turbo#84

• https://github.com/heartcombo/responders?tab=readme-ov-file#configuring-error-and-redirect-statuses

  Note: the application responder generated for new apps already configures a different set of defaults: 422 Unprocessable Entity for errors, and 303 See Other for redirects. Responders may change the defaults to match these in a future major release.

• https://en.wikipedia.org/wiki/HTTP_302

  Many web browsers implemented this code in a manner that violated this standard, changing the request type of the new request to GET, regardless of the type employed in the original request (e.g. POST).[1] For this reason, HTTP/1.1 (RFC 2616) added the new status codes 303 and 307 to disambiguate between the two behaviours, with 303 mandating the change of request type to GET, and 307 preserving the request type as originally sent. Despite the greater clarity provided by this disambiguation, the 302 code is still employed in web frameworks to preserve compatibility with browsers that do not implement the HTTP/1.1 specification.[2]

  (emphasis mine)

• https://datatracker.ietf.org/doc/html/rfc2616#section-10.3.4

  If the 302 status code is received in response to a request other
  than GET or HEAD, the user agent MUST NOT automatically redirect the
  request unless it can be confirmed by the user, since this might
  change the conditions under which the request was issued.

  Note: RFC 1945 and RFC 2068 specify that the client is not allowed
  to change the method on the redirected request. However, most
  existing user agent implementations treat 302 as if it were a 303
  response, performing a GET on the Location field-value regardless
  of the original request method. The status codes 303 and 307 have
  been added for servers that wish to make unambiguously clear which
  kind of reaction is expected of the client.

  (emphasis mine)

Default responder created by rails g responders:install: https://github.com/heartcombo/responders/blob/9bdc60dfbfa8001641c1c4df7bc73c3fc2a4cf41/lib/generators/responders/install_generator.rb#L10-L25

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.92%. Comparing base (6d716df) to head (bf8be73).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #918   +/-   ##
=======================================
  Coverage   98.91%   98.92%           
=======================================
  Files          14       14           
  Lines         555      557    +2     
=======================================
+ Hits          549      551    +2     
  Misses          6        6           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tagliala]

@javierjulio

As per https://github.com/heartcombo/responders?tab=readme-ov-file#configuring-error-and-redirect-statuses, newly generated application responders default to

self.error_status = :unprocessable_entity
self.redirect_status = :see_other

(and also include include Responders::HttpCacheResponder)

So it may have sense to change those fields, regardless of Turbo support, and release a major version

[javierjulio]

Can you please retest this without the Responders::HttpCacheResponder inclusion? When retesting, can you please make sure that forms for both new/create and edit/update, work as expected? Both success and failure scenarios. I just want to make sure there isn't any regressions there for ActiveAdmin. I don't believe there would be.

[tagliala]

Updated with a ref to a9fb8e1

[javierjulio]

Thank you for including the reference. It makes sense why it was changed. Referencing the "why" is always helpful so I appreciate it.

How did retesting this go with ActiveAdmin? I'd like to ensure that submitting forms, both successful and with validation errors, works as expected. Essentially, no regressions.

Do you think we should release this in a major version? ActiveAdmin is locked to v1.x for InheritedResources so earlier version won't be able to use it so releasing a major version of this is safe. We could permit it in ActiveAdmin v4 though since it's still beta.

[tagliala]

How did retesting this go with ActiveAdmin? I'd like to ensure that submitting forms, both successful and with validation errors, works as expected. Essentially, no regressions.

According to my tests in the default test app, there are no visible changes. I will run active admin specs against this branch

Do you think we should release this in a major version?

It is a potential breaking change with some custom javascript expecting a 200 from the server, so in any case it makes sense to release in a major version

ActiveAdmin is locked to v1.x for InheritedResources so earlier version won't be able to use it so releasing a major version of this is safe. We could permit it in ActiveAdmin v4 though since it's still beta.

Probably it is a good choice

[javierjulio]

Thanks! Let's release as v2 then.

[tagliala]

All Active Admin specs (both master and 3-0-stable) pass against this branch

---

Screenshots

Before

After

[tagliala]

@javierjulio rebased

• Should I bump the major in this branch?
• Should we release a beta with this change?

[javierjulio]

@tagliala awesome thanks. I think this is safe as a major release, no beta release needed. You are welcome to bump the major version here or in another PR, I'm fine with either. Thank you.