fix(ras-acc): add product summary card to auth flow

[chickenn00dle]

All Submissions:

• Have you followed the Newspack Contributing guideline?
• Does your code follow the WordPress' coding standards and VIP Go coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Closes https://app.asana.com/0/1206943664367857/1207328906416559/f

This PR adds the product price summary card to the auth flow:

How to test the changes in this Pull Request:

Before testing, be sure to checkout the epic/ras-acc branch in the main Newspack Plugin.

• Add a donation block to any page or post.
• Add some checkout buttons the same page or post. Be sure to include:
  • A simple product
  • A simple subscription
  • A variable product
  • A variable subscription
• As a logged out reader, go to the page or post where the above have been added
• Click the donate button and confirm the auth modal opens with a product summary card at the top of the modal with the product name and price (as pictured above).
• Repeat the above steps with other donation options, as well as with each of the checkout buttons

Other information:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes, as applicable?
• Have you successfully ran tests with your changes locally?

[dkoo]

@chickenn00dle this works, but fetching the markup via REST API causes a noticeable delay (~1-3 seconds on my local environment) when clicking the Checkout Button or submitting the Donate block.

What if we included the product details we need for this component in the hidden form fields we include in Checkout Button and Donate block forms and build the DOM elements on the fly with JS? It looks like we already include some product info here so adding some more shouldn't be a big deal if it helps us avoid a roundtrip REST API request and the delay it would cause. WDYT?

[chickenn00dle]

I am injecting the input as a string here. An alternative would be to move all of the render logic from save.js to this render callback instead. I didn't go this route since I didn't want to complicate this PR too much. But if you think it would be better, I can get a commit up for this.

[dkoo]

I'm fine with doing it this way. I would rather every block render its content via the render callbacks instead of a save JS method, but that's a personal preference and beyond the scope of this PR. I also agree that attributes isn't the right place for this data since they're not editable block attributes.

Another non-blocking idea, if it's not too complex: we could unify the approach to appending all of these various hidden inputs. I think the others are being injected via JS right now, right?

In this render callback we could build a JSON string with all of the hidden input names and values, and do a single injection as a data- attribute added to the wrapper div or the form element inside (or even just wrap the whole $content string in another container). Then use a createHiddenInput helper in JS to create all the hidden inputs based on this data attribute.

But if that sounds too complex I'm okay with the split approach for now.

[chickenn00dle]

Thanks for the suggestion! I started working on this and realized I wasn't handling the tiered or untiered versions of the donation block 😞

I'll go ahead and implement this strategy while I work on getting those added.

[chickenn00dle]

Alright @dkoo, this should be good for another round of review now.

I should note that this got quite complicated as the various modes of the donation block behave inconsistently, so I'd appreciate a close look at these newer changes 🙇

[chickenn00dle]

Thanks @dkoo!

What if we included the product details we need for this component in the hidden form fields we include in Checkout Button and Donate block forms and build the DOM elements on the fly with JS? It looks like we already include some product info here so adding some more shouldn't be a big deal if it helps us avoid a roundtrip REST API request and the delay it would cause. WDYT?

My initial approach to this was this strategy but I ran into some complications with outputting the hidden inputs for price/frequency for dynamic products such as donations or variable products in general. I also didn't want to add any new attributes to the checkout button block since this would require publishers to resave their blocks. And I didn't want to re-implement any of the backend price string logic on the frontend. So I went with fetching via REST.

That said, I did take another stab at this, and managed to get this working without that fetch and moving the price summary content into a hidden input. Tbh, it feels a bit "hacky" to me but let me know what you think or if you have any suggestions for improvement!

[dkoo]

Thanks for the change in approach! It feels much snappier now. 💨

[dkoo]

I'm fine with doing it this way. I would rather every block render its content via the render callbacks instead of a save JS method, but that's a personal preference and beyond the scope of this PR. I also agree that attributes isn't the right place for this data since they're not editable block attributes.

Another non-blocking idea, if it's not too complex: we could unify the approach to appending all of these various hidden inputs. I think the others are being injected via JS right now, right?

In this render callback we could build a JSON string with all of the hidden input names and values, and do a single injection as a data- attribute added to the wrapper div or the form element inside (or even just wrap the whole $content string in another container). Then use a createHiddenInput helper in JS to create all the hidden inputs based on this data attribute.

But if that sounds too complex I'm okay with the split approach for now.

[dkoo]

Hey @chickenn00dle, thanks for catching that we missed the Tiered and Untiered layouts before. I've tested all and they're all working nicely.

Thanks also for unifying the approach for hidden inputs—given the added complexity from the various Donate block variations, I think this was the right call in the end. However, I do see that because we've changed the render output for the blocks, existing blocks are throwing validation errors in the editor:

Block validation: Block validation failed for `newspack-blocks/checkout-button` ( 
Object { name: "newspack-blocks/checkout-button", icon: {…}, keywords: (4) […], attributes: {…}, providesContext: {}, usesContext: [], selectors: {}, supports: {…}, styles: (2) […], variations: [], … }
 ).

Content generated by `save` function:

<div class="wp-block-newspack-blocks-checkout-button has-custom-width wp-block-button__width-100"><form><button class="wp-block-button__link has-primary-background-color has-background" type="submit">Variable subscription</button><input type="hidden" name="newspack_checkout" value="1"/><input type="hidden" name="after_success_button_label" value="Continue"/></form></div>

Content retrieved from post body:

<div class="wp-block-newspack-blocks-checkout-button has-custom-width wp-block-button__width-100"><form><button class="wp-block-button__link has-primary-background-color has-background" type="submit">Variable subscription</button><input type="hidden" name="product_id" value="433"/><input type="hidden" name="newspack_checkout" value="1"/><input type="hidden" name="is_variable" value="1"/><input type="hidden" name="after_success_button_label" value="Continue"/></form></div>

It doesn't prevent these blocks from working, but I wonder if it's worth refactoring the block to a dynamic block after all? Maybe in a separate PR? If not, then we'd need to implement a block deprecation/migration for this block content. Converting to a dynamic block seems like it would be less onerous to me, but what do you think?

[dkoo]

Marking "changes requested" just to make it clear that we need to handle the block validation one way or another (either migrating to a dynamic block, or implementing deprecation/migration). Once we decide on an approach this can be merged to the epic if we decide to tackle that in a separate PR.

[chickenn00dle]

Thanks for your patience with reviewing on this one @dkoo!

but I wonder if it's worth refactoring the block to a dynamic block after all? Maybe in a separate PR? If not, then we'd need to implement a block deprecation/migration for this block content. Converting to a dynamic block seems like it would be less onerous to me, but what do you think?

Good catch on the validation errors. I went ahead and just refactored in this PR. I had some trouble replicating the functionality of getBorderClassesAndStyles, getSpacingClassesAndStyles, and getColorClassesAndStyles on the backend though, so ended up having to apply these styles and classes manually. Let me know if I've messed anything up there.

[dkoo]

@chickenn00dle thanks for the more extensive refactoring to convert the Checkout Button block to a dynamic render. I tested this on a site that had a Checkout Button block already, and although I did see an "invalid block" error in the editor after installing this branch, the block still worked on the front-end both before and after block recovery (and the recovery succeeded). I'll approve this, but let's keep an eye on things and make sure to add a note about this to the testing plan.

[chickenn00dle]

Thanks @dkoo!

but let's keep an eye on things and make sure to add a note about this to the testing plan.

I've added a note for this here: 1wlwA6SAL0B8leDQ5yrb-cHrQCxOBB4X4ReM6T5zdKPo-gdoc