Map Block: Use the A8C's Mapbox access token if the site doesn't provide one #14307

Copons · 2020-01-08T15:44:06Z

Fix https://github.com/Automattic/Dotcom-roadmap/issues/74

As this is a proof of concept, I haven't included the real A8C Mapbox access token yet.
For testing purpose, the MAPBOX_A8C_ACCESS_TOKEN constant can be locally updated with the real A8C token (a12s know where to find it), or with a personal one.

Changes proposed in this Pull Request:

Use the Automattic's Mapbox access token if the site doesn't provide one.
Hide the Mapbox Access Token field when using Automattic's.
Add some spacing in the Map block placeholder text.

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

Enhancement of an existing feature.

Internal reference: pb5gDS-ev-p2

Testing instructions:

Prerequisites

Have a Simple, Atomic, and Jetpack test sites ready.
Make sure none of them has a Mapbox token saved. In that case, before applying this PR, please remove it in the Mapbox Access Token field in the Map block sidebar.

Jetpack

Open the editor and insert a Map block.
Make sure the Map block loads in the placeholder state, and asks for a token.
Provide a token, and make sure the map loads correctly.
Make sure it's possible to change and remove the token in the Mapbox Access Token field in the block sidebar.
Save the post and check the front end.
Make sure the Map block is rendered correctly.
Update the token in the Mapbox Access Token field in the block sidebar with a random incorrect string.
Wait a bit for the API call to complete and then make sure the block reverted to the placeholder state, with an "incorrect token" error notice.

Simple

Apply D37418-code and sandbox the API.
Open the editor and insert a Map block.
Make sure the Map block loads without asking for an access token.
Make sure there is no Mapbox Access Token field in the block sidebar.
Save the post and check the front end.
Make sure the Map block is rendered correctly.

Atomic

Install and activate the Jetpack Beta plugin.
In wp-admin, navigate to Jetpack -> Jetpack Beta; search for the update/mapbox-access-token feature branch, and activate it.
Repeat the same testing steps for a Simple site (skip the first step).

Proposed changelog entry for your changes:

Map Block: Use the Automattic's Mapbox access token on WordPress.com sites.

matticbot · 2020-01-08T15:44:13Z

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 D37418-code before merging this PR. Thank you!

jetpackbot · 2020-01-08T15:46:12Z

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: February 11, 2020.
Scheduled code freeze: February 4, 2020

Generated by 🚫 dangerJS against 9c25745

creativecoder · 2020-01-08T17:11:26Z

@Copons It's possible we might want to rotate this key at times. What do you think of providing the key through the wpcom API in some way so that we can rotate it when needed without a Jetpack release?

matticbot · 2020-01-08T18:20:22Z

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

Copons · 2020-01-08T18:23:44Z

@creativecoder good point. I've created D37434-code and updated this PR accordingly.
It needs more polish though.

Copons · 2020-01-09T15:46:33Z

I'm putting this in review, even though I'm not super happy about how it provides the access token to the editor.
I'd rather fetch it directly in JS than doing so in PHP and sending the token to JS via wp_localize_script, but it always fails because of the CORS.
At the same time, I don't find such improvement a blocker for this change.

matticbot · 2020-01-09T16:16:32Z

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

matticbot · 2020-01-09T17:28:28Z

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

jeherve

I'd rather fetch it directly in JS than doing so in PHP and sending the token to JS via wp_localize_script, but it always fails because of the CORS.

Do you think it may be possible to change things at a higher level, and query WordPress.com for an API key here?

jetpack/_inc/lib/core-api/wpcom-endpoints/service-api-keys.php

Line 111 in 75d9618

