Automattic / crowdsignal-forms

Gutenberg blocks for Crowdsignal
GNU General Public License v2.0
13 stars 9 forks source link

Prevent blocks from being used in the Site Editor #272

Closed jgcaruso closed 5 months ago

jgcaruso commented 6 months ago

There have been a few instances recently where users have been trying to use Poll blocks in FSE and when attempting to vote on the public page, there is a JS error saying an attribute is missing. This plugin may have been created before FSE was available so this scenario was not properly handled.

All of the crowdsignal blocks only support being rendered in a Post context in order for their content to be properly saved since they only get processed on post save hooks.

This PR adds some checks to the block in the editor to try to determine if it is running in an FSE context (no post id) or in a query loop (queryId is available in context). Query loop should not be supported on FSE or in a post because the save hook would always attempt to save against the post that is being saved, not the dynamic post id from the query loop. So the result would be weird.

This PR also adds a check on the rendered page, so if "platform data" (for polls) or surveyId is missing (the data that matches the poll and options to IDs on Crowdsignal) then it won't attempt to render the poll. This is to protect against any existing polls that may have already been created. Hopefully the missing poll on the page would prompt someone to check the editor, see the message, and then not contact support because of a broken poll. This change required a small refactor to cache the data since the "platform data" function is being called twice now, as well as a small bug fix for a weird array with a space in it when data could not be found.

Finally, this PR fixes some issues with the docker container used for local dev / testing.

There is currently an issue with the NPS block prior to this PR where if you open a post for editing that already had an NPS block on it, it fails to render in the editor. I've opened a separate issue to track this https://github.com/Automattic/crowdsignal-forms/issues/273

I also bumped the minimum WordPress version to ensure that we have support for usesContext because it seems it was only added at some point in 2020 (WP 5.5?), and current minimum 5.0 version was released in 2018. But having a more modern version will allow us to use more modern features when we are ready to.

Testing

  1. Apply this PR to get the docker fixes, then run the steps to setup the dev environment. See https://github.com/Automattic/crowdsignal-forms/tree/master/docker#setup
  2. Check out master again. We want to do these next steps against existing code.
  3. On a new post, create a Poll, Vote and Applause block.
    • use the /poll, /vote and /applause blocks, not the embeds
    • also test with the /nps and /feedback blocks
  4. Save the post
  5. In FSE, add a Poll, Vote and Applause block anywhere on the homepage not in a query loop
  6. In FSE, add a Poll, Vote and Applause block to a query loop's "Post Template"
  7. Save the page and view the public pages.
  8. The polls should render, but voting should fail with console errors on the homepage
  9. Checkout this PR branch again
  10. run make client
  11. refresh the public page, none of the Poll, Vote or Applause blocks should render
  12. View the public post page, voting should work as expected
  13. View FSE, you should see error messages in replacement of the blocks
  14. View the Post editor for your test post. The block should render as expected (with the exception of the nps block as noted in the description above)
jgcaruso commented 6 months ago

@gikaragia

Conceptually, it seems like a weird limitation to not allow users to create polls to templates.

I agree, having a single poll that exists on every page as part of the template could make sense (someone was trying to do just that in a sidebar which is why I had to fix this).

I think this plugin was created before FSE existed, so it just never considered how to support it. Different hooks would probably need to be created to make that work, and api endpoints changed to support a new set of data for linking the blocks to crowdsignal data. Its probably a big effort as you mentioned.

Couldn't we store these in attributes instead?

I vaguely remember an issue with storing too much identifying data in the attributes. It may have had something to do with copy/pasting blocks, or just in general keeping things in sync and deciding which end of the integration (WP site or Crowdsignal API) gets to generate the IDs for polls and questions.