[wp-trac] [WordPress Trac] #40702: Add WordCamps and meetup events to the News Dashboard widget

WordPress Trac noreply at wordpress.org
Thu May 11 18:08:39 UTC 2017


#40702: Add WordCamps and meetup events to the News Dashboard widget
-------------------------------------------------+-------------------------
 Reporter:  iandunn                              |       Owner:
     Type:  enhancement                          |      Status:  new
 Priority:  normal                               |   Milestone:  4.8
Component:  Administration                       |     Version:  trunk
 Severity:  normal                               |  Resolution:
 Keywords:  needs-dev-note has-patch needs-      |     Focuses:
  unit-tests dev-feedback                        |  javascript, rest-api
-------------------------------------------------+-------------------------

Comment (by 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 .
 `
 1. 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
 [https://gist.github.com/iandunn/15a5fdaa78d4bfcd2ce29e0453887d7f 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?

--
Ticket URL: <https://core.trac.wordpress.org/ticket/40702#comment:27>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform


More information about the wp-trac mailing list