Add WordCamps and meetup events to the News Dashboard widget

Description

The attached patch updates the existing WordPress News dashboard widget to also include upcoming meetup events and WordCamps near the current user’s location.

If the site has multiple users, each one will be shown the events that are close to their individual location. The dashboard widget will try to automatically detect their location, but they’ll also be able to enter any city they like.

@iandunn As clever as the $netmask = 4 === strlen( inet_pton( $address ) ) ? '255.255.255.0' : 'ffff:ffff:ffff:ffff:0000:0000:0000:0000'; line is, I almost feel it's being too clever by half, at the expense of readability. Can we break that out into easier digested chunks?

Let's not introduce wp_get_nearby_events(), you could check the global $wp_version instead.

There aren\'t any events scheduled near %s at the moment. Would you like to <a href="https://make.wordpress.org/community/handbook/meetup-organizer/welcome/">organize one</a>?: aren\'t should be are&#8217;t, the URL should be a separate string and the placeholder needs a translator comment.

There should be no need to have .rtl classes in dashboard.css. The build task generates a dashboard-rtl.css file automatically.

A few cases of esc_html_e() should be replaced with _e().

HTML around placeholders like <strong>%s</strong> should be moved into the argument.

DocBlocks should use third-person singular verbs

get_community_events_script_data() should get the wp_ prefix.

aren\'t should be are&#8217;t

I need some more feedback on these:

It seems like inet_ntop() isn't available on PHP < 5.3 on Windows platforms.

maybe_anonymize_ip_address() returns early if either of them don't exist. Do you see any cases where that won't avoid the problem?

Let's not introduce wp_get_nearby_events(), you could check the global $wp_version instead.

Do you mean update the plugin to check $wp_version during its bootstrap? Wouldn't that would require everyone whos running the plugin to update to the latest version before they upgrade to 4.8? Since we can't guarantee that they will, it seems like that would introduce the chance for fatal errors.

Although, the upgrade routine also deactivates the plugin, just to be safe, so maybe that's enough? What do you think?

the URL should be a separate string and the placeholder needs a translator comment.

Do you mean like this?

<?php printf(
/* translators: %1$s is the city the user searched for; %2$s is a wordpress.org URL with locale-specific instructions for organizing a meetup. */
__( 'There aren&#8217;t any events scheduled near %1$s at the moment. Would you like to <a href="%2$s">organize one</a>?' ),
'{{data.location.description}}',
/* translators: Replace the URL if a locale-specific one exists */
__( 'https://make.wordpress.org/community/handbook/meetup-organizer/welcome/' )
); ?>

I agree with wp_get_nearby_events(). This shouldn't be added to core. The upgrade routine should be enough, but I also recommend updating the plugin as well. Most of the ~40 users will update it pretty quickly I think.

Translator comments are usually written a bit different. I'd write something like this

The URL itself doesn't need a translator comment as there's no placeholder in it.

Few other observations:

There's a typo in the short description of the maybe_anonymize_ip_address() method (missing 'to').

The @return lines shouldn't be aligned with the params.

true and false in the docblocks shouldn't be in backticks.

In dashboard.js, I think $container.on( 'click', '.community-events-toggle-location', app.toggleLocationForm ); and $container.on( 'click', '.community-events-cancel', app.toggleLocationForm ); can be combined by using a .community-events-toggle-location, .community-events-cancel' selector.

The short descriptions in the docblocks in the JS need full stops at the end.

rest_get_community_events_permissions_check() is missing an @since annotation

To be more consistent with our REST routes, I feel like the community events rest route should be in its own controller class, just like the others. Right now it's a pretty odd exception.

… that way it would also be better testable. Right now there aren't any tests at all for that REST API endpoint.

I uploaded 40702-ajax.diff as an alternative approach using the original admin-AJAX endpoint instead of the REST API.

I want to see the REST API used in the near future, but it seems like we need more time to settle on good long-term decisions. So, I think using admin-AJAX in the short term will give us that time, but without blocking the feature from being merged to 4.8.

I'll be making more tweaks to the -ajax series of patches today, to incorporate feedback people have made that isn't related to the REST API. I wanted to get the first -ajax patch out there now, though, so it can start getting feedback.

Removed backticks around formal data types. I left them around for properties though, since otherwise it's not obvious that it's a data point. e.g.: "@return false|array false on failure; an array containing location and events items on success." I'm happy to remove those if people think that's best, though.

