Remember post-print notification checkbox state #2300

waclawjacek · 2021-01-13T17:51:21Z

Description

Currently, the Mark this order as complete and notify the customer and Notify the customer with shipment details checkboxes in the Create shipping label modal are always set to a default state (unchecked). This is problematic for users who have workflows that do not match this behavior.

This PR introduces a new option post_print_notification_settings in WC_Connect_Options which stores the state of both checkboxes.

The settings can be manipulated via a REST controller which automatically happens when a label is printed (automatically or after Print is clicked) in the Create shipping label modal.

Important changes

Relying on functions rather than state when sending

Previously, the state values fulfillOrder and emailDetails were set to a result calculated from checkbox state and order status:

const onFulfillAndEmailOrderChange = ( value ) => {
	// Don't change order status if already finished.
	props.setFulfillOrderOption( orderId, siteId, value && ! isOrderFinished( order.status ) );
	// Email only if order is already complete.
	props.setEmailDetailsOption( orderId, siteId, value && isOrderFinished( order.status ) );
};

This PR repurposes these values to only contain the user preference of whether to send the notification if applicable.

Instead of relying on the state directly, the shouldFulfillOrder() and shouldEmailDetails() functions should be used instead.

Naming

The name is pretty long. :) I was also considering "Post-Print Actions" or just "Notification Settings" but for now settled for the most verbose "Post-Print Notification Settings".

Default state removal

I have removed the default state for the emailDetails and fulfillOrder fields from /client/extensions/woocommerce/woocommerce-services/lib/initialize-labels-state/index.js. I believe it was not used anywhere. If someone more familiar with the inner workings of the plugin could please let me know if this might be problematic, that would be great!

Related issue(s)

Fixes #2005

Steps to reproduce & screenshots/GIFs

The screenshot shows an order that is not completed yet. The Notify the customer with shipment details checkbox is presented in the same modal for completed orders.

Checklist

unit tests

changelog.txt entry added

To store the post-print notification settings, a new WC_Connect_Options option "post_print_notification_settings" is introduced with a setter and getter in WC_Connect_Service_Settings_Store.

Add a REST controller to allow manipulating the post-print notification settings via the REST API.

Output the post-print notification settings currently stored in the database so the front-end scripts can pick them up.

Previously the state values `emailDetails` and `fulfillOrder` were set based on whether a checkbox is checked and if `isOrderFinished()` returns true. These values are now set based on the user preference stored in the back-end at page load and are only changed when the checkbox is checked or unchecked. `isOrderFinished()` is no longer checked here. To check if a specific email should actually be sent, use `shouldEmailDetails()` and `shouldFulfillOrder()` that do include this check rather than relying on the state values directly.

The test failed when ran in a timezone UTC+N where N > 0. Added code checking what the expected result in the current timezone should be.

Instead of "value", "enabled" is used to indicate whether a notification is enabled by the back-end.

waclawjacek · 2021-01-13T17:55:46Z

There is currently a delay between when the checkbox state is loaded as opposed to paperSize which is implemented in a similar way. This might have to be improved.

harriswong · 2021-01-15T20:56:11Z

👀

harriswong · 2021-01-15T23:38:11Z

classes/class-wc-connect-service-settings-store.php

