Problem/Motivation

When I add a facet based on a boolean field, I only see the "TRUE" value facet back in the interface.

Steps to reproduce

  1. Set up a local 8.0.x environment for development
  2. Go to /admin/structure/types/add and add a Movie content type. Mine had the machine name movie. Click the Save button.
  3. Go to /admin/structure/types/manage/movie/fields/add-field. Under Choose a type of field, click Boolean. Set Label = Closed captioning (machine name, field_closed_captioning), then click the Continue button. Click the Save button on the second page to ensure accept the defaults.
  4. Go to /node/add/movie. Set Title = The Gold Rush, Closed captioning = FALSE (i.e.: unchecked), then click the Save button.
  5. Go to /node/add/movie. Set Title = MacGyver, Closed captioning = TRUE (i.e.: checked), then click the Save button.
  6. Go to /admin/config/search/search-api/add-index. Set Index name = Movies; Datasources = Content, Server = elasticsearch_server, click Save
  7. Go to /admin/config/search/search-api/index/movies/fields/add/nojs, and click the Add button in the title and field_closed_captioning rows. Then click the Done button. Onthe next page, ensure the field_closed_captioning row has Type = Boolean. Click the Save change button.
  8. Go to /admin/config/search/search-api/index/movies. Click the Index now button. You should see the Status message Successfully indexed 2 items.
  9. Go to /admin/structure/views/add. Set View name = Movie search. Under View settings, "Show Index movies sorted by Unsorted". Under Page settings, check Create a page, and ensure Path = movie-search. Click the Save and edit button.
  10. You are taken to the View-edit page. Under Fields, you should already see Content datasource: Closed captioning (indexed field). Next to Fields, click Add. In the popup, check Title (indexed field) in the Content data-source category. Click the Add and configure fields button. Click the Apply button on the second page to accept the defaults. Click the Save button on the View-edit page. You should see the Status message The view Movie search has been saved..
  11. Go to /admin/config/search/facets/add-facet. Set Facet source = View Movie search, display Page. Set Field = Closed captioning (field_closed_captioning). Set Name = Closed captioning (machine name closed_captioning. Click the Save button.
  12. Go to /admin/structure/block/library/stark?region=content. Click the Place block button in the Closed captioning row, Facets category. Click the Save block button on the second page to accept the defaults. You are taken back to the Block layout page; click the Save blocks button at the bottom. You should see the Status message The block settings have been updated.
  13. Go to /movie-search.
    Expected behavior: You see a Closed captioning facet with links to 0 and 1.
    Actual behavior: You see a Closed captioning facet with links to ! and 1. You see search results for The Gold Rush and MacGyver.
  14. Assuming that ! refers to the "FALSE" value, in the Closed captioning filter, click the ! option. The page reloads.
    Expected behavior: You see 1 search result, The Gold Rush.
    Actual behavior: You see 1 search result, MacGyver.

Proposed resolution

The facet parser should be changed. It checks on an empty() statement. However the value we receive back from ES is 0. This is considered empty and therefore discarded.

2 solutions:
Different check:

    'filter' => (!isset($value['key']) || $value['key'] === '') ? '!' : \sprintf('"%s"', $value['key']),

Or change the default value to 0.

    'filter' => empty($value['key']) ? '0' : \sprintf('"%s"', $value['key']),

The patch in the attachement uses solution 1 since I do not know if the "!" is used for something specific.

Remaining tasks

  1. Write a merge request for 8.0.x - merge request !94 created in #4 by @mparker17 with @marckwee's code
  2. Write a merge request for 9.0.x - merge request !194 created by @mparker17 in #11
  3. Add tests - added by @mparker17 in #10
  4. Community review - reviewed by @fathershawn in #13
  5. Maintainer review - done by @mparker17 in #7
  6. Commit to 9.0.x - done by @mparker17 in #15
  7. Commit to 8.0.x - done by @mparker17 in #17
  8. Release 9.0.x - released in 9.0.0-alpha3 by @mparker17
  9. Release 8.0.x - released in 8.0.0-alpha7 by @mparker17

User interface changes

None.

Introduced terminology

None.

API changes

None.

Data model changes

None.

Release notes snippet

Fixed an issue where FALSE values would not show for boolean facets.

CommentFileSizeAuthor
#2 bool-zero-value-check-facet-3526730-1.patch642 bytesmarckwee

Issue fork elasticsearch_connector-3526730

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

marckwee created an issue. See original summary.

marckwee’s picture

marckwee commented
StatusFileSize
new bool-zero-value-check-facet-3526730-1.patch642 bytes

Patch to solve issue above using method 1

mparker17 made their first commit to this issue’s fork.

mparker17 opened merge request !94

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue tags: +Needs tests

@marckwee, thank you for the contribution!

I committed your code to a merge request to make it easier for me to review.

I took a very quick look at your patch: the changes seem clear to me, but I haven't yet had a chance to run the changed code... I may have more feedback when I do.

I didn't see any automated tests in your patch... the Elasticsearch Connector module maintainers prefer to accept merge requests that have passing automated tests. Automated tests ultimately benefit you, because they ensure that future changes to this module (i.e.: by other people) will not break the functionality that your site depends on!

I don't think that we have any automated tests for Facets yet (I wrote a manual test but haven't had the time to turn it into code yet).

