Opened 8 years ago
Closed 8 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)
Change History (25)
#1
follow-up:
↓ 2
@
8 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
#2
in reply to:
↑ 1
@
8 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.
#4
follow-up:
↓ 5
@
8 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()
andbp_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
@
8 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()
andbp_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.
#6
@
8 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.
#7
follow-up:
↓ 8
@
8 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
@
8 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. :-)
#12
@
8 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.
#14
@
8 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.
#15
follow-up:
↓ 16
@
8 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
@
8 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.
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?