Skip to content

Price section - Small refactor and style fix #34558

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
Merged
octaedro merged 11 commits into from
Sep 15, 2022
Merged

Price section - Small refactor and style fix #34558

octaedro merged 11 commits into from
Sep 15, 2022

Conversation

@octaedro
Copy link
Contributor

@octaedro octaedro commented Sep 2, 2022 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

This PR fixes a few things related to the prices section and addresses a couple of suggestions made for this PR.

It adds:

  • a small visual bugfix (the help tooltip popped up too high)
  • a refactor of the method to sanitize the price.
  • a change in the Sale price validation (now the Sale price has to be equal to or higher than the List price).
  • an error message when the Sale price has an error.

Closes # 38-gh-woocommerce/mothra-private.

How to test the changes in this Pull Request:

  1. Verify that the Feature this product tooltip looks and works correctly.

Screen Shot 2022-09-07 at 12 26 57

  1. Verify that the prices look correct. Check the testing instructions here too.
  2. Create a new product without List price but with a Sale price and verify that the error message Sale price cannot be equal to or higher than list price. is being shown after the Sale price loses the focus.
  3. Set a Sale price equal to or higher than the List price and verify that the warning Sale price cannot be equal to or higher than list price. is visible under the Sale price.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@github-actions github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Sep 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2022 •
edited
Loading

Test Results Summary

Commit SHA: 2bc84f4

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests11800201200m 55s
E2E Tests186001018716m 53s
 To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@octaedro octaedro changed the title Update/30 list price field follow up Price section - Small refactor and style fix Sep 2, 2022
@octaedro octaedro force-pushed the update/30_list_price_field_follow_up branch from 80ded93 to eb5120c Compare September 7, 2022 14:20
@octaedro octaedro requested a review from a team September 9, 2022 12:43
@octaedro octaedro marked this pull request as ready for review September 9, 2022 17:32
louwie17
louwie17 reviewed Sep 14, 2022
View reviewed changes
Copy link
Contributor

@louwie17 louwie17 left a comment

Choose a reason for hiding this comment

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

Overall nice work @octaedro, I just left a couple comments and one in relation to this test instruction:

Create a new product without List price but with a Sale price and verify that the List price is set and the Sale price has been deleted after saving. This should happen after pressing Save draft, or Publish, or Publish & duplicate, or Copy to a new draft.

This only worked for me when I never touched the list price and only added the sale price. But as soon as I edited the list price and cleared it, this logic wouldn't work anymore. I believe this would be the same for existing products.
I left a comment within the code to check for empty string as well.

Also I was curious if this logic would be semi redundant once @mdperez86 PR is in with disabling the action buttons?

plugins/woocommerce-admin/client/products/product-form-actions.tsx Outdated

const maybeSetRegularPrice = ( product: Product ) => {
if (
product.regular_price === undefined &&
Copy link
Contributor

@louwie17 louwie17 Sep 14, 2022

Choose a reason for hiding this comment

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

Should we also check if regular_price is an empty string?
As this will break as soon as the user edits the list price, but clears it.

Copy link
Contributor Author

@octaedro octaedro Sep 14, 2022

Choose a reason for hiding this comment

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

Nice catch! This would be redundant considering Maikel's PR here. I removed the method maybeSetRegularPrice in commit 0e65863

plugins/woocommerce-admin/client/products/use-product-helper.ts Outdated
} );
const context = useContext( CurrencyContext );
const { getCurrencyConfig } = context;
const { decimalSeparator } = getCurrencyConfig();
Copy link
Contributor

@louwie17 louwie17 Sep 14, 2022

Choose a reason for hiding this comment

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

Should we move the above two lines into the sanitizePrice function? given the decimealSeperator isn't used anywhere else, and would needlessly be triggered on every re-render. It's probably not super JS extensive, but it be nice to keep that stuff to a minimum.

Copy link
Contributor Author

@octaedro octaedro Sep 14, 2022

Choose a reason for hiding this comment

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

Good idea! I changed it in commit b60ef79

@octaedro
Copy link
Contributor Author

octaedro commented Sep 14, 2022

Thank you @louwie17 for your review,
I just updated the testing instructions (they were outdated) and addressed the suggestions you made.
Could you take another look at this PR?

@octaedro octaedro requested a review from louwie17 September 14, 2022 20:45
@github-actions github-actions bot added the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Sep 14, 2022
@octaedro octaedro force-pushed the update/30_list_price_field_follow_up branch from 0e65863 to 2bc84f4 Compare September 14, 2022 20:52
@github-actions github-actions bot removed the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Sep 14, 2022
louwie17
louwie17 approved these changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

@louwie17 louwie17 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates on this @octaedro, this tested well and changes look good :)

@octaedro octaedro merged commit dadb1d6 into trunk Sep 15, 2022
@octaedro octaedro deleted the update/30_list_price_field_follow_up branch September 15, 2022 13:56
@github-actions github-actions bot added this to the 7.0.0 milestone Sep 15, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2022

Hi @octaedro, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

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

Reviewers

@louwie17 louwie17 louwie17 approved these changes

Assignees

No one assigned

Labels

plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@octaedro @louwie17