Are you interested in learning how to write an automated test?

If you describe how to run a test manually, then I may be able to suggest how to automate it (i.e.: if you're interested in learning to write a test), or write a test myself (if you aren't interested in learning to write a test).

marckwee’s picture

marckwee commented

Hi MR Parker,

I wouldn't mind to write tests for the issues. However I am a bit lost with using MR/PR with Drupal instead of patches + what kind of tests to write. Can you give me some pointers.=)

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

@marckwee, apologies for the delay in getting back to you! I'm happy to help how I can!

I can't see any metadata in your patch file to indicate how you created it, but I found that the merge-request workflow was pretty similar to how I was generating patches before, and it was also pretty similar to how my teams were working on client sites, so it was an easy transition for me. (hopefully those anecdotes are encouraging?)

At a high level, in the MR-based workflow, you:

  1. Use the Create issue fork button (or Create new branch link) in the d.o issue (below the Issue Summary, just above the Comments h2) to create an Issue Fork and Branch for your changes.
    • note that I've already done this for you for this issue!
  2. Clone the module to your local machine (using the Version control tab on the project page)
    • Note it will work if you clone the module into your existing site, but I prefer to work on modules in an isolated sandbox, because I've run into many instances where my client's site's configuration is contributes to the problem(s) I've been trying to fix and/or affects my patch in a way that works for my client's site but not the rest of the Drupal community.
    • I've documented how I set up an isolated sandbox test environment for Elasticsearch Connector: here are my instructions for setting up a local environment — hopefully this is helpful
  3. Use the Show commands link beside the name of the Issue Fork in the d.o issue (below the Issue Summary, above the Create new branch link) to add the Issue Fork as a git remote, then switch to the branch
  4. Make your changes in the branch and commit
  5. Push your commits to the Issue fork
    • Under a patch-based workflow, at this point, I would have instead run git diff against the main branch to generate the patch file
  6. Make a merge request.
    • note that I've already done this for you for this issue!
  7. Look for feedback from testbot either in the d.o issue or in the git.drupalcode.org UI

For more-detailed documentation, see the guide on Using GitLab to contribute to Drupal. The part of the guide about Creating issue forks explains the d.o issue UI, with screenshots (which I found helpful!)

Please let me know if you have any questions! (I'll try to reply to you more-quickly in the future!)

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Version: 8.0.0-alpha2 » 8.0.x-dev
Status: Active » Needs work

(updated the status (Active -> Needs work), and updated the version to 8.0.x as this issue applies to the dev branch as well)

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue tags: +Needs issue summary update

Briefly checking the status of this issue after releasing 8.0.0-alpha5...

  1. The existing tests are passing — so this merge request didn't cause any regressions
  2. But the new code path doesn't have any tests, and it will need a test before I can merge it.
  3. The issue summary will need to be updated before release.
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs issue summary update

I added a comment, wrote an automated test, and I have updated the issue summary and added steps to reproduce.

@marckwee if you'd be willing to test the latest patch, I would very much appreciate it! (And, if it works, please change this issue's status to "Reviewed and tested by the community")

Thank you very much for your patience with me!

mparker17 opened merge request !194

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Version: 8.0.x-dev » 9.0.x-dev
Issue summary: View changes
Issue tags: +needs backport to 8.0.x

(Created a merge request for the 9.0.x branch, and tagged a reminder so that I don't forget to merge it to 8.0.x)

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Needs review » Reviewed & tested by the community

The code change is a well structured and targeted replacement for empty() which would have screened out false

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

Thank you @fathershawn for the review!

  • mparker17 committed feb844be on 9.0.x 
    fix: #3526730 Boolean facet does not show the "FALSE" value

By:...
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Version: 9.0.x-dev » 8.0.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -needs backport to 8.0.x

Merged to 9.0.x; merging to 8.0.x next.

  • mparker17 committed 59927390 on 8.0.x 
    fix: #3526730 Boolean facet does not show the "FALSE" value

By:...
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Patch (to be ported) » Fixed

Merged to 8.0.x. I'll update the issue when this issue is released.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

The changes in this issue were released in elasticsearch_connector-9.0.0-alpha3, and elasticsearch_connector-8.0.0-alpha7

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.