Controller CRUD resources — refactoring workflow

[nimmolo]

I know I said GraphQL would obviate refactoring the controllers, and now i'm eating my words. Buen Provecho! 🥝

In a potential API/separate-front-end app, obviously routes would want to follow a similar regular pattern to the Rails defaults, so I'm thinking refactoring the controllers and updating paths ahead of time would "clear the path(s)" for minimal eventual URL disruption in the future. As a bonus, it would also reaquaint me with some of the things the app does now, that I have never used.

I'd like to start splitting up ObserverController and refactoring other controllers to emulate @JoeCohen 's redo of HerbariaController. That improved a lot, but the primary thing i'm interested in is changing routes to default resources routes like show and index, regularizing CRUD, and providing redirects for the old routes. I also want to leave the controller code in better shape than I found it -- there's a chance we'll decide against the GraphQL/Svelte/React front end, after all.

Currently I'm working on UsersController, since it keeps raising its hand (via CodeClimate noticing the TODO). Hopefully it'll get easier following Joe's example, and ideally I'd love to work on it together, Joe I don't know if this is something you'd like to do. So I wanted to open this discussion to make some space to record the practices that work for MO. Anyone - please feel free to dump any notes you've made or pointers or TODOs! And Joe, I apologize for saying this would not be necessary, after all your work.

[nimmolo]

One question: I wonder if we really need to redirect the POST/PATCH/DELETE paths in routes.rb...

I'm thinking the only valid source of any request matching those paths is the live app itself. (You can't bookmark an "update observation" request... i mean you could, but it wouldn't work). If the view that creates such a request uses the new path, then everybody uses the new path.

So it would seem that only paths using GET need redirecting...?

[pellaea]

re: no need to redirect old non-GET requests -- Yes, totally agree with you.

…

On Fri, Jun 17, 2022 at 2:17 PM andrew nimmo ***@***.***> wrote: One question: I wonder if we really need to redirect the POST/PATCH/DELETE paths in routes.rb... I'm thinking the only valid source of any request matching those paths is the live app itself. (You can't bookmark an "update observation" request... i mean you could, but it wouldn't work). If the view that creates such a request uses the new path, then everybody uses the new path. So it would seem that only paths using GET need redirecting...? — Reply to this email directly, view it on GitHub <#1031 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYTNNOBMURNMRGQNVCBNF3VPS6LTANCNFSM5ZDAVJCA> . You are receiving this because you are subscribed to this thread.Message ID: <MushroomObserver/mushroom-observer/repo-discussions/1031/comments/2973674 @github.com>

[JoeCohen]

@nimmolo: Yes, I would like to work on this?
And I never thought about the redirect issue you mention. It sure would make things easier to redirect only show and index.
(And we can also prune some of the routes that I needlessly redirected.)

[nimmolo]

@JoeCohen Glad to hear that despite the difficulty you're still up for controller refactoring.

@pellaea This is the discussion thread, i believe you must get these via email, but this one's on the web here.
Responding to your question on the PR thread, so it's here for reference independently of the PR.

Yes, I was also getting confused about when to name the parameter explicitly and when not to. Thanks for catching the additional need for params: {} in get/post in the controller tests! So, can you help me understand the path helpers even more? Are there two general forms:

controller_action_path(param: hash) -- e.g., observer_how_to_help_path()
resource_method_path(object_or_id) -- e.g., user_path(252) or ... user_post_path(form_data?)???

What I finally seem to have figured out is that the rails routes dump is my "source of truth" for paths. (Not the routes.rb file, which is hard to figure out.) To get the most up-to-date reference about which routes need which parameters, dump routes on a refactored controller branch.

So here are the new refactored routes for UsersController.
In this table, "Prefix" is the route or url helper prefix, e.g. users_by_name_path, and the last column specifies the parameters needed for the path. What I am noticing is, the fact that the /:id is in these paths without parentheses, means that they should be called directly as in user_path(@user.id)

