Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 10 years ago

#7371 closed enhancement (fixed)

No action for group photo upload?

Reported by: bhargavbhandari90 Owned by: boonebgorges
Priority: normal Milestone: 2.8
Component: Groups Version: 2.7.2
Severity: normal Keywords: has-patch 2nd-opinion needs-testing
Cc:

Description

We have this action for user profile picture upload function "bp_avatar_ajax_set()" into “buddypress/bp-core/bp-core-avatars.php”

do_action( 'xprofile_avatar_uploaded', (int) $avatar_data['item_id'], $avatar_data['type'] );

This action is only for user photo upload.

What if we want to add some hook for group photo..?

Attachments (6)

7371.diff (1.5 KB ) - added by boonebgorges 10 years ago.
7371-1.patch (2.5 KB ) - added by bhargavbhandari90 10 years ago.
7371-2.patch (2.4 KB ) - added by bhargavbhandari90 10 years ago.
Some code formatting.
7371-3.patch (1.7 KB ) - added by bhargavbhandari90 10 years ago.
Changed action name and added new parameter
7371-group-create.diff (2.2 KB ) - added by jdgrimes 10 years ago.
Also fire the action when an avatar is uploaded as part of group creation
7371-typo.diff (789 bytes ) - added by jdgrimes 10 years ago.
Fix a typo: s/atavar_data/avatar_data/

Download all attachments as: .zip

Change History (25)

@boonebgorges
10 years ago

#1 follow-up: @boonebgorges
10 years ago

  • Component CoreGroups
  • Keywords has-patch added
  • Milestone Future Release2.8
  • Priority highnormal

Hi @bhargavbhandari90 - Thanks for the ticket! You're right that we should have an action here.

7371.diff introduces a new action in the AJAX and non-AJAX contexts. I don't love the action name 'groups_avatar_uploaded', but it parallels the existing user profile action.

@bhargavbhandari90 Does it seem like this would serve your purpose?

#2 in reply to: ↑ 1 @bhargavbhandari90
10 years ago

Replying to boonebgorges:

Hi @bhargavbhandari90 - Thanks for the ticket! You're right that we should have an action here.

7371.diff introduces a new action in the AJAX and non-AJAX contexts. I don't love the action name 'groups_avatar_uploaded', but it parallels the existing user profile action.

@bhargavbhandari90 Does it seem like this would serve your purpose?

Thank you @boonebgorges . It's good one.

But I have second thought on this.

What if we send whole array to that action instead of those two parameters only?

I am also giving my patch for this, so let me know if it's possible or not.

And I also don't like the action name 'groups_avatar_uploaded', so renamed action name is also in the patch.

#3 @bhargavbhandari90
10 years ago

  • Keywords 2nd-opinion needs-testing added

@bhargavbhandari90
10 years ago

Some code formatting.

#4 follow-up: @boonebgorges
10 years ago

Thanks for the patch, @bhargavbhandari90. A few comments:

  • I agree that it would be good to send the whole array to the action. However, we can't change the existing parameters sent to the action, as this change will break existing uses. We could add the whole array as a new parameter.\
  • The new action appears in two places: groups_screen_group_admin_avatar() and bp_avatar_ajax_set(). The parameters passed to the function must be the same (or at least compatible) in each of these places.
  • The name 'xgroup_avatar_uploaded' doesn't match our naming conventions. 'xprofile' is short for 'Extended Profile'. I'd prefer somethting like 'bp_groups_uploaded_avatar', but this is fairly different from 'xprofile_avatar_uploaded', so I feel like we should just go with 'groups_avatar_uploaded'.

#5 in reply to: ↑ 4 @bhargavbhandari90
10 years ago

Replying to boonebgorges:

Thanks for the patch, @bhargavbhandari90. A few comments:

  • I agree that it would be good to send the whole array to the action. However, we can't change the existing parameters sent to the action, as this change will break existing uses. We could add the whole array as a new parameter.\
  • The new action appears in two places: groups_screen_group_admin_avatar() and bp_avatar_ajax_set(). The parameters passed to the function must be the same (or at least compatible) in each of these places.
  • The name 'xgroup_avatar_uploaded' doesn't match our naming conventions. 'xprofile' is short for 'Extended Profile'. I'd prefer somethting like 'bp_groups_uploaded_avatar', but this is fairly different from 'xprofile_avatar_uploaded', so I feel like we should just go with 'groups_avatar_uploaded'.

