Skip to content

Fixed issue #21650 #23253

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

Merged
merged 1 commit into from
Aug 26, 2019
Merged

Conversation

geet07
Copy link

@geet07 geet07 commented Jun 14, 2019

Description (*)

Saving product with options with thousands separator in price fails.

Fixed Issues (if relevant)

  1. magento/magento2#<21650>: Saving product with options with thousands separator in price fails

Manual testing scenarios (*)

  1. Old PR Refference
  2. Set admin user locale to for example dutch
  3. Add Product with custom options (Price: 1000).
    Once Product saved, Again edit the same product with custom options now you can save all values successfully.
    Before:
  4. When opening product for the second time, the price of the product has been changed to 1.000,00 and thus throws an error when trying to save.
  5. products with a custom option with a negative price, also give an error. You enter them as -100 but when reopening a product it's shown as 100- and cannot be saved.
    schermafbeelding 2019-03-08 om 12 59 38
    schermafbeelding 2019-03-08 om 12 59 19

Questions or comments

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 are green)

@m2-assistant
Copy link

m2-assistant bot commented Jun 14, 2019

Hi @geet07. 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 give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

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

@geet07 geet07 requested a review from orlangur June 14, 2019 06:32
@orlangur orlangur self-assigned this Jun 14, 2019
@geet07
Copy link
Author

geet07 commented Jun 19, 2019

Hi @orlangur, Can you please review this PR. I want to close this issue.

@geet07 geet07 requested a review from dmytro-ch June 20, 2019 18:14
@dmytro-ch
Copy link
Contributor

Hi @geet07, could you please fix the static test recommendation?

@geet07
Copy link
Author

geet07 commented Jul 4, 2019

Hi @geet07, could you please fix the static test recommendation?

@geet07 geet07 closed this Jul 4, 2019
@m2-assistant
Copy link

m2-assistant bot commented Jul 4, 2019

Hi @geet07, 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.

@geet07 geet07 reopened this Jul 4, 2019
@ghost ghost unassigned orlangur Jul 4, 2019
@dmytro-ch dmytro-ch self-assigned this Jul 9, 2019
@engcom-Alfa
Copy link
Contributor

Hi @geet07 !

I have no problems with saving product on 2.3-develop.

Manual testing scenario:
1.Add Product with custom options (Price: 1000).
2. Push save product

Actual result:
Product saved

Сan you give more details for testing scenario? Thanks!

@orlangur
Copy link
Contributor

orlangur commented Jul 18, 2019

@geet07 do we support space separators now? See https://en.wikipedia.org/wiki/Decimal_separator#Digit_grouping

Hi @orlangur,

Now space is supporting also. Please check

@orlangur
Copy link
Contributor

@geet07 also, to enforce new correct behavior, please write a test covering mentioned cases. I would prefer JavaScript Unit Test, there is no need to write a Selenium-based test for just checking validation.

@geet07
Copy link
Author

geet07 commented Jul 22, 2019

@geet07 also, to enforce new correct behavior, please write a test covering mentioned cases. I would prefer JavaScript Unit Test, there is no need to write a Selenium-based test for just checking validation.

Hi @orlangur,

I have added test cases also, Please check it again
Now, this validation is supporting the following type of numbers:

  1. 125
  2. 1000.50
  3. 1,000,000.50
  4. 10 000
  5. 10.000,00
  6. 10'000.00

@geet07
Copy link
Author

geet07 commented Aug 5, 2019

Hi @orlangur, have you checked this issue?

@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-5420 has been created to process this Pull Request
✳️ @sidolov, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

Before:
before

After:
after

@engcom-Foxtrot
Copy link
Contributor

Failing tests fixed in internal engcom repository.

@m2-assistant
Copy link

m2-assistant bot commented Aug 26, 2019

Hi @geet07, 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.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.4 milestone Aug 26, 2019
@VladimirZaets VladimirZaets added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Sep 10, 2019
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.

9 participants