Skip to content

Update SolrKeywordSearchHandler.java#2378

Closed
sammyjava wants to merge 4 commits intointermine:devfrom
sammyjava:patch-13
Closed

Update SolrKeywordSearchHandler.java#2378
sammyjava wants to merge 4 commits intointermine:devfrom
sammyjava:patch-13

Conversation

@sammyjava
Copy link
Member

@sammyjava sammyjava commented Jun 9, 2021

Wrap faceted search term in quotes so it works on things like "C. arietinum".

Details

Summary of pull request, including references to relevant tickets (if applicable).

Testing

Besides running unit tests, how can the reviewer test your feature / bug fix? Are there edge cases to be aware of?

Checklist

Before your pull request can be approved, be sure to check all boxes:

  • Passing unit test for new or updated code (if applicable)
  • Passes all tests – according to Travis
  • Documentation (if applicable)
  • Single purpose
  • Detailed commit messages
  • Well commented code
  • Checkstyle

Sam Hokin added 4 commits June 9, 2021 11:24
Wrap faceted search term in quotes so it works on things like "C. arietinum".
Shortened the line to less than 100 chars (stupid requirement!)
"+ should be on a new line." You realize that the American taxpayer is paying me to update this commit for each one of these stupid checkstyle fails.
@danielabutano danielabutano added this to the InterMine 5.0.4 milestone Nov 22, 2021
@sergiocontrino
Copy link
Member

the change is fine, but i am not sure about the intended behaviour.
i thought it was solving a bug in filtering by organism, but this seems to work. if this is meant to allow matches with multi words terms, this can already be done enclosing them with " in the UI. I think that the default search behaviour is meant to have an OR between terms. is this we want to change?

@sammyjava
Copy link
Member Author

sammyjava commented Dec 6, 2021

Yes, I'm not totally sure this was what fixed the problem. If you like, I can retest with this update backed out.

@sergiocontrino
Copy link
Member

hi @sammyjava , sorry for the long time it took us to check this! (it is again release time..)
if the issue was the faceting by organism, this is working in dev, so i think we can skip the PR. if the issue was to be able to search with multi-words terms i would probably use the " in the search box, but we can discuss with @rachellyne if this is the expected behaviour.
if i misunderstood just please let me know, i can implement and do the testing here (at last and at least..). thanks!

@sammyjava
Copy link
Member Author

Sounds fine, I think I'll remove this update in my fork and see what happens that I didn't like before, if it happens. :)

@danielabutano danielabutano removed this from the InterMine 5.0.4 milestone Dec 13, 2021
@sergiocontrino
Copy link
Member

i think we can close this, please reopen @sammyjava if needed. thanks!

@sammyjava
Copy link
Member Author

I agree. :)

@sammyjava
Copy link
Member Author

So, @danielabutano and @sergiocontrino a user noticed that a recent mine of mine doesn't properly facet on organism (built after I'd removed this change). I can confirm that this bug is reproduced in the current FlyMine and that this PR does, in fact, fix it on my mines.

  1. Do a FlyMine keyword search on "glucose": you'll get 4930 results
  2. Select R. norvegicus -- works correctly, you get 353 displayed results, all R. norvegicus. (But note that this is the only Rattus species in the mine.)
  3. Unselect R. norvegicus and select D. pseudoobscura which should display only 31 results. But it displays 221 results for that and EVERY other species in Drosophila

This is, in fact our bug. We have several species within the same genus, and the Organism facet filter always returns all organisms within the chosen genus, because of this issue with the space in G. species that is fixed with this PR.

So, I'm going to add a PR again for a future update (the repo for this one is gone), I'll reference it below.

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.

3 participants