respond configuration method

[HoneyryderChuck]

most/all success responses to POST actions involve setting a flash message and a redirect response. This abstract it by having a method which does both, and can be overridden if the user wants to do something else, like rendering a success page, run some js, smth else.

from #368

[HoneyryderChuck]

@jeremyevans what do you think of this approach? If you agree, I can apply the same substitution inn other routes.

[jeremyevans]

I'm not sure how applicable this is to all features. From my earlier review of the login feature, it wouldn't work there. Can you do an analysis of the places where you could use this?

[HoneyryderChuck]

I've seen the same being applied in all of the other cases where a "flash then redirect" pattern is applied. You're right, login is the exception here. I guess one could redefine the function in it:

def login_response(saved_redirect)
  return super unless saved_redirect
  # otherwise, set flash and redirect here
end

The name for such a pattern couldd be improved ("smth_response" is a bit too broad). "flash_redirect_login"? Smth else?

Another thing that came up was that all of this would now be done via a function with define_method, which is (correct me if I'm wrong) slower than using def. Is this a concern? Should class_eval be used instead?

[jeremyevans]

define_method is slower than def all else being equal (less so starting in Ruby 3.3 due to optimizations I worked on). The other reason class_eval would be faster is you could skip using send. However, I'm not sure the optimization is worth it in this case. I would stick with define_method.

If login is the only odd duck here, let's apply this pattern to the other features, and define login_response manually instead of using response (let's switch the feature method from respond to response because it defines methods ending in response).

[HoneyryderChuck]

done.

[jeremyevans]

Looks mostly good. A few minor issues, please see inline changes.

Additionally, there needs to be at least one spec that uses a *_response auth method, and all auth methods added must be documented (run rake check_method_doc to check documentation).

[jeremyevans]

This moves the code from inside the transaction block to outside. Not a big deal in this case, but we should probably attempt to avoid unnecessary changes when refactoring.

[HoneyryderChuck]

I figured this was an overlook from the initial implementation, given you probably don't want to enforce the redirection from within the transaction (and none of the other implementations do?). Was I wrong in this assessment?

[jeremyevans]

No. I think the change makes sense. However, it's probably better as a separate commit.

[jeremyevans]

login_response makes sense as an auth method, but not as an auth value method.

[HoneyryderChuck]

can you clarify what's the distinction? FTR I've been defining only auth_value_methods in rodauth-oauth, and perhaps i shouldn't, but looking at the source code it seems that they're almost the same, except value methods allow for values instead of routines?

I'll do the change meanwhile.

[jeremyevans]

auth_value_method is for methods where it may make sense to assign a static value, that does not depend on runtime state. That's not likely for login_response, since by default, it redirects (throws), it does not return a value. It's also inconsistent with the response feature method, since that adds an auth_method and not an auth_value_method.

[jeremyevans]

Thanks for the changes. I'll try to test and merge on Monday.

[jeremyevans]

@HoneyryderChuck I merged this and added some more changes on top of it. Can you review 97477ce and 608b329 and provide your thoughts?

[HoneyryderChuck]

The first commit looks ok. The second commit... I understand why you're doing it. I've dealt with that limitation when first writing the test. Still, it's something that I tripped over in the past regarding cleanly returning from a route. In a nutshell, the problem is the following:

change_account_view
puts "1" #=> executes

redirect some_path
puts "2" #=> never does

I guess this quirk is more of a roda render plugin than rodauth itself, but ideally (IMO) calling a _view method should immediately yield control back to the router, just like redirect does. I at least find it always surprising when the former doesn't. Just to provide anecdotal comparison, it's a behaviour I also find confusing in rails, where neither rendering a view nor redirecting prevent user code from executing (and potentially overriding the previous statement). While roda does it better for redirects, not doing it for views as well feels as confusing.

Nevertheless, this is just an opinion, and even if you agree, it'd be a bigger change than this MR would require, and the second commit does prevent users from shooting themselves in the foot, so 👍

[jeremyevans]

Rendering does not necessarily imply anything to do with the response body. A Roda route block may want to render, use the result of the render to send an email, include the result as part of a database query or web request, and then redirect afterward. It is common to use the result of a render as a response body, and Roda makes it easy, but rendering is not tied to the response like redirecting is. That's an important fundamental difference between rendering and redirecting.

In Rodauth, the view methods are always the last methods called in a route block, so the return value is used as the response body, and therefore an explicit halt is not needed. The reason for the second commit is to avoid undefined behavior if the user overrides a response method without understanding what is required. I think it is less likely a user will misunderstand what is needed when overriding a view method. Additionally, overriding a view method incorrectly does not have the same failure cases as overriding a response method incorrectly. Therefore, I don't think we need to take similar precautions for view methods that we take for response methods.

[HoneyryderChuck]

It is common to use the result of a render as a response body, and Roda makes it easy, but rendering is not tied to the response like redirecting is.

That may be so, it's just that perhaps I'd be expecting to have something that'd make the "render html and return response" more straightforward, so I wouldn't be caught accidentally doing:

if smth
  error_page_view
end
original_page_view

But I'm ok with it not really being considered a problem. Something that I should perhaps address as a discussion later on.

Thx again!