-
Notifications
You must be signed in to change notification settings - Fork 799
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.
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
OpenTable: Accept more embed options #14639
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: February 17, 2020. |
extensions/blocks/opentable/utils.js
Outdated
|
||
let domain = searchParams.get( 'domain' ); | ||
if ( ! domain || domain.length === 0 ) { | ||
domain = 'com'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might be able to get this from the URL, but I don't think it matters.
scruffian, Your synced wpcom patch D38705-code has been updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That cleans things up quite nicely.
I think we'd benefit from a test for getAttributesFromEmbedCode
here, to avoid any regressions in the future, just like we've done for Calendly:
https://github.com/Automattic/jetpack/blob/2c3dbd93a0cc735df5593a147532cf5de02627ba/extensions/blocks/calendly/test/utils.js
91ff024
to
5ca4e7a
Compare
scruffian, Your synced wpcom patch D38705-code has been updated. |
1 similar comment
scruffian, Your synced wpcom patch D38705-code has been updated. |
23e741d
to
e7a5f01
Compare
I added unit tests for this function. In the process I realised that it wasn't actually working correctly - we weren't picking up the correct true/false values from the widget code. I have fixed this now, but to do so I had to change the The only issue I see with this is that users who have previously changed these settings to something other than the default will lose them if they edit a post with a block in. I'm not sure its worth adding code to account for this. What do you think? |
In the past we have. You can check the VideoPress for an example: That's a lot for a small change, though. Could we make the type change before we get to the attributes instead? |
scruffian, Your synced wpcom patch D38705-code has been updated. |
1 similar comment
scruffian, Your synced wpcom patch D38705-code has been updated. |
315ba12
to
8fa3eaf
Compare
Based on some useful feedback from @pablinos I did this a different way; we are keeping the boolean attributes and just casting them in the validation helper. |
I also added some tests for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a couple of comments about the regex and error handling. Testing this threw up another bug that I've tried to address in #14678
extensions/blocks/opentable/utils.js
Outdated
@@ -0,0 +1,80 @@ | |||
export const embedRegex = /< *script[^>]*src *= *["']?([^"']*)/i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it likely that we'll get <. script
as input? If we do want to strip whitespace, should we use \s
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems unlikely, but this is the REGEX we already had in there...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, so it is! Seems odd, but if it's working :)
restaurantId = searchParams.getAll( 'restref' ); | ||
} | ||
if ( ! restaurantId || restaurantId.length === 0 ) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this works, but should we add a negative test to make sure that we don't get any attributes with some other invalid URL/embed code? I think this is the point we would get to if someone was to paste in something silly like
<script
src="https://someotherservice.com/embed.js?params=4"
></script>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we should probably, but I think that would be good for a different PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice. I wish we could support regular URLs as well, such as https://www.opentable.com/r/bistro-deryne-budapest?corrid=dae839dd-c3f2-42c1-8796-91668437dbf7 , but that's for another time I guess :)
Before we merge I only have one nitpick to save us some confusion later on.
This also adds a check that the host of the URL includes OpenTable
Yeah, I don't think we've got a way of getting the restaurant ID from those URLs currently. although it is available in the HTML source of that page, so we could maybe do a request on the server and grab this vie an API endpoint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be good to go now.
r203007-wpcom |
* 8.3 release: changelog * Changelog: add #14516 * Changelog: add #14574 * Bring in changes from 8.2.1 and 8.2.2 * Update stable version * Bring in 8.2.3 changes * Changelog: add #14714 * Changelog: add #14639 * Changelog: add #14678 * Changelog: add #14673 * Changelog: add #14687 * Changelog: add #14704 * Changelog: add #14702 * Changelog: add #14541 * Changelog: add #14657 * Changelog: add #14622 * Changelog: add #14582 * Changelog: add #14638 * Changelog: add #14633 * Changelog: add #14571 * Changelog: add #14592 * Changelog: add #14539 * Changelog: add #14514 * Changelog: add #14643 * Changelog: add #14494 * Changelog: add #13739 * Changelog: add #14707 * Changelog: add #14736 * Changelog: add #14706 * Changelog: add #14730 * Changelog: add #14685 * Changelog: add #14727 * Changelog: add #14711 * Changelog: add #14742 * Changelog: add #14746 * Changelog: add #14725 * Changelog: add #13999 * Changelog: add #14740 * Changelog: add #14759 * Changelog: add #14703 * Changelog: add #14753 * Changelog: add #14754 * Changelog: add #14645 * Cahngelog: add #14599
Changes proposed in this Pull Request:
Testing instructions:
https://www.opentable.com/widget/reservation/preview?rid=1&lang=en-US
Proposed changelog entry for your changes: