WordPress.org

Make WordPress Core

Opened 3 weeks ago

Closed 8 days ago

#40702 closed task (blessed) (fixed)

Add WordCamps and meetup events to the News Dashboard widget

Reported by: iandunn Owned by: azaozz
Milestone: 4.8 Priority: normal
Severity: normal Version: trunk
Component: Administration Keywords: has-patch
Focuses: javascript, rest-api Cc:

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.

More background is available in the following posts:

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.

Attachments (30)

40702.diff (53.9 KB) - added by iandunn 3 weeks ago.
40702.2.diff (54.9 KB) - added by iandunn 3 weeks ago.
Add method to anonymize user IP
40702.3.diff (55.3 KB) - added by iandunn 3 weeks ago.
Improve netmask readability, avoid sending IP when unnecessary
40702.4.diff (56.8 KB) - added by iandunn 3 weeks ago.
upgrade routine, css tweaks, jshint corrections, rest sanitize instead of validate
40702.5.diff (56.6 KB) - added by iandunn 3 weeks ago.
Minor tweaks for comment:8, part 1
40702-ajax.diff (56.4 KB) - added by iandunn 3 weeks ago.
Switch from REST API to admin AJAX until long-term API decisions are made
40702-ajax.2.diff (56.0 KB) - added by iandunn 3 weeks ago.
Removed deprecated noop, unused AJAX response properties; i18n improvements; minor cleanup
40702.6.diff (26.2 KB) - added by flixos90 2 weeks ago.
40702.7.diff (23.4 KB) - added by flixos90 2 weeks ago.
40702.8.diff (24.0 KB) - added by flixos90 2 weeks ago.
40702.9.diff (24.0 KB) - added by iandunn 2 weeks ago.
Fixes bug in 40702.8.diff
40702-localize.diff (2.1 KB) - added by iandunn 2 weeks ago.
Alternate approach to adding script data
40702-duplicate-translations.diff (3.0 KB) - added by dd32 2 weeks ago.
40702.10.diff (27.5 KB) - added by adamsilverstein 2 weeks ago.
40702.ajax-fix.diff (1.5 KB) - added by obenland 2 weeks ago.
40702.11.diff (26.4 KB) - added by adamsilverstein 2 weeks ago.
40702.ajax-fix.2.diff (1.7 KB) - added by iandunn 2 weeks ago.
A failed attempt at improving on 40702.ajax-fix.diff
40702-docs-cs.diff (1.7 KB) - added by Soean 13 days ago.
documentation and coding standards
40702-upgrade.diff (2.0 KB) - added by iandunn 11 days ago.
Updates revisions numbers, documents reason for proxying request
40702-geoip.diff (1.4 KB) - added by iandunn 10 days ago.
Always pass the IP address, so the API can improve search results
40702-geoip.2.diff (4.8 KB) - added by iandunn 10 days ago.
Work in progress to provide better UX for geolocated IP locations
40702.12.diff (8.3 KB) - added by obenland 10 days ago.
40702-geoip.3.diff (10.7 KB) - added by iandunn 9 days ago.
Expands on geoip.2 for better IP location handling; adds better handling for searched locations
40702-request-args.diff (2.4 KB) - added by coreymckrill 9 days ago.
40702-geoip.4.diff (12.7 KB) - added by iandunn 9 days ago.
Incorporates 40702-request-args.diff and fixes 2 bugs
40702-geoip.5.diff (14.6 KB) - added by iandunn 9 days ago.
Replace the upgrade routine with a temporary hack, to improve UX
40702-transient-hack.diff (2.1 KB) - added by iandunn 9 days ago.
Replace the upgrade routine with a temporary hack, to improve UX
40702-transient-rename.diff (556 bytes) - added by iandunn 9 days ago.
Rename the transient instead of adding the hack, props ocean90
40702-transient-rename.2.diff (1.7 KB) - added by iandunn 9 days ago.
Removes the old upgrade routine
40702-transient-rename.3.diff (2.4 KB) - added by iandunn 8 days ago.
refresh; revert db_version; rename 2nd transient

Download all attachments as: .zip

Change History (108)

@iandunn
3 weeks ago

#1 @iandunn
3 weeks ago

  • Keywords has-patch has-unit-tests added

This ticket was mentioned in Slack in #core-restapi by iandunn. View the logs.


3 weeks ago

#3 @azaozz
3 weeks ago

  • Milestone changed from Awaiting Review to 4.8

@iandunn
3 weeks ago

Add method to anonymize user IP

#4 @iandunn
3 weeks ago

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.

#5 @georgestephanis
3 weeks ago

@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
3 weeks ago

Improve netmask readability, avoid sending IP when unnecessary

#6 @iandunn
3 weeks ago

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
3 weeks ago

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

#7 @iandunn
3 weeks ago

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.

#8 @ocean90
3 weeks ago

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&#8217;t, the URL should be a separate string and the placeholder needs a translator comment.

@iandunn
3 weeks ago

Minor tweaks for comment:8, part 1

#9 @iandunn
3 weeks ago

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

#10 @rmccue
3 weeks ago

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

#11 @swissspidy
3 weeks ago

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.

This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.


3 weeks ago

This ticket was mentioned in Slack in #core-restapi by iandunn. View the logs.


3 weeks ago

