Skip to content

Conversation

@barryhughes
Copy link
Member

@barryhughes barryhughes commented Nov 29, 2022

This change builds on #34282 and makes a small amendment to improve compatibility with extensions like Composite Products.

In the earlier PR, we made it so that the quantity selector is still visible even if the min and max allowed quantities are set to the same value (for example, if precisely 2 units must be purchased). Whereas, previously, it was still rendered but was invisible.

However, as part of that earlier change, we also stopped outputting the min and max attributes in some cases ... unfortunately, this posed a compatibility concern for some extensions which expect and depend on the min and max attributes being available. So, in this PR, we restore them.

All Submissions:

Changes proposed in this Pull Request:

Always render the min and max attributes for product quantity selectors, even in readonly mode where the quantity is fixed.

  • This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

Start by re-testing as per the original instructions contained in #34282:

  1. Add https://github.com/woocommerce/woocommerce-min-max-quantities.
  2. Create a simple product with min_qty=max_qty.
  3. Go add the product page. You will be able to see the quantity input there.

Optionally, you can continue to test by installing and activating Composite Products. To learn more about how this extension works please see these docs but, in summary, all you need to do is:

  1. Add two new products such as "RAM 128KB" ($5) and "RAM 256KB" ($10).
  2. Add a new composite product. In the Components tab add a new component called "Memory" and link the two products you created in the last step.
    • Initially, set the min to 1 and max to 5.
    • Enable the priced individually option.

With that done:

  1. As a customer, visit the product page. You should be able to purchase the product and the price in the cart should match your expectations.
  2. Now edit the composite product and set the min and max values to the same value. Save.
  3. Repeat the process ... of course, you can still specify the type of memory (128KB or 512KB) but can longer specify how much you want. Add to the cart, pricing again should match your expectations (based on the unit cost and the predetermined quantity, etc).

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 created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?

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.

Even if these are set to the same value (and so the quantity cannot be changed--which results in a readonly quantity selector) we must output them for compatibility with code (example: Composite Products) that expects them to always be present.
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Nov 29, 2022
@barryhughes barryhughes marked this pull request as ready for review November 29, 2022 17:57
@barryhughes barryhughes requested review from a team and Konamiman and removed request for a team November 29, 2022 17:58
@barryhughes
Copy link
Member Author

barryhughes commented Nov 29, 2022

💡 Re failing required check "highlight template changes", in this case our initial goal is to ship this in 7.2.0 (I've made a request for a code freeze exception) so we don't require a further bump. I assume we can just ignore and request that Atlas force merge later on.

@github-actions
Copy link
Contributor

Test Results Summary

Commit SHA: 58ed07d

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202611m 2s
E2E Tests186006019213m 41s

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.

@jimjasson
Copy link
Contributor

Thank you @barryhughes for fixing this so quickly! Really appreciate it!

I tested the fix and confirmed that the issue we witnessed in Composite Products has been resolved.

Here's a video before the fix (the total price is not updated):

Screen.Capture.on.2022-11-30.at.10-24-07.mp4

and here's a video after the fix (the total price is updated):

Screen.Capture.on.2022-11-30.at.10-25-09.mp4

@jorgeatorres jorgeatorres requested review from jorgeatorres and removed request for Konamiman December 1, 2022 18:47
Copy link
Member

@jorgeatorres jorgeatorres left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @barryhughes!

@jorgeatorres jorgeatorres merged commit fee3e10 into trunk Dec 1, 2022
@jorgeatorres jorgeatorres deleted the fix/34282-min-max-attrs branch December 1, 2022 19:08
@github-actions github-actions bot added this to the 7.3.0 milestone Dec 1, 2022
@samueljseay samueljseay modified the milestones: 7.3.0, 6.0.0, 7.2.0 Dec 1, 2022
github-actions bot pushed a commit that referenced this pull request Dec 1, 2022
…35767)

Even if these are set to the same value (and so the quantity cannot be changed--which results in a readonly quantity selector) we must output them for compatibility with code (example: Composite Products) that expects them to always be present.
samueljseay pushed a commit that referenced this pull request Dec 7, 2022
* Always output the min and max attributes for the quantity selector. (#35767)

Even if these are set to the same value (and so the quantity cannot be changed--which results in a readonly quantity selector) we must output them for compatibility with code (example: Composite Products) that expects them to always be present.

* Prep for cherry pick 35767

Co-authored-by: Barry Hughes <[email protected]>
Co-authored-by: WooCommerce Bot <[email protected]>
@Stas0238
Copy link

Hello everyone !
I would also recommend you to add a condition (of $readonly) for size attribute because if input has number type then size attribute is not needed and it shows an error on SEO testers -> https://prnt.sc/JEi-wYw0Lh2K
Details -> https://prnt.sc/5c1lUmUZWfyW
Please, double check if it does not create any issues in other SEO/HTMLvalidator areas but I had to notify you with this information I recently got from testers.
Regards, @Stas0238

@barryhughes
Copy link
Member Author

Hi @Stas0238, please do create a fresh bug report if you see problems. That's the best way to ensure it is seen and triaged 👍

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

Labels

plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants