Skip to content

[ISM][#18651] TierPrice save float percentage value [2.3] #19140

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[ISM][#18651] TierPrice save float percentage value [2.3] #19140

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 10, 2018

Tierprice can't save float percentage value

Description

Problem:
After save tierprice for product, discount field lost value after comma.
Fix:
Updated getPercentage method in TierPrices:

  • Type conversation changed: int -> float
  • Added strict types
  • Database has remained unchanged since the percentage_value is a decimal field

Fixed Issues

  1. Tierprice can't save float percentage value #18651: Tierprice can't save float percentage value

Manual testing scenarios

  1. Add new product
  2. Add new tierprice with discount value "2.5"
  3. Save the product
  4. Value "2.5" is saved

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 magento-engcom-team added Partner: ISM eCompany Pull Request is created by partner ISM eCompany partners-contribution Pull Request is created by Magento Partner Component: Catalog Release Line: 2.3 labels Nov 10, 2018
@magento-engcom-team
Copy link
Contributor

Hi @SikailoISM. 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

@rogyar
Copy link
Contributor

rogyar commented Nov 10, 2018

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @rogyar. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @rogyar, here is your new Magento instance.
Admin access: http://34.228.235.121/pr-19140//admin
Login: admin Password: 123123q

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.1 milestone Nov 10, 2018
@magento-engcom-team
Copy link
Contributor

@SikailoISM thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@orlangur orlangur self-assigned this Nov 10, 2018
orlangur
orlangur requested changes Nov 10, 2018
View reviewed changes
Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

#19030 (comment) CR note is missed.

@orlangur
Copy link
Contributor

@SikailoISM please apply necessary changes.

orlangur
orlangur reviewed Nov 19, 2018
View reviewed changes
app/code/Magento/Catalog/Model/Product/Attribute/Backend/TierPrice/SaveHandler.php
*/
private function getPercentage(array $priceRow): ?int
private function getPercentage(array $priceRow): float
Copy link
Contributor

Choose a reason for hiding this comment

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

Will such type hinting work when there is int?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Just checked :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@swnsma nice idea ;) such signature will convert to float though which is not desired: https://3v4l.org/eE28s

Please remove return type and make it possible to pass int unchanged.

@swnsma swnsma closed this Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@orlangur orlangur

@rogyar rogyar

Labels
Component: Catalog Partner: ISM eCompany Pull Request is created by partner ISM eCompany partners-contribution Pull Request is created by Magento Partner Progress: needs update Release Line: 2.3
Projects
None yet
Milestone
Release: 2.3.1
Development

Successfully merging this pull request may close these issues.
5 participants
@magento-engcom-team @rogyar @orlangur @swnsma @VladyslavSikailo