WordPress.org

Make WordPress Core

Opened 5 months ago

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

Download all attachments as: .zip

Change History (108)

@iandunn
5 months ago

#1 @iandunn
5 months ago

  • Keywords has-patch has-unit-tests added

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


5 months ago

#3 @azaozz
5 months ago

  • Milestone changed from Awaiting Review to 4.8

@iandunn
5 months ago

Add method to anonymize user IP

#4 @iandunn
5 months 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
5 months 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
5 months ago

Improve netmask readability, avoid sending IP when unnecessary

#6 @iandunn
5 months 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
5 months ago

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

#7 @iandunn
5 months 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
5 months 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
5 months ago

Minor tweaks for comment:8, part 1

#9 @iandunn
5 months 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
5 months 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
5 months 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.


5 months ago

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


5 months ago

@iandunn
5 months ago

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

#14 @iandunn
5 months 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
5 months ago

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

#15 @iandunn
5 months 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
5 months 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
5 months 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
5 months ago

Let's discuss it during dev-chat?

#19 follow-up: @swissspidy
5 months 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
5 months ago

Replying to swissspidy:

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

Good point

#21 @azaozz
5 months 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
5 months 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
5 months ago

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

@flixos90
5 months ago

#24 @flixos90
5 months 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
5 months 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 5 months ago by azaozz (previous) (diff)

#26 @flixos90
5 months 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
5 months 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
5 months ago

#28 @flixos90
5 months 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
4 months ago

#29 @flixos90
4 months 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
4 months ago

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

Looking to see if we can make it work.

@iandunn
4 months ago

Fixes bug in 40702.8.diff

#31 @iandunn
4 months 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
4 months ago

Alternate approach to adding script data

#32 @iandunn
4 months 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
4 months 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
4 months 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
4 months 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
4 months 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
4 months ago

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

#37 @iandunn
4 months 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
4 months 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
4 months 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.


4 months ago

#41 @jnylen0
4 months 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
4 months 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
4 months ago

  • Type changed from enhancement to task (blessed)

@Soean
4 months ago

documentation and coding standards

#44 @Soean
4 months ago

  • Keywords has-patch added

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

#45 @obenland
4 months ago

In 40669:

Dashboard: Community events formatting improvements

Props Soean.
See #40702.

@iandunn
4 months ago

Updates revisions numbers, documents reason for proxying request

#46 @iandunn
4 months 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
4 months 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
4 months 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
4 months 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
4 months 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
4 months 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
4 months ago

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

#52 @iandunn
4 months 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
4 months ago

Work in progress to provide better UX for geolocated IP locations

#53 @iandunn
4 months 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
4 months 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
4 months ago

#55 in reply to: ↑ 50 ; follow-up: @michelleweber
4 months ago

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

#56 in reply to: ↑ 55 @michelleweber
4 months 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
4 months 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
4 months 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
4 months 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
4 months 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
4 months 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
4 months 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
4 months ago

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

#63 @coreymckrill
4 months 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
4 months 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
4 months ago

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

#65 @iandunn
4 months 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.


4 months ago

@iandunn
4 months ago

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

#67 @iandunn
4 months 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.


4 months ago

#69 @azaozz
4 months 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
4 months ago

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

#70 @iandunn
4 months 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.


4 months ago

@iandunn
4 months ago

Rename the transient instead of adding the hack, props ocean90

#72 @iandunn
4 months 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.


4 months ago

@iandunn
4 months ago

Removes the old upgrade routine

#74 @iandunn
4 months 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
4 months 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
4 months ago

refresh; revert db_version; rename 2nd transient

#76 @iandunn
4 months 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
4 months 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
4 months 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.