Skip to:
Content

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#3059 closed defect (bug) (fixed)

Sticky topics get pushed to second page

Reported by: boonebgorges Owned by: sushkov
Milestone: 1.5 Priority: minor
Severity: normal Version:
Component: Forums Keywords: has-patch
Cc: has-patch

Description

When you sticky a topic, it gets stuck to the top of the group forum list. It stays there until there are enough newer forum topics to push the sticky's original position to the next page. Then the sticky gets pushed off the first page and sticks to the top of the second page, until it gets pushed to the first. Etc.

I assume that this is not by design. Sticky topics should be sticky no matter what.

Marking 1.3 for the moment because I have a client who needs it fixed, it's totally puntable though.

Attachments (3)

3059.diff (4.1 KB) - added by sushkov 3 years ago.
bbpress-3059.1.diff (1.4 KB) - added by boonebgorges 3 years ago.
3059.2.diff (13.0 KB) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 boonebgorges3 years ago

Did a bit of research, and this is unfortunately not a straightforward fix. There are two strategies:

1) Include the stickies in the regular loop. This requires a bunch of fancy footwork that BB_Query (somewhat surprisingly) does not support. The bbPress plugin does have code for the proper offsetting; see http://trac.bbpress.org/browser/branches/plugin/bbp-includes/bbp-topic-template.php#L121. I am not keen on the idea of rewriting all of this logic for our implementation of bbPress, as it is going to mean a LOT of fumbling around with offsets.

2) Do a separate loop. This is a much easier stopgap solution. The downside is that it would mean a funky change in the default theme between 1.3 (where the stickies would be in a separate loop, affecting pagination, etc) and 1.4 (where the bbPress would integrate stickies properly into the main loop). On the other hand, individual theme authors could pretty easily implement this on their own, so perhaps it's not necessary put into bp-default in light of the impending bbPress plugin.

Does anyone have any thoughts? Maybe this is a wontfix, in anticipation of bbPress plugin?

comment:2 DJPaul3 years ago

I say wontfix because this is really a bbPress bug. Could you report it upstream as you've looked into the code, please?

comment:3 DJPaul3 years ago

  • Keywords dev-feedback added

comment:4 DJPaul3 years ago

  • Milestone changed from 1.3 to 1.4

sushkov3 years ago

comment:5 sushkov3 years ago

  • Keywords has-patch added
  • Owner set to sushkov
  • Status changed from new to accepted

Added another loop. Most of the code was required to do the recalculation for pagination and topic numbering.

bbPress (not as a plugin) will likely be dropped in nearest future (there's almost no activity around it lately), thats why I opt for this hack (which is a rewrite of an older hack, see the deleted lines).

Some testing would be great.

comment:6 DJPaul3 years ago

Boone, did you get this fixed in trunk?

comment:7 boonebgorges3 years ago

No, I lost track. I can test it when I get back from vacation, or you can have a look. I think sushkov's solution is good, as it's what bbPress itself does.

comment:8 DJPaul3 years ago

Great; let's see if this can get into 1.3. Leaving the milestone as-is until we have time to check the patch.

comment:9 markaduffy3 years ago

Hi, i applied this patch to buddypress 1.2.7. The stickies are being moved to the front though the paging count gets screwed up.

comment:10 DJPaul3 years ago

  • Keywords needs-patch added; dev-feedback has-patch removed

comment:11 sushkov3 years ago

@markaduffy the patch was written based on trunk, I didn't test it on 1.2.7 just because it was nominated for a 1.(3|4) release.

The pagination part is tricky (and I'm not very proud with the current solution, though it works) but it should be "fixable" by changing one or two lines of code.

Thanks for the feedback.

comment:12 markaduffy3 years ago

@sushkov What version is the development trunk at?

These are the versions i'm using

Wordpress 3.0.1
Buddypress 1.2.8 (Upgraded today)
bbPress 1.0.2

Last edited 3 years ago by markaduffy (previous) (diff)

comment:13 johnjamesjacoby3 years ago

  • Cc has-patch added
  • Milestone changed from 1.4 to 1.3

If patch is good on trunk, let's scoot this in.

comment:14 boonebgorges3 years ago

  • Milestone changed from 1.3 to 1.4

sushkov - I'm sorry, I can't get this patch to work right. It's screwing up pagination pretty badly.

I think that a proper patch will be quite difficult without some modifications to bbPress itself. In particular, when you have a page that contains non-sticky posts, it's almost possible to query those posts with BB_Query as it stands. What you really need is the ability to set an offset/number (rather than page/per_page) on BB_Query. See my attached bbpress-3059.1.diff to see what I mean. JJJ said he's happy to put a patch into bbPress if it makes our job easier.

In any case, I spent about three hours on this today and didn't make much progress, so I'm giving up for the moment. If a site admin wants to have a separate sticky loop, with *separate* pagination, it's easy to do, and can be done at the theme level.

sushkov, if you can provide a patch that paginates properly (on all different scenarios: different values for $_GETn? and $_GETp?; # of stickies that is greater than $per_page; etc), we can move this back to the 1.3 milestone. Otherwise we'll try to get it in 1.3.1.

Thanks!

boonebgorges3 years ago

comment:15 boonebgorges3 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from 1.4 to 1.3

I must have dreamt about this ticket last night, because I woke up this morning with a fairly clear picture of how to solve the problem. In any case, 3059.2.diff seems to be a fully-functional solution for displaying stickies, with proper pagination. It uses sushkov's patch as an inspiration, but redoes the logic somewhat.

In order for this patch to work, you must also apply bbpress-3059.1.diff to your bbPress installation. I'm going to talk to jjj about getting it into the bbPress trunk.

boonebgorges3 years ago

comment:16 boonebgorges3 years ago

After a conversation with jjj, I am probably going to refresh this a bit so that the sticky logic does not apply on global directories. Two reasons: individual group admins should not have the ability to "spam" global directories with their group stickies, and a large number of groups will easily mean pages and pages of stickies on the global directory.

I'll work on this in the next few days, and once jjj gets around to putting the bbpress patch in, I will be able to resolve this ticket.

comment:17 johnjamesjacoby3 years ago

  • Severity set to normal

Patch is committed on the bbPress side of things.

comment:18 boonebgorges3 years ago

Thanks, JJJ.

comment:19 boonebgorges3 years ago

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

(In [4764]) Fixes sticky logic on group forum directories so that stickies always appear at the top of the order, without breaking pagination. Fixes #3059. Props sushkov for the inital patch. Introduces bp_forums_enable_global_directory_stickies() to allow easy modification of the default global forum directory behavior, which is to show group stickies in their normal freshness-based position.

comment:20 sushkov3 years ago

Awesomesouce :P

Note: See TracTickets for help on using tickets.