Fix editing Shipping methods auto-saving #38431

octaedro · 2023-05-24T17:40:19Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

This PR fixes the issue with Shipping Methods auto-saving the entire form. Now, when a shipping method is added and there is another change in the form, the Save changes button will remain enabled, and no other change will be saved except for the added method.

Closes #38114.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Go WooCommerce > Settings > Shipping (URL: wp-admin/admin.php?page=wc-settings&tab=shipping).
Create a shipping zone and click edit.
Update the zone name and add a new shipping method
Notice how the loading icon shows up on the shipping method table, and the form gets saved without clicking Save changes.
Add another change to the form (like changing Zone name).
Add another shipping method.
Verify that the Save changes button is still enabled and only the added shipping method is saved.

We currently don't have a method to delete shipping zones individually, which is why I opted for this workaround. The form will be saved automatically if we add a new shipping method. However, if another change is made, then pressing Save changes will be necessary to save it.

When editing a specific shipping method, all the changes in the form will be saved after pressing the Save changes button inside the modal.

github-actions · 2023-05-24T17:48:01Z

Hi @louwie17,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

github-actions · 2023-05-24T18:05:42Z

Test Results Summary

Commit SHA: 0d86f40

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	1m 12s
E2E Tests	195	0	0	10	0	205	15m 15s

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.

louwie17

@octaedro unfortunately this looks like it will require a whole lot more work, as the request to woocommerce_shipping_zone_add_method already saves the shipping method.

As I am looking at this, it looks like these steps should be implemented, if we really wanted to do it well (otherwise we should just add some clarifying notes in different places):

New Ajax call to retrieve all the shipping method data ( title, description, etc )
On add shipping method replace call to woocommerce_shipping_zone_add_method with retrieving all shipping method data (with new ajax call) and display within table (keep track of this data )
Keep track of changes within shipping method changes and change Save changes button to Done or something to that affect.
When user hits Save changes we submit all the changed data ( new methods, and updated method changes, etc...)

@pmcpinto this was to address a bug found during one of the user flow calls and turns out to be quite a big task and re-factor that will probably take a good week (or longer ) of developer time.
Did we potentially want to re-consider this?
I am also trying to think of alternate (less dev heavy ) solutions, like maybe just adding a note that adding a new shipping method auto saves the form?

louwie17 · 2023-05-29T13:35:41Z

plugins/woocommerce/client/legacy/js/admin/wc-shipping-zone-methods.js

 										);
 									}
 								}
 								// Trigger save if there are changes, or just re-render


Hmm although these changes do prevent the Save changes from being disabled again, it is actually the request above that saves the data. So even if I don't hit Save changes and refresh the page the new shipping method is still in the list.

pmcpinto · 2023-05-30T14:12:31Z

Hey @louwie17 thanks for the heads up.

Did we potentially want to re-consider this?
I am also trying to think of alternate (less dev heavy ) solutions, like maybe just adding a note that adding a new shipping method auto saves the form?

Given the time it will take, it makes total sense to reconsider this. Displaying a snackbar informing the user that all the changes were saved could be a good workaround. It's not perfect but it does the job. @jarekmorawski what do you think?

louwie17 · 2023-05-30T14:55:54Z

Given the time it will take, it makes total sense to reconsider this. Displaying a snackbar informing the user that all the changes were saved could be a good workaround. It's not perfect but it does the job. @jarekmorawski what do you think?

A quick follow up as I also talked with @octaedro about this more. This issue does address the issue where a user may edit the zone name and then add a shipping method, that only the shipping methods are saved and the user still has to click Save changes for the zone name update.

If we want, we could keep that structure, where all the changes within the shipping methods (adding/removing/editing) is saved automatically, but outside of that requires the call to Save changes?

alopezari · 2023-05-31T14:52:53Z

Hi @octaedro!

I reviewed the changes in the E2E tests and they look good. I ran them in multiple combinations (the whole spec in headless, headed and UI mode, as well as running all these tests individually) and they always passed. I also ran them several times consecutively in order to spot potential flaky issues, but they seem to be pretty consistent.

I don't know if it will need any further changes based on the recent comments, in that case, let us know and we'll happily review the testing details again. Thank you!

louwie17

Overall this is testing quite well, nice work on this @octaedro!
I did leave two suggestions, on the window confirmation dialog before removing and showing a window alert instead of a console alert.

