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 query_string issue and range search issue #269

Merged
merged 7 commits into from
Feb 23, 2024

Conversation

Fayne
Copy link
Contributor

@Fayne Fayne commented Feb 5, 2024

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
Copy link

codecov-commenter commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

Additional details and impacted files
@@            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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hkulekci
Copy link
Contributor

hkulekci commented Feb 6, 2024

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

@matchish
Copy link
Owner

matchish commented Feb 6, 2024

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
Copy link
Contributor Author

Fayne commented Feb 18, 2024

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

Thanks a lot.

RangeQuery::GTE => 900,
]))->get();

$this->assertEquals($expensiveProducts->count(), $greaterCount);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertEquals( mixed $expected, mixed $actual, string $message = '' )

should be
$this->assertEquals($greaterCount, $expensiveProducts->count());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for incorrect order. Fixed it. Please review.

@matchish
Copy link
Owner

@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
Copy link
Contributor Author

Fayne commented Feb 20, 2024

@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
Copy link
Contributor Author

Fayne commented Feb 20, 2024

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

…ty() function instead of is_null() function to check user passes a query string or not.

For unit testing issue, we have result too big issue, now i'm using paginate method to replace get() method.
@matchish
Copy link
Owner

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
Copy link
Contributor Author

Fayne commented Feb 23, 2024

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.

@matchish matchish merged commit 3985ffa into matchish:master Feb 23, 2024
7 checks passed
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.

4 participants