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

Fixed issue: Magento backend catalog "MSRP" without currency symbol #24379

Closed
wants to merge 9 commits into from
Closed

Fixed issue: Magento backend catalog "MSRP" without currency symbol #24379

wants to merge 9 commits into from

Conversation

00F100
Copy link

@00F100 00F100 commented Aug 31, 2019

In this PR fix values of special_price and msrp into products list admin

Description (*)

The values of special_price and msrp into admin products list

Fixed Issues (if relevant)

  1. Magento backend catalog "MSRP" without currency symbol #21910: Magento backend catalog "MSRP" without currency symbol

Manual testing scenarios (*)

  1. Admin Menu > Stores > Configuration > Sales > Sales > Minimum Advertised Price = Yes
  2. Navigate to admin catalog product grid
  3. Using "Columns" selection, add 'Manufacturer's Suggested Retail Price'(MSRP) column in the grid

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 Aug 31, 2019

Hi @00F100. 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 Guide documentation.

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Aug 31, 2019

CLA assistant check
All committers have signed the CLA.

@00F100
Copy link
Author

00F100 commented Aug 31, 2019

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @00F100, here is your new Magento instance.
Admin access: https://pr-24379.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@00F100 00F100 changed the title Issue 21910 Fixed issue: Magento backend catalog "MSRP" without currency symbol Aug 31, 2019
@rodrigowebjump rodrigowebjump self-requested a review August 31, 2019 00:28
@rodrigowebjump rodrigowebjump self-assigned this Aug 31, 2019
@orlangur orlangur self-assigned this Sep 2, 2019
@orlangur
Copy link
Contributor

orlangur commented Sep 2, 2019

@00F100 is it still an issue considering #20906 (comment)?

@00F100
Copy link
Author

00F100 commented Sep 2, 2019

in this pr fix 3 columns: cost, special_price and msrp, not only cost.

@00F100
Copy link
Author

00F100 commented Sep 2, 2019

if cost has been fixed, i remove from this PR

@orlangur
Copy link
Contributor

orlangur commented Sep 2, 2019

@00F100 did you find commits where Cost was fixed? Would be nice to make fix consistent with core one.

@00F100
Copy link
Author

00F100 commented Sep 2, 2019

yes, equals except here extends Price, not send all to Price class.

@rodrigowebjump
Copy link
Member

@00F100 Please try to follow the fix approach used in https://github.com/magento/magento2/pull/20907/files

Const NAME is the component name, not the column name that can be defined by xml file.

@00F100 00F100 requested a review from akaplya as a code owner September 15, 2019 06:31
@00F100
Copy link
Author

00F100 commented Sep 15, 2019

@rodrigowebjump now it's good?

@00F100
Copy link
Author

00F100 commented Sep 15, 2019

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @00F100, here is your new Magento instance.
Admin access: https://pr-24379.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@00F100
Copy link
Author

00F100 commented Sep 16, 2019

CLA is empty :(

Captura de tela de 2019-09-16 10-42-55

@00F100
Copy link
Author

00F100 commented Sep 16, 2019

CLA not working

Captura de tela de 2019-09-16 14-02-24

Captura de tela de 2019-09-16 14-02-17

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.

This fix is incorrect and similar to https://github.com/magento/magento2/pull/20907/files.

In a proper fix, like it was made for Cost, new columns will not be added to grid by default.

Please specify commits where it was fixed for Cost and do a similar fix for Special Price and MSRP.

@rodrigowebjump
Copy link
Member

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @rodrigowebjump, here is your new Magento instance.
Admin access: https://pr-24379.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@rodrigowebjump
Copy link
Member

@magento give me 2.3-develop instance

@magento-engcom-team
Copy link
Contributor

Hi @rodrigowebjump. Thank you for your request. I'm working on Magento 2.3-develop instance for you

@magento-engcom-team
Copy link
Contributor

Hi @rodrigowebjump, here is your Magento instance.
Admin access: https://i-24379-2-3-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

@00F100
Copy link
Author

00F100 commented Sep 23, 2019

@orlangur please, help me. In version 2.3-develop Cost not formated and i not find commit to change Cost how do you say.

@00F100
Copy link
Author

00F100 commented Sep 23, 2019

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @00F100, here is your new Magento instance.
Admin access: https://pr-24379.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@00F100
Copy link
Author

00F100 commented Sep 23, 2019

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @00F100, here is your new Magento instance.
Admin access: https://pr-24379.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@00F100 00F100 requested a review from orlangur September 23, 2019 19:23
@orlangur
Copy link
Contributor

@00F100, sorry, I don't have time to do it instead of you. Try searching using info from #20906 (comment), probably something was mentioned in git log.

@00F100 00F100 closed this Oct 4, 2019
@m2-assistant
Copy link

m2-assistant bot commented Oct 4, 2019

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

@00F100 00F100 deleted the issue-21910 branch October 4, 2019 02:06
@orlangur
Copy link
Contributor

orlangur commented Oct 4, 2019

Sorry @00F100, I would like to find an accordance here with you.

magento 2.3.3 is not on github

I wasn't aware, sorry, just saw the #20906 (comment) which is from July.

I don't know what was the way to fix it in 2.3.3, this issue may be even irreproducible then. What I know for sure is that adding columns to grid (even invisible ones) is not a proper solution.

To find a proper solution let's wait for a release publishing, I'll ask EngCom guys in Slack if there is any ETA.

@magento magento deleted a comment from 00F100 Oct 7, 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.

6 participants