vagrant@ubuntu-focal:/vagrant/mushroom-observer$ rails routes -c=UsersController
               Prefix Verb  URI Pattern                      Controller#Action
users_by_contribution GET   /users/by_contribution(.:format) users#by_contribution
        users_by_name GET   /users/by_name(.:format)         users#by_name
                users GET   /users(.:format)                 users#index
            edit_user GET   /users/:id/edit(.:format)        users#edit {:id=>/\d+/}
                 user GET   /users/:id(.:format)             users#show {:id=>/\d+/}
                      PATCH /users/:id(.:format)             users#update {:id=>/\d+/}
                      PUT   /users/:id(.:format)             users#update {:id=>/\d+/}

Here are a small number of the non-refactored routes for ObserverController. Hypothesis: for example in observer_show_observation_path, the parentheses around (/:id) imply that the param needs to be named. I don't know yet, will test and revise this answer. Watch this space :)

vagrant@ubuntu-focal:/vagrant/mushroom-observer$ rails routes -c=ObserverController
                               Prefix Verb     URI Pattern                                              Controller#Action
                                 root GET      /                                                        observer#list_rss_logs
          observer_show_notifications GET      /observer/show_notifications(.:format)                   observer#show_notifications
                                      GET|POST /observer(/show_notifications(/:id))(.:format)           observer#show_notifications {:id=>/\d+/}
                    observer_show_obs GET      /observer/show_obs(.:format)                             observer#show_obs
                                      GET|POST /observer(/show_obs(/:id))(.:format)                     observer#show_obs {:id=>/\d+/}
            observer_show_observation GET      /observer/show_observation(.:format)                     observer#show_observation
                                      GET|POST /observer(/show_observation(/:id))(.:format)             observer#show_observation {:id=>/\d+/}
                observer_show_rss_log GET      /observer/show_rss_log(.:format)                         observer#show_rss_log

[pellaea]

I'm following, thank you, Nimmo. Okay, so what are the corresponding path helpers for the users controller? I think I know most, but not the last.

users_by_contribution_path
users_by_name_path
users_path
edit_user_path(id) ← ??
user_path(id)
???_path(id) ← PATCH or PUT

[nimmolo]

This is kind of neat about the generated resource routes — there's far fewer of them.
edit_user_path(id) gets you the form.
But you just put or patch to user_path(id).
The HTTP verb lets Rails know which controller action to route to, you'll find them in the right column.

It seems a lot cleaner than current routes, where we have a lot of show-only pages that are automatically getting additional post routing, the way our routes.rb is currently written.

[nimmolo]

re: why three indexes? -- The first (/observer/index) is just a routing thing. Nowadays it would be done in config/routes.rb instead. The remaining pair is repeated in all the controllers. One is the main entry point for doing an index of whatever object: it creates the query. The second is the re-entry point if you've already started a query and are resorting it or changing the filter parameters or whatever. The query itself is passed back in via the "q" parameter. Any index can be further modified on the fly with a bunch of other parameters -- e.g., "by" to change the order, "page" and "letter" for pagination, and I think in the unique case of rss_logs "type" or "types"... whatever parameter is used to allow users to specify which type or types they wish to see in the activity log. Furthermore, there are additional entry points in some cases, especially for observations. You can get to a page of observation results by several paths: all observations, observations by a given user, search results, advanced search results, observations for a species list, observations of a name, observations at a location, etc. All of these use show_selected_observations(query) to display the actual results. But then the sorting and pagination and prev/index/next controls all send the user back to list_observations. It's the general purpose index that just takes whatever query is stored in "q", modifies it according to the sorting, pagination, type filter and whatever other modifier parameters are available no matter which entry point the user arrived at, and displays the result. It probably would be okay to merge index_observations into list_observations, and just make sure that list_observations provides appropriate defaults -- just make sure "q" and "by" and all the rest are not passed in if you want it to create a new query! I thought it was simpler and made more sense to keep the actions which created queries (index_observations, etc.) separate from the general purpose re-entry action (list_observation). At very least I probably could have named list_observations better! As you are the one doing all the tremendous amount of work involved in refactoring these behemoths, I say you get to choose going forward whatever makes most sense to you. The above was my reasoning. But my day is done. This is a bright new day! Enjoy!

@pellaea , Thank you for the explanation. Moving this also to the refactor discussion since it applies to other controllers.

How about something like this, routing everything to index and handling the "curation" via params?

  def index
    # Fresh version of the index, no existing query
    if !params[:q].present?
      query = create_query(:RssLog, :all,
                           type: @user ? @user.default_rss_type : "all")
    # POST requests with param `show_#{type}``: potentially show multiple types 
    # of objects. These requests come from the checkboxes in tabset
    elsif request.method == "POST"
      types = RssLog.all_types.select { |type| params["show_#{type}"] == "1" }
      types = "all" if types.length == RssLog.all_types.length
      types = "none" if types.empty?
      types = types.map(&:to_s).join(" ") if types.is_a?(Array)
      query = find_or_create_query(:RssLog, type: types)
    # GET requests with param `type``: show a single type of object
    # These come from simple links: the tabs in tabset
    elsif params[:type].present?
      types = params[:type].split & (["all"] + RssLog.all_types)
      query = find_or_create_query(:RssLog, type: types.join(" "))
    # Unfiltered
    else
      query = find_query(:RssLog)
      query ||= create_query(:RssLog, :all,
                             type: @user ? @user.default_rss_type : "all")
    end
    show_selected_rss_logs(query, id: params[:id].to_s, always_index: true)
  end

Question: Does the first condition (no query) partly duplicate the last?

