Skip to:
Content

bbPress.org

Opened 5 years ago

Closed 5 years ago

#2528 closed defect (fixed)

Topic Merge Freshness

Reported by: netweb Owned by: netweb
Milestone: 2.6 Priority: normal
Severity: normal Version: 2.5
Component: Component - Topics Keywords: needs-patch needs-unit-tests
Cc:

Description

Merge Topic Fresness

  • Merge 'TopicB 2:00pm' topic into 'TopicA 1:00pm'
  • 'TopicA' freshness is now 2:00pm
  • Merge 'TopicC 3:00pm' topic into 'TopicA 1:00pm'
  • 'TopicA 1:00pm' topic freshness remains at 2:00pm

Related: #2329

See: bbp_merge_topic_handler and bbp_merge_topic_count

Attachments (2)

2528.1.diff (1.6 KB) - added by netweb 5 years ago.
2528.2.diff (2.4 KB) - added by netweb 5 years ago.

Download all attachments as: .zip

Change History (11)

#1 @netweb
5 years ago

  • Milestone changed from Awaiting Review to Future Release

Not a deal breaker, moving to future release pending a patch, then can be moved into an active milestone.

Workaround: Use the repair tools to recalculate freshness.

#2 @netweb
5 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Future Release to Awaiting Review

Merging two topics:

  • 1st topic 10am (post_date is 10am 2014-05-30 10:00:00)
  • 2nd topic 11am (post_date is 11am 2014-05-30 11:00:00)

If we merge the 11am topic into the 10am topic then $destination_topic of 10am is older than the $source_topic of 11am so the $destination_topic date should be updated to 2014-05-30 10:59:59

Expected: Older topic (10am) post_date should now be updated to 10:59:59
Actual: No change to either topics post_date

If you merge the 10am topic into the 11am topic then the 11am topic post_date is updated to 2014-05-30 09:59:59, so we are doing it backwards to what we actually want.

Relevant function bbp_merge_topic_handler

<?php
// Check if the destination topic is older than the source topic
if ( strtotime( $source_topic->post_date ) < strtotime( $destination_topic->post_date ) ) {

        // Set destination topic post_date to 1 second before source topic
        $destination_post_date = date( 'Y-m-d H:i:s', strtotime( $source_topic->post_date ) - 1 );

        // Update destination topic
        wp_update_post( array(
                'ID'            => $destination_topic_id,
                'post_date'     => $destination_post_date,
                'post_date_gmt' => get_gmt_from_date( $destination_post_date )
        ) );
}

Currently we are actually "Checking if the source topic less than than the destination topic"

