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

Default AJAX Relevance search sort order is wrong #8458

Merged
merged 2 commits into from Jan 8, 2018

Conversation

@sbordun
Copy link
Contributor

commented Nov 2, 2017

Questions Answers
Branch? develop
Description? default AJAX Product search Relevance sort order is wrong
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? search for "summer" in demo store. All products with the "summer" in the product name should appear at the first lines in the AJAX search results, but I see them at the 3rd and 4th positions (results shown immediately but in the ASC relevance order instead of DESC). This error is continuation of the pull request #8399. The issue was not fixed in full. as an addition to the fix in pull request #8399 I suggest the following change in the controllers/front/listing/SearchController.php: ->setSortOrder(new SortOrder('product', Tools::getProductsOrder('by', 4), Tools::getProductsOrder('way', 1))) instead of ->setSortOrder(new SortOrder('product', Tools::getProductsOrder('by'), Tools::getProductsOrder('way')))

Important guidelines


This change is Reviewable

default AJAX Relevance search sort order is wrong
search for "summer" in demo store. All products with the "summer" in the product name should appear at the first lines in the AJAX search results, but I see them at the 3rd and 4th positions (results shown immediately but in the ASC relevance order instead of DESC). This error is continuation of the pull request #8399. The issue was not fixed in full. as an addition to the fix in pull request #8399 I suggest the following change in the controllers/front/listing/SearchController.php: ->setSortOrder(new SortOrder('product', Tools::getProductsOrder('by', 4), Tools::getProductsOrder('way', 1))) instead of ->setSortOrder(new SortOrder('product', Tools::getProductsOrder('by'), Tools::getProductsOrder('way')))
@prestonBot

This comment has been minimized.

Copy link
Collaborator

commented Nov 2, 2017

Hello sbordun!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@sbordun

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2017

additional explanation to this pull request:

My understanding that Search results should be sorted according to biggest "weight". It is different from Default Page sort on category page (Shop Parameters > Product Settings, Pagination Section).

The weight could be set through the admin : Shop Parameters -> Search, WEIGHT section.
As I understand - products with the biggest weight have to be on the top of the search (Order By Relevance DESC).
But the source code has the bug and : products with the biggest weight are shown on the bottom of the search (Order By Relevance ASC) because the "relevance sorting for the search" was not part of the fix (CO: Fix search relevance wrong sort order BOOM-3915 #8399).
My fix fixed the Relevance sorting issue for the Product Search .

I hope someone will review my fix soon

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


controllers/front/listing/SearchController.php, line 69 at r1 (raw file):

        $query = new ProductSearchQuery();
        $query
            ->setSortOrder(new SortOrder('product', Tools::getProductsOrder('by', 4), Tools::getProductsOrder('way', 1)))

I'm not ok with this.

The sort order is set here to use the default values for "by" (PS_PRODUCTS_ORDER_BY config value) and "way" (PS_PRODUCTS_ORDER_WAY config value).
By hard coding values here, you are altering the default behavior which is "for default sort order, use configuration".


Comments from Reviewable

@LittleBigDev
Copy link
Contributor

left a comment

Hello @sbordun, thank you for this PR.
I'm afraid you will have to find another way to fix this problem. See my comment above.

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

Review status: all files reviewed at latest revision, 1 unresolved discussion.


controllers/front/listing/SearchController.php, line 69 at r1 (raw file):

if no value, then use configuration

Sorry, I mean "for default sort order, use configuration"


Comments from Reviewable

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

@LittleBigDev i don't agree with you, sorting products on product list has nothing to do with sorting in search results, we need to sort those results by relevance

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

I agree with this. Search results should be sorted by relevance by default.
But the PR is altering SearchController::getProductSearchQuery() which is used by ProductListingFrontController::getProductSearchVariables() which is used by ProductListingFrontController::doProductSearch() which is used.... everywhere ! (best sales, categories, manufacturers, suppliers, new products, etc.)
So, we need another way to fix the search results issue without altering other lists.

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

@LittleBigDev are you sure? every page, for example BestSales, NewProducts etc. has their own file to provide SortOrder...

BestSalesProductSearchProvider

i don't think that having this code in SearchController should affect other controllers/data providers, SearchController should affect only searching products in index.php?controller=search and ajax to this page

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

Well, I didn't audit it in details. But for sure, NewProductsController is calling ProductListingFrontController::doProductSearch() with no tweaks. At least this result would be altered.

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

@LittleBigDev

https://github.com/PrestaShop/PrestaShop/blob/develop/controllers/front/listing/NewProductsController.php#L53

like i said, it would be so silly to rely on SearchController on those pages...

FYI, i'm using fix from this PR on one of the stores i'm building and every page is working as expected, however i'm using AS4 so i'm not 100% sure ;-)

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

