Skip to:
Content

BuddyPress.org

Opened 8 weeks ago

Last modified 18 hours ago

#8581 new feature request

Extending no content activities with images and call of actions

Reported by: vapvarun Owned by:
Milestone: 10.0.0 Priority: normal
Severity: normal Version:
Component: Activity Keywords: has-patch
Cc:

Description

We have certain activities like
1- Changed Avatar
2- Added friend
3- Created Groups
4- Joined Groups
5- Changing Cover image for profile or Groups
these activities can be improved with image-rich features which may give a glimpse about that specific group or members with using a cover image as background.
https://demos.wbcomdesigns.com/wp-content/uploads/2021/10/image-1.png

Attachments (8)

8581.patch (71.5 KB) - added by vapvarun 2 weeks ago.
Initial draft
8581-a.patch (3.2 KB) - added by oztaser 13 days ago.
8581-b.patch (31.8 KB) - added by imath 10 days ago.
8581-c.patch (99.0 KB) - added by vapvarun 6 days ago.
revised patch
profile-update.png (523.7 KB) - added by vapvarun 6 days ago.
Activity update on profile avatar change
group-join.png (389.6 KB) - added by vapvarun 6 days ago.
Activity update on group join
friend-connection.png (358.5 KB) - added by vapvarun 6 days ago.
Activity update on adding new friend
8581.2.patch (61.9 KB) - added by imath 4 days ago.

Download all attachments as: .zip

Change History (33)

#1 @imath
8 weeks ago

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

I really like the idea, looking forward to test the patch 😉

#2 @vapvarun
6 weeks ago

Hi @imath, will need the input of saving earlier upload avatar by a member, we can use the same folder which keeps the current avatar of a member or link avatar with wp attachment to display within activity content.
like if I have changed my avatar yesterday that activity will display yesterday's uploaded avatar and if I changed my avatar today again the that will display what's uploaded today and it will not impact yesterday's avatar change activity.

#3 @imath
6 weeks ago

  • Keywords dev-feedback added

I see, we don't keep previously uploaded avatars. I'll think about it and I'll be back!

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


6 weeks ago

@vapvarun
2 weeks ago

Initial draft

#5 @oztaser
2 weeks ago

Hi @vapvarun

First of all thanks for working on this! I'm not sure is it the right time to give feedbacks about the patch but I have a few. Please ignore if you're still working on any of them.

  1. Storing image data as base64 in the database feels me is not the best way. It'll increase the storage requirement by 33% [ 1 ]. It will cause extra load to MySQL and object cache storage. I understand the reason of storing images in database, we want to show previous images on activities but we really need that? I'm not sure but we may can handle this situation with BP Attachments?
  2. Why do we need storing image file names as meta value? If we want to add this value to alt attribute of <img> tag or anywhere we want, can't we use title or name of activity item?

If we decide to go with base64,

  1. I think we need to move fetching and saving image process into a new function. We should check remote request (like is it WP_Error object? and HTTP response code. For example wp_remote_get returns a WP_Error object when timeout). We also should check response data is empty at least.
  2. We should provide a filter for passing arguments to remote request. I believe this could useful for developers when trying to send custom header or user-agent. For example, our CDN is waiting for user agent to return the images.

Thanks again!

[ 1 ]: https://developer.mozilla.org/en-US/docs/Glossary/Base64

#6 @vapvarun
2 weeks ago

@oztaser Thanks for the feedback.

My initial approach was using wp attachment as bp attachment but it's still in development.

I had discussed with @imath during the team meeting he suggested going with base64.

"Using base64 images, we make sure they are only deleted when the activity is deleted"

It will definitely increase the database storage. We are open to suggestions to update the patch.

for fetching and displaying these images, we can easily wrap them using a new function with filters.

#7 @imath
13 days ago

  • Keywords needs-refresh added; needs-patch removed

Hi @vapvarun

First thanks a lot for your initial patch. I can read from it you put a lot of energy into it. I already saw some possible improvements, but first I'm going to test and play a bit with it.

Second, thanks a lot @oztaser for alerting us about the base64 db overheat warning! My initial thought was: I absolutely wanted to avoid creating a WordPress Attachments to store the images. Because:

  1. in this case it can be deleted outside of the activity and we have a risk of an error due to a vanished image
  2. we shouldn't use WordPress attachments for community members generated media imho.

So the base64 is a bad idea, I'll try to think about it to suggest an alternative, as I've been playing with revisions on the BP Attachments feature as a plugin (I really need to find time to progress on it!!) & I have something in mind :)

Finally looking at the patch, we might be too ambitious about it. I saw you were creating new activity actions @vapvarun, I believe we should focus on existing ones first, make sure we have something robust and then create the new ones.

Important things about the feature are:

  1. avoid generating too much activity when the user is changing his profile image many times in a short amount of time because, he's not happy with the ones he chose...
  2. show the version of the avatar when the activity was created.

Let's try to work on a. and b. :)

@oztaser could you look at a. 🙏. I think it's already in BP Core, the goal is too only keep the latest activity about a profile image change. The idea is, before saving a new activity having the 'new_avatar' type check if the user already created one of this type like in less than an hour ago and if so delete this activity and then publish the new activity.

I'll look at b. 👌

@vapvarun could you narrow the patch to existing no-content activities only ?

#8 @oztaser
13 days ago

Hi there!

Thanks for the explanation of the situation about storing images.

@imath, I've created the patch for avoiding multiple new_avatar activities per hour. I've also added a filter to the time period.

I'm open to any suggestions about the patch.

P.S: I've created the patch from master branch. I thought this is most clear way because I was not sure about how the function I've edited would change.

@oztaser
13 days ago

#9 @imath
12 days ago

@oztaser your patch looks pretty nice and thanks a lot for the PHPUnit tests 💪. I'll commit it as is for now, and we'll always be able to create a specific function out of it when we'll add support for cover image and group activities.

#10 @imath
12 days ago

  • Keywords dev-feedback removed

@vapvarun I've just tested your patch, generated activities are looking awesome!

https://cldup.com/hQ0agOpcbY.png

I have a few suggestions I'd like you to work on 🙏. Let's focus on existing activity types first.

the new_avatar activity type

  1. Including the cover image into the generated activity is problematic imho. It introduces a new question: Which one should we use? The one that was available at the time the avatar was changed or the latest one, in your patch it's the latest one. I believe, we shouldn't include it at all to focus on what has changed: the avatar.
  2. We should probably use another class than .avatar otherwise the theme may resize it. In the screen-capture, TwentyNineteen is doing so. I'd suggest .profile-image instead.
  3. The avatar should be full, maybe centered? Only the @username should be added and the link could be the activity directory permalink + ?r=username this way it add @username into the activity whats-new textarea. Well actually what about this layout:
Avatar
Mention @username View Profile


The friendship activity type

https://cldup.com/0sm9ZXEl-X.png

I think this one can stay the way it is, or we could simply replace the .avatar class by a .profile-image one.

#11 @vapvarun
12 days ago

Thanks, @imath @oztaser
working on it to improve further and will include the above suggestion, removing new cover image activities, for existing avatar change activities, I am using current cover images, I think that will be a good approach for now as we will not have to look for what was the cover image at that time, we can also add a filter there that will help developers to replace the current image with more generic activity type image.

#12 @imath
12 days ago

In 13154:

Avoid generating many new_avatar activities in a short time frame

For an hour time frame, if a user publishes many new_avatar activities, only the latest one will be kept.

Props oztaser

See #8581

#13 @vapvarun
12 days ago

@oztaser we can use file_get_contents in case we are getting timeout from wp_remote_get.
as on local system or controlled environment, we may get empty output from it.

is it okay to go with the following approach? I was trying to avoid calling file_get_contents directly inside code but after checking inside bp core codes, it's already been used 2 times

        $data = wp_remote_get( $avatar_url);
                if ( is_array( $result ) && ! is_wp_error( $result ) ) {
                $data = wp_remote_retrieve_body( $data );
                        } else {
                         $data = file_get_contents( $avatar_url );
                        }

#14 @oztaser
12 days ago

@vapvarun We probably get the same error if we try to fetching file from URL by file_get_contents. I also prefer use wp_remote_get for all HTTP requests but I think it makes sense if use absolute file path instead of file URL with file_get_contents.

I don't see any problem using file_get_contents with absolute path but I'm not sure. Maybe @imath has an any knowledge.

#15 @imath
12 days ago

In 13155:

Get the right user latest new_avatar activity before replacing it

In [13154] we missed setting the filter key of the array argument passed to the bp_activity_get() function.

See #8581

#16 @imath
12 days ago

I have an alternative to using base64 encoded images. In short it consists in adding an history subdirectory to the user avatar one. I'll submit a patch tomorrow. @vapvarun don't worry about using file_get_contents() or wp_remote_get() we won't need it, I believe 😉.

NB: Old new_avatar activities won't have the activity meta, so we'll need to only display the rich content if we have this meta.

@imath
10 days ago

#17 @imath
10 days ago

  • Keywords has-patch added; needs-refresh removed

Here’s the patch I’m suggesting about the need to use the profile image file the way it was at the time when the activity was generated (the b. point of my previous comment).

  1. Improve the BP Attachments API so that it can manage file revisions (It will be useful for the BP Attachments feature as a plugin) and avatar/cover images history.
  2. Instead of deleting an avatar when it’s being changed, move it to an history subdirectory.
  3. Avoid using an activity meta to store the previous avatar. Instead, use the date the activity was created on and edit the way we name avatars so that it uses a timestamp instead of uniqid(). To achieve this I had to edit a bunch of functions to transport the exact same timestamp the avatars were created inside the bp_members_new_avatar_activity function to use this timestamp as the activity date.
  4. Introduce 2 new core functions: bp_attachments_list_directory_files() and bp_attachments_list_directory_files_recursively(). The second one help use to return the current and historical avatars inside a flat array.
  5. Introduce the bp_avatar_get_version_src() function to get the user avatar according to the date it was created.
  6. Force bp_activity_has_content() to be true for new_avatar activity having an existing avatar for the activity date. See the code here.
  7. Add a late filter to ’bp_get_activity_content_body’ to generate the rich content. See the code here. For now it simply outputs the avatar.

