matchish / laravel-scout-elasticsearch

Search among multiple models with ElasticSearch and Laravel Scout
MIT License
702 stars 113 forks source link

Fix query_string issue and range search issue #269

Closed Fayne closed 6 months ago

Fayne commented 7 months ago

When i call like this

Product::search()
    ->where('price', new RangeQuery('price', [
        RangeQuery::GTE => 900,
    ]))

search body will be:

GET /_search
{
  "query": {
    "bool": {
      "filter": [
        {
          "term": {
            "price": {
              "range": {
                "gte": 900
              }
            }
          }
        }
      ],
      "must": [
        {
          "query_string": {
            "default_field": "name",
            "query": ""
          }
        }
      ]
    }
  }
}

This is not what i want. Please check if the PR work for you.

codecov-commenter commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.08%. Comparing base (00e3caf) to head (47c8b97).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #269 +/- ## ========================================= Coverage 96.08% 96.08% - Complexity 193 194 +1 ========================================= Files 36 36 Lines 638 639 +1 ========================================= + Hits 613 614 +1 Misses 25 25 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hkulekci commented 7 months ago

Hello @Fayne, thank you for your contribution and solution. Could you please provide us with a test for that bit?

matchish commented 7 months ago

Ideally if you'll add a test that fails to another branch https://github.com/matchish/laravel-scout-elasticsearch/blob/master/tests/Feature/SearchTest.php and then if we merge current branch with the solution to branch with test it pass

Fayne commented 7 months ago

Hi @matchish , @hkulekci , i have added a unit testing for this update. if any misunderstanding, please let me know.

Thanks a lot.

matchish commented 6 months ago

@Fayne Thank you for your contribution with the unit testing. I appreciate your effort to help improve the project. However, it seems there has been a slight misunderstanding regarding the test you added. In the test_empty_query_string method, the test is currently set up to create a number of products with prices between 200 and 300, and then it searches for products with a price greater than or equal to 900. The assertion at the end expects the count of expensive products to equal the number of products created, which would not be correct since none of the created products would match the search criteria (price >= 900).

Request was to create correct test(it should search between 200 and 300) but in a branch without your solution. So the test should fail. Then we merge the test to branch with yours solution and it should pass. It's RED GREEN test approach for tests https://www.codecademy.com/article/tdd-red-green-refactor

Anyway I think we are safe, so feel free just to fix the test and we can merge

Fayne commented 6 months ago

@Fayne Thank you for your contribution with the unit testing. I appreciate your effort to help improve the project. However, it seems there has been a slight misunderstanding regarding the test you added. In the test_empty_query_string method, the test is currently set up to create a number of products with prices between 200 and 300, and then it searches for products with a price greater than or equal to 900. The assertion at the end expects the count of expensive products to equal the number of products created, which would not be correct since none of the created products would match the search criteria (price >= 900).

Request was to create correct test(it should search between 200 and 300) but in a branch without your solution. So the test should fail. Then we merge the test to branch with yours solution and it should pass. It's RED GREEN test approach for tests https://www.codecademy.com/article/tdd-red-green-refactor

Anyway I think we are safe, so feel free just to fix the test and we can merge

Hi @matchish ,

I updated the unit testing function. Wish this time i didn't make the stupid mistake as last time. 😂

Fayne commented 6 months ago

Hi @matchish I added 3 assertions and it's working fine on my side. Could you please review it? Thanks.

matchish commented 6 months ago

It's ready for release) 2 small things

  1. Update README if you think it could help other developers https://github.com/matchish/laravel-scout-elasticsearch/blob/master/README.md
  2. Update CHANGELOG https://github.com/matchish/laravel-scout-elasticsearch/blob/master/CHANGELOG.md (version 7.6.0 in my opinion)
Fayne commented 6 months ago

It's ready for release) 2 small things

  1. Update README if you think it could help other developers https://github.com/matchish/laravel-scout-elasticsearch/blob/master/README.md
  2. Update CHANGELOG https://github.com/matchish/laravel-scout-elasticsearch/blob/master/CHANGELOG.md (version 7.6.0 in my opinion)

Hi @matchish,

I updated the readme and changelog. Please review.