Calendly Block: Show an error when the embed url is not found

[Copons]

Fixes #14805

Changes proposed in this Pull Request:

• Show a loading spinner when loading the embed.
• Show an error when the embed url is not found.

When inserting an URL, the block tests it through the resolve-redirect endpoint.
On success, it sets the block's attributes and loads the embed.
On fail, it empties the URL attributes and shows an error notice.

To make this work with the automatic block transformation when pasting a Calendly URL, the resolve-redirect check runs on block mount, and whenever the URL attribute changes.
This, unless the block style is link, as this implies that the URL passed the test already (and showing a "loading spinner" would look awkward.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

• If you're an Automattician, include a shortlink to the p2 discussion with Jetpack Product here.

Testing instructions:

• Insert a Calendly block and add a correct Calendly URL.
• Make sure the block has a loading spinner and then show the embed as expected.
• Insert a second Calendly block with a correct URL, and set it as link style.
• Save and reload the editor.
• Make sure both blocks show up fine; the embedded one should show the loading spinner, while the other should not.
• Insert another Calendly block, this time using an incorrect Calendly URL (e.g. https://calendly.com/totally-incorrect-url).
• After the loading spinner, the block should revert to the placeholder with an error notice.
• Save and reload. The incorrect block should show up in placeholder state.
• Try again by pasting a correct and an incorrect URL in the editor. Make sure the behaviour of both transforms is the same as described above.

Proposed changelog entry for your changes:

• Calendly block: Show an error when the embed URL is not found.

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello Copons! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D39450-code before merging this PR. Thank you!

[jetpackbot]

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 26fafa2

[jeherve]

This works well for me. 🚢

[apeatling]

Followed all steps and they worked as described. 👍

[pablinos]

As you can change the URL and get it wrong, do you think it would be good to cover checking the URL for the link style too? I realise we probably don't want to show the spinner, but it would be good to let them it's invalid.

[scruffian]

I agree that it would be good to do this check for both styles of embed...

[Copons]

@pablinos The useEffect here is triggered when the block mounts and the url changes.
It happens in these cases:

1. When the visual editor loads from scratch.
2. When the user switches from code to visual editor.
3. When the user pastes a Calendly URL that gets transformed into a Calendly block.

Cases 1 and 2: we don't really need to check the URL, as it's implied that it has been already checked (more about it later).
If you try to change the URL in the code editor, the block should break anyway.

Case 3 is where this comes into play.
Since pasting a link always results in an embedded block, we can safely avoid testing for link style blocks.
On the other hand, for the same reason we are basically forced to test on embedded blocks regardless.

---

Now, the resolve-redirect check runs always when you manually change the embed code (see the parseEmbedCode method).
So if you have a link style block and change its URL, it will pass through resolve-redirect.

---

Now, by testing this again I could see two things that look off to me:

1. I'm kinda wondering why the useEffect is not triggered when the URL changes after a parseEmbedCode.
   I mean, it works as it should, but I'm curious why. 😄
   I'm probably failing to notice something that I already solved a few days ago and totally forgot, though.

2. Unfortunately, the useEffect is triggered by the block preview in the style selector as well...
   I'm not super keen in having dozens of useless calls to /resolve-redirect/https://calendly.com/wordpresscom/jetpack-block-example, so I'll need to discard that case as well.

[matticbot]

Copons, Your synced wpcom patch D39450-code has been updated.

[scruffian]

This looks good. My only concern is that it really slows down the preview in the sidebar. I see this for quite a long time:

[Copons]

I'm kinda wondering why the useEffect is not triggered when the URL changes after a parseEmbedCode.

Ok, for some reasons I was wrong on this, or my dev tools skills aren't good enough.
useEffect was indeed being triggered when the url was changed by parseEmbedCode resulting in one additional, unnecessary, apiFetch call.

This happened because the useEffect was conditionally running whenever props.attributes.url changed, as I was under the impression that pasting an URL would first create an empty block, and then fill it with the URL, thus requiring the check to run on url change.

This is not the case: as it turns out, pasting an URL directly creates the block with the url prefilled.
So useEffect effectively needs to only run on component mount (empty array [] as second argument), instead of on url change ([ url ] as second argument).

What does this mean practically? Let's see case by case.

1. Normal block creation
   Block is inserted empty. useEffect exits immediately. When the user inserts the URL, parseEmbedCode tests it.

2. Block transform on pasted URL
   User pastes a Calendly URL. Block is created with the url attribute prefilled. useEffect tests it.

3. URL is changed from sidebar
   parseEmbedCode tests the URL. Since the block is already mounted, useEffect doesn't even run.

4. Editor loads with block in embed mode
   If block has no URL, nothing happens. Otherwise, useEffect tests it.
   This is an unintended (but unavoidable as far as I know) side effect of the case 2.

5. Editor loads with block in link mode
   Since case 2 (pasting a Calendly URL) can only create a block in embed mode, we don't need to test the URL on old blocks in link mode. useEffect exits immediately.

6. Block example renders in the inserter or the sidebar
   useEffect exists immediately when the URL to test is the Calendly example URL.

[Copons]

My only concern is that it really slows down the preview in the sidebar.

@scruffian have you tried with the latest changes? Specifically 08f921a should fix exactly that, as before we were also unintentionally resolving the example URL, and so slowing down the render of the preview embed.

[Copons]

I wonder if we could do away with useEffect altogether by calling parseEmbedCode when we paste the embed code...

I've tried this a bit, as it seems like the most sensible solution, but I think the async gets in the way.
If I try to run the resolve test with await in the transform before creating the block, nothing happens when the response come back.
Though, I might be doing something dumb in there, so feel free to give it a try if you have some spare cycles.

[jeherve]

Now that we rely on wpcom/v2/resolve-redirect here, in the Pinterest block, and in the Eventbrite block, would it make sense / be possible to extract this into a shared function maybe? What do you think?

[Copons]

Good thinking!
I'll update this PR by adding an utility function, and then will take care of the other blocks separately to avoid bloating this.

[matticbot]

Copons, Your synced wpcom patch D39450-code has been updated.

[matticbot]

Copons, Your synced wpcom patch D39450-code has been updated.

[Copons]

In 26fafa2 I've created a new testEmbedUrl utility that should greatly simplify how we test embeddable URLs with resolve-redirect, while leaving enough freedom to each implementation.

I think the function documentation is clear enough, and there's also an example, but for the sake of clarity:

• testEmbedUrl accepts an optional setIsResolvingUrl argument, based upon the useState pattern, which will automatically update the block state with the resolve-redirect state (basically sets isResolvingUrl = true before calling the endpoint, and isResolvingUrl = false before resolving the promise).
  This should turn out to be useful to have a loading state in the block.
  Also, this doesn't work with the old this.setState(). Blocks that still use it will need to keep track of the request state manually.

• testEmbedUrl returns a promise, so each block will need to handle the success in .then() and the fail in .catch().
  This is mostly because each block do different stuff when testing the URL. In some cases (like in Calendly's useEffect) they don't even need to handle a success case.
  I'm not a huge fan of thenables, but I think having an async/await approach here would be much less readable, or verbose (useEffect callbacks, for example, can't be async, so one would need to create a new async function that awaits for the test, and call that function inside the useEffect... not really a great dev experience!)

tl;dr this is how it works:

const Block = () => {
  const [ url, setUrl ] = useState( '' );
  const [ isResolvingUrl, setIsResolvingUrl ] = useState( false );

  const setUrlAttribute = () => {
    testEmbedUrl( url, setIsResolvingUrl )
      .then( () => setAttribute( { url } ) )
      .catch( () => setAttribute( { url: undefined } ) );
  };

  if ( isResolvingUrl ) {
    return <Spinner />;
  }
  return (
    <form onSubmit={ setUrlAttribute }>
      <TextControl value={ url } onChange={ setUrl } />
    </form>
  );
}

[scruffian]

LGTM

[apeatling]

Tested and confirmed working. 👍

[jeherve]

This looks good to me. It should be good to merge!

Since it introduces a new file, you'll need to rebuild the matching wpcom diff manually I'm afraid.

[Copons]

r204401-wpcom