I'm probably doing this wrong, but I think I can't find anything for taxonomy term names. I need to do this for a customer project and I'd love to help out but I don't understand this well enough to actually be of service for now. I tried this with beta 5 the last dev as well as with both git-HEAD projects.

Steps to reproduce:

  1. Install search api, db defaults, autocomplete
  2. Add the term name to the index as a fulltext field (see tags.png)
  3. Configure autocomplete for this index, select the tags field in the "retrieve from server" plugin (see autocomplete-config.png)
  4. Go to frontend, search for on the values in the term

After .4, the expected result is to see the values of the term in the autocomplete, actually nothing shows up.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

borisson_ created an issue. See original summary.

borisson_’s picture

Title: Autocomplete on taxonomy term » Autocomplete on taxonomy term name
Category: Task » Bug report
borisson_’s picture

I spent some time on this yesterday as well as some more this morning but I can't figure it out, this might belong in search_api_db as I think that the problem in there.

First of all, it looks like the min_chars configuration from autocomplete is not passed into db properly, but that seems like a non-issue for me.

The results we get back from the db modules is an empty array and that shouldn't happen. I'd like to write a test for this but I'm not sure how to start. I think this should be a kernel test in search_api_db.

drunken monkey’s picture

Does the information that is passed to the DB backend look correct? Then yes, please move it to the Search API queue, DB backend component, and we'll try to come up with a test there.

borisson_’s picture

Project: Search API Autocomplete » Search API
Component: Code » Database backend

It does look correct.

drunken monkey’s picture

Status: Active » Needs review
FileSize
8.08 KB

This adds infrastructure for testing autocomplete, plus two assertions for simple autocompletion cases.
For me, both of them pass. If they do for you, too, please try to add a failing assertion based on your specific scenario. (If necessary, you can of course also change the index (in the test code, not the export) or other config, or add more items.)

Status: Needs review » Needs work

The last submitted patch, 6: 2867144-6--autocomplete_test.patch, failed testing.

drunken monkey’s picture

Damn, that #$*@ @group gets me every single time …

Status: Needs review » Needs work

The last submitted patch, 8: 2867144-8--autocomplete_test.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
619 bytes
8.16 KB

Oh, didn't know about that annotation … Great work, Drupal testing infrastructure!
(Hm, though it remains to be seen whether this will make it download the module or skip the test …)

However, forgot to say before that I'm not sure anyways whether we should commit this patch at this point. The Autocomplete module is far from stable, I'll still surely change lots of things (e.g., most class/interface names), and I'm not sure breaking the Search API tests each time is a good idea. (It would at least ensure we keep our integration up-to-date, but it's still less than ideal, I'd say. Even if both modules are maintained by the same person.)

Status: Needs review » Needs work

The last submitted patch, 10: 2867144-10--autocomplete_test.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
830 bytes
8.16 KB

I think the @requires should be on the class, let's test that. Will need to test the actual patch still.

Status: Needs review » Needs work

The last submitted patch, 12: autocomplete_on-2867144-12.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
907 bytes
8.5 KB

Status: Needs review » Needs work

The last submitted patch, 14: autocomplete_on-2867144-14.patch, failed testing.

borisson_’s picture

Status: Needs work » Reviewed & tested by the community

I tested the patch and it works, looks good. I think the tests are still failing because dependencies are calculated on commit, not just in a patch.

drunken monkey’s picture

Yes, seems that's the problem. Then let's just go with the patch in #8.

However, I actually thought you wanted to use the test to show what doesn't work? (Even if the test bot fails in any case, we can use it locally for verifying/debugging.) Would be great if you could try to come up with a failing test based on the patch.

Also, see my notes about committing this in #10 – what's your opinion on that?

borisson_’s picture

I think we should already add this, since autocomplete doesn't have a lot of tests yet. And this side does need tests. I did want to use this patch for a usecase that didn't work, however I did get it to work with the latest dev versions so I think it was just a case of having older modules and/or bad configuration in my original setup.

This test doesn't use a lot of internals of what the autocomplete module does, so committing it now doesn't seem like a bad idea.

drunken monkey’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
986 bytes
8.21 KB

This test doesn't use a lot of internals of what the autocomplete module does, so committing it now doesn't seem like a bad idea.

It's not really about internals. The SearchApiAutocompleteSearchInterface class name, e.g., will definitely be changed.
Hm, so maybe just remove the type hint there for now? (Patch attached, interdiff compared to #8.)
However, the method signature itself might also change, and with the discrepancy between dev version and the version used by the d.o test bot, it might be tricky to adapt to that without causing test fails in the mean time.
Furthermore, now that we know this is working, I'd say it's pretty unlikely to break unless we actually change that method. In which case we can also just run the tests manually, if we want to verify we didn't break anything.

But all of that considered, if you still think we should add it, then let's do it. (With or without type hint, your choice.) We'll survive if the test bot chokes at some point on this for a day or two.

Status: Needs review » Needs work

The last submitted patch, 19: 2867144-19--autocomplete_test.patch, failed testing.

borisson_’s picture

I think we should add the test now, we can always change it later when we decide to change the class / method names. The patch here seems to be failing though, so let's fix that before commit.

drunken monkey’s picture

Status: Needs work » Needs review

The patch here seems to be failing though, so let's fix that before commit.

It's working fine for me. What failures do you get?
(The test bot failing, as discussed, is just a fault in the test bot regarding new module dependencies. It should be fixed once we commit the patch – at least we'll have to hope that.)

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Ah yes, I forgot about that. Sorry.

  • drunken monkey committed e8f5167 on 8.x-1.x
    Issue #2867144 by drunken monkey, borisson_: Added a test for the...
drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Ah, OK, no problem.
Committed. Let's hope for the best. ;)

Hm, should we then mark this as "Fixed" or move it back to "Active" for your original problem?

borisson_’s picture

Let's keep it as fix for now, I can reopen / open a new issue when I get to that project again :)

drunken monkey’s picture

Didn't pass after all, for some reason, had to disable it again for now. Maybe after we've made another release for the Search API? (Which we probably should do soon in any case.)

drunken monkey’s picture

Status: Fixed » Needs review
FileSize
564 bytes

Let's try again.
(Just realize I should have probably removed it from the changelog after disabling the test. But well, no big deal.)

Status: Needs review » Needs work

The last submitted patch, 28: 2867144-28--reenable_autocomplete_test.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
1.24 KB

Ah, wait, seems we forgot to actually depend on the Search API Autocomplete module in the test module! Maybe that'll solve it?

Status: Needs review » Needs work

The last submitted patch, 30: 2867144-30--reenable_autocomplete_test.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
564 bytes

Apparently not.
But I now committed that dependency, maybe that helps. Otherwise, let's wait for the next release (1.2), then try again and if it still doesn't work go complain in the Drupal CI issue queue.
Perhaps it won't work as long as the Autocomplete module doesn't have a stable release?

Status: Needs review » Needs work

The last submitted patch, 32: 2867144-32--reenable_autocomplete_test.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Postponed