Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#4636 closed defect (bug) (fixed)

Reinstate activity item for user profile changes

Reported by: DJPaul Owned by: boonebgorges
Milestone: 2.0 Priority: normal
Severity: normal Version: 1.1
Component: Extended Profile Keywords: has-patch needs-testing
Cc:

Description

Back in 1.0.3, when you edited your profile, an activity item was generated: 'x updated the "y" information on their profile'.

For some reason, this was removed in 1.1; see #4279 and http://buddypress.trac.wordpress.org/browser/tags/1.0.3/bp-xprofile.php#L540

I cannot find any details of why this was removed. We should consider re-adding it; if it's decided that we don't want this, the bp_activity_set_action('updated_profile') call in bp-xprofile-activity.php should be removed.

Attachments (3)

4636.patch (4.1 KB) - added by ericlewis 7 years ago.
4636.02.patch (5.9 KB) - added by boonebgorges 7 years ago.
4636.03.patch (13.4 KB) - added by boonebgorges 7 years ago.

Download all attachments as: .zip

Change History (21)

#1 @DJPaul
8 years ago

If this is removed, also take out the unset() in bp_activity_get_types().

#2 @DJPaul
8 years ago

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

Putting in 1.7 but bumpable if we don't get a patch.

#3 @johnjamesjacoby
8 years ago

  • Component changed from Activity to XProfile
  • Milestone changed from 1.7 to 1.8

No patch. Bumping to 1.8.

#4 @ericlewis
7 years ago

In 1.0.3, the activity update referenced the Extended Profile group that the user edited, not the specific field that the user edited. e.g. 'Eric edited the "base" profile info on his profile'

That seems a bit obtuse to introduce back into core here, rather than getting more granular in the update description. e.g. 'Eric edited the hometown on his profile.' (This is also 9 years of Facebook user experience talking)

Also, currently the xprofile_profile_field_data_updated action is hit even if the value for this field hasn't changed. Wouldn't we want logic here to validate a change has actually been made?

#5 @boonebgorges
7 years ago

That seems a bit obtuse to introduce back into core here

Agree 100%. We'll need phrasing that is pretty general, though, because profile fields can have very different names that we can't predict ("Full Name", "Hometown", "Favorite Ninja Turtles", "Biography", "What is your birthday?", etc etc etc). Maybe something like: "Eric edited '[fieldname]' on their profile". I'm not a huge fan of the singular "their" here, but there's not much we can do given the way English works.

Wouldn't we want logic here to validate a change has actually been made?

Make it so.

@ericlewis
7 years ago

#6 @ericlewis
7 years ago

In 4636.patch, I've reworked a few of the save profile field data functions to use a WP_Error on failure rather than returning false to assist in determining success of updating, and whether fields are truly updated.

#7 follow-up: @boonebgorges
7 years ago

Thanks for the patch, ericlewis.

In general, I'm a big fan of returning WP_Error objects rather than a dummy false value. It's possible (however unlikely) that changing the return value of these functions will break something else, though. For example, if someone runs a check like this:

$success = xprofile_set_field_data( $foo, $bar, $baz );
if ( false === $success ) { // or even ! $success
   echo 'oh noez';
}

It's fairly unlikely that we're doing anything like this in BuddyPress, but could you please grep through the codebase to be sure? It's almost certain that we'll cause problems with at least some plugins, but IMO it's worth it in this case, because the WP_Error objects are just so much better (though I'd like to hear another dev chime in with an opinion on that).

I'm particularly concerned about your 'same_value' return value. This is a case where the function currently returns *true*. Check this out: https://buddypress.trac.wordpress.org/browser/trunk/bp-xprofile/bp-xprofile-classes.php?annotate=blame#L1033 When the value exists, it'll hit the first if case (UPDATE...). Even when the $value is the same as previously, the $last_updated value is *different*, so the UPDATE query returns 1 rather than 0. (Side note: in what is, I believe, a bug introduced in r5643 #4930, the function returns true *even if the last_updated value is the same*. It seems like a bug to me because in this case no "update" has taken place, but then again update_metadata() also appears to return true in similar cases, so maybe I have the wrong intuition here.)

I know you need this functionality to make your activity logic work. So, suggestions welcome. One is to do something like this inside of xprofile_set_field_data():

// Before doing anything, get the existing value
$prev_value = xprofile_get_field_data( $field, $user_id );

// ... do all of the update logic, then at the end of the function

$updated = $field->save();
do_action( 'xprofile_set_field_data', $updated, $field, $user_id, $value, $is_required, $prev_value );
return $updated;

Then you can hook your activity logic to the 'xprofile_set_field_data', and check $value against $prev_value. This maintains backward compatibility for all current return values. (It also makes your WP_Error improvements irrelevant for the current task, though I like them independently.) Other ideas are welcome.

A general note on your use of bp_activity_add(). We definitely want to keep this out of the database class. Use a hook, and if you don't have the hook that you need in the place that you need it, create a new hook (like I did in my suggestion above).

Thanks for your work on this!

Last edited 7 years ago by boonebgorges (previous) (diff)

#8 in reply to: ↑ 7 @ericlewis
7 years ago

Replying to boonebgorges:

It's fairly unlikely that we're doing anything like this in BuddyPress, but could you please grep through the codebase to be sure?

xprofile_set_field_data() is used in a few other spots in the codebase, but only as function calls; the return value isn't used.

#9 @boonebgorges
7 years ago

  • Milestone changed from 1.8 to 1.9

I don't think we're going to be able to solve these issues in the next couple days - it looks like there are some backward compatibility oddities to tackle. Let's look at it for 1.9.

#10 @boonebgorges
7 years ago

  • Milestone changed from 1.9 to 2.0

#11 @boonebgorges
7 years ago

  • Keywords has-patch added; needs-patch removed

I've done some thinking about ericlewis's original patch, and come to a couple conclusions.

  • I may be going a bit overboard, but it does seem that changing return value format of existing functions is going too far, just to do what we're doing here.
  • We should have a throttle, so that you don't get more than one update for a certain period of time (I chose 2 hours)
  • Field-by-field updates are not helpful, and the extra mechanisms needed for lumping them into a single update ("Boone updated Hometown, Display Name...") would add a lot of overhead for not much gain.
  • I think we'd annoy a lot of people with the "Eric updated their profile" syntax.

4636.02.patch addresses these concerns. I went with "Eric's profile was updated", which is the best way I could think of in English to say it without resorting to user gender. Updates are throttled in two-hour chunks.

Feedback welcome.

#12 @henry.wright
7 years ago

Hi Boone - Just took the liberty of looking through patch 4636.02. Is it worth thinking about profile field privacy settings. For example, should it be made public knowledge that a private field has been updated?

Perhaps a check could be done before xprofile_record_activity is called?

#13 @boonebgorges
7 years ago

  • Keywords needs-refresh added; has-patch removed

Thanks for the feedback, henry.wright. With 4636.02, there's no mechanism in place to test which fields have been successfully updated. It doesn't check to see which (or even whether) fields have changed since before the update. I guess I was thinking that there weren't many instances where someone would go and save their profile without updating anything, so I figured it wasn't worth the check. But I guess I'd forgotten about private fields. It does seem likely that someone would go and update only a private field, and this would make the activity update confusing at best.

So, I guess back to the drawing board. But the question remains of how to treat private fields. I guess the logic is something like this:

  • If a user has updated at least one public field, post an activity update
  • If user has updated no fields, or if all the fields updated are less than completely public, don't post an update

The way our hide_sitewide flag works, I think it'll be impossible to do anything more fine-grained.

#14 @henry.wright
7 years ago

Sorry for throwing the spanner in the works!

I guess the logic is something like this:

  • If a user has updated at least one public field, post an activity update
  • If user has updated no fields, or if all the fields updated are less than completely public, don't post an update

This seems logical to me.

#15 @boonebgorges
7 years ago

  • Keywords has-patch needs-testing added; needs-refresh removed

4636.03.patch adds the logic for checking for unchanged and/or private fields. As before, I've left all the old functions alone, and simply queried the old data at the time of profile edit. Make sure you update to the latest trunk to test, as it uses a function I'm about to commit.

#16 @boonebgorges
7 years ago

In 7836:

Introduce xprofile_get_field_visibility_level()

This is a simple function for grabbing the visibility level attached to a given
field by a specific user, sensitive to admin-provided defaults and override
settings.

See #4636

#17 @boonebgorges
7 years ago

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

In 7998:

Reinstate updated_profile activity item

Once upon a time, a user's activity stream was updated when that user updated
his/her profile. This changeset reintroduces (and rethinks) that functionality.

  • New phrasing is "Boone's profile was updated", to avoid gendered pronouns in English
  • updated_profile items are limited to one per user per two-hour period
  • Activity items are posted only when at least one publicly-visible xprofile field has been changed

Fixes #4636

Props ericlewis, boonebgorges

#18 @boonebgorges
7 years ago

In 8035:

Don't remove 'updated_profile' from registered activity actions

This activity type was reintroduced in r7998. See #4636

Note: See TracTickets for help on using tickets.