As discussed earlier today, while the primary priority was to get the feature ready for merge (which it apparently is now), it's desired that this feature should use the REST API. It would be a great example of how the API can be leveraged for admin functionality, and I honestly don't think we should use admin-ajax.php for a new feature like that.

The original patches included a quickly-created proxy endpoint. During today's REST API chat this approach was discussed, and we came to the conclusion that instead a proper WP_REST_Controller class should be implemented and in addition, the endpoint should be split into two separate endpoints, as it was previously returning two types of resources (first a location resource, then a list of event resources). We decided to go for community-events/events and community-events/location and furthermore agreed that the events endpoint should be embeddable inside the location endpoint, so that the dashboard widget can still leverage only one API request.

40702.6.diff​ introduces these two REST controllers and replaces the admin-ajax.php approach with a proper REST API approach, which is a lot more flexible and can be an example of how we can leverage the REST API for admin components in the future.

I'm positive that we can do the necessary work to improve this feature and merge the endpoint by Friday in time for Beta 1. I will work on unit tests for the controller tomorrow.

Looking at 40702.6.diff, it seems really complicated. All that is needed here is a simple proxy to access the API at wp.org. Replacing ~20 lines of admin-ajax with ~500 lines of REST endpoints is.... too much? :)

There seem to be some "new" things. For example an admin can see another user/admin data. Don't see a good reason for this:

Then, if the other user's data is not set, it will be set wrong according to the current user's IP.

Another weakness perhaps is the need for get_item_schema(). Setting this means that the API at wp.org has to be more or less "frozen". What if we need to fix something there? I mean, this is a proxy, nothing more. Perhaps two REST controllers are too much? Starting to think this is not a very good example on how to use the REST API :)

I agree that viewing another user's data doesn't make sense with the IP detection, so that part could be removed/adjusted.

Regarding the item schema, I don't think it's a problem. If the .org API ever changed, we would need to modify the core code anyway, so we could as well adjust the schema.

And about the amount of code, I get your concern to some extent, but I don't think that solution is worse just because it is heavier. It defines proper structures for location and event resources (in comparison to the AJAX callback just returning "anything we don't know about"). Also most of the code isn't actual code, but schema and param definition.

To summarize my thoughts, just because something is simple functionality doesn't mean we shouldn't leverage a proper REST API structure for it. In addition it would finally be an admin component using the REST API. :)

The wrong template is shown when searching for a location that wasn't found (e.g., narnia). templateParams.unknownCity should be set, so that the correct template gets rendered. Instead, it's showing the normal template, but the city name is missing: Attend an upcoming event near .

The same thing happens when no location is saved, and api.w.org returns no_location_found. In this case, though, no error should be shown at all, since it was an automatic request.

Lines 307-320 in the patch remove the code that codes both of those situations.

I share some of Ozz's concerns, particularly about exposing a user's location to admins. There are some situations where the admin will already have access to that data, but many others where they wouldn't. There will probably be at least a small number of situations where the user would not feel comfortable sharing their location. Since there doesn't seem to be any specific use case for exposing that data, it seems like it should be removed.

cc @adamsilverstein, in case you have any thoughts on leveraging wp.api here.

Minor nitpicky stuff

dashboard.js - Grouping requestParams._embed = 1; with the other requestParams statements seems like it would improve readability

40702.7.diff​ disables the routes for any user. It is now only possible to get the location of the current user via community-events/location/me and the events close to the current user via community-events/events/me. Furthermore the location and timezone parameters can now be passed to the events endpoint as well to get the same time of precision for their results (this was missing in the previous patch).

40702.8.diff​ is another update that addresses the points @iandunn mentioned in his review, with the most significant change being that the unknownCity property is now set in order to show the correct notification about the location not being detected.

I think 40702.8.diff fixes the two errors in comment:30. I noticed a new one, though, which is fixed in 40702.9.diff.

When an actual HTTP error occurs (like when testing with the mu-plugin mentioned in comment:30), 40702.8.diff was treating it like a no_location_available event, rather than an actual error. So, the We couldn't locate... template was being shown, instead of the error template.

It seemed like an odd situation to use HTTP status codes when returning WP_Errors. I can see how it makes sense for the 401 error in get_current_item_permissions_check(), but a 503 in get_current_item() when the actual HTTP transaction succeeded seems misleading, and had the unintended side effect above.

The original code treated HTTP-layer errors differently than application-layer "errors" on purpose.

I need to write some QUnit tests soon, but in the meantime I these are the situations that need to be tested, and their expected results:

User-initiated requests

