Browse files

Fix tag attach/detach for non-admin users

Take 2, thanks to atrol for keeping his eyes open.
  • Loading branch information...
1 parent 94f4143 commit 46ff26ec28a251817c7fff9992efcb3501477655 @rombert rombert committed Mar 7, 2012
Showing with 2 additions and 2 deletions.
  1. +2 −2 api/soap/mc_tag_api.php
@@ -149,13 +149,13 @@ function mci_tag_set_for_issue ( $p_issue_id, $p_tags, $p_user_id ) {
foreach ( $t_tag_ids_to_detach as $t_tag_id ) {
if ( access_has_bug_level ( config_get('tag_detach_threshold'), $p_issue_id, $p_user_id ) ) {
- tag_bug_detach( $t_tag_id, $p_issue_id, $p_user_id);
+ tag_bug_detach( $t_tag_id, $p_issue_id);
foreach ( $t_tag_ids_to_attach as $t_tag_id ) {
if ( access_has_bug_level ( config_get('tag_attach_threshold'), $p_issue_id, $p_user_id ) ) {
- tag_bug_attach( $t_tag_id, $p_issue_id, $p_user_id);
+ tag_bug_attach( $t_tag_id, $p_issue_id);

3 comments on commit 46ff26e

atrol commented on 46ff26e Mar 7, 2012

It's Ok now, but we would get a better performance if $p_user_id is a parameter for calls of tag_bug_detach and tag_bug_attach to avoid unnecessary calls of auth_get_current_user_id.

tag_bug_detach( $t_tag_id, $p_issue_id, true, $p_user_id);
tag_bug_attach( $t_tag_id, $p_issue_id, $p_user_id);

Maybe too nitpicking ;-)


Maybe, maybe not :-) I actually prefer it this way because method arguments are consistent between the two invocations. That's what 'helped' me create the bug.

As for the actual performance, let's wait for someone to complain :-) My gut feeling is that 2-3 extra calls to get the current user id don't mean much compared to the overhead of SOAP calls.


I haven't reviewed the code, but we should only pass user id to web service calls when there is a scenario to use the web service method and pass in a different id than the caller. Even if we allow that, we may have a config that only allows admins or managers to do such thing, so there may be some authentication / authorization involved.

Please sign in to comment.