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

Add validation to enable Save attributes and Save variations buttons #37046

Merged
merged 13 commits into from Mar 9, 2023

Conversation

octaedro
Copy link
Contributor

@octaedro octaedro commented Mar 2, 2023

All Submissions:

Changes proposed in this Pull Request:

This PR disables the Save attributes and Save variations buttons until a value is added in the Name and Value fields.
We want to prevent triggering the action with empty values.

Screen Capture on 2023-03-03 at 11-12-25

Closes #37021.

How to test the changes in this Pull Request:

  1. Go to Products > Add New > Product data > Attributes
  2. The button Save attributes will be disabled.
  3. Add a name and one or more values.
  4. The save button should be enabled after filling out the inputs.
  5. Next to Product data change the product type to Variable product.
  6. Go to Variations and repeat steps 3 and 4.
  7. The save buttons should work correctly.

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?
  • Have you included testing instructions?

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 the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 2, 2023
@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Merging #37046 (7475492) into trunk (6f8f35b) will increase coverage by 0.0%.
The diff coverage is 87.5%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             trunk   #37046   +/-   ##
========================================
  Coverage     46.7%    46.7%           
+ Complexity   17188    17187    -1     
========================================
  Files          429      429           
  Lines        64821    64829    +8     
========================================
+ Hits         30251    30276   +25     
+ Misses       34570    34553   -17     
Impacted Files Coverage Δ
...ocommerce/includes/admin/class-wc-admin-assets.php 0.0% <0.0%> (ø)
plugins/woocommerce/includes/class-wc-tracker.php 89.8% <100.0%> (+0.1%) ⬆️
.../includes/import/class-wc-product-csv-importer.php 75.1% <100.0%> (+0.2%) ⬆️
...lugins/woocommerce/includes/wc-order-functions.php 76.4% <0.0%> (+0.1%) ⬆️
plugins/woocommerce/includes/class-wc-tax.php 79.0% <0.0%> (+0.5%) ⬆️
...udes/data-stores/class-wc-order-data-store-cpt.php 91.1% <0.0%> (+3.5%) ⬆️

@octaedro octaedro changed the title Fix/37021 add validation when saving attributes Add validation to enable Save attributes and Save variations buttons Mar 3, 2023
@octaedro octaedro force-pushed the fix/37021_add_validation_when_saving_attributes branch from 9a2225d to b7aa25b Compare March 3, 2023 13:59
@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2023

Test Results Summary

Commit SHA: 7475492

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202611m 10s
E2E Tests189006019513m 20s

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 marked this pull request as ready for review March 3, 2023 14:16
@octaedro octaedro requested a review from a team March 3, 2023 14:16
@octaedro octaedro self-assigned this Mar 3, 2023
@octaedro octaedro added the focus: product Issues related to product or product page. label Mar 3, 2023
Comment on lines 57 to 64
var attribute_name = new_attribute_data.find( 'input[name^="attribute_names"]' ).val();
var attribute_value = new_attribute_data
.find( 'textarea[name^="attribute_values"]' )
.val();

if ( ! attribute_name || ! attribute_value ) {
return;
}
Copy link
Contributor Author

@octaedro octaedro Mar 3, 2023

Choose a reason for hiding this comment

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

I removed this validation in this commit because we're already disabling the button and it would duplicate the validation.
I'm not opposed to being more defensive, but I wanted a second opinion.

@octaedro octaedro marked this pull request as draft March 3, 2023 15:36
@octaedro octaedro force-pushed the fix/37021_add_validation_when_saving_attributes branch from 2dc4aa9 to 23cd959 Compare March 3, 2023 18:14
@octaedro octaedro marked this pull request as ready for review March 6, 2023 15:58
@@ -64,6 +64,7 @@ test.describe.serial( 'Add New Variable Product Page', () => {
'val1 | val2'
);
}
await page.keyboard.press( 'ArrowUp' );
Copy link
Contributor Author

@octaedro octaedro Mar 6, 2023

Choose a reason for hiding this comment

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

We need to call this event because the Playwright method page.fill is not triggering the keyboard event. I we don't do this the e2e will fail.

@nathanss
Copy link
Contributor

nathanss commented Mar 8, 2023

I found some bugs, the validation is not keeping track of all actions that can make it change. Some action seems to be making it lose "sync":

bug1.mov
bug2.mov

Fernando Marichal added 11 commits March 8, 2023 12:42
# Conflicts:
#	plugins/woocommerce/includes/admin/meta-boxes/views/html-product-data-variations.php
# Conflicts:
#	plugins/woocommerce/client/legacy/js/admin/meta-boxes-product-variation.js
# Conflicts:
#	plugins/woocommerce/includes/admin/meta-boxes/views/html-product-data-variations.php
@octaedro octaedro force-pushed the fix/37021_add_validation_when_saving_attributes branch from 47e4831 to 932745d Compare March 8, 2023 15:44
@octaedro
Copy link
Contributor Author

octaedro commented Mar 8, 2023

Nice catch @nathanss!
I just fixed the bugs you found, could you take another look at this PR?

@joelclimbsthings joelclimbsthings requested review from nathanss and removed request for a team March 8, 2023 20:30
Copy link
Contributor

@nathanss nathanss left a comment

Choose a reason for hiding this comment

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

Nice job. The issues seem to have been fixed! Approving it.

@nathanss nathanss merged commit 3edd8f4 into trunk Mar 9, 2023
20 checks passed
@nathanss nathanss deleted the fix/37021_add_validation_when_saving_attributes branch March 9, 2023 13:32
@github-actions github-actions bot added this to the 7.6.0 milestone Mar 9, 2023
@jtobiesen
Copy link

jtobiesen commented May 16, 2023

zen- 6267640, experiencing bug 2 from @nathanss earlier video.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: product Issues related to product or product page. plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send empty form when clicking "Save attribute"
3 participants