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

Layered Navigation: “Equalize product count” not working as expected #21968

Merged
merged 2 commits into from
Apr 20, 2019
Merged

Conversation

Nazar65
Copy link
Member

@Nazar65 Nazar65 commented Mar 27, 2019

Description (*)

Create a custom attribute of type price.
Got to Stores > Configuration > Catalog > Layered Navigation and set “price navigation step calculation” to “Automatic (equalize product counts)”

issue comes from -> #6715
where Someone says that "Both parts of ternary operators are equals."

I would expect ten ranges with equalized product counts (as the name suggests).

Fixed Issues (if relevant)

  1. Layered Navigation: “Equalize product count” not working as expected #21960: Issue title
  2. ...

Manual testing scenarios (*)

Create a custom attribute of type price.
Got to Stores > Configuration > Catalog > Layered Navigation and set “price navigation step calculation” to “Automatic (equalize product counts)”

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @Nazar65. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@Nazar65
Copy link
Member Author

Nazar65 commented Mar 27, 2019

Hi @dmytro-ch why on hold ?

@dmytro-ch
Copy link
Contributor

Hi @Nazar65,
thank you for your contribution!

However, the solution is a bit weird. Why do we really need the ternary operator with both equal parts?

empty($to) ? $to : $to

What is the possible case?
I guess we need to find a proper solution instead of it.

@Nazar65
Copy link
Member Author

Nazar65 commented Mar 28, 2019

@dmytro-ch ok, let me explain , we have 2 products with values 45 and 145, so condition looks like we have 2 ranges from * to 100 and from 100 to * in this piece of code
DeepinScreenshot_select-area_20190328104817
We create labels and if $to == * {$to = '';} and bug comes here -> epmty($to) sets 0 wich is wrong, because we have wrong scenario here ->
DeepinScreenshot_select-area_20190328105014 we need $toPrice to be ''; but we have 0 and dont pass if condition and 0 will be calculated wich is wrong.
But i agree with you, Is that will be better ?

 if ($to == '*') {
              $to = null;
            }
$label = $this->renderRangeLabel(empty($from) ? 0 : $from, $to);
protected function renderRangeLabel($fromPrice, $toPrice)
    {
        $formattedFromPrice = $this->priceCurrency->format($fromPrice);
        if ($toPrice === null) {
            return __('%1 and above', $formattedFromPrice);
        } else {
            if ($fromPrice != $toPrice) {
                $toPrice -= .01;
            }
            return __('%1 - %2', $formattedFromPrice, $this->priceCurrency->format($toPrice));
        }
    }

@magento-engcom-team
Copy link
Contributor

Hi @dmytro-ch, thank you for the review.
ENGCOM-4637 has been created to process this Pull Request

@soleksii
Copy link

soleksii commented Apr 8, 2019

✔️ QA Passed

@m2-assistant
Copy link

m2-assistant bot commented Apr 20, 2019

Hi @Nazar65, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants