Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 4 years ago

#7371 closed enhancement (fixed)

No action for group photo upload?

Reported by: bhargavbhandari90 Owned by: boonebgorges
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.7.2
Component: Groups 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 4 years ago.
7371-1.patch (2.5 KB) - added by bhargavbhandari90 4 years ago.
7371-2.patch (2.4 KB) - added by bhargavbhandari90 4 years ago.
Some code formatting.
7371-3.patch (1.7 KB) - added by bhargavbhandari90 4 years ago.
Changed action name and added new parameter
7371-group-create.diff (2.2 KB) - added by jdgrimes 4 years ago.
Also fire the action when an avatar is uploaded as part of group creation
7371-typo.diff (789 bytes) - added by jdgrimes 4 years ago.
Fix a typo: s/atavar_data/avatar_data/

Download all attachments as: .zip

Change History (25)

@boonebgorges
4 years ago

#1 follow-up: @boonebgorges
4 years ago

  • Component changed from Core to Groups
  • Keywords has-patch added
  • Milestone changed from Future Release to 2.8
  • Priority changed from high to normal

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
4 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
4 years ago

  • Keywords 2nd-opinion needs-testing added

@bhargavbhandari90
4 years ago

Some code formatting.

#4 follow-up: @boonebgorges
4 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
4 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
4 years ago

Changed action name and added new parameter

#6 @bhargavbhandari90
4 years ago

  • Milestone changed from 2.8 to 2.7.4

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

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

#7 follow-up: @boonebgorges
4 years ago

  • Milestone changed from 2.7.4 to 2.8
  • Owner set to boonebgorges
  • Status changed from new to reviewing

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
4 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
4 years ago

@bhargavbhandari90 Thank you for the updated patch!

#10 @boonebgorges
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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
4 years ago

@boonebgorges Thank you.

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

#12 @Offereins
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@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
4 years ago

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

In 11315:

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

Props Offereins.
Fixes #7371.

@jdgrimes
4 years ago

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

#14 @jdgrimes
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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
4 years ago

Fix a typo: s/atavar_data/avatar_data/

#15 follow-up: @jdgrimes
4 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
4 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
4 years ago

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

#18 @djpaul
4 years ago

In 11392:

Groups: fix typo in filter arg.

See #7371

Props jdgrimes

#19 @djpaul
4 years ago

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

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.