[buddypress-trac] [BuddyPress Trac] #6229: Notification content

[buddypress-trac]

#6229: Notification content
---------------------------------------+--------------------
 Reporter:  danbp                      |       Owner:
     Type:  defect (bug)               |      Status:  new
 Priority:  normal                     |   Milestone:  2.2.2
Component:  Component - Notifications  |     Version:
 Severity:  normal                     |  Resolution:
 Keywords:  has-patch                  |
---------------------------------------+--------------------
Changes (by boonebgorges):

 * keywords:  2nd-opinion => has-patch


Comment:

 Fix looks OK to me. For anyone following along, the issue is that
 `BP_Notifications_Template` is parsing the 'npage' URL param, but is not
 then passing it along properly to `BP_Notifications_Notification::get()`.
 johnjamesjacoby's new variable properly includes the 'page' parameter.

 I'm indifferent about the `query_vars` bit. If you like it, roll with it.

 See [attachment:6229.02.diff] for updated tests. The test you originally
 added is pretty good (though I made it more precise - as it stood, you
 were only testing the number of items being returned, but that doesn't
 tell us whether the correct page of results has been returned, at least
 not directly). However, the test you wrote doesn't actually describe the
 bug - it passes even without your patch to `BP_Notifications_Template`. I
 have added an additional test in 02.diff that does demonstrate the fix.

 This is not a regression but it's a pretty lousy bug, so 2.2.2 is fine if
 no one else has objections.

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/6229#comment:4>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac