Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pattern search forms #2337

Draft
wants to merge 95 commits into
base: main
Choose a base branch
from
Draft

Pattern search forms #2337

wants to merge 95 commits into from

Conversation

nimmolo
Copy link
Contributor

@nimmolo nimmolo commented Aug 26, 2024

  • This PR creates a new “faceted search” form for Observation and Name. These live at new endpoints, names/search/new and observations/search/new, with new controllers, e.g. Names::FiltersController.
  • The forms are different per model because the keywords available in Query for each model are different.
  • This also enables writing regular controller tests for each form.
  • In each form, there’s a field for each search keyword (param). The form commits to the same controller’s create action and sends the params as a regular param hash.
  • Even though these are separate new form endpoints, the UI would eventually render them in a single modal form for pattern searches.
    • When the user selects the model they’re searching, Stimulus calls the model’s controller endpoint (via requestjs, similar to AJAX) to get the search form.
    • Turbo then populates the search modal with the form. (Turbo facilitates keeping responses in Ruby (the regular controller response) while making them appear on the page via Javascript — i.e. without separate page reloads.)
  • I would like to merge this PR separately after testing, but before building the modal search form. That way we will be able to test each of these forms at the un-advertised endpoints, and be sure they're working, before changing anything about the search UI.

Strategy

Conceptually it may seem straightforward to initiate a query from the form's keyword params, but they’re not ready yet: the PatternSearch class does validation and translating for both the values and the param keys. It translates human input values into values that are executable in AR/SQL, and translates our pattern search keywords into the actual Query class param names.

So I considered two ways of handling these search params in the create action:

  1. Translate the incoming query string into the type of pattern search string that an expert user would enter, if they knew how. This involves a couple steps:
  • Reassemble the params into a URL query string, the way they came in (filtering for permitted params): ?pattern=&date=2024-08-24&id=2345
  • Then do a bunch of string manipulation to change this string into the type of pattern string expected by PatternSearch.new. Change the & into spaces, escape the commas in addresses, re-encode the whole string as a single pattern param, and send it to the index action at names, where if everything’s ok, we show the results.
  1. Assemble the params directly into a sanitized, corrected query_params hash and initiate a query from the create action, then send the query to the names index action. Doing this without duplicating PatternSearch functionality would involve a new type of object in the PatternSearch class that could validate the native query params and instantiate a query from that hash of params, rather than parsing the string (which is what it’s set up to do currently — there’s no direct “hash outlet” to plug into).

The way i’m handling it is the first one. I actually prefer the directness of 2, and may try it in the future, but it made my head swim thinking about all the validation and error handling. 1 is a roundabout way of doing things: putting params back into a string and altering the string rather than keeping a clean param hash, but it leverages a lot of work that the PatternSearch class already does. The values for the params are human input, and require translation to be valid query_params, so even if we used the native Query class param keys, we’d have to translate and validate the values.

@coveralls
Copy link
Collaborator

coveralls commented Aug 26, 2024

Coverage Status

coverage: 92.704% (-0.7%) from 93.452%
when pulling c3ff609 on pattern-search-controllers
into f65b414 on main.

@nimmolo nimmolo changed the title Pattern search controllers Pattern search forms Aug 31, 2024
@nimmolo
Copy link
Contributor Author

nimmolo commented Sep 5, 2024

Faceted search form preview.

Fields are simplified when form opens, unless populated (from the stored session[:pattern]):
image
Some fields un-collapsed:
image

@nimmolo
Copy link
Contributor Author

nimmolo commented Sep 22, 2024

@mo-nathan Thanks for checking it out. I know you're busy with mobile.

  • Has author, Author, Include synonyms: I think what you missed is that the fields are collapsed — these options are in the Names form already. (Or did you mean the obs search form?) Screengrabs, description and reason for collapsing are written up above in this thread. We may need the word "more" by the collapse caret, that might be more necessary here than in the create obs form. Or not... check it out with fresh eyes and let me know. I added "More" for now provisionally, i do think it helps.
  • Searching by substring - On Names you can currently search for "Amanita" and check Include subtaxa?, and that will get you the search you describe. (I had a "pattern" field at the top of both forms before, and removed it because Jason and I didn't believe it allowed for anything the subfields don't already do. I still can't think of a use case on Observations that isn't covered by the subfields. It might still be useful, for some other reason, but let's try to think of one.)
  • Thanks for the bug report. Now fixed.
  • I don't know how we'd provide users with flash messages about the reasoning behind the search results, this would seem to require oracular incantations that i haven't learned at my level of apprenticeship. Useful though. Reasonable goal... for a different PR?

@nimmolo
Copy link
Contributor Author

nimmolo commented Sep 24, 2024

@JoeCohen — I would like to hear your feedback before I write any tests for this.

@mo-nathan — Prefilled fields and selects are working again.
Getting together code for the global toggle, but that looks like several hours work and should probably be a separate PR.

@JoeCohen
Copy link
Member

Random suggestions:

Observations

  • Top level search form heading, e.g., Observation Search or Name Search (or even Search for Observations ...)
  • Return to the form if there's an error or there are no hits. This gives users a chance to improve the search without losing what they've already entered. IMO this is a blocker.
  • A nice-to-have would be a map where users could create/adjust a box instead of having to enter the coordinates.
  • Arrange the NSEW fields to mimic their relative locations and how they appear in Locations
    N
  W   E
    S
  • Limit form width on big screens. On my screen the form is almost 20" wide.
  • Hide the old pattern search form, especially its Search button.
  • "Must exactly match the entire Locality field." should be visible on the form without clicking the help link. Otherwise the results can be unexpected, especially for new users.
  • I get this error: Filter terms come after the bare search pattern, “east:-121.297 north:45.6991 south:44.7226 west:-122.314 has_notes:yes”. Maybe you forgot to enclose the value for :region in double quotes?
    for this search
    http://localhost:3000/?pattern=name%3A%22Boletus+edulis%22+include_subtaxa%3Ano+notes%3AAbies+region%3AOregon%5C%2C+USA+east%3A-121.297+north%3A45.6991+south%3A44.7226+west%3A-122.314+has_notes%3Ayes

Names

  • See above. Many comments also apply to Names
  • Needs a Name field

Out of time & energy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants