Skip to:
Content

Opened 15 months ago

Closed 7 weeks ago

#4786 closed defect (bug) (fixed)

Big activity stream bug after editing an entry in the activity dashboard - introduced in 1.6.3

Reported by: rossagrant Owned by: boonebgorges
Milestone: 2.0 Priority: normal
Severity: normal Version:
Component: Activity Keywords: has-patch 2nd-opinion
Cc: DJPaul

Description

Hi guys,

I have found a rather large bug in BP 1.6.3 that occurs when you edit an activity stream item from the backend.

I noticed it when editing a forum reply and created the following screencast:

http://www.screenr.com/dfO7

SINCE THEN I HAVE TESTED IT THOROUGHLY AND IT AFFECTS ALL EDITED STEAM ITEMS - not just forum replies, but literally everything you edit from the dashboard.

Steps to reproduce:

  • Create a stream item by posting a forum post for example.
  • Visit the 'Activity' dashboard.
  • Select the entry to edit. You don't need to edit it, just hit 'Update'.
  • Visit the stream and you will see that the item is now styled according to two incorrect classes of 'mini' and 'friend-request'.
  • Clicking on the time stamp - which would normally take you to the forum post for example, now diverts you to a single view friend request style page.

I have tested this on 3 installs, 1 brand new install and it seems to affect ALL activity items that get edited in the backend.

One way to correct this as a very basic workaround for forum posts is to trash the post, then restore it, so that the item is created again correctly. Editing it once more causes the issue once more.

Attachments (1)

4786.patch (754 bytes) - added by boonebgorges 7 weeks ago.

Download all attachments as: .zip

Change History (15)

comment:1 hnla15 months ago

The reason the visual appearance is borked is that on edit and return to li item the item has had the class tokens 'friendship_accepted' & 'mini' added to the parent li element. '.mini' we generally use to apply a left float to the activity header to keep all on one line but isn't an applicable class for a forum post or other major li items.

A temp fix would be to add:

.activity-list li.bbpress.mini .activity-header p {float: none;}

The compound class would fail silently when issue resolved, however as this happens on a variety of entries one would to repeat for other activity items such as '.album'

comment:2 rossagrant15 months ago

This seems to occur on forum posts, BP-album uploads, but DOESN'T actually happen on status updates or WP posts like I first thought.

Status updates and WP posts keep their intended ul.activity-list li class and are not given the .mini attribute.

comment:3 rossagrant15 months ago

We need to remember that styling isn't the only issue here too. The timestamp takes users to the single view of the item as opposed to it's intended location.

So forum posts should go to the forum post in the thread once the time stamp is clicked and BP-Album items should take you to the actual picture in the album, not these single views that we are seeing.

comment:4 r-a-y15 months ago

The problem is with plugins not correctly registering their activity actions.

I've posted a ticket with a fix for bbPress here: #BB2176.

comment:5 DJPaul15 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Closing ticket as this is a bbPress bug.

comment:6 rossagrant14 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Reported that this was happening with BP Album entries as well as BB Press and Foxly is throwing the ball back in Buddypress' court.

See here for details: https://github.com/BP-Media/bp-album/issues/1#issuecomment-13587995

Any thoughts?

comment:7 johnjamesjacoby14 months ago

  • Milestone set to 1.8

This is already fixed in bbPress for 2.3. The bug is because activity actions aren't registered ahead of time for the edit action to be able to reuse the same action. It's not exactly a bug, but it's also not clear that an action priority earlier than 10 is necessary.

Punting to 1.8 to look into it then, since it's not a regression, and it's late in 1.7 to be tweaking these things.

comment:8 r-a-y14 months ago

  • Cc DJPaul added

I've replied to the Github ticket.

I think this is a pretty big awareness issue for plugin developers as most plugins will probably have never used bp_activity_set_action() before.

I have to say that until this ticket, I never used to register my activity actions either. And bp_activity_set_action() is relatively old since it was introduced in v1.1!

A post on the BP devel blog should probably be made. The BP Skeleton Component should also be updated as well,.

FYI, the activity dashboard currently looks for registered activity actions here and was commited in r5484:
https://buddypress.trac.wordpress.org/browser/tags/1.6.4/bp-activity/bp-activity-admin.php#L426

The alternative is to make the activity dashboard validation a little bit more flexible.

I'm going to ping DJPaul as he's the one who built the activity dashboard.

comment:9 boonebgorges11 months ago

  • Keywords needs-patch added
  • Milestone changed from 1.8 to 1.9

I'm not sure I fully understand the issue, so I'm moving to 1.9 for review.

comment:10 boonebgorges5 months ago

  • Milestone changed from 1.9 to 2.0

comment:11 boonebgorges7 weeks ago

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

The problem, as r-a-y suggests, is that plugins sometimes (often?) do not properly register their activity actions. As a result, the existing type does not show up in the 'Type' dropdown when editing an activity item, and when the item is saved, it's changed over to whatever type happens to be selected in the <select> box (like created_group).

4786.patch is my suggested fix. When the type for the current item has not been registered properly with bp_activity_set_action(), we append it to the end of the dropdown. Obviously, we don't have a natural language label for the type, so I just display the raw type string (like 'bp_doc_created').

I've left the on-save sanitization in place (https://buddypress.trac.wordpress.org/browser/tags/1.6.4/bp-activity/bp-activity-admin.php#L422). Unregistered activity items won't be whitelisted, but that just means that $activity->type is not changed from the value saved in the database.

Clearly, it'd be preferable for plugin authors to register their types in the correct way (and that'll be even more important when we have format_action callbacks #3856). But with this minimal change, we can at least prevent major breakage.

boonebgorges7 weeks ago

comment:12 DJPaul7 weeks ago

Nice idea; how do you feel about throwing a _doing_it_wrong() in here so we hopefully flag for plugin developers that they've missed something?

comment:13 boonebgorges7 weeks ago

I like that idea. Thanks for the feedback.

comment:14 boonebgorges7 weeks ago

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

In 8085:

Better fallback support on Activity Admin for items with an unregistered type

The 'Type' dropdown on the activity edit screen is populated with types that
have been registered with bp_activity_set_action(). However, many legacy
plugins do not register their activity types correctly. This adds support
for editing these types of activity items.

Fixes #4786

Note: See TracTickets for help on using tickets.