+
+			/*
+			 * WC_Connect_Options::update_option() returns `false` if the new value is the same as the old one.
+			 * As this is not an issue in this case, we return `true` here and leave the option unchanged.


Nice comment!

harriswong · 2021-01-15T23:46:43Z

classes/class-wc-rest-connect-post-print-notification-settings-controller.php

@@ -0,0 +1,34 @@
+<?php


I see we use classes/class-wc-rest-connect-shipping-label-preview-controller.php (connect/label/preview) when we save the paper_size preference. I am a little surprised that I can't find an existing end point to update settings. So this new end point makes sense.

Were you also not able to find any existing end point to update settings?

Yes!

I was considering adding the notification-settings-saving logic to an existing endpoint but:

Saving when shipping rates are requested or purchased is too early because the checkbox might be toggled afterward.

Saving when the label PDF is requested (this happens automatically after a request to purchase a label is made) was another option I considered but:

the notifications are not yet sent at this point so again, this is too early;

this is a GET request. The paper size setting is saved upon this request, but this makes more sense than saving notification settings here - the paper size has to be provided anyway so a label can be printed in the selected size.

Saving when the label PDF is displayed (either after "Print" is clicked or the PDF is opened automatically) sometimes triggers another request. This will be either a request to mark an order as completed, to send the email with details, but can also be no request at all. In such cases, a separate call would have to be made anyway.

Based on the above, I decided that placing the logic in a separate REST endpoint accessed after the "Print" button is clicked would be the cleanest solution.

The choice of the new end point makes a lot of sense now. Thank you!

harriswong · 2021-01-15T23:47:46Z

classes/class-wc-connect-service-settings-store.php

+		 * @return true|WP_Error WP_Error if an error occurred, `true` otherwise.
+		 */
+		public function set_post_print_notification_setting( $name, $enabled ) {
+			$allowed_names = array(


We can't trust the user input and I like how you add the check here instead of the controller's level. 💯

harriswong · 2021-01-15T23:52:04Z

classes/class-wc-rest-connect-post-print-notification-settings-controller.php

+			return $error;
+		}
+
+		return new WP_REST_Response( array( 'success' => true ), 200 );


Nice! There are a few different ways in our existing code to return a response/error. I am glad you picked WP_REST_Response :D 👍 💯

harriswong · 2021-01-16T00:09:32Z

...commerce-services/views/shipping-label/label-purchase-modal/rates-step/test/shipping-rate.js

@@ -94,7 +95,9 @@ describe( 'ShippingRate', () => {
 		} );

 		it( 'renders the delivery date', () => {
-			expect( shippingRateWrapper ).to.contain( <div className="rates-step__shipping-rate-delivery-date">January 1</div> ); // eslint-disable-line
+			const expectedDate = moment( shippingRateWrapper.props().rateObject.delivery_date ).format( 'LL' ).split( ',' )[0];


Is this the timezone issue? :D

Yes! The issue was that the time provided in createShippingRateWrapper() (which is 2020-01-01T23:59:00.000Z) can, in some timezones, not match the test's expectation of January 1. :)

harriswong · 2021-01-16T00:16:11Z

...nsions/woocommerce/woocommerce-services/views/shipping-label/label-purchase-modal/sidebar.js

+		} else {
+			// Don't change order status if already finished.
+			props.setFulfillOrderOption( orderId, siteId, value );
+		}


Instead of updating both all the time, now it updates either or based on order.status. I think this is fine since we have defaults and we don't need to update both every time.

harriswong

Tested locally. I checked "Mark this order as complete and notify the customer" and print a label. It refreshed with the "Notify the customer with shipment details" unchecked.

I opened up another order and "Mark this order as complete and notify the customer" is checked by default.

Thanks for adding all the tests! LGTM! 👍

c-shultz · 2021-01-16T00:58:37Z

Nice work pushing this forward and digging so deep into a new codebase, @waclawjacek 🥳

c-shultz · 2021-03-19T16:28:21Z

Let's go ahead and hold off on this until we can give WCShip more focus, and do so more thorough testing.

waclawjacek · 2021-03-19T20:06:48Z

The implementation here should be complete. I didn't merge this yet because I wasn't sure if the Redux state + respective React components are managed in a reliable and backward-compatible way, taking into account all the possible contexts the app can be initialized and run in.

If someone with more knowledge than me in this field could give it a look, that would be fantastic!

If #2367 is implemented, the approach taken here might need to change. The assumption in this PR was that things happen after printing. If they happen after purchase instead, maybe an additional REST endpoint will not be necessary, etc.

harriswong · 2022-11-15T01:52:46Z

@waclawjacek Bump! I stumbled upon this when I am looking at cleaning up the Calypso stuff. I think we can take a look at this again.

waclawjacek added 7 commits January 12, 2021 22:03

Add Settings Store support for post-print notifications.

5c8d9af

To store the post-print notification settings, a new WC_Connect_Options option "post_print_notification_settings" is introduced with a setter and getter in WC_Connect_Service_Settings_Store.

Add Post_Print_Notification_Settings REST controller.

105f6d7

Add a REST controller to allow manipulating the post-print notification settings via the REST API.

Pass post-print notification settings to shipping labels view.

856d413

Output the post-print notification settings currently stored in the database so the front-end scripts can pick them up.

Add changelog entry for post-print notifications.

0602ab2

Fix timezone issue in shipping rate test.

eea60c0

The test failed when ran in a timezone UTC+N where N > 0. Added code checking what the expected result in the current timezone should be.

Fix invalid field name "value" instead of "enabled".

428d66c

Instead of "value", "enabled" is used to indicate whether a notification is enabled by the back-end.

harriswong reviewed Jan 15, 2021

View reviewed changes

harriswong reviewed Jan 16, 2021

View reviewed changes

harriswong approved these changes Jan 16, 2021

View reviewed changes

waclawjacek mentioned this pull request Mar 11, 2021

Mark order as complete doesn't work in Firefox if the label is not printed #2367

Open

brezocordero changed the base branch from develop to trunk April 21, 2021 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remember post-print notification checkbox state #2300

Remember post-print notification checkbox state #2300

waclawjacek commented Jan 13, 2021

waclawjacek commented Jan 13, 2021

harriswong commented Jan 15, 2021

harriswong Jan 15, 2021

harriswong Jan 15, 2021

waclawjacek Jan 18, 2021

harriswong Jan 18, 2021 •

edited

Loading

harriswong Jan 15, 2021 •

edited

Loading

harriswong Jan 15, 2021 •

edited

Loading

harriswong Jan 16, 2021

waclawjacek Jan 18, 2021

harriswong Jan 16, 2021

harriswong left a comment

c-shultz commented Jan 16, 2021

c-shultz commented Mar 19, 2021

waclawjacek commented Mar 19, 2021

harriswong commented Nov 15, 2022

Remember post-print notification checkbox state #2300

Are you sure you want to change the base?

Remember post-print notification checkbox state #2300

Conversation

waclawjacek commented Jan 13, 2021

Description

Important changes

Relying on functions rather than state when sending

Naming

Default state removal

Related issue(s)

Steps to reproduce & screenshots/GIFs

Checklist

waclawjacek commented Jan 13, 2021

harriswong commented Jan 15, 2021

harriswong Jan 15, 2021

Choose a reason for hiding this comment

harriswong Jan 15, 2021

Choose a reason for hiding this comment

waclawjacek Jan 18, 2021

Choose a reason for hiding this comment

harriswong Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

harriswong Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

harriswong Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

harriswong Jan 16, 2021

Choose a reason for hiding this comment

waclawjacek Jan 18, 2021

Choose a reason for hiding this comment

harriswong Jan 16, 2021

Choose a reason for hiding this comment

harriswong left a comment

Choose a reason for hiding this comment

c-shultz commented Jan 16, 2021

c-shultz commented Mar 19, 2021

waclawjacek commented Mar 19, 2021

harriswong commented Nov 15, 2022

harriswong Jan 18, 2021 •

edited

Loading

harriswong Jan 15, 2021 •

edited

Loading

harriswong Jan 15, 2021 •

edited

Loading