Extending no content activities with images and call of actions

[vapvarun]

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.

[imath]

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

[vapvarun]

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.

[imath]

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

[vapvarun]

Initial draft

[oztaser]

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

[vapvarun]

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

[imath]

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 ?

[oztaser]

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.

[imath]

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

[imath]

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

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

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

[vapvarun]

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.

[imath]

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

[vapvarun]

@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 );
                        }

[oztaser]

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

[imath]

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

[imath]

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]

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.

[vapvarun]

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

[oztaser]

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.

[imath]

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]

revised patch

[vapvarun]

Profile Update: ​https://prnt.sc/20xaefu
Group Join: ​https://prnt.sc/20xaff4
Friend Connection: ​https://prnt.sc/20xafmj

[vapvarun]

Activity update on profile avatar change

[vapvarun]

Activity update on group join

[vapvarun]

Activity update on adding new friend

[imath]

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]

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.

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

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

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

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

[imath]

FYI, just updated my GH branch with this:

[vapvarun]

Join Group: ​https://nimb.ws/YoRltZ
Friends: ​https://nimb.ws/FqR4hn
Created Group: ​https://nimb.ws/3OdVfo
Changed Avatar: ​https://nimb.ws/VyW4qU
Profile Update: ​https://nimb.ws/f89pR0
New Register: ​https://nimb.ws/Z5c0Mm
with center align the view with activity type template structure like this
​https://nimb.ws/TwYEQl
@imath

[imath]

Thanks for your layout suggestions @vapvarun I think my explanations were not very clear 😬. It’s ok to have the activity look like a profile header for one type of activity. What I think is each type should have a different layout. I think it’s minor, we’ll be able to improve them during the beta period, no worries 😉. I’ll start committing what we already have: which is really great 👍.

[vapvarun]

revised patch with activity type template

[vapvarun]

revised patch with activity type template

[imath]

In 13174:

Introduce two new functions to list files into the BP Attachments API

• bp_attachments_list_directory_files() returns a list of file objects contained into the directory thanks to its absolute path passed as the function argument.
• bp_attachments_list_directory_files_recursively() returns a list of file objects contained into the directory and its subdirectories thanks to the directory absolute path passed as the function argument.

Props vapvarun, oztaser

See #8581

[imath]

In 13175:

Introduce a new method to the BP Attachment API to add file revisions

The BP_Attachment::add_revision() method is primarily used by the BP_Attachment_Avatar class to keep the history of previously uploaded user avatars (and potentially other objects avatar).

When a user uploads a new avatar, the previously uploaded one is moved inside an history subdirectory of the user's avatar directory. This avatars history is then available for the new_avatar activities to display the avatar the user had at the time these were published.

Props vapvarun, oztaser

See #8581

[imath]

In 13176:

Introduce 2 functions to get an avatar version and the avatars history

• bp_avatar_get_version() can be used to get a version of an avatar according to its timestamp. Since [13175] the BP_Attachment_Avatar class is using a timestamp inside the avatar file name instead of a generated unique string (uniqid()), this change allows us to request avatars according to the timestamp included into their names.
• bp_avatar_get_avatars_history() can be used to get the file objects list of previously uploaded avatars.

Props vapvarun, oztaser

See #8581

[imath]

In 13177:

Make sure the new_avatar activity can use the avatar crop results

Since [13175] the BP_Attachment_Avatar::crop() method include the timestamp the avatar was generated on into its returned array. This array is now transported into the action the function creating a new_avatar activity hooks to. This function (bp_members_new_avatar_activity()) now accepts 3 more parameters included the transported crop results. This makes it possible to use the timestamp when the avatar was generated on as the new_avatar activity recorded time. Doing so, we don't need to add an extra activity meta to store the name of the avatar file to be sure to display the avatar the user had at the time the activity was created.

Props vapvarun, oztaser

See #8581

[imath]

In 13178:

Add Ajax handlers for the recycle and delete previous avatar actions

These handlers will be used by the avatar UI:

• to reset a previously uploaded user avatar as the current user avatar (bp_avatar_ajax_recycle_previous_avatar()),
• to permanently remove a previously uploaded user avatar (bp_avatar_ajax_delete_previous_avatar()).

Props vapvarun, oztaser

See #8581

[imath]

In 13179:

Add a new "Recycle" tab to the Avatar UI

Thanks to this tab users can manage their avatars history. They can choose to:

• reuse a previously uploaded avatar as their current profile photo,
• permanently delete an avatar from their avatars history.

Props vapvarun, oztaser

See #8581

[imath]

In 13180:

Remove a new_avatar activity when an avatar was deleted from history

Once the user removed an item from their avatars story, the corresponding new_avatar also needs to be deleted.

Props vapvarun, oztaser

See #8581

[imath]

In 13181:

Generate a richer content for "no-content" activities

Create the "generated-content" support for the following activity types: 'new_member', 'new_avatar', 'friendship_created', 'created_group', 'joined_group' & 'updated_profile'.

• Introduce the bp_activity_type_part() function to return the name of the activity type template part.
• edit the bp_activity_has_content() function to generate a richer content for activity types supporting the feature. For such activities the $activities_template global is extended adding a generated_content property to each activity in the loop having a type supporting the "generated-content" feature.
• Introduce the bp_activity_generated_content_part() template tag which use the current activity generated_content property to output the requested information passed as the function parameter.

Props vapvarun, oztaser

See #8581

[imath]

In 13182:

BP Nouveau: add the generated content type parts & needed styles

Props vapvarun, oztaser

See #8581

[imath]

In 13183:

BP Legacy: add the generated content type parts & needed styles

Props vapvarun, oztaser

Fixes #8581