0014558: Monitors get lost when calling mc_issue_update via SOAP - MantisBT - Signup temporarily disabled due to spam

ID	Project	Category	View Status	Date Submitted	Last Update

0014558	mantisbt	api soap	public	2012-08-07 02:31	2013-04-06 08:15

Reporter	schreibm	Assigned To	rombert
Priority	normal	Severity	minor	Reproducibility	always
Status	closed	Resolution	fixed
Product Version	1.2.5
Target Version	1.2.12	Fixed in Version	1.2.12

Summary	0014558: Monitors get lost when calling mc_issue_update via SOAP
Description	When I want to update a field of an issue via SOAP, the monitors of that issue get lost. The function "mc_issue_update" is calling "mci_issue_set_monitors" (line 829). Within the "mci_issue_set_monitors" the ids of the monitors are extracted and for each id the method "bug_monitor( $p_issue_id, $t_user_id);" is called to add all monitors that are part of the update issue SOAP body. After adding all, all previously existing monitors are removed by calling "bug_unmonitor( $p_issue_id, $t_user_id);". The "bug_monitor" function is checking if the user that should be added as monitor is already monitoring the bug and does not add it in that case. IMHO the issue here is, that first all monitors from the SOAP call are added (in fact they are not added if they already are monitoring the issue) and after that all existing monitors are removed. I guess it should be the other way around: First remove all existing monitors and than add all that are passed with the SOAP Body. Find attached a patch that should fix the issue.
Tags	No tags attached.
Attached Files	monitors.patch (1,654 bytes) Index: api/soap/mc_issue_api.php =================================================================== --- api/soap/mc_issue_api.php (revision 173162) +++ api/soap/mc_issue_api.php (working copy) @@ -342,7 +342,23 @@ $t_monitors = array(); foreach ( $p_monitors as $t_monitor ) $t_monitors[] = $t_monitor['id']; - + + foreach ( $t_existing_monitors as $t_user_id ) { + + if ( $p_user_id == $t_user_id ) { + if ( ! access_has_bug_level( config_get( 'monitor_bug_threshold' ), $p_issue_id ) ) + continue; + } else { + if ( !access_has_bug_level( config_get( 'monitor_delete_others_bug_threshold' ), $p_issue_id ) ) + continue; + } + + if ( in_array( $p_user_id, $t_monitors) ) + continue; + + bug_unmonitor( $p_issue_id, $t_user_id); + } + foreach ( $t_monitors as $t_user_id ) { if ( $p_user_id == $t_user_id ) { @@ -359,21 +375,7 @@ bug_monitor( $p_issue_id, $t_user_id); } - foreach ( $t_existing_monitors as $t_user_id ) { - if ( $p_user_id == $t_user_id ) { - if ( ! access_has_bug_level( config_get( 'monitor_bug_threshold' ), $p_issue_id ) ) - continue; - } else { - if ( !access_has_bug_level( config_get( 'monitor_delete_others_bug_threshold' ), $p_issue_id ) ) - continue; - } - - if ( in_array( $p_user_id, $t_monitors) ) - continue; - - bug_unmonitor( $p_issue_id, $t_user_id); - } } monitors.patch (1,654 bytes) 14558.patch (1,793 bytes) diff --git a/api/soap/mc_issue_api.php b/api/soap/mc_issue_api.php index 57aa6ca..c40a0bb 100644 --- a/api/soap/mc_issue_api.php +++ b/api/soap/mc_issue_api.php @@ -352,38 +352,42 @@ function mci_issue_set_monitors( $p_issue_id , $p_user_id, $p_monitors ) { foreach ( $p_monitors as $t_monitor ) $t_monitors[] = $t_monitor['id']; - foreach ( $t_monitors as $t_user_id ) { + // unmonitor existing monitors + foreach ( $t_existing_monitors as $t_user_id ) { if ( $p_user_id == $t_user_id ) { if ( ! access_has_bug_level( config_get( 'monitor_bug_threshold' ), $p_issue_id ) ) continue; } else { - if ( !access_has_bug_level( config_get( 'monitor_add_others_bug_threshold' ), $p_issue_id ) ) + if ( !access_has_bug_level( config_get( 'monitor_delete_others_bug_threshold' ), $p_issue_id ) ) continue; } - if ( in_array( $p_user_id, $t_existing_monitors) ) + if ( in_array( $p_user_id, $t_monitors) ) continue; - bug_monitor( $p_issue_id, $t_user_id); + bug_unmonitor( $p_issue_id, $t_user_id); } - - foreach ( $t_existing_monitors as $t_user_id ) { + + // monitor monitors based on the passed in user ids + foreach ( $t_monitors as $t_user_id ) { if ( $p_user_id == $t_user_id ) { if ( ! access_has_bug_level( config_get( 'monitor_bug_threshold' ), $p_issue_id ) ) continue; } else { - if ( !access_has_bug_level( config_get( 'monitor_delete_others_bug_threshold' ), $p_issue_id ) ) + if ( !access_has_bug_level( config_get( 'monitor_add_others_bug_threshold' ), $p_issue_id ) ) continue; } - if ( in_array( $p_user_id, $t_monitors) ) + if ( in_array( $p_user_id, $t_existing_monitors) ) continue; - bug_unmonitor( $p_issue_id, $t_user_id); + bug_monitor( $p_issue_id, $t_user_id); } + + } /** 14558.patch (1,793 bytes)

rombert 2012-08-07 05:52 reporter ~0032487	Thanks for raising this issue. I had hit the problem earlier but I was unable to reproduce. I'll look into your patch in detail. In the meantime you can consider supplying a git-formatted patch, for proper attribution.

schreibm 2012-08-07 13:26 reporter ~0032493 Last edited: 2012-08-07 13:26	I am not a git expert, hope 14558.patch is well formatted

rombert 2012-08-09 16:21 reporter ~0032530	Thanks for the investigation and patch, I appreciate your digging into the code. I've done some digging as well and it seems that the error is that I was checking the wrong user id for monitoring/unmonitoring. I've attached an updated patch which applies on top of master-1.2.x which works for me (tm) and which also uses clearer variable names. I'd appreciate it if you could verify this before I will commit it. Fix-14558-Monitors-get-lost-when-calling-mcissueupda.patch (2,525 bytes) From 9258fc19fa903987584cad2e76800db79e93252c Thu, 9 Aug 2012 23:19:23 +0300 From: Robert Munteanu <robert@lmn.ro> Date: Thu, 9 Aug 2012 23:19:07 +0300 Subject: [PATCH] Fix #14558: Monitors get lost when calling mc_issue_update via SOAP diff --git a/api/soap/mc_issue_api.php b/api/soap/mc_issue_api.php index 51d8668..a575b43 100644 --- a/api/soap/mc_issue_api.php +++ b/api/soap/mc_issue_api.php @@ -341,20 +341,23 @@ * @param array $p_monitors An array of arrays with the <em>id</em> field set to the id * of the users which should monitor this issue. */ -function mci_issue_set_monitors( $p_issue_id , $p_user_id, $p_monitors ) { +function mci_issue_set_monitors( $p_issue_id , $p_requesting_user_id, $p_monitors ) { if ( bug_is_readonly( $p_issue_id ) ) { - return mci_soap_fault_access_denied( $p_user_id, "Issue '$p_issue_id' is readonly" ); + return mci_soap_fault_access_denied( $p_requesting_user_id, "Issue '$p_issue_id' is readonly" ); } - $t_existing_monitors = bug_get_monitors( $p_issue_id ); + // 1. get existing monitor ids + $t_existing_monitor_ids = bug_get_monitors( $p_issue_id ); - $t_monitors = array(); + // 2. build new monitors ids + $t_new_monitor_ids = array(); foreach ( $p_monitors as $t_monitor ) - $t_monitors[] = $t_monitor['id']; + $t_new_monitor_ids[] = $t_monitor['id']; - foreach ( $t_monitors as $t_user_id ) { + // 3. for each of the new monitor ids, add it if it does not already exist + foreach ( $t_new_monitor_ids as $t_user_id ) { - if ( $p_user_id == $t_user_id ) { + if ( $p_requesting_user_id == $t_user_id ) { if ( ! access_has_bug_level( config_get( 'monitor_bug_threshold' ), $p_issue_id ) ) continue; } else { @@ -362,15 +365,16 @@ continue; } - if ( in_array( $p_user_id, $t_existing_monitors) ) + if ( in_array( $t_user_id, $t_existing_monitor_ids) ) continue; bug_monitor( $p_issue_id, $t_user_id); } - foreach ( $t_existing_monitors as $t_user_id ) { + // 4. for each of the existing monitor ids, remove it if it is not found in the new monitor ids + foreach ( $t_existing_monitor_ids as $t_user_id ) { - if ( $p_user_id == $t_user_id ) { + if ( $p_requesting_user_id == $t_user_id ) { if ( ! access_has_bug_level( config_get( 'monitor_bug_threshold' ), $p_issue_id ) ) continue; } else { @@ -378,7 +382,7 @@ continue; } - if ( in_array( $p_user_id, $t_monitors) ) + if ( in_array( $t_user_id, $t_new_monitor_ids) ) continue; bug_unmonitor( $p_issue_id, $t_user_id); Fix-14558-Monitors-get-lost-when-calling-mcissueupda.patch (2,525 bytes)

schreibm 2012-08-10 04:52 reporter ~0032535	I verified your patch. You can commit it, it works fine!

rombert 2012-08-10 17:48 reporter ~0032540	Thanks for the confirmation.

grangeway 2013-04-05 17:56 reporter ~0036151	Marking as 'acknowledged' not resolved/closed to track that change gets ported to master-2.0.x branch

MantisBT: master-1.2.x 05f7c33c 2012-08-10 10:41 rombert Details Diff	Fix 0014558: Monitors get lost when calling mc_issue_update via SOAP		Affected Issues 0014558
	mod - api/soap/mc_issue_api.php		Diff File
	mod - tests/soap/IssueUpdateTest.php		Diff File
MantisBT: master 9a8d41dd 2012-08-10 10:41 rombert Details Diff	Fix 0014558: Monitors get lost when calling mc_issue_update via SOAP		Affected Issues 0014558
	mod - api/soap/mc_issue_api.php		Diff File
	mod - tests/soap/IssueUpdateTest.php		Diff File