HTTP error - show .community-events-error-occurred

no location found - render tmpl-community-events-could-not-locate

location found with events - render tmpl-community-events-event-list

location found with no events - render tmpl-community-events-no-upcoming-events

App-initiated requests

HTTP error - no error is shown, show enter_closest_city message

no location found result - no error is shown, show enter_closest_city message

location found with events - same as user-initiated, but the form is closed

location found with no events - same as user-initiated, but the form is closed

@azaozz, 40702-localize.diff moves the wp_localize_script() calls from wp-admin/index.php and wp-admin/network/index.php into script-loader.php.

Now that it's done, though, I think it feels a bit clunkier than the way things were before. There are ~10 other screens that currently call wp_localize_script() from their files instead of in script-loader.php, so it seems like that might be an ok approach.

It might just be about the same either way, though, just with a different set of trade-offs. Either way, I don't think it's a big concern.

40702-duplicate-translations.diff​ duplicates some of the translations to be both within the HTML templates as well as within the JS data array.
Calling wp_get_community_events_script_data() just to fetch a static string doesn't seem worthwhile.

40702-localize.diff​ seems clunky, and I think might fatal in bulk mode (ie. load-scripts.php?). The proper way to do that would be following the example of wp_localize_jquery_ui_datepicker() or wp_just_in_time_script_localization() (although the latter lacks an is-enqueued check, which it gets away with since it's not loading data).

main change is swapping out the ajax call for a wp.api.collections.CommunityEventsMine fetch call

built off 40702.8.diff​ so doesn't include the latest changes (I can rebase)

switches the route community-events/locations/me to /mine. /me is currently a special route keyword for the wp-api client that returns a model instead of a collection. maybe the client should only do that for the default core routes?

includes some minor fixes in the wp-api client in part to accommodate the multi-part routes used here and also the lack of a matching model (the client assumes each collection will have a matching model endpoint)

40702.ajax-fix.diff​ fixes a bug where submitted locations don't persist across page loads because the transient key doesn't get generated correctly since location information is missing in the user setting.

40702.ajax-fix.diff​ fixes a bug where submitted locations don't persist across page loads because the transient key doesn't get generated correctly since location information is missing in the user setting.

@iandunn Does that look sane?

:facepalm:

I can't believe I missed that in testing. I guess sleep is kinda important after all...

40702.ajax-fix.diff​ looks and tests good to me.

I played around with an alternative approach in 40702.ajax-fix.2.diff, but I think your patch is better. I like that mine avoids sending the unnecessary data in the AJAX response, but I don't like that it duplicates the location trimming statement in both ajax-actions.php and includes/dashboard.php. I could make that a DRY function, but that seems a bit overkill.

40702-duplicate-translations.diff​ duplicates some of the translations to be both within the HTML templates as well as within the JS data array.
Calling wp_get_community_events_script_data() just to fetch a static string doesn't seem worthwhile.

I personally like it because it makes things DRY, which makes it more maintainable and less prone to errors in the future. That's not a huge deal, though, especially in this case. I don't have any objection to 40702-duplicate-translations.diff being committed.

40702-localize.diff​ seems clunky, and I think might fatal in bulk mode (ie. load-scripts.php?). The proper way to do that would be following the example of wp_localize_jquery_ui_datepicker() or wp_just_in_time_script_localization() (although the latter lacks an is-enqueued check, which it gets away with since it's not loading data).

I tested with SCRIPT_DEBUG set to true and false, and didn't notice any errors. I agree that it's kind of clunky though.

Namespaces in general should follow the pattern of vendor/v1, where vendor is typically your plugin or theme slug, and v1 represents the first version of the API.

Also, at the very least this is going to break some stuff on WP.com if we try to enable this endpoint there. It wouldn't surprise me if there is other code out there that relies on a namespace having only a single slash.

It looks like the commit numbers that upgrade.php uses didn't get updated in r40607, so I uploaded 40702-upgrade.diff​ to do that.

That patch also documents the reason why the request is proxied through WP, instead of being made directly to api.wordpress.org by the user's browser. That functionality is important, but without documentation it's not immediately obvious why.

40702-geoip.diff​ changes the Events class to always pass the user's IP address when making requests to api.w.org.

There's been a lot of feedback after beta 1 about the location results not being accurate enough, and this is one thing that can help with that. When the API has multiple potential locations -- e.g., Paris, France, Paris, Texas, USA, etc -- it can geolocate the IP and use that as a clue to figure out which is the most likely the one the user intended.

I don't think that this has a significant impact on privacy, since the API would already have the IP from the initial request (if it were to store it, which it doesn't). If anyone is concerned, though, ​it would be very easy to write a plugin to redact the IP from the request. If anyone has interest in that, please do it and let me know. If nobody else does, I'll try to make time for it in a week or two when higher priorities have been sorted.

40702-geoip.2.diff​ is a work in progress which tries to provide a better UX when the location is determined via IP geolocation.

The API can't return the location for those requests (see ​#meta2823), so as of ​[meta5499], it just returns an ip param to let the client know that the location was successfully determined based on the IP address. Since the client doesn't know the name of the location, though, it has to show the user generic messages now.

I think the UX is good, but the code still feels a bit clunky. I don't see a simpler way at the moment, though. I'd love to get some more eyes on it, and testing.

I'm also working on a 40702-geoip.3.diff, which handles the additional case where a location is determined based on a user search, then saved, then the transient expires. In those cases, the client sends the coordinates in order to fetch new events, but the API doesn't return the location description anymore, so Core has to re-use the description it already has. That patch isn't in a usable state yet, though.

The browser's request for events is proxied with this method, rather than having the browser make the request directly to api.wordpress.org, because it allows results to be cached server-side and shared with other users and sites in the network. This makes process more efficient, since increasing the number of visits that get cached data means users don't have to wait as often; if the user's browser made the request directly, it would also need to make a second request to WP in order to pass the data for caching. Having WP make the request also introduces the opportunity to anonymize the IP before sending it to w.org, which mitigates privacy concerns that some users have.

Ok, after @obenland's commits I think the only things left on this ticket are:

Get more eyes on 40702-geoip.2.diff​. After sleeping on it, I think this is still in good shape, but I'm going to take a more detailed look again today. I think there might be a few tweaks based on how 40702-geoip.3.diff turns out.

Finish 40702-geoip.3.diff, which will improve the refreshing of expired event transients for locations that were matched via user query (as opposed to IP geolocation).

40702-request-args.diff​ changes the get_request_url method to get_request_args, which returns an array instead of a string. This allows us to separate the Events API endpoint URL from the request args in wp_remote_get(). This will by DRYer in the case that the request args are ever used in another context, and also allows the args to be filtered on the http_request_args hook.

40702-geoip.3.diff​ finishes the work that was started in 40702-geoip.2.diff​, to improve the handling of locations determined by geolocating the IP address. It also introduces improvments for handling locations determined by manual searches. As a whole, this patch addresses a lot of the feedback from beta1 about location accuracy, and also is required to support some changes on the API side from ​#meta2823 (specifically, ​[meta5497] and ​[meta5499].

Breakdown of changes

For locations determined by IP address, the IP itself is treated as the location, rather than the result of the geolocation. This is necessary because the API no longer results a location for IP queries. It also is nice for certain use cases, like when the user is traveling. As of r40781, if the user travels to a new location, their location and events are not updated. With this patch, they will always be shown events that match their current location. As a result of not having a geolocated description, the user is just shown a generic message. That's probably better for UX anyway, since it won't distract users that the location doesn't match their city exactly, even though the events are close enough to be relevant.

For locations determined by a manual search, the patch handles the new way that the API responds to requests. It no longer returns the location description, so the client just uses the one it already has saved instead. Because the API doesn't return a complete location each time, the original saved location is preserved.

attend_event_near_generic moved from a template to a .l10n string, see comment:61. no-upcoming-events ended up needing to remain a template, though, since it needs to apply {{ data.location.description }}. I combined the two inside a single template, though, because that seemed a bit cleaner than how it was before.

Renamed get_client_ip to get_unsafe_client_ip, because it now needs to be public and static. That will have the unintended side-effect of encouraging usage by plugins, so I think it's best to be explicit that the method isn't designed to verify the authenticity of the returned IP.

I've done a lot of testing, and I think this is pretty solid. I think it needs a few others testing it just to confirm that, though. Can anyone try it out and give feedback? You'll need to set up the mu-plugin from comment:53.

40702-geoip.4.diff​ combines 40702-geoip.3.diff​ with 40702-request-args.diff​ from @coreymckrill. The former can take advantage of the fact that the latter makes $request_args available, and using that variable makes a couple places in the code more intuitive and readable, as opposed to getting the same values from less obvious sources.

The upgrade routine doesn't need to deactivate the old feature plugin, because the plugin has a noop routine. Deactivating it in the routine was just an extra safety net, but should not be needed in reality.