Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pinterest Block: Check for valid URL before embedding #15055

Merged
merged 1 commit into from
Mar 27, 2020
Merged

Pinterest Block: Check for valid URL before embedding #15055

merged 1 commit into from
Mar 27, 2020

Conversation

apeatling
Copy link
Member

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 [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review To request a review from Crew. Label will be renamed soon. [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 as a code owner March 19, 2020 23:58
@apeatling apeatling self-assigned this Mar 19, 2020
@matticbot
Copy link
Contributor

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

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
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 To request a review from Crew. Label will be renamed soon. labels Mar 20, 2020
@Copons
Copy link
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 would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 20, 2020
@apeatling
Copy link
Member 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
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 To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Mar 26, 2020
@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. 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
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
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 [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
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