public static function get_service_api_key( $request ) {

In doing so you would be able to reuse the existing elements used to query /wpcom/v2/service-api-keys/mapbox.

Maybe that would mean adding an extra element to the response returned by get_service_api_key (in addition to service_api_key) so that we know that the key returned comes from Automattic and the whole token settings sidebar interface should not be displayed?

extensions/blocks/map/map.php

Copons · 2020-01-14T11:27:36Z

Thanks for the review @jeherve!

Do you think it may be possible to change things at a higher level, and query WordPress.com for an API key here?

jetpack/_inc/lib/core-api/wpcom-endpoints/service-api-keys.php

Line 111 in 75d9618

public static function get_service_api_key( $request ) {

In doing so you would be able to reuse the existing elements used to query /wpcom/v2/service-api-keys/mapbox.

Maybe that would mean adding an extra element to the response returned by get_service_api_key (in addition to service_api_key) so that we know that the key returned comes from Automattic and the whole token settings sidebar interface should not be displayed?

That was my initial approach, but I found a few blockers and went for the WPCOM endpoint instead.
I will try again now with the WPCOM endpoint backing the whole thing! 👍

matticbot · 2020-01-14T16:46:34Z

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

_inc/lib/core-api/wpcom-endpoints/service-api-keys.php

extensions/blocks/map/map.php

matticbot · 2020-01-14T18:16:48Z

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

matticbot · 2020-01-14T18:20:47Z

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

matticbot · 2020-01-15T16:30:47Z

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

jeherve

I am merging this for now, this tests well for me.

Quick note that we'll probably need to revisit how we fetch the API key here:
https://github.com/Automattic/jetpack/blob/update/mapbox-access-token/_inc/lib/core-api/wpcom-endpoints/service-api-keys.php#L315

While this works well for public sites, it wont't work for private sites on WordPress.com as we need to be authenticated to make requests to those sites.

We'll consequently probably need to go through wpcom_json_api_request_as_blog there.

jeherve · 2020-01-23T13:48:36Z

r202027-wpcom

creativecoder · 2020-01-24T22:57:09Z

Quick note that we'll probably need to revisit how we fetch the API key here:
https://github.com/Automattic/jetpack/blob/update/mapbox-access-token/_inc/lib/core-api/wpcom-endpoints/service-api-keys.php#L315

While this works well for public sites, it won't work for private sites on WordPress.com as we need to be authenticated to make requests to those sites.

Because of new sites on WordPress.com being "private by default", we'll need the api key request to be authenticated. Using the map block as part of scaffolding content for new sites (which are private) is a primary reason for this update to the map block.

Tracking in #14475

* [not verified] Remove empty readme section * Initial changelog for 8.2 * Changelog: add #14220 * Changelog: add #14252 * Changelog: add #14291 * Changelog: add #14309 * Changelog: add #14304 * Changelog: add general connection log. * Changelog: add #14275 * Changelog: add #14313 * Changelog: add #14213 * Changelog: add #14357 * Add sync testing instructions * Add 8.1.1 changelog back See eeaafab and 61757eb * Changelog: add #14371 * Changelog: add #14386 * Changelog: add #14471 * Changelog: add #14325 * Changelog: add #14194 * Changelog: add #14340 * Changelog: add #14418 * Changelog: add #14417 * Changelog: add #14075 * Changelog: add #14467 * Changelog: add #14307 * Changelog: add #14326

Copons added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Map labels Jan 8, 2020

Copons requested a review from a team January 8, 2020 15:44

Copons self-assigned this Jan 8, 2020

matticbot added the Touches WP.com Files label Jan 8, 2020

creativecoder self-requested a review January 8, 2020 17:07

Copons added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jan 9, 2020

jeherve added this to the 8.2 milestone Jan 9, 2020

jeherve requested changes Jan 10, 2020

View reviewed changes

extensions/blocks/map/map.php Outdated Show resolved Hide resolved

extensions/blocks/map/map.php Outdated Show resolved Hide resolved

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] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 10, 2020

Copons added [Status] In Progress 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 Jan 14, 2020

jeherve requested changes Jan 14, 2020

View reviewed changes

_inc/lib/core-api/wpcom-endpoints/service-api-keys.php Outdated Show resolved Hide resolved

extensions/blocks/map/map.php Outdated Show resolved Hide resolved

Copons added 10 commits January 23, 2020 12:49

Use a site specific endpoint

f59e994

Move the access token request in the service API keys endpoint

6c204f3

Only allow Simple and Atomic sites to use the A8C token

26dac09

Cache the Automattic token for a month

4774fc0

Default the API source to "site"

cb35cce

Avoid hitting the API if the site is not connected to WPCOM

d19f5be

Update the API keys endpoint to work on WPCOM

9c9bd4d

Try to simplify get_service_api_key_mapbox()

97d4ea1

Fix incorrect cached token check

30cb6f0

Cleanup the service-api-keys file

9c25745

jeherve force-pushed the update/mapbox-access-token branch from 9336c14 to 9c25745 Compare January 23, 2020 11:50

jeherve approved these changes Jan 23, 2020

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! 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 Jan 23, 2020

jeherve merged commit 56165fe into master Jan 23, 2020

jeherve deleted the update/mapbox-access-token branch January 23, 2020 13:48

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 23, 2020

jeherve added the [Status] Needs Testing We need to add this change to the testing call for this month's release label Jan 23, 2020

Copons mentioned this pull request Jan 23, 2020

Map Block: Fix missing Mapbox access token on WPCOM #14440

Closed

jeherve mentioned this pull request Jan 23, 2020

Mapbox: query for an API key differently depending on platform #14442

Merged

This was referenced Jan 24, 2020

Map Block: WordPress.com hosted sites can override Mapbox API key #14473

Closed

Map Block: API key for WordPress.com hosted sites #14475

Closed

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Jan 27, 2020

jeherve added a commit that referenced this pull request Jan 27, 2020

Changelog: add #14307

983bc5a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map Block: Use the A8C's Mapbox access token if the site doesn't provide one #14307

Map Block: Use the A8C's Mapbox access token if the site doesn't provide one #14307

Copons commented Jan 8, 2020 •

edited

Loading

matticbot commented Jan 8, 2020

jetpackbot commented Jan 8, 2020 •

edited

Loading

creativecoder commented Jan 8, 2020

matticbot commented Jan 8, 2020

Copons commented Jan 8, 2020

Copons commented Jan 9, 2020

matticbot commented Jan 9, 2020

matticbot commented Jan 9, 2020

jeherve left a comment

Copons commented Jan 14, 2020

matticbot commented Jan 14, 2020

matticbot commented Jan 14, 2020

matticbot commented Jan 14, 2020

matticbot commented Jan 15, 2020

jeherve left a comment

jeherve commented Jan 23, 2020

creativecoder commented Jan 24, 2020

Map Block: Use the A8C's Mapbox access token if the site doesn't provide one #14307

Map Block: Use the A8C's Mapbox access token if the site doesn't provide one #14307

Conversation

Copons commented Jan 8, 2020 • edited Loading

Changes proposed in this Pull Request:

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

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Jan 8, 2020

jetpackbot commented Jan 8, 2020 • edited Loading

creativecoder commented Jan 8, 2020

matticbot commented Jan 8, 2020

Copons commented Jan 8, 2020

Copons commented Jan 9, 2020

matticbot commented Jan 9, 2020

matticbot commented Jan 9, 2020

jeherve left a comment

Choose a reason for hiding this comment

Copons commented Jan 14, 2020

matticbot commented Jan 14, 2020

matticbot commented Jan 14, 2020

matticbot commented Jan 14, 2020

matticbot commented Jan 15, 2020

jeherve left a comment

Choose a reason for hiding this comment

jeherve commented Jan 23, 2020

creativecoder commented Jan 24, 2020

Copons commented Jan 8, 2020 •

edited

Loading

jetpackbot commented Jan 8, 2020 •

edited

Loading