Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6229 closed defect (bug) (fixed)

Notification content

Reported by: danbp Owned by:
Milestone: 2.2.2 Priority: normal
Severity: normal Version:
Component: Toolbar & Notifications Keywords: has-patch
Cc:

Description

As already explained on #6205, the content doesn't go to next page when on the notification screen.

Tested with BP 2.2.1 MS, and 2013 - no other plugin installed.

This is a different problem and why i open a new ticket. Apologize if i doing it wrong.

Attachments (4)

notification-content.jpg (43.6 KB) - added by danbp 5 years ago.
notification-content2.jpg (45.6 KB) - added by danbp 5 years ago.
6229.01.patch (4.3 KB) - added by johnjamesjacoby 5 years ago.
Fix & Test
6229.02.diff (5.4 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (12)

#1 @johnjamesjacoby
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 2.2.2

Moving to 2.2.2 to investigate further.

Likelihood of this being a regression seems small, as I don't think this code was modified in such a way to cause breakage it during the 2.2 cycle.

@johnjamesjacoby
5 years ago

Fix & Test

#2 @johnjamesjacoby
5 years ago

  • Keywords 2nd-opinion added; needs-patch removed

6229.01.patch includes a fix and unit test to support results of BP_Notifications_Notification::get() return the correctly paginated results.

It also adds a query_vars variable to BP_Notifications_Template which is a bit redundant, but it proved helpful for passing the same array to both BP_Notifications_Notification::get() and BP_Notifications_Notification::get_total_count() and for keeping pristine the variables used to translate the request into queried results. Since no other components work this way, I'm open to removing it and using a locally scoped array instead, but I thought it was a neat so included it here.

This ticket was mentioned in Slack in #buddypress by jjj. View the logs.


5 years ago

@boonebgorges
5 years ago

#4 @boonebgorges
5 years ago

  • Keywords has-patch added; 2nd-opinion removed

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

#5 @johnjamesjacoby
5 years ago

In 9570:

Notifications: Ensure pagination works with $_GET parameters.

Props boonebgorges. See #6229. (trunk)

#6 @johnjamesjacoby
5 years ago

In 9571:

Notifications: Ensure pagination works with $_GET parameters.

Props boonebgorges. See #6229. (2.2 branch)

#7 @johnjamesjacoby
5 years ago

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

Thank you Boone for the updated patches.

Marking as fixed.

#8 @danbp
5 years ago

It works ! Merci beaucoup Boone and John.

Note: See TracTickets for help on using tickets.