louwie17 · 2023-06-01T09:31:35Z

plugins/woocommerce/client/legacy/js/admin/wc-shipping-zone-methods.js

-					model.set( 'methods', methods );
-					model.logChanges( changes );
-					view.render();
+					shippingMethodView.block();


We also discussed showing a confirmation dialog first, to see if the user really wanted to delete the shipping method, could we still add that?

louwie17 · 2023-06-01T09:33:50Z

plugins/woocommerce/client/legacy/js/admin/wc-shipping-zone-methods.js

+							shippingMethodView.unblock();
+						},
+						error: function( jqXHR, textStatus, errorThrown ) {
+							console.error( 'There was an error removing the shipping method.', errorThrown );


Instead of a console.error I think we should trigger a window.alert similar to other places within this JS file.
See window.alert( data.strings.save_failed ); cases. We did have to add a new localized string, it looks like there is already one for add_method_failed, so we could also create one for remove_method_failed?

Good call @louwie17! I just added the changes you mentioned. Could you take another look at this PR?

Please scratch my comment. I'll add a couple of e2e tests before pinging you for a re review.

pmcpinto · 2023-06-02T11:06:49Z

A quick follow up as I also talked with @octaedro about this more. This issue does address the issue where a user may edit the zone name and then add a shipping method, that only the shipping methods are saved and the user still has to click Save changes for the zone name update.
If we want, we could keep that structure, where all the changes within the shipping methods (adding/removing/editing) is saved automatically, but outside of that requires the call to Save changes?

This approach can work well. @jarekmorawski what do you think?

louwie17

LGTM 🚀 Thanks for your hard work on this @octaedro and for adding the E2E tests as well, nice work!!!

* Fix Shipping Methods auto save * Add changelog * Fix e2e tests * Fix e2e tests * Add `shipping_zone_remove_method` * Fix `onDeleteRow` * Fix lint * Fix delete method * Add confirmation prompt and error message * Fix lint * Fix js * Add e2e test * Fix tests

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label May 24, 2023

octaedro marked this pull request as ready for review May 24, 2023 17:42

octaedro self-assigned this May 24, 2023

octaedro requested a review from a team May 24, 2023 17:47

github-actions bot added the focus: e2e tests Issues related to e2e tests label May 25, 2023

louwie17 requested changes May 29, 2023

View reviewed changes

octaedro requested a review from louwie17 June 1, 2023 01:14

louwie17 requested changes Jun 1, 2023

View reviewed changes

octaedro requested a review from louwie17 June 1, 2023 20:53

louwie17 approved these changes Jun 5, 2023

View reviewed changes

octaedro added 13 commits June 5, 2023 10:56

Fix Shipping Methods auto save

208920f

Add changelog

18b3f35

Fix e2e tests

ce8b0f5

Fix e2e tests

8406b8d

Add shipping_zone_remove_method

7716a36

Fix onDeleteRow

91eb309

Fix lint

ab19e21

Fix delete method

b6fcbea

Add confirmation prompt and error message

c6411ad

Fix lint

1f81f2c

Fix js

1b57397

Add e2e test

b8ebe41

Fix tests

0d86f40

octaedro force-pushed the fix/38114_editing_shipping_methods_auto_saves branch from 4758bcf to 0d86f40 Compare June 5, 2023 14:01

octaedro merged commit f2aae0a into trunk Jun 5, 2023

octaedro deleted the fix/38114_editing_shipping_methods_auto_saves branch June 5, 2023 16:07

github-actions bot added this to the 7.9.0 milestone Jun 5, 2023

Fix editing Shipping methods auto-saving #38431

Fix editing Shipping methods auto-saving #38431

Uh oh!

Conversation

octaedro commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Uh oh!

github-actions bot commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

louwie17 May 29, 2023

Choose a reason for hiding this comment

Uh oh!

pmcpinto commented May 30, 2023

Uh oh!

louwie17 commented May 30, 2023

Uh oh!

alopezari commented May 31, 2023

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

louwie17 Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

louwie17 Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

octaedro Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

octaedro Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

pmcpinto commented Jun 2, 2023

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

octaedro commented May 24, 2023 •

edited

Loading

github-actions bot commented May 24, 2023 •

edited

Loading

github-actions bot commented May 24, 2023 •

edited

Loading