Wait, I missed the overridden getProductSearchQuery(), which is tweaking sort order.
Thank you @kpodemski.
(and I just see your comment about this exact line).

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

This is actually a good solution.
:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@LittleBigDev
Copy link
Contributor

left a comment

Thank you @sbordun for this fix and @kpodemski for forcing me to dig deeper.

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

Message to QA : double check other products lists sort order.

@@ -66,7 +66,7 @@ protected function getProductSearchQuery()
{
$query = new ProductSearchQuery();
$query
->setSortOrder(new SortOrder('product', Tools::getProductsOrder('by'), Tools::getProductsOrder('way')))
->setSortOrder(new SortOrder('product', Tools::getProductsOrder('by', 4), Tools::getProductsOrder('way', 1)))

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Nov 30, 2017

Member

We had a look yesterday at all these PRs regarding the sort orders, and yours seem to fix the issue.
However, if you look at the method Tools::getProductsOrder(), you could see that you force the values here.

This line is the equivalent of

->setSortOrder(new SortOrder('product', 'position', 'desc'))

which is quite easier to read, don't you think?

This comment has been minimized.

Copy link
@kpodemski

kpodemski Dec 11, 2017

Contributor

@Quetzacoalt91 are you sure that if you hardcode these values you can still change the sorting way and sorting field?

This comment has been minimized.

Copy link
@sbordun

sbordun Dec 12, 2017

Author Contributor

@Quetzacoalt91 you are right :
Tools::getProductsOrder('by', 4) means 'position'
Tools::getProductsOrder('way', 1) means 'desc'
Your version of the fix is the same as mine but simpler and faster.

@kpodemski, the only one place where the customer can change the sorting way of the search is on the search page after clicking the search button. I tested it - it is working.

@Quetzacoalt91 , should I change the code in my branch to your version of the fix or you will do it?

This comment has been minimized.

Copy link
@LittleBigDev

LittleBigDev Dec 12, 2017

Contributor

@sbordun yes I think you should :)

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Dec 12, 2017

Member

@kpodemski I can confirm again this later, this is what I checked on the first try. :)

@codacy-bot

This comment has been minimized.

Copy link

commented Dec 12, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity decreasing per file
==============================
+ controllers/front/listing/SearchController.php  -3
         

See the complete overview on Codacy

@sbordun

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2017

The simplified code change was committed.

@Quetzacoalt91, @LittleBigDev, @kpodemski could you review it?

Thanks

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2017

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@LittleBigDev
Copy link
Contributor

left a comment

👍

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2017

👍

@sbordun

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2017

@Quetzacoalt91, @LittleBigDev, @kpodemski what will be the next step to push this change into release?

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

We need a feedback from @vincentbz, because he is facing a similar but more generic issue and wants to have a look at this. After "pm feedback" label is removed, we will include the PR in the merge list.

A milestone needs to be defined. Poke @eternoendless ?

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

Review status: all files reviewed at latest revision, 1 unresolved discussion.


controllers/front/listing/SearchController.php, line 69 at r1 (raw file):

Previously, sbordun wrote…

@Quetzacoalt91, @LittleBigDev, @kpodemski this discussion is marked as unresolved. Could you review and mark it as resolved ?
What will be the next step to push this change into release?

This will not change the PR status tho. If PR is approved, we can merge it and ignore the unresolved discussions in Reviewable.


Comments from Reviewable

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

@kpodemski and anybody else interested in the consequences of this PR.

develop branch

capture du 2017-12-13 10-12-49

PR / Search "robe" with default sort

(Notice the empty select input -> this may be why #8399 exists)
capture du 2017-12-13 10-14-13

PR with another sorting (still taken in account)

capture du 2017-12-13 10-14-34

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

Already validated by @vincentbz.
For the QA team, this PR must be checked with #8399 at the same time.

@marionf marionf added QA ✔️ and removed waiting for QA labels Dec 15, 2017

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.4.0 milestone Jan 8, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit 40de19c into PrestaShop:develop Jan 8, 2018

2 of 3 checks passed

code-review/reviewable 1 discussion left (kpodemski, Quetzacoalt91)
Details
Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Copy link
Member

commented Jan 8, 2018

Thank you everybody for your help on that one. It will be available from PS 1.7.4.0

@eternoendless eternoendless changed the title default AJAX Relevance search sort order is wrong Default AJAX Relevance search sort order is wrong Jan 10, 2018

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2018

@Quetzacoalt91 can we please move it to 1.7.3.x? as for now people are saying that searching in 1.7 is broken... yet another reason to not move to 1.7

@eternoendless

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

We could cherry-pick it on 1.7.3.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.