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 URLs for non-default combinations #15013

Merged
merged 1 commit into from Aug 28, 2019

Conversation

@Amazzing
Copy link
Contributor

commented Aug 7, 2019

Questions Answers
Branch? develop
Description? Products displayed in listings do not always represent default combinations. For example default combination of a product is RED, and it costs 10 EUR. If displayed product represents default combination, then {$product.url} is same as {$product.canonical_url}, it does not have an attribute anchor, so it is OK. But if displayed product represents another combination, let's say color GREEN, it has another image, may be another price, and it should have another link, with attribute anchor. When customer clicks on that link, he should see the GREEN combination when product page is loaded.
Type? improvement
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? Try to assign non-default combination to one of products in {$listing.products} before displaying /product.tpl. You will notice, that image and price will be updated basing on current combination, but URL will stay same, because $product.canonical_url is always same, no matter what combination is selected. After applying proposed changes URLs for non-default combinations will be updated. This modification does not affect the canonical URL. It will stay same, without anchor, and it will be used for default combinations.

This change is Reviewable

Products in listings not always represent default combinations. For example default combination is RED, and it costs 10 EUR. If displayed product represents default combination, $product.url is same as $product.canonical_url, it does not have an attribute anchor, so it is OK. 
But if displayed product represents another combination, let's say color GREEN, it has another price, and it should have another link, with attribute anchor. When customer clicks on that product, he should see the GREEN combination when product page is loaded.
@Amazzing Amazzing requested a review from PrestaShop/prestashop-core-developers as a code owner Aug 7, 2019
Copy link
Contributor

left a comment

Thank you @Amazzing
Seems legit to me, but this should be validated by @colinegin
And of course validate by our QA team, many people are on vacation at the moment but this will be dealt as soon as they come back ;)
(Don't hesitate to ping us in a couple weeks in case this PR is overlooked)

@colinegin

This comment has been minimized.

Copy link
Collaborator

commented Aug 19, 2019

Hello @Amazzing ,

Thanks for your PR and please excuse the delay.
Let me know if we tested correctly your PR :

  • we applied a stock zero to the default combination of a product
  • on front office listing we chose on the faceted search a value which displays another combination than the default one
  • we went to the product page --> if I understood your PR, it should display the combination related to the value chosen on the product listing, right ? Currently this still shows the default out of stock combination.

Thanks for your help,

Coline

@Amazzing

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Hello @colinegin,

As far as I know faceted search does not check stock for selected combinations. It always displays default combination. So it is not suitable for testing this PR. But there can be other modules that use non-default combinations in product listing.

Here is a simplified way to display a non-default combination without installing extra modules:

  1. First you should assign custom image and custom price for a combination in order to recognize it easily in product listing. It should be a non-default combination, let's say it is going to be combination ID 100 for product ID 1.
  2. Open /classes/Product.php and find method getProductProperties($id_lang, $row, Context $context = null)
  3. Add the following code at the beginning of method for testing purposes: if ($row['id_product'] == 1) { $row['id_product_attribute'] = 100;}
  4. Open category page and find that product in the list. You will notice that image and price are displayed basing on combination 100. Link to product page should also represent combination 100. Before PR this link represented default combination.
@colinegin

This comment has been minimized.

Copy link
Collaborator

commented Aug 20, 2019

Alright, now I understand clearly !
Your PR is really useful, although i'm afraid many merchants use the faceted search module, so your fix won't benefit these ones.
@Amazzing Would you be able to also fix this issue for the faceted search module ?
@marionf @PierreRambaud for your information, this improvement would be really interesting for Faceted Search.

@Amazzing

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

I am not sure I will have time for fixing that issue on faceted search in near future. It's more complicated than it may seem at first sight. Checking all possible combinations for stock availability may have impact on filtering time especially on stores with thousands of products. But you are right, it can be a useful improvement if implemented properly.

As for this PR - it is not directly related to mechanism of faceted search.
It is related to links in any product listings, no matter what module or controller is used for displaying them

@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Aug 22, 2019
@sarahdib sarahdib added this to the 1.7.7.0 milestone Aug 22, 2019
@PierreRambaud PierreRambaud merged commit 59ec589 into PrestaShop:develop Aug 28, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

Thanks @Amazzing

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