Here for RssLogs, I'd like to make the "type" param incorporate the "show_#{type}" param.
(Currently it's either &type=location or &show_location=1&show_observation=1)

I'm picturing the type param value could be something like &type=location,name,observation&q=234lkdfu
and then the checkbox form and index method would be modified to build or parse that joined array.

[pellaea]

Thanks, Nimmo. (For moving into discussion. Oh shoot, is this in the discussion now?... Ah yes, it is, good!) re: are the first and last cases somewhat redundant? -- Yes, by necessity, I think. The problem is the "q" parameter can expire. In that case it should fall back on the standard, primary, "display all objects" index, which of course is the first case. Hmmm, just throwing this out there, you could have it instead redirect back to the same action(!) without the "q" parameter if the query has expired. It should happen only very rarely. And, in fact, it gives an opportunity to include a warning message that the query has expired, for example, too. Then there will be no duplication of code, and no need for contorted control flow in the code. It also makes the URL reflect better what is actually being displayed -- to wit, the standard index without any parameters. re: your proposed handling of type/types/show_type/whatever -- Sounds fine. I don't know that anyone actually ever uses those features to start with, and I certainly do not, so I don't really care that much how they behave or are implemented. Whatever makes most sense to you. You have a better feel for UI and modern coding standards than I do, so I trust you. Fortunately, the other controllers are not burdened with that type crap.

…

On Mon, Jun 20, 2022 at 2:55 PM andrew nimmo ***@***.***> wrote: re: why three indexes? -- The first (/observer/index) is just a routing thing. Nowadays it would be done in config/routes.rb instead. The remaining pair is repeated in all the controllers. One is the main entry point for doing an index of whatever object: it creates the query. The second is the re-entry point if you've already started a query and are resorting it or changing the filter parameters or whatever. The query itself is passed back in via the "q" parameter. Any index can be further modified on the fly with a bunch of other parameters -- e.g., "by" to change the order, "page" and "letter" for pagination, and I think in the unique case of rss_logs "type" or "types"... whatever parameter is used to allow users to specify which type or types they wish to see in the activity log. Furthermore, there are additional entry points in some cases, especially for observations. You can get to a page of observation results by several paths: all observations, observations by a given user, search results, advanced search results, observations for a species list, observations of a name, observations at a location, etc. All of these use show_selected_observations(query) to display the actual results. But then the sorting and pagination and prev/index/next controls all send the user back to list_observations. It's the general purpose index that just takes whatever query is stored in "q", modifies it according to the sorting, pagination, type filter and whatever other modifier parameters are available no matter which entry point the user arrived at, and displays the result. It probably would be okay to merge index_observations into list_observations, and just make sure that list_observations provides appropriate defaults -- just make sure "q" and "by" and all the rest are not passed in if you want it to create a new query! I thought it was simpler and made more sense to keep the actions which created queries (index_observations, etc.) separate from the general purpose re-entry action (list_observation). At very least I probably could have named list_observations better! As you are the one doing all the tremendous amount of work involved in refactoring these behemoths, I say you get to choose going forward whatever makes most sense to you. The above was my reasoning. But my day is done. This is a bright new day! Enjoy! @pellaea <https://github.com/pellaea> , Thank you for the explanation. Moving this also to the refactor discussion since it applies to other controllers. How about something like this, routing everything to index and handling the "curation" via params? def index # Fresh version of the index, no existing query if !params[:q].present? query = create_query(:RssLog, :all, type: @user ? @user.default_rss_type : "all") # POST requests with param `show_#{type}``: potentially show multiple types # of objects. These requests come from the checkboxes in tabset elsif request.method == "POST" types = RssLog.all_types.select { |type| params["show_#{type}"] == "1" } types = "all" if types.length == RssLog.all_types.length types = "none" if types.empty? types = types.map(&:to_s).join(" ") if types.is_a?(Array) query = find_or_create_query(:RssLog, type: types) # GET requests with param `type``: show a single type of object # These come from simple links: the tabs in tabset elsif params[:type].present? types = params[:type].split & (["all"] + RssLog.all_types) query = find_or_create_query(:RssLog, type: types.join(" ")) # Unfiltered else query = find_query(:RssLog) query ||= create_query(:RssLog, :all, type: @user ? @user.default_rss_type : "all") end show_selected_rss_logs(query, id: params[:id].to_s, always_index: true) end Question: Does the first condition (no query) partly duplicate the last? Here for RssLogs, I'd like to make the "type" param incorporate the "show_#{type}" param. (Currently it's either &type=location or &show_location=1&show_observation=1) I'm picturing the type param value could be something like &type=location,name,observation&q=234lkdfu and then the checkbox form and index method would be modified to build or parse that joined array. — Reply to this email directly, view it on GitHub <#1031 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYTNNMMIRIRSQCGS627JALVQC5B7ANCNFSM5ZDAVJCA> . You are receiving this because you were mentioned.Message ID: <MushroomObserver/mushroom-observer/repo-discussions/1031/comments/2988275 @github.com>

[nimmolo]

Thanks - Which of those conditions (in my condensed index) catches if q has expired?
Should the "no-q" condition come rather last? Like this:

  def index
    # POST requests with param `show_#{type}``: potentially show multiple types
    # of objects. These requests come from the checkboxes in tabset
    if request.method == "POST"
      types = RssLog.all_types.select { |type| params["show_#{type}"] == "1" }
      types = "all" if types.length == RssLog.all_types.length
      types = "none" if types.empty?
      types = types.map(&:to_s).join(" ") if types.is_a?(Array)
      query = find_or_create_query(:RssLog, type: types)
    # GET requests with param `type``: show a single type of object
    # These come from simple links: the tabs in tabset
    elsif params[:type].present?
      types = params[:type].split & (["all"] + RssLog.all_types)
      query = find_or_create_query(:RssLog, type: types.join(" "))
    # Unfiltered
    elsif params[:q].present?
      query = find_query(:RssLog)
      query ||= create_query(:RssLog, :all,
                             type: @user ? @user.default_rss_type : "all")
    # Fresh version of the index, no existing query
    else
      query = create_query(:RssLog, :all,
                            type: @user ? @user.default_rss_type : "all")
    end
    show_selected_rss_logs(query, id: params[:id].to_s, always_index: true)
  end

[pellaea]

The only way to tell if the query has expired is to try to look it up in the query_records table. That's what find_query() does. It looks like it returns nil if the query_record is no longer there.

…

On Mon, Jun 20, 2022 at 3:28 PM andrew nimmo ***@***.***> wrote: Thanks - Which of those conditions (in my condensed index) catches if q has expired? Should the "no-q" condition come rather last? Like this: def index # POST requests with param `show_#{type}``: potentially show multiple types # of objects. These requests come from the checkboxes in tabset if request.method == "POST" types = RssLog.all_types.select { |type| params["show_#{type}"] == "1" } types = "all" if types.length == RssLog.all_types.length types = "none" if types.empty? types = types.map(&:to_s).join(" ") if types.is_a?(Array) query = find_or_create_query(:RssLog, type: types) # GET requests with param `type``: show a single type of object # These come from simple links: the tabs in tabset elsif params[:type].present? types = params[:type].split & (["all"] + RssLog.all_types) query = find_or_create_query(:RssLog, type: types.join(" ")) # Unfiltered elsif params[:q].present? query = find_query(:RssLog) query ||= create_query(:RssLog, :all, type: @user ? @user.default_rss_type : "all") # Fresh version of the index, no existing query else query = create_query(:RssLog, :all, type: @user ? @user.default_rss_type : "all") end show_selected_rss_logs(query, id: params[:id].to_s, always_index: true) end — Reply to this email directly, view it on GitHub <#1031 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYTNNOW5MZQFIFO3BTHFU3VQDA4HANCNFSM5ZDAVJCA> . You are receiving this because you were mentioned.Message ID: <MushroomObserver/mushroom-observer/repo-discussions/1031/comments/2988447 @github.com>

[nimmolo]

Further notes on controller/controller_test refactoring, from ObservationsController:

• In the controller, when splitting the "create_{object}" and "edit_{object}" methods, be careful that any necessary form defaults are set for the create and update methods, as they are for new and edit. In other words, ensure defaults are as they were before, when requests were routed through a single create_object or edit_object form for both new/create and edit/update forms.
• In new form templates, the action should be create. The HTTP method for the create route remains post as before, and the form_tag doesn't need further modification.
• In edit form templates, the action should be update, but the form_tag needs a further param: method: :put (or method: :patch). HTTP post requests won't work with the default resource :update route, and test methods need to be changed to send put (or patch) requests to the update action. (Documentation here. Note that form_tag and form_for are now soft-deprecated in favor of form_with, but i'm not making those changes this round.)
• I added corresponding put_with_dump, put_requires_user etc methods to the controller_extensions.
• I edited test names to more clearly reflect the routes, so that something like test_edit_get becomes something like test_load_edit_form. Whereas, test_edit_post, test_edit_{whatever} or any test that actually posts puts an update, is now test_update_{whatever}.
• #{model}_search action needs to supply a default no_hits_title title to show_index_of_objects
• Be sure create and update re-render the new and edit forms on error
• Links to destroy must have method: :delete

[pellaea]

Oh geez, of course! The path is the same, so obviously the path helper is the same. Duh. :)

…

On Sun, Jun 19, 2022 at 11:40 PM andrew nimmo ***@***.***> wrote: This is kind of neat about the generated resource routes — there's far fewer of them. edit_user_path(id) gets you the form. But you just put or patch to user_path(id). The HTTP verb lets Rails know which controller action to route to, you'll find them in the right column. It seems a lot cleaner than current routes, where we have a lot of show-only pages that are automatically getting additional post routing, the way our routes.rb is currently written. — Reply to this email directly, view it on GitHub <#1031 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYTNNOXLHTYXKBVVBAONDTVP7R2FANCNFSM5ZDAVJCA> . You are receiving this because you were mentioned.Message ID: <MushroomObserver/mushroom-observer/repo-discussions/1031/comments/2982666 @github.com>