@vapvarun you’ll be mostly interested in 6 & 7 to generate your rich content.

NB: when possible we should always try to avoid template actions such as 'bp_activity_entry_content’ as these kind of actions might miss inside some overrides.

#18 @vapvarun
9 days ago

@imath, thanks for the help; it will work; I tested the patch, the previous avatar is getting saved; just a trailing slash is missing when displaying the previous avatar. I will submit the patch shortly, including the above suggestion.

#19 @oztaser
8 days ago

Hi there!

I've read the patch and test it locally. It works very well. Thanks for working on that, @imath. I've just noticed a few code style problem :) :

  • There is a var_dump on bp_activity_has_content function :)
  • I think there is no need to create empty $avatar_id variable on bp_avatar_get_version_src
  • There is no need else block on add_revision line 592. We can just move the line out side of the else block.

I have one last concern about the new_avatar activity feature and other new activities we store old file. Should we remove avatars from history directory when new_avatar activity is removed? What you think about that? I think it can be important for user privacy.

#20 @imath
8 days ago

Hi @oztaser thanks for these catches, I believe @vapvarun will include them on the patch he’s working on.

You’re absolutely right about deleting avatars inside the history directory when the corresponding activity is deleted.
We’ll include it as well.

In a future release, users should also be able to delete it from the Avatar UI.

@vapvarun
6 days ago

revised patch

@vapvarun
6 days ago

Activity update on profile avatar change

@vapvarun
6 days ago

Activity update on group join

@vapvarun
6 days ago

Activity update on adding new friend

#22 @imath
6 days ago

Thanks a lot for the revised patch @vapvarun That’s my priority of the week-end! I’ll test it asap, your screenshots looks very promising 🤩

@imath
4 days ago

#23 @imath
4 days ago

I've tested your patch @vapvarun and it works great. Thanks a lot for your work on it.

I first want to say sorry for what I'm about to write. I know both of you already invested a lot of time into this ticket, and I must say I also did!

But I think it's not ready yet. We need to work a little more on it.

  1. I've changed the logic to be more consistent with what I believe is the BuddyPress way of doing things.

Instead of using a filter, I'm kind of introducing a new template into our template hierarchy. The activity type one. It's useful here and I believe it can also be useful for other activity types.
I also think we need the $activities_template global to contain the generated content parts.

Here's how it's working now:

  • use specific template parts located into the buddypress/activity/type-parts/ of the BP Ttemplate Pack
  • Perfom a template part look up in bp_activity_content_body(), if a template is found load it instead of getting the content body using bp_activity_get_content_body(): this way we don't need the activity filters anymore. I think it's also best because we were sanitizing empty content otherwise because all the bp_activity_get_content_body filters were used for nothing. You'll also see the bp_activity_content_body() function is now using a new $context parameter so that we can control where these template loads are occuring. I believe it should only be done inside a Theme.
  • introduce new template tags:
    • bp_activity_the_content_body() to bypass the activity types template parts look up or to use this tag inside an activity types template part.
    • bp_activity_has_generated_content_part() is checking if the property of a generated content exists
    • bp_activity_generated_content_part() is outputing the the property of a generated content
    • bp_activity_get_generated_content_part() is returning the output after making sure it has been sanitized.
  • Add the generated content from bp_activity_has_content() making sure to avoid as much as possible code duplication.
  1. Sometimes the cover image or the avatar are disabled...

I've added checks about this and had to edit the styles a bit in case the user has no cover image for instance. In this case including the grey background is not great imho. So I'm just outputting the avatar.

I haven't tested, but there should be adaptations to add to styles in case the avatar is disabled.

  1. Are we covering all existing no content activities? Should we be more creative about the layouts?

As you will see I've found at least one we were not covering so far: the created_group one. We need to review the activities to find the ones that are not covered yet.

You will also see the friendship_created output is the same as the new_avatar one, and I did the same for created_group and joined_group (I was a bit lazy or a bit tired).

When looking at the activity stream, we basically just outputting the user profile header or the group profile header everytime. I believe we should try to make these layouts different.

  1. As @oztaser noticed, we need to allow users to manage "historical avatars". I don't think we should delete an avatar when the corresponding activity has been deleted. I think we should do the other way around. Delete the activity when an historical avatar has been deleted. This means we need a new tab inside the Avatar UI to pick an old avatar or delete old avatars.
  1. Let's drop the embed activities from this feature for now.

That being said, I think the 8581.2.patch is becoming a very huge patch. So I'd like us to keep it the way it is (unless it can be improved) and to add the needed steps from different patches. This will require us to apply 8581.2.patch, commit it locally before creating new patches. We can alternatively use this branch of my GH copy of trunk.

I'll try to work on adding the new Avatar UI tab in the coming days. @vapvarun can you bring us some creativity about the layouts and check if we're still missing some no content activities?

#24 @imath
32 hours ago

FYI, just updated my GH branch with this:

https://cldup.com/R_PHdaMWMz.png

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


18 hours ago

Note: See TracTickets for help on using tickets.