Store: Save the changes made to any WCS shipping method #21875

DanReyLop · 2018-01-26T16:24:52Z

Closes #19527

This PR attaches the WCS-specific logic to the Action List that manages saving changes in the shipping zones/methods. For creating a normal shipping method, these are the steps needed to create it:

POST /shipping/zones/<id>/methods with the method_type.
PUT /shipping/zones/<id>/methods/<method_id> with the rest of the settings.

For creating a WCS shipping method:

POST /shipping/zones/<id>/methods with the method_type.
PUT /shipping/zones/<id>/methods/<method_id> with the enabled flag to true or false (not needed if it wasn't changed by the user).
POST /connect/services/<methodType>/<id> with the rest of the settings.

Since this is the latest PR of the chain, it also has some cleanup, specially with unused actions and reducers.

matticbot · 2018-01-26T16:24:53Z

Test live: https://calypso.live/?branch=add/wcs-shipping-methods-save

a8ci18n · 2018-01-26T16:30:29Z

client/extensions/woocommerce/state/data-layer/ui/shipping-zones/index.js

+
+	if ( isWcsMethod && ! isEmpty( extraMethodProps ) ) {
+		steps.push( {
+			description: translate( 'Updating Shipping Method (extra settings)' ),


Hi! I've found a possible matching string that has already been translated 1 time:
translate( 'Updating Shipping Method' ) ES Score: 7

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

allendav · 2018-01-31T23:30:18Z

client/extensions/woocommerce/woocommerce-services/state/service-settings/actions.js

+	type: WOOCOMMERCE_SERVICES_SERVICE_SETTINGS_UPDATE_FIELD,
+	siteId,
+	methodId,
+	methodType: 'wc_services_usps', // Will work for the other methods too since they share the same reducer


This action creator only works for USPS? Or does the comment mean it will work for others too?

Edit: If I follow this change correctly, it would appear this hardcoding means that WOOCOMMERCE_SERVICES_SHIPPING_ZONE_METHOD_UPDATE can only update USPS methods - is that correct? What about Canada Post?

I tested Canada Post and it updates the fields and saves them fine. But I think we should avoid hardcoding this value, even if it means extracting it from somewhere else, or passing it from the component as an argument.

Edit: Looks like this is being used to determine if the shipping method is WCS. Would methodType: 'wc_services be enough here?

This action creator only works for USPS? Or does the comment mean it will work for others too?

It works for the other WCS methods too.

Looks like this is being used to determine if the shipping method is WCS. Would methodType: 'wc_services be enough here?

No. The methodType needs to match one of the builtInShippingMethods:

wp-calypso/client/extensions/woocommerce/state/ui/shipping/zones/methods/reducer.js

Line 32 in 9fec0bd

export const builtInShippingMethods = {

Since all the WCS methods use the same reducer, it doesn't matter what methodType is used, as long as it's in that object. I'll reword the comment to make it clearer.

I still think we should avoid hardcoding this and try to dynamically retrieve it somehow, but not going to fight for it - if it works, it works :)

allendav · 2018-01-31T23:33:41Z

client/extensions/woocommerce/woocommerce-services/state/service-settings/reducer.js

-	}
-	return Object.assign( {}, state, newObj );
+reducers[ WOOCOMMERCE_SERVICES_SERVICE_SETTINGS_UPDATE_FIELD ] = ( state, action ) => {
+	return objectPath.set( state, action.path, action.value );


Very nice - object-path-immutable is a nice improvement here

allendav · 2018-02-01T00:00:23Z

It would be best if there were tests for the new action creators and reducers that were added.

allendav · 2018-02-01T00:55:55Z

I have exactly one zone (the "rest of the world" zone) and one instance (# 15) of WooCommerce Services USPS in it (and a flat rate and free shipping and a local pickup)

But with this branch, I notice requests for an instance # 15 and an instance # 0 that doesn't exist (and yet WCS responds successfully)

marcinbot · 2018-02-01T11:21:30Z

I noticed a couple of UI problems that probably belonged to the previous PRs:

we don't show anything in the Cost column. If the method is disabled, it results in a string - Disabled being displayed on the main shipping settings page. The original designs showed the number of services selected, but then the column probably shouldn't be named Cost. CC @kellychoffman
We limit the number of non-local-pickup methods to 1 per zone, i.e. Free Shipping and Flat Rate can be added only once. But you can add more than one instance of a WCS method. Should we limit that as well?

But with this branch, I notice requests for an instance # 15 and an instance # 0 that doesn't exist (and yet WCS responds successfully)

@allendav: 0 is the id for rest of the world.

kellychoffman · 2018-02-01T19:59:54Z

we don't show anything in the Cost column.

We around and around about this. Thinking this makes the most sense: "Live rates calculated at checkout"

DanReyLop · 2018-02-06T12:59:25Z

But with this branch, I notice requests for an instance # 15 and an instance # 0 that doesn't exist (and yet WCS responds successfully)

@allendav /method_name/0 fetches the schema for that shipping method. Since in Calypso everything is prefetched and then the UI works just with local state, we need to fetch the schemas/layouts for all the methods even if the user haven't added them yet to any shipping zone. See #21613

We limit the number of non-local-pickup methods to 1 per zone, i.e. Free Shipping and Flat Rate can be added only once. But you can add more than one instance of a WCS method. Should we limit that as well?

@marcinbot I explicitly added the logic to allow more than 1, but I can remove it if it doesn't make sense: https://github.com/Automattic/wp-calypso/pull/21871/files#diff-67d8b69b04cddf7fe76d0d1d93e1ba3eR286

We around and around about this. Thinking this makes the most sense: "Live rates calculated at checkout"

Done in 9fa9763

marcinbot

The linter breaks on the newly added comment, and there's an issue of the hardcoded string which I don't feel that strongly about, but otherwise it looks good. Preapproving

* Added save logic for WCS shipping methods * Update the state after the WCS shipping method is saved * Simplified reducers, remove unused code. * Fixed tests * Fixed lint errors * Added "Cost" summary for the WCS methods * Fix lint error

* Copy all the data-driven form files into Calypso This copies all the data-driven form files that were originally in the `client/apps/settings` folder on the `woocommerce-services` repo into Calypso. The import paths have been fixed, and the required generic files (such as components) have been copied as well, but there was no more work beyond that. This PR should be a baseline for the rest of the feature, I didn't want to make it even bigger. In the process of bringing everything to Calypso, I removed the parts that don't apply to the Shipping Services form, such as toggles, indicators or textareas (they're only used in the WP-Admin self-help page. * Fixed lint errors * Remove "reselect" dependency * Add the WCS shipping methods to the "Method type" dropdown, when they have been returned from the server. * Fixed tests * Change the data-driven endpoint builder name, fix the URL. * Fetch the WooCommerce Services shipping methods' schemas when the shipping methods are fetched. * Added comment for translators * Rename "shipping-schema" to "shipping-method-schema" to clarify * Renamed action constant * Load each WCS shipping method settings when loading the shipping zones * Simplified settings fetching, now get stored at the same Redux branch as the built-in methods * Fixed tests * remove reducer/actions unused code (doesn't apply to the Calypso flow) * Add back the required "NumberInput" component (for the Fallback Rate field) * Fix import paths * Plug the shipping method settings UI into the Calypso dialog * Handle "default" schema values for the shipping settings forms * Remove dead code and tests * Partialy re-implementing memoization for the Errors selector. `lib/create-selector` doesn't allow objects as cache keys (`reselect` does), so sadly every time this function is called will return a different object, which will trigger a repaint. This needs to be addressed in the future, the easiest solution would be to install `reselect` or tweak `create-selector` to work like `reselect`. * Added FedEx, because why not. * Fix the "Packaging Manager" link that was pointing to WP-Admin. * Fixed the Errors selector, with good memoization. * Fixed React warning. * Added validation to the form. * Trigger a full validation when the "Close dialog" button is clicked, abort closing it if there are errors. * Validate that there must be at least one service enabled. * Renamed selector * "isWcsEnabled" now returns "true" if it's active, "false" if it's not, and "null" if it's loading (which still evaluates to "boolean falsey" to not break any functionality). * Added an optional parameter "siteId" to the areShippingZonesFullyLoaded selector, use it when the calling component is passed the "siteId". * Fixed race condition. Only check the (static) WCS feature flag before fetching WCS methods schemas and settings, no need to see if the plugin is actually enabled. If it's not enabled, the REST endpoints won't return WCS methods in the first place. * Added tests for the modified selectors * Added selector test * Store: Save the changes made to any WCS shipping method (#21875) * Added save logic for WCS shipping methods * Update the state after the WCS shipping method is saved * Simplified reducers, remove unused code. * Fixed tests * Fixed lint errors * Added "Cost" summary for the WCS methods * Fix lint error * Fixed accessibility lint error (missing "for" in a <label>) * Fixed "Price adjustment" tooltip * Fixed margin-bottom * Fix partially-selected checkboxes styles in Firefox * Fix service group closing itself when typing a negative adjustment value. * Make only the content of the method modal be scrollable

Daniel Rey Lopez added 4 commits January 26, 2018 10:40

Added save logic for WCS shipping methods

a4bb8cc

Update the state after the WCS shipping method is saved

7c892bb

Simplified reducers, remove unused code.

37fe491

Fixed tests

c358b87

DanReyLop added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Store Store Services labels Jan 26, 2018

DanReyLop self-assigned this Jan 26, 2018

a8ci18n reviewed Jan 26, 2018

View reviewed changes

Fixed lint errors

9fec0bd

allendav reviewed Jan 31, 2018

View reviewed changes

Added "Cost" summary for the WCS methods

9fa9763

marcinbot approved these changes Feb 8, 2018

View reviewed changes

Fix lint error

5cfcb80

DanReyLop merged commit 4c27e77 into add/wcs-shipping-methods-validate Feb 8, 2018

DanReyLop deleted the add/wcs-shipping-methods-save branch February 8, 2018 15:10

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store: Save the changes made to any WCS shipping method #21875

Store: Save the changes made to any WCS shipping method #21875

DanReyLop commented Jan 26, 2018

matticbot commented Jan 26, 2018

a8ci18n Jan 26, 2018

allendav Jan 31, 2018 •

edited

Loading

marcinbot Feb 1, 2018 •

edited

Loading

DanReyLop Feb 6, 2018

marcinbot Feb 8, 2018

allendav Jan 31, 2018

allendav commented Feb 1, 2018

allendav commented Feb 1, 2018 •

edited

Loading

marcinbot commented Feb 1, 2018

kellychoffman commented Feb 1, 2018

DanReyLop commented Feb 6, 2018

marcinbot left a comment

Store: Save the changes made to any WCS shipping method #21875

Store: Save the changes made to any WCS shipping method #21875

Conversation

DanReyLop commented Jan 26, 2018

matticbot commented Jan 26, 2018

a8ci18n Jan 26, 2018

Choose a reason for hiding this comment

allendav Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

marcinbot Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

DanReyLop Feb 6, 2018

Choose a reason for hiding this comment

marcinbot Feb 8, 2018

Choose a reason for hiding this comment

allendav Jan 31, 2018

Choose a reason for hiding this comment

allendav commented Feb 1, 2018

allendav commented Feb 1, 2018 • edited Loading

marcinbot commented Feb 1, 2018

kellychoffman commented Feb 1, 2018

DanReyLop commented Feb 6, 2018

marcinbot left a comment

Choose a reason for hiding this comment

allendav Jan 31, 2018 •

edited

Loading

marcinbot Feb 1, 2018 •

edited

Loading

allendav commented Feb 1, 2018 •

edited

Loading