Customizer: keep Simple Payments Widget Customizer in sync between instances

[rodrigoi]

Changes proposed in this Pull Request:

Keeps all the instances of the Simple Payments Widget Customizer in sync by observing the widget-synced and widget-updated events, querying the server for the updated list of products.

Before	After

Testing instructions:

Starting with a Jetpack Professional site with at least one product loaded (two for dramatic effect):

• Add a Simple Payments Widget to the site sidebar or footer.
• Add a new Product by clicking Add New and filling the form.
• Add a second Simple Payments Widget to the site sidebar or footer.
• Expand the list of products on the second widget.

The list of products should show the newly added product on the correct position.

• Add a new Product by clicking Add New and filling the form on the second widget.
• Expand the first widget.
• Expand the list of products on the first widget.

The list of products should show the newly created product on the second widget on the product list on the correct position.

• On the first widget, edit the currently selected product name.
• Expand the second widget.
• Expand the list of products on the second widget.

The edited product should have the new name.

[dmsnell]

will this end up jarring the dropdown if someone has it open? could we do a replacement only on the items that need inserting or removal? are we instead ready to accept this rare glitch?

[Copons]

So, this is a valid concern, but with the upcoming code freeze this is not the right time to do something too clever.

If we edit a SP product and then immediately (before the widget-sync completes) reopen the dropdown, the worse that can happen is that the dropdown will disappear and reappear, which imho, is not like the highest priority bug ever. 😄

[rodrigoi]

I agree that a better reconciliation process between the server and the client would be better, but I'd rather add that later as janitorial task.

[dmsnell]

all good. I mainly want to make sure we're aware of the behaviors we're introducing and not be surprised by them later 👍

[dmsnell]

what does this line do? could we add a comment at the end tersely explaining what selectedProduct is here for and what its value will be?

[Copons]

The logic there is:

• temporarily store the <select> selected product
• empty the <select>
• fill the <select> with the updated products
• re-select the product that was selected before the switcheroo

I'm fine with over-documenting stuff, but in this particular case I'd say it's just a matter of remembering the good old days of yore when we were more used to look at jQuery code. 😄

[dmsnell]

I guess I'm getting lost even with the jQuery calls

the end() in the sequence is awkward since it's not at the end
the last .val( selectedProduct ) is also a bit puzzling to me since it gives me the impression that it's resetting the value of the select to the value polled in line 87R.

even quick explanation of what's being set to what in line 96R would go a long way.

[rodrigoi]

jQuery, known for it's awkwardness defines end() as:

End the most recent filtering operation in the current chain and return the set of matched elements to its previous state.

In this particular case, it ends the find filter operation by returning $select, where we can append the updated list of option elements and restore the selected value.

[dmsnell]

I won't say anymore after this 🤝 but I think that the fact that we have to refer to documentation and answer this here is indication that someone in the future is going to come in and have no idea what's going on when a single sentence or fragment could lead them in on it…

…
	.end() // end current operation and return original element
	…
	.val( selectedProduct ); // and append into the original `<select>`

I clearly don't understand what I'm talking about, but I question whether the next person who comes around to this will either

[rodrigoi]

I've added a comment for the end call on 719071c9d9e5e62b0e6aa222ca02bd65be750518.

[gwwar]

Let's make this into multiple statements, if folks are finding this difficult to read. eg

$select.find( 'option' ).remove();
$select.append( addOptions );
$select.val( selectedProduct );

[rodrigoi]

updated on 8e8f21b55a67a7e0d97b8dcd51a2e1c1c0ac7fca.

[Copons]

This LGTM, although there's one thing that bugs me a little bit: we've got 3 ajax functions that start with the exact same checks:

• check the nonce
• check if the user can use the customizer
• check if the user has enough capabilities to create and publish a SP (although, we don't check if the user can delete them!).

So, I'm wondering if we could move (in a follow up is good as well) these checks into their own separate methods. Something like:

private function user_can_do_simple_payments( $action = 'get' ) {
  if ( ! check_ajax_referer( 'customize-jetpack-simple-payments', 'customize-jetpack-simple-payments-nonce', false ) ) {
    return( array( 'bad_nonce', 400 ) );
  }

  if ( ! current_user_can( 'customize' ) ) {
    return( array( 'customize_not_allowed', 403 ) );
  }

  $post_type_object = get_post_type_object( Jetpack_Simple_Payments::$post_type_product );
  if (
    ( 'save' === $action && (
      ! current_user_can( $post_type_object->cap->create_posts ) ||
      ! current_user_can( $post_type_object->cap->publish_posts )
    ) ) ||
    ( 'delete' === $action && ! current_user_can( $post_type_object->cap->delete_posts ) )
  ) {
    return( array( 'insufficient_post_permissions', 403 ) );
  }

  return true;
}

// ...

public function ajax_save_payment_button() {
  $user_can = $this->user_can_do_simple_payments( 'save' );
  if ( $user_can !== true ) {
    wp_send_json_error( $user_can[0], $user_can[1] );
  }
  // ...
}

(this also needs rebasing!)

[dmsnell]

This LGTM, although there's one thing that bugs me a little bit: we've got 3 ajax functions that start with the exact same checks…So, I'm wondering if we could move (in a follow up is good as well) these checks into their own separate methods

ahhhh, looks like the start of discovering a hidden API semantic, looks like we are learning what kind of design needs we might have for the Simple Payments API 🤔 😉