Observations::LocationsController, not Locations::MergesController

[nimmolo]

• Renames controller to disambiguate what it does.
• Alters routes and path helpers accordingly.
• Slightly updates the form HTML.

---

This PR refactors two controllers I created without really understanding what they did: Locations::MergesController (containing only one "new" GET form action, and an unused "create" route) and Locations::ObservationsController (containing only an "update" PUT/PATCH callback... for the other controller's "new" form!).

It combines these two controllers into a new Observations::LocationsController, with an edit and update action, and adjusts routes accordingly. The routes are not used outside a couple places in the site.

The edit form provides a UI to deal with Observations that have a given where attribute but no Location. In other words, these obs just have a "where" string a user entered, that maybe matches one or more of our Locations. Clicking a link in the form will associate all Observations that have the same "where" string with the matched Location.

To be clear, this form does not (never did) deal with Location record merges, which can only be done manually by a web admin. It provides a bulk assignment of a Location association to all matching Observations, where there was no association before. It seems to belong more properly under "Observations", since it updates "Observation" records.

[coveralls]

coverage: 94.525% (+0.002%) from 94.523%
when pulling 10797a6 on nimmo-observations-locations-controller
into 0e94b1f on main.

[nimmolo]

@JoeCohen For review, I'd just like to ask for you to give it a front-end try-out, to be sure that this works exactly as previously. Not having used it before, I don't know!

The link to this form appears on the index for "Observations at where", which i believe should show all observations with a given "undefined" location (that is to say, no location, only a "where").

[nimmolo]

@JoeCohen This is ready to go, but I won't merge it until you've certified it.

[JoeCohen]

1. Merger of two defined Locations worked fine. You should be able to create a controller, capybara, or system test for this. I don't have time to do that today.

• Login as someone with admin privileges.
• Turn on admin mode. Click on the A icon near the top right. (or goto http://localhost:3000/admin/session?turn_on=true)
• Edit one Location.
• Input the name of second Location.
• Save
• appropriate assertions. Perhaps first location is gone, second location exists and has not about merger.

2. I couldn't use the merger form, but that might be the results of the particular strings I was using. Probably also system-testable. Here's what I think I did:

• Create Observation
• Type name of a non-existent location. In my case I used State of Oregon, USA.
• Click Create button
• It will give flash warning "Unrecognized state or province ..."
• Click Create button again.
• It creates the Observation, and redirects you to the Create Location page.
• Go to /locations without creating the Location.
• It should list the existing locaiton in the lh column and an undefined location in the rh column
• Click Merge
• Click one of the Locations
• add appropriate assertions

The problem I ran into was after creating an undefined location State of Oregon, USA and clicking Merge, it did not include Oregon, USA as one of the options.
Please consider ignoring this issue because (1) it's obscure, and (2) I can work around it (click on the undefined location, edit the name to == the name of a defined location).

[nimmolo]

@JoeCohen - this is very helpful.
re:

1. I don't think i touched any code relating to merging two defined locations, but i can try to write a test for it. I gather from what you're saying that when you're editing a location in admin mode and you try to edit a location that matches an existing location name, we return location merge options to the form during validation. I'm guessing that happens via form_location_feedback, but I haven't checked into it.
2. I noticed the same thing - using a string that should be similar to existing locations, it didn't come up with any matching locations when I tried this in dev mode. I don't think that's ok, that means something is not working. I'm going to check now if it's working on main

[JoeCohen]

For # 1, there's no form involved, and it works fine. So if you haven't touched any relevant code you should probably just ignore it. (If in admin mode, I edit location A, changing its name to be the same as the name of location B, it merges A into B as soon as I click Update.)

…

On Mon, Feb 19, 2024 at 11:33 AM andrew nimmo ***@***.***> wrote: @JoeCohen <https://github.com/JoeCohen> - this is very helpful. re: 1. I don't think i touched any code relating to *merging* two defined locations, but i can try to write a test for it. I gather from what you're saying that when you're *editing* a location in admin mode and you try to edit a location that matches an existing location name, we return location merge options to the form during validation. I'm guessing that happens via form_location_feedback, but I haven't checked into it. 2. I noticed the same thing - using a string that should be similar to existing locations, it didn't come up with any matching locations when I tried this in dev mode. I don't think that's ok, that means something is not working. — Reply to this email directly, view it on GitHub <#1920 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALDFCXXTYK2IQFVJYGR73YUOSHVAVCNFSM6AAAAABDNZ4XF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJTGA3DEMJVHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[nimmolo]

@JoeCohen - for 2, on main, after creating the Obs with an undefined location, when I go to merge the undefined location "USA, State of Oregon" into "USA, Oregon, Malheur Co., Ontario" I get a success message "Successfully merged location USA, State of Oregon into USA, Oregon, Malheur Co., Ontario."

But the observation I just made with "State of Oregon" does not then have its location updated. So I don't think this is working on main currently either.

For now I'm going to try to debug it on this branch, rather than try to sort out main retroactively.

[nimmolo]

@JoeCohen - Sorted out the bug in 2, which is also on main. (One problem was that we were searching for the user's "location_format name" in the name column, which in some cases is not the right string to search with.)

I also noticed that this page can return an unmanageably long page of results.

• To introduce paging (and thereby speed up this page for admins) i'm combining @matches and @others. The links in each list are identically formed and do the same thing, so it's just a case of "better matches come first". @matches still come at the top when combined with @others.
• I've also attempted to improve the algorithm for @others. Notes in the controller. Tried it on several types of undefined location where strings, seems to improve relevance of results, but some are hard to match.
• Fixed the "confirm" message (note, these must now be data: { turbo_confirm: "cannot be undone" })
• Fixed formatting of location name in flash message

[JoeCohen]

Everything went smoothly with the new form.
Thanks for re-doing this, fixing the old bug, and adding the test.

[nimmolo]

Deployed