<?php
// Check if the destination topic is older than the source topic
if ( strtotime( $source_topic->post_date ) < strtotime( $destination_topic->post_date ) ) {

It should be "Checking if the source topic is greater than than the destination topic"

<?php
// Check if the destination topic is older than the source topic
if ( strtotime( $source_topic->post_date ) > strtotime( $destination_topic->post_date ) ) {

Or maybe a clearer way to write the function based on the comment verbiage:

<?php
// Check if the destination topic is older than the source topic
if ( strtotime( $destination_topic->post_date ) < strtotime( $source_topic->post_date ) ) {

Now in theory if we merge two topics, the 1:20pm into 1:10pm per:

  • #25596 May 30, 2014 at 1:10 pm Topic One Topic 1:10pm
  • #25597 May 30, 2014 at 1:30 pm Topic One Reply 1:30pm
  • #25598 May 30, 2014 at 1:50 pm Topic One Reply 1:50pm
  • #25599 May 30, 2014 at 1:20 pm Topic Two Topic 1:20pm
  • #25600 May 30, 2014 at 1:40 pm Topic Two Reply 1:40pm
  • #25601 May 30, 2014 at 2:00 pm Topic Two Topic 2:00pm

Expected Results:

  • #25596 May 30, 2014 at 1:19 pm Topic One Topic 1:10pm
  • #25599 May 30, 2014 at 1:20 pm Topic Two Topic 1:20pm
  • #25597 May 30, 2014 at 1:30 pm Topic One Reply 1:30pm
  • #25600 May 30, 2014 at 1:40 pm Topic Two Reply 1:40pm
  • #25598 May 30, 2014 at 1:50 pm Topic One Reply 1:50pm
  • #25601 May 30, 2014 at 2:00 pm Topic Two Topic 2:00pm

Actual results:

  • #25596 May 30, 2014 at 1:19 pm Topic One Topic 1:10pm
  • #25597 May 30, 2014 at 1:30 pm Topic One Reply 1:30pm
  • #25598 May 30, 2014 at 1:50 pm Topic One Reply 1:50pm
  • #25599 May 30, 2014 at 1:20 pm Topic Two Topic 1:20pm
  • #25600 May 30, 2014 at 1:40 pm Topic Two Reply 1:40pm
  • #25601 May 30, 2014 at 2:00 pm Topic Two Topic 2:00pm

It looks like the replies from the source topic are not getting updated menu_order

IDpost_datepost_contentpost_namepost_typepost_parentmenu_order
255962014-05-30 13:19:59Topic One Topic 1:10pmtopic-one-topic-110pmtopic254510
255972014-05-30 13:30:00Topic One Reply 1:30pm25597reply255961
255982014-05-30 13:50:00Topic One Reply 1:50pm25598reply255962
255992014-05-30 13:20:00Topic Two Topic 1:20pmreply-to-topic-one-topic-110pmreply255963
256002014-05-30 13:40:00Topic Two Reply 1:40pmreply-to-topic-one-topic-110pm-2reply255961 <- Source Reply
256012014-05-30 14:00:00Topic Two Topic 2:00pmreply-to-topic-one-topic-110pm-3reply255962 <- Source Reply

#3 @netweb
5 years ago

  • Milestone changed from Awaiting Review to 2.6
  • Owner set to netweb

#4 @johnjamesjacoby
5 years ago

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

In 5404:

Correctly compare source & destination topic post_date values in bbp_merge_topic_handler(). Fixes issue where incorrect post would be updated when merging older topics into newer ones.

Also update the reply position for all newly relocated replies. Hat-tip netweb. Fixes #2528.

@netweb
5 years ago

#5 @netweb
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This is still not quite right:

  • Merged replies and source topic topic are assigned a post title
  • Merged replies and source topic topic are assigned a post name
  • Reply menu_order is assigned the old source topic menu_order rather than destination menu_order
    • (i.e. Don't update menu_order until after the post_parent has been updated)

Attached patch 2528.1.diff addresses the above and also removes source topic's last and count meta data.


The above patch only fully works alongside patch 2623.1.diff in #2623, specifics details are in that ticket, without 2623.1.diff reply_to values are created of themselves in post meta for merged replies.

Related: #2623


Also because our source topic post_name after merging get's an updated post meta key _wp_old_slug that is a WordPress 301 redirect using the old slug to the new slug this should allow us to forward the URL of reply notifications from the old topic per #2329 if #2424 is implemented.

Related: #2329 #2424


Similar issues to the above are also needed for the 'Split Topic' function, see #2624

@netweb
5 years ago

#6 @netweb
5 years ago

In 2528.2.diff refreshes 2528.1.diff with the following additions:

  • Reverts r5404 strtotime - Antiprops netweb. (I blame the epoch's, long detailed description available if req.)
  • Removes the bbp_update_reply_to() "adjust reply to values" reply to updates, a threaded reply is always a reply to a reply, if we move a reply as part of a merge any reply to another reply will still be a reply to the same reply ID.

Patch 2528.2.diff​ fully tested with 'non-threaded' and "thread replies" -> https://cloudup.com/cvnzD1f85HG

#7 @johnjamesjacoby
5 years ago

In 2528.2.diff why set the post_name to $reply->ID? We seem to set it to false everywhere else, and let wp_update_post() take care of the slug for us.

This ticket was mentioned in IRC in #bbpress-dev by jjj. View the logs.


5 years ago

#9 @johnjamesjacoby
5 years ago

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

In 5489:

In bbp_merge_topic_handler() delete last and count metas, and update the reply position after call to wp_update_post(). Fixes bug where merging two topics with odd timestamps could result in orphaned or incorrect meta data and hierarchy positioning. Props netweb. Fixes #2528.

Note: See TracTickets for help on using tickets.