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

Fix searching and faceting on multiline values #82

Merged
merged 3 commits into from
Jul 3, 2018
Merged

Conversation

techgique
Copy link
Member

Remove the %0D in the API requests for facets and filters.
Fixes #81
However, I'm concerned that this may potentially break other queries if we want to allow searches trying to match values with CRs, perhaps via <textarea> inputs for API search. We aren't using any currently that I'm aware of. None of the current facet URLs in Cather Letters have %0D in them, so this approach seems safe at the moment.
@jduss4 is more familiar with this repository and may have other ideas how to implement this, perhaps making its behavior with Orchid less complicated. I realize now that I didn't add comments, so we'll want to add some information in the code too.

Values indexed which have newline characters weren't matched if passed
as parameters, such as when stored in hidden inputs once being faceted

They would only work when searched via preconstructed URI-encoded URLs
@techgique techgique requested a review from jduss4 June 23, 2018 01:15
@jduss4
Copy link
Contributor

jduss4 commented Jun 29, 2018

I'm tempted to say that we ought to change the data repository to automatically normalize all the spaces in fields going to text fields, and probably the keywords, too. I agree that this could potentially break other queries for searches... Hmm. What do you think?

@techgique
Copy link
Member Author

See comment on the related issue: #81 (comment)

The normalize space method is different than removing vertical space though, right? We're can't globally remove vertical space.

@jduss4
Copy link
Contributor

jduss4 commented Jun 29, 2018

@techgique the original normalize-space from XSLT does remove vertical space, so it depends, I guess, if we want to imitate that entirely.

http://www.xsltfunctions.com/xsl/fn_normalize-space.html

@jduss4
Copy link
Contributor

jduss4 commented Jul 2, 2018

Looking at these changes, I think I feel kinda confused. Like, if the elasticsearch index has a field with a return line in it, then how would the searches here which remove the return line actually match against it? Would you be up for writing a couple tests? I don't think we have any integration tests, unfortunately, but I would feel a little better about having the expected behavior lined out in them with these changes.

@techgique
Copy link
Member Author

They match after this change because the indexed values in Elasticsearch only have LF characters for new lines. But when Orchid renders the values in hidden <input>s to pass along with subsequent changes to one's searching, it adds an additional CR character that causes the API request not to match what's in Elasticsearch.

I can work on adding comments and tests for this if you want to handle the data repo changes for keyword fields?

@jduss4 jduss4 merged commit 0358e38 into master Jul 3, 2018
@jduss4 jduss4 deleted the multiline-search branch July 3, 2018 15:07
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.

2 participants