@iandunn
3 weeks ago

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

#14 @iandunn
3 weeks ago

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
3 weeks ago

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

#15 @iandunn
3 weeks ago

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

#16 @obenland
2 weeks ago

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?

#17 @iandunn
2 weeks ago

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.

#18 @obenland
2 weeks ago

Let's discuss it during dev-chat?

#19 follow-up: @swissspidy
2 weeks ago

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.

#20 in reply to: ↑ 19 @obenland
2 weeks ago

Replying to swissspidy:

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

Good point

#21 @azaozz
2 weeks ago

only 40+ active installs.

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

#22 @azaozz
2 weeks ago

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.

#23 @iandunn
2 weeks ago

  • Keywords needs-dev-note added; has-patch has-unit-tests removed

@flixos90
2 weeks ago

#24 @flixos90
2 weeks ago

  • Keywords has-patch needs-unit-tests dev-feedback added

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.

#25 @azaozz
2 weeks ago

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:

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

Last edited 2 weeks ago by azaozz (previous) (diff)

#26 @flixos90
2 weeks ago

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

#27 follow-up: @iandunn
2 weeks ago

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
2 weeks ago

#28 @flixos90
2 weeks ago

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
2 weeks ago

#29 @flixos90
2 weeks ago

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.

#30 in reply to: ↑ 27 @adamsilverstein
2 weeks ago

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

Looking to see if we can make it work.

@iandunn
2 weeks ago

Fixes bug in 40702.8.diff

#31 @iandunn
2 weeks ago

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
2 weeks ago

Alternate approach to adding script data

#32 @iandunn
2 weeks ago

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

#33 follow-up: @dd32
2 weeks ago

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

#34 @adamsilverstein
2 weeks ago

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)

#35 @obenland
2 weeks ago

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?

#36 follow-up: @adamsilverstein
2 weeks ago

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
2 weeks ago

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

#37 @iandunn
2 weeks ago

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.

#38 in reply to: ↑ 33 @iandunn
2 weeks ago

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.

#39 @obenland
2 weeks ago

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.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


2 weeks ago

#41 @jnylen0
2 weeks ago

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.

#42 in reply to: ↑ 36 @iandunn
2 weeks ago

  • Keywords has-patch needs-unit-tests dev-feedback removed

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.

#43 @ocean90
2 weeks ago

  • Type changed from enhancement to task (blessed)

@Soean
13 days ago

documentation and coding standards

#44 @Soean
13 days ago

  • Keywords has-patch added

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

#45 @obenland
13 days ago

In 40669:

Dashboard: Community events formatting improvements

Props Soean.
See #40702.

@iandunn
11 days ago

Updates revisions numbers, documents reason for proxying request

#46 @iandunn
11 days ago

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.

#47 @iandunn
10 days ago

  • Keywords needs-dev-note removed

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

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

#48 follow-up: @iandunn
10 days ago

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

#49 in reply to: ↑ 48 @obenland
10 days ago

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

#50 follow-up: @obenland
10 days ago

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

#51 @iandunn
10 days ago

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
10 days ago

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

#52 @iandunn
10 days ago

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
10 days ago

Work in progress to provide better UX for geolocated IP locations

#53 @iandunn
10 days ago

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.

#54 @obenland
10 days ago

In 40774:

Dashboard: Always pass the IP when getting events

Allows the API to determine event locations more accurately.

Props iandunn.
See #40702.

@obenland
10 days ago

#55 in reply to: ↑ 50 ; follow-up: @michelleweber
10 days ago

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

#56 in reply to: ↑ 55 @michelleweber
10 days ago

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.

#57 @obenland
10 days ago

In 40776:

Dashboard: Properly localize data for events

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

Props dd32, iandunn.
See #40702.

#58 @obenland
10 days ago

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.

#59 @iandunn
10 days ago

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

#60 follow-up: @obenland
10 days ago

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

#61 in reply to: ↑ 60 @iandunn
10 days ago

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.

#62 @obenland
10 days ago

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
9 days ago

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

#63 @coreymckrill
9 days ago

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.

#64 @iandunn
9 days ago

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
9 days ago

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

#65 @iandunn
9 days ago

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.

This ticket was mentioned in Slack in #core by iandunn. View the logs.


9 days ago

@iandunn
9 days ago

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

#67 @iandunn
9 days ago

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.

This ticket was mentioned in Slack in #core by iandunn. View the logs.


9 days ago

#69 @azaozz
9 days ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

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
9 days ago

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

#70 @iandunn
9 days ago

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.

This ticket was mentioned in Slack in #core by iandunn. View the logs.


9 days ago

@iandunn
9 days ago

Rename the transient instead of adding the hack, props ocean90

#72 @iandunn
9 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

This ticket was mentioned in Slack in #core by iandunn. View the logs.


9 days ago

@iandunn
9 days ago

Removes the old upgrade routine

#74 @iandunn
9 days ago

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

#75 @SergeyBiryukov
8 days ago

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
8 days ago

refresh; revert db_version; rename 2nd transient

#76 @iandunn
8 days ago

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

#77 @SergeyBiryukov
8 days ago

In 40803:

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

Props iandunn, ocean90.
See #40702.

#78 @azaozz
8 days ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

Note: See TracTickets for help on using tickets.