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

Resolve "Discount Amount" field is validated after the page load without any action from user in Create New Catalog Rule form issue23777 #23779

Conversation

edenduong
Copy link
Contributor

@edenduong edenduong commented Jul 19, 2019

Description (*)

  1. "Discount Amount" field is validated after the page load without any action from user in Create New Catalog Rule form #23777: Resolve "Discount Amount" field is validated after the page load without any action from user in Create New Catalog Rule form

When the page loading, I see the function in vendor/magento/module-ui/view/base/web/js/form/element/abstract.js

        changed = utils.compare(rules, this.validation).equal;

        if (changed) {
            this.required(!!rules['required-entry']);
            this.validate();
        }

try to log it:

image

Because the "discount_amount" in the xml field has the default validate (required-entry) doesn't match with the default "Apply" field "Apply as percentage of original" (required-entry, validate-number-range 0-100) so that field was validated after the page load. So we should add the "validate-number-range 0-100" to "discount_amount" default validate. It will match and no-validation anymore :)

Fixed Issues (if relevant)

  1. "Discount Amount" field is validated after the page load without any action from user in Create New Catalog Rule form #23777: "Discount Amount" field is validated after the page load without any action from user in Create New Catalog Rule form

Manual testing scenarios (*)

  1. In back end, go to Marketing -> Catalog Price Rule -> Add New Rule
  2. Scroll to the Action fieldset
  3. See the " Discount Amount" Field

Result:

  1. Normal field, no error when page loads.

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 Jul 19, 2019

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

Stepa4man
Stepa4man previously approved these changes Jul 19, 2019
Copy link
Contributor

@Stepa4man Stepa4man left a comment

Choose a reason for hiding this comment

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

Hi @edenduong thank you for your contribution.

We already have similar pull request for this issue.
Not sure that the author will continue work.

I just copy my comment from that PR here:
Adding default value is not actually the best solution. We don't need zero here. Otherwise, we need a user to pay attention to this field. Better find out why validation fires during page rendering instead of validating by hitting the save button?

I see you are a cool contributor and I would appreciate if you'll investigate and provide a better solution here.

Thanks.

@ghost ghost assigned Stepa4man Jul 19, 2019
@Stepa4man Stepa4man dismissed their stale review July 19, 2019 07:47

Accidentally approved

Copy link
Contributor

@Stepa4man Stepa4man left a comment

Choose a reason for hiding this comment

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

Please see my comment. Thanks

@magento-engcom-team
Copy link
Contributor

Hi @Stepa4man, thank you for the review.
ENGCOM-5467 has been created to process this Pull Request
✳️ @Stepa4man, 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

@edenduong
Copy link
Contributor Author

edenduong commented Jul 19, 2019

@Stepa4man: Thank you for your feedback. I will find other solution :) I suggest the default value for the form because I see in the salerule, it fill the default 0.

Thank you very much. :)

@Stepa4man
Copy link
Contributor

@Stepa4man: Thank you for your feedback. I will find other solution :) I suggest the default value for the form because I see in the salerule, it fill the default 0.

Thank you very much. :)

@edenduong From the user experience adding a predefined value of the most important field in Rule form is not a pretty good solution.

Sometimes merchant creates a lot of rules one by one manually and this routine could cause some unfilled form fields.
If merchant creates the rule and forget to fill discount amount I believe he would appreciate validation, that this field is empty instead of saving 0 discount accidentally.

Not everything that we have already done in Magento is the best solution:)
That's why we are here! To make Magento great again!

@edenduong edenduong force-pushed the 2.3-bugfix/set-default-discount-amount-issue23777 branch from e4e891f to 76ac8d9 Compare July 19, 2019 09:40
…out any action from user in Create New Catalog Rule form issue23777
@edenduong edenduong force-pushed the 2.3-bugfix/set-default-discount-amount-issue23777 branch from 76ac8d9 to 4954bd1 Compare July 20, 2019 01:00
@edenduong
Copy link
Contributor Author

edenduong commented Jul 20, 2019

Hello @Stepa4man :

I have just changed the source code.
When the page loading, I see the function in vendor/magento/module-ui/view/base/web/js/form/element/abstract.js

        changed = utils.compare(rules, this.validation).equal;

        if (changed) {
            this.required(!!rules['required-entry']);
            this.validate();
        }

try to log it:

image

Because the "discount_amount" in the xml field has the default validation (required-entry) doesn't match with the default "Apply" field "Apply as percentage of original" - callback validation rule (required-entry, validate-number-range 0-100) => that field was validated after the page load. So we should add the "validate-number-range 0-100" to "discount_amount" default validation. It will match and no-validation anymore :)

@edenduong edenduong requested a review from Stepa4man July 20, 2019 01:07
@Stepa4man
Copy link
Contributor

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

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

Copy link
Contributor

@Stepa4man Stepa4man left a comment

Choose a reason for hiding this comment

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

Good job, @edenduong. Thanks for your effort!

@magento-engcom-team
Copy link
Contributor

Hi @Stepa4man, thank you for the review.
ENGCOM-5467 has been created to process this Pull Request
✳️ @Stepa4man, 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-Delta engcom-Delta self-assigned this Jul 22, 2019
@engcom-Delta
Copy link
Contributor

✔️ QA passed

@m2-assistant
Copy link

m2-assistant bot commented Jul 27, 2019

Hi @edenduong, 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 pushed a commit that referenced this pull request Jul 27, 2019
…age load without any action from user in Create New Catalog Rule form issue23777 #23779
@magento-engcom-team magento-engcom-team added this to the Release: 2.3.3 milestone Jul 27, 2019
@VladimirZaets VladimirZaets added the Auto-Tests: Not Required Changes in Pull Request does not require coverage 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.

None yet

6 participants