Skip to content

Pinterest Block: Check for valid URL before embedding#15055

Merged
jeherve merged 1 commit intomasterfrom
fix/pintrest-error-message
Mar 27, 2020
Merged

Pinterest Block: Check for valid URL before embedding#15055
jeherve merged 1 commit intomasterfrom
fix/pintrest-error-message

Conversation

@apeatling
Copy link
Copy Markdown
Contributor

@apeatling apeatling commented Mar 19, 2020

Fixes: #14157

Changes proposed in this Pull Request:

Check more than just pin.it URLs to make sure they resolve a valid URL before embedding.

Testing instructions:

  • Add a pinterest block
  • Try to embed the URL https://www.pinterest.co.uk/invalidurltest/
  • Make sure that an error is returned to say that the URL could not be embedded
  • Check a valud URL like https://www.pinterest.co.uk/apeatling/ and make sure the embed works (I don't have any pins).

Proposed changelog entry for your changes:

  • Fixed issue where invalid Pinterest URLs would be incorrectly embedded.

@apeatling apeatling added Bug When a feature is broken and / or not performing as intended [Status] Needs Review This PR is ready for review. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Mar 19, 2020
@apeatling apeatling requested a review from a team March 19, 2020 23:58
@apeatling apeatling self-assigned this Mar 19, 2020
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 19, 2020

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

@apeatling apeatling requested a review from a team March 19, 2020 23:58
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Mar 20, 2020

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 fbfe60e

@jeherve jeherve added this to the 8.4 milestone Mar 20, 2020
jeherve
jeherve approved these changes Mar 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests well for me! 👍

@jeherve jeherve added [Block] Pinterest [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 20, 2020
@Copons
Copy link
Copy Markdown
Contributor

Copons commented Mar 20, 2020

This works fine, but what about using the withNotices HOC for the error message?

See #14884

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 20, 2020
@apeatling
Copy link
Copy Markdown
Contributor Author

apeatling commented Mar 20, 2020 •
edited
Loading

@Copons I replicated how it looked in the Eventbrite block, I wasn't aware of the withNotices HOC so I'll update it for that.

@Copons
Copy link
Copy Markdown
Contributor

Copons commented Mar 20, 2020 •
edited
Loading

@apeatling Ah, you're right!
Coincidentally, Pinterest and Eventbrite were the two blocks left still waiting for the update: #14896

We might as well ship this as is, and update with withNotices in a follow up, as originally planned. 🙂

@apeatling apeatling added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 26, 2020
@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 27, 2020
@jeherve jeherve merged commit f8d92dc into master Mar 27, 2020
@jeherve jeherve deleted the fix/pintrest-error-message branch March 27, 2020 19:12
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 27, 2020
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
Changelog:add #15055
1b1bf11
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
@Copons
Copy link
Copy Markdown
Contributor

Copons commented Mar 31, 2020

@apeatling heads up: I'm taking care of the notices in #15218.

I'm replacing all standalone resolveRedirect with the shared testEmbedUrl utility.
When I did it for Calendly, because of the overlap, I've also converted its notices into withNotices.
Now that I'm doing the same for Eventbrite and Pinterest, it's just easier (for me, that is 😅) to replicate the same changes I've done on Calendly.

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Apr 16, 2020

r205942-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

@apeatling apeatling

Labels

[Block] Pinterest Bug When a feature is broken and / or not performing as intended [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files

Projects

None yet

Milestone

8.4

Development

Successfully merging this pull request may close these issues.

Pinterest: return error when a Pinterest URL cannot be turned into an embed

6 participants

@apeatling @matticbot @jetpackbot @Copons @jeherve @kraftbj