Ok. Agree with you. So for action name I'll go with the name 'groups_avatar_uploaded'.

And I added that third parameter which will be array as I said before. I checked that and will not make harm to existing users.

@bhargavbhandari90
10 years ago

Changed action name and added new parameter

#6 @bhargavbhandari90
10 years ago

  • Milestone 2.82.7.4

@boonebgorges I am changing the milestone. I think this should be in next version. What you say?

Last edited 10 years ago by bhargavbhandari90 (previous) (diff)

#7 follow-up: @boonebgorges
10 years ago

  • Milestone 2.7.42.8
  • Owner set to boonebgorges
  • Status newreviewing

Thanks for the updated patch, @bhargavbhandari90. I'll look more closely at it in the upcoming days.

The 2.7.4 release is only for security fixes and regressions. Your request is for an enhancement, so it belongs in the 2.8 milestone. 2.8 is due out in late January 2017.

#8 in reply to: ↑ 7 @bhargavbhandari90
10 years ago

Replying to boonebgorges:

Thanks for the updated patch, @bhargavbhandari90. I'll look more closely at it in the upcoming days.

The 2.7.4 release is only for security fixes and regressions. Your request is for an enhancement, so it belongs in the 2.8 milestone. 2.8 is due out in late January 2017.

@boonebgorges All right buddy. Thanks for the update. :-)

#9 @boonebgorges
10 years ago

@bhargavbhandari90 Thank you for the updated patch!

#10 @boonebgorges
10 years ago

  • Resolutionfixed
  • Status reviewingclosed

In 11314:

Improve actions fired after avatar upload.

  • Introduce the groups_avatar_uploaded action, which paralells the xprofile_avatar_uploaded filter, and runs after either an AJAX or a non-AJAX group avatar upload.
  • Update instances of the xprofile_avatar_uploaded action so that they receive the array of arguments passed to the upload/crop handler.
  • Improve documentation for existing actions.

Props bhargavbhandari90.
Fixes #7371.

#11 @bhargavbhandari90
10 years ago

@boonebgorges Thank you.

Last edited 10 years ago by bhargavbhandari90 (previous) (diff)

#12 @Offereins
10 years ago

  • Resolution fixed
  • Status closedreopened

@boonebgorges I noticed your commit does not include the $avatar_data parameter in the documented version of the xprofile_avatar_uploaded action. It is in the docblock, but not in the function call.

#13 @boonebgorges
10 years ago

  • Resolutionfixed
  • Status reopenedclosed

In 11315:

After [11314], ensure that promised data is passed to xprofile_avatar_uploaded action.

Props Offereins.
Fixes #7371.

@jdgrimes
10 years ago

Also fire the action when an avatar is uploaded as part of group creation

#14 @jdgrimes
10 years ago

  • Resolution fixed
  • Status closedreopened

I was just looking for an action like this, and I stumbled upon this ticked. I realized though, that avatars can also be uploaded as part of the group creation process, and it looks like this action isn't being fired there. I think that probably this was just an oversight, so I've uploaded 7371-group-create.diff, which fires the action in groups_action_create_group() as well.

@jdgrimes
10 years ago

Fix a typo: s/atavar_data/avatar_data/

#15 follow-up: @jdgrimes
10 years ago

I've also just discovered that there is a typo in bp_avatar_ajax_set(), after getting "Undefined variable: atavar_data". See 7371-typo.diff.

#16 in reply to: ↑ 15 @bhargavbhandari90
10 years ago

Replying to jdgrimes:

I've also just discovered that there is a typo in bp_avatar_ajax_set(), after getting "Undefined variable: atavar_data". See 7371-typo.diff.

Good one. We need to check the entire avatar system now.

#17 @DJPaul
10 years ago

The typo is only two weeks old; introduced in r11314 with the patch in this very ticket.

#18 @djpaul
10 years ago

In 11392:

Groups: fix typo in filter arg.

See #7371

Props jdgrimes

#19 @djpaul
10 years ago

  • Resolutionfixed
  • Status reopenedclosed

In 11393:

Groups: fire groups_avatar_uploaded action after uploading avatar during group creation process.

Fixes #7371

Props jdgrimes

Note: See TracTickets for help on using tickets.