Add WordCamps and meetup events to the News Dashboard widget

[iandunn]

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.

More background is available in the following posts:

• ​Nearby WordPress Events on make/Core.
• ​First Quarter Check-In on make/Core.
• ​Showing Upcoming Local Events in wp-admin on make/Community.

There are a few minor things left to clean up in the patch, but it's close enough to start reviewing and getting a final round of feedback.

Props @afercia, @andreamiddleton, @azaozz, @camikaos, @coreymckrill, @chanthaboune, @courtneypk, @dd32, @iandunn, @iseulde, @mapk, @mayukojpn, @melchoyce, @nao, @obenland, @pento, @samuelsidler, @stephdau, @tellyworth.

[iandunn]

Add method to anonymize user IP

[iandunn]

40702.2.diff adds a way to partially anonymize user IPs before they're sent to api.wordpress.org, in order to add some privacy. props @georgestephanis.

[georgestephanis]

@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?

[iandunn]

Improve netmask readability, avoid sending IP when unnecessary

[iandunn]

Good point, 40702.3.diff breaks it into a full if... else....

I also realized that the w.org API only uses the IP when there isn't a specific location being searched for, so I adjusted WP_Community_Events::get_request_url() to only send it when necessary.

[iandunn]

upgrade routine, css tweaks, jshint corrections, rest sanitize instead of validate

[iandunn]

40702.4.diff adds an upgrade routine; some CSS tweaks; corrects a JSHint violation; and switches from validating REST input to sanitizing it.

Note that the revision number in the upgrade routine should probably be updated once we know the actual number.

[ocean90]

Some feedback for 40702.3.diff​:

• There should be no need to have .rtl classes in dashboard.css. The build task generates a dashboard-rtl.css file automatically.
• It seems like inet_ntop() isn't available on PHP < 5.3 on Windows platforms.
• 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, see ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#language
• get_community_events_script_data() should get the wp_ prefix.
• 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’t, the URL should be a separate string and the placeholder needs a translator comment.

[iandunn]

Minor tweaks for comment:8, part 1

[iandunn]

Thanks for the feedback @ocean90 :)

These ones are fixed in 40702.5.diff:

• 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/' )
); ?>

[rmccue]

(Just noting I want to do a better review on the API part of this before merge, although I don't have time today. :) )

[swissspidy]

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

/* translators: 1: the city the user searched for, 2: meetup organization documentation URL */

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.

See ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/ for help with the inline docs.

---

FYI I also mentioned some other things on the make/core post that aren't strictly related to this patch.

[iandunn]

Switch from REST API to admin AJAX until long-term API decisions are made

[iandunn]

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.

[iandunn]

Removed deprecated noop, unused AJAX response properties; i18n improvements; minor cleanup

[iandunn]

Changes in 40702-ajax.2.diff:

• Removed the deprecated wp_get_nearby_events() noop function
• Improved i18n string and translator comment in #tmpl-community-events-no-upcoming-events
• Removed unused properties from the AJAX response
• Fixed typo in maybe_anonymize_ip_address() docblock
• Fixed @return alignment
• 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.
• Combined the toggleLocationForm selectors
• Added full stops to JS and PHP docblocks.
• removed @access public, but left @access protected

[obenland]

I don't think we've ever had core handle plugin deactivations when feature projects were merged. That was usually handled on the plugin side. Are we moving away from that?

[iandunn]

I included it there because I saw it there for MP6 in upgrade_380(). I'm happy to remove it if that'd be better, though.

[obenland]

Let's discuss it during dev-chat?

[swissspidy]

I think we also had something similar in place for the REST API. However, the community events is super small compared to that, with only 40+ active installs.

[obenland]

Replying to swissspidy:

the community events is super small compared to that, with only 40+ active installs.

Good point

[azaozz]

only 40+ active installs.

Thinking we can leave it in for now (we don't want any fatal errors), remove before RC?

[azaozz]

In 40607:

Dashboard: Update the existing WordPress News dashboard widget to also include upcoming meetup events and WordCamps near the current user’s location.

Props @afercia, @andreamiddleton, @azaozz, @camikaos, @coreymckrill, @chanthaboune, @courtneypk, @dd32, @iandunn, @iseulde, @mapk, @mayukojpn, @melchoyce, @nao, @obenland, @pento, @samuelsidler, @stephdau, @tellyworth.
See #40702.

[flixos90]

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.

[azaozz]

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 a lot of "new" things. For example an admin can see another user/admin data. Don't see a good reason for this:

 if ( $request['id'] !== get_current_user_id() ) { 
    $user = get_userdata( $request['id'] );
    ....

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 :)

[flixos90]

Thanks for the quick response @azaozz!

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. :)

[iandunn]

40702.6.diff tests pretty good for me, but I did find two bugs:

1. 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 .
2. 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.

Side note: I've been ​using a `pre_http_request` filter to easily test error conditions. That might come in handy to others. That wouldn't help with either of the two bugs above, though.

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
• get_item_for_user_id() - $data = array() is not needed, it gets overridden immediately
• prepare_item_for_response() - I think The description, country, etc vars would be more readable if the ? and : were aligned
• prepare_item_for_response() returns an array, but the docblock says it'll be a WP_REST_Response
• get_item_permissions_check() is 95% duplicated in both controllers. Would it be better to make it that DRY? Or at least leave a comment in both that changes to either might need to be synced?

[flixos90]

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).

[flixos90]

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.

[adamsilverstein]

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

Looking to see if we can make it work.

[iandunn]

Fixes bug in 40702.8.diff

[iandunn]

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

[iandunn]

Alternate approach to adding script data

[iandunn]

@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.

[dd32]

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).

[adamsilverstein]

40702.10.diff​ Uses the wp.api JS client for the API connection handling.

• 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)

[obenland]

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?

[adamsilverstein]

40702.11.diff​ is based on .10 and switches to using the wp-api client. @iandunn here are the actual ​changes required to switch: (minus the console logging :))

[iandunn]

A failed attempt at improving on 40702.ajax-fix.diff

[iandunn]

Replying to obenland:

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.

I think your patch is simpler, which makes it more maintainable.

[iandunn]

Replying to dd32:

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.

[obenland]

In 40651:

Dashboard: Persist location for community events

Fixes a bug where cached events & location data was not accessible because the
cache key could not be regenerated without latitude and longitude information.

Discovered and fixed during #wcber contributor day.

Props soean, kubik-rubik, obenland.
See #40702.

[jnylen0]

Can we please change the namespace of this new API endpoint from wp/dashboard/v1 to wp-dashboard/v1? The extra slash seems contrary to the recommendations at ​https://developer.wordpress.org/rest-api/extending-the-rest-api/adding-custom-endpoints/#namespacing:

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.

[iandunn]

Let's move the REST API stuff to #40748 so we can close this ticket out. I'm working on some tweaks to 40702.11.diff​ and will upload that to the new ticket soon.

[Soean]

documentation and coding standards

[Soean]

The 40702-docs-cs.diff patch improves the documentation and coding standards.

[obenland]

In 40669:

Dashboard: Community events formatting improvements

Props Soean.
See #40702.

[iandunn]

Updates revisions numbers, documents reason for proxying request

[iandunn]

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.

[iandunn]

Dev notes aren't needed for this, since the changes are so minimal for plugins.

See ​https://wordpress.slack.com/archives/C02RQBWTW/p1495053471501193

[iandunn]

@obenland or @azaozz, could you review comment:46 for beta2?

Once that's committed, I think the only open issue is whether we want to tweak how the JS data gets localized. See comment:32, comment:33, and comment:38.

Once those two are decided, we can close this ticket.

[obenland]

Replying to iandunn:

@obenland or @azaozz, could you review comment:46 for beta2?

Yeah, I'll get that in today, can look at l10n tomorrow

[obenland]

@michelleweber Do you think I could ask you for some wording polish in 40702-upgrade.diff​? Do we do that at all for code docs?

[iandunn]

The issue with upgrade revision numbers is fixed in r40773. it looks like Trac just isn't smart enough to match strings like See [40607], #40702.

I have a few more tweaks to make after the discussion in today's Core meeting, though. They'll be minor and I should have a patch ready today or tomorrow.

[iandunn]

Always pass the IP address, so the API can improve search results

[iandunn]

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.

Related #40794

[iandunn]

Work in progress to provide better UX for geolocated IP locations

[iandunn]

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.

In order to test it, you'll need to add this to an mu-plugin:

// test 40702-geoip.2.diff
function change_user_agent( $agent ) {
	return str_replace( '4.8-beta1', '4.8-beta2', $agent );
}
add_filter( 'http_headers_useragent', 'change_user_agent' );

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.

[obenland]

In 40774:

Dashboard: Always pass the IP when getting events

Allows the API to determine event locations more accurately.

Props iandunn.
See #40702.

[michelleweber]

@obenland, sure -- will take a look shortly.

[michelleweber]

Replying to michelleweber:

@obenland, sure -- will take a look shortly.

Here's a slightly cleaned up version:

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.

[obenland]

In 40776:

Dashboard: Properly localize data for events

Moves localization to script-loader and removes dependency for two strings.

Props dd32, iandunn.
See #40702.

[obenland]

In 40777:

Dashboard: Document request proxy for events.

Documents the reason why the request is proxied through WP, instead of being
made directly to api.wordpress.org.

Props iandunn, michelleweber.
See #40702.

[iandunn]

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

1. 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.
2. 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).

[obenland]

@iandunn Can $this->maybe_anonymize_ip_address( $this->get_unsafe_client_ip() ) be merged into one function and called get_client_ip()? They're always used in conjunction and feels overly verbose

Instead of using templates for translation strings, can we use the dashboard l10n object?

[iandunn]

Replying to obenland:

@iandunn Can $this->maybe_anonymize_ip_address( $this->get_unsafe_client_ip() ) be merged into one function and called get_client_ip()? They're always used in conjunction and feels overly verbose

I don't see any problems with that. Do you have time to do that?

Instead of using templates for translation strings, can we use the dashboard l10n object?

Doh, good point. I'll make that change in the next iteration of the patch.

[obenland]

In 40781:

Dashboard: Combine methods to retreive IP

They're only used in one place, no reason to be so verbose about it.

See #40702.

[iandunn]

Expands on geoip.2 for better IP location handling; adds better handling for searched locations

[coreymckrill]

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.

[iandunn]

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.

[iandunn]

Incorporates 40702-request-args.diff and fixes 2 bugs

[iandunn]

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.

See comment:63 and comment:64 for the details on those patches.

This patch also 2 bugs in 40702-geoip.3.diff​:

1. An edge case where a WP_Error object was treated as an array.
2. An edge case where an IP value of false would be saved to the database, and could never be overwritten with a valid IP.

[iandunn]

Replace the upgrade routine with a temporary hack, to improve UX

[iandunn]

40702-geoip.5.diff​ replaces the upgrade routine with a temporary hack to clear the transient. This achieves the same goal, but ​is better UX because it doesn't trigger an upgrade screen. Props @pento for the idea. #40815 will handle the removal of it for 4.8.1.

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.

[azaozz]

In 40790:

Dashboard: Improve the handling of locations determined by geolocating the IP address and by entering a city name. Fix couple of edge cases, and some names.

Props iandunn coreymckrill.
Fixes #40702.

[iandunn]

Replace the upgrade routine with a temporary hack, to improve UX

[iandunn]

40702-transient-hack.diff​ is the same code that was mentioned in comment:67. It was left out of r40790 because it needs more discussion.

[iandunn]

Rename the transient instead of adding the hack, props ocean90

[iandunn]

40702-transient-rename.diff​ is an alternative approach to clearing the transient. It's a much more elegant solution than 40702-transient-hack.diff​. Props @ocean90 for the idea.

[iandunn]

Removes the old upgrade routine

[iandunn]

I forgot to remove the old upgrade routine in 40702-transient-rename.diff​, so I uploaded 40702-transient-rename.2.diff​ to fix that.

[SergeyBiryukov]

In 40802:

Dashboard: Append the current locale to dashboard RSS widget cache key in wp_dashboard_rss_control(), for consistency with the changes to wp_dashboard_cached_rss_widget() in [33183] and [33192].

See #32804, #40702.

[iandunn]

refresh; revert db_version; rename 2nd transient

[iandunn]

40702-transient-rename.3.diff​ adds these changes to the previous version:

• Refreshes after r40802
• Reverts $db_version to the value it had before r40773, so that the network upgrade notice is not triggered.
• Renames the dash transient for the secondary news feed

[SergeyBiryukov]

In 40803:

Dashboard: Change the cache key for dashboard RSS widget; remove the unnecessary database upgrade routine.

Props iandunn, ocean90.
See #40702.

[azaozz]

This is done. Please open new tickets for any bugs.