-
Notifications
You must be signed in to change notification settings - Fork 21
Remember post-print notification checkbox state #2300
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
5c8d9af
105f6d7
856d413
a6e804a
0602ab2
eea60c0
428d66c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,6 +160,67 @@ public function set_preferred_paper_size( $size ) { | |
| return WC_Connect_Options::update_option( 'paper_size', $size ); | ||
| } | ||
|
|
||
| /** | ||
| * Gets post-label-printing notification settings. | ||
| * | ||
| * @return array Array of boolean values, indexed by setting names. | ||
| */ | ||
| public function get_post_print_notification_settings() { | ||
| $defaults = array( | ||
| 'mark_order_complete_and_notify_customer' => false, | ||
| 'notify_customer_with_shipment_details' => false, | ||
| ); | ||
|
|
||
| $settings = WC_Connect_Options::get_option( 'post_print_notification_settings', $defaults ); | ||
|
|
||
| return array_merge( $defaults, $settings ); | ||
| } | ||
|
|
||
| /** | ||
| * Updates post-label-printing notification settings. | ||
| * | ||
| * @param string $name Name of the setting to enable/disable. | ||
| * @param bool $enabled Whether the setting should be enabled. | ||
| * | ||
| * @return true|WP_Error WP_Error if an error occurred, `true` otherwise. | ||
| */ | ||
| public function set_post_print_notification_setting( $name, $enabled ) { | ||
| $allowed_names = array( | ||
| 'mark_order_complete_and_notify_customer', | ||
| 'notify_customer_with_shipment_details', | ||
| ); | ||
|
|
||
| if ( ! in_array( $name, $allowed_names, true ) ) { | ||
| return new WP_Error( | ||
| 'invalid_notification_setting_name', | ||
| __( 'Invalid notification setting name supplied.', 'woocommerce-services' ) | ||
| ); | ||
| } | ||
|
|
||
| $old_settings = WC_Connect_Options::get_option( 'post_print_notification_settings' ); | ||
| $settings = $old_settings; | ||
| $settings[ $name ] = (bool) $enabled; | ||
|
|
||
| /* | ||
| * 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice comment! |
||
| */ | ||
| if ( $settings === $old_settings ) { | ||
| return true; | ||
| } | ||
|
|
||
| $result = WC_Connect_Options::update_option( 'post_print_notification_settings', $settings ); | ||
|
|
||
| if ( ! $result ) { | ||
| return new WP_Error( | ||
| 'save_failed', | ||
| __( 'Updating the option failed.', 'woocommerce-services' ) | ||
| ); | ||
| } | ||
|
|
||
| return $result; | ||
| } | ||
|
|
||
| /** | ||
| * Attempts to recover faulty json string fields that might contain strings with unescaped quotes | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| <?php | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see we use Were you also not able to find any existing end point to update settings?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! I was considering adding the notification-settings-saving logic to an existing endpoint but:
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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The choice of the new end point makes a lot of sense now. Thank you! |
||
|
|
||
| if ( ! defined( 'ABSPATH' ) ) { | ||
| exit(); | ||
| } | ||
|
|
||
| if ( class_exists( 'WC_REST_Connect_Post_Print_Notification_Settings_Controller' ) ) { | ||
| return; | ||
| } | ||
|
|
||
| class WC_REST_Connect_Post_Print_Notification_Settings_Controller extends WC_REST_Connect_Base_Controller { | ||
| protected $rest_base = 'connect/post-print-notification-settings'; | ||
|
|
||
| public function post( $request ) { | ||
| $result = $this->settings_store->set_post_print_notification_setting( $request['name'], $request['enabled'] ); | ||
|
|
||
| if ( is_wp_error( $result ) ) { | ||
| $error = new WP_Error( | ||
| 'save_failed', | ||
| sprintf( | ||
| __( 'Unable to update notification setting. %s', 'woocommerce-services' ), | ||
| $result->get_error_message() | ||
| ), | ||
| array( 'status' => 400 ) | ||
| ); | ||
|
|
||
| $this->logger->log( $error, __CLASS__ ); | ||
|
|
||
| return $error; | ||
| } | ||
|
|
||
| return new WP_REST_Response( array( 'success' => true ), 200 ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! There are a few different ways in our existing code to return a response/error. I am glad you picked |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import { expect } from 'chai'; | |
| import { configure, mount } from 'enzyme'; | ||
| import Adapter from 'enzyme-adapter-react-16' | ||
| import { CheckboxControl, RadioControl } from '@wordpress/components'; | ||
| import { moment } from 'i18n-calypso'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
|
|
@@ -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]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the timezone issue? :D
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! The issue was that the time provided in |
||
|
|
||
| expect( shippingRateWrapper ).to.contain( <div className="rates-step__shipping-rate-delivery-date">{ expectedDate }</div> ); // eslint-disable-line | ||
| } ); | ||
|
|
||
| } ); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't trust the user input and I like how you add the check here instead of the controller's level. 💯