Fix #14558: Monitors get lost when calling mc_issue_update via SOAP

rombert · rombert · commit 9a8d41dd2dd0 · 2012-08-11T00:47:08.000+03:00
diff --git a/api/soap/mc_issue_api.php b/api/soap/mc_issue_api.php
@@ -341,44 +341,48 @@ function mci_issue_get_notes( $p_issue_id ) {
  * @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 {
 			if ( !access_has_bug_level( config_get( 'monitor_add_others_bug_threshold' ), $p_issue_id ) )
 				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 {
 			if ( !access_has_bug_level( config_get( 'monitor_delete_others_bug_threshold' ), $p_issue_id ) )
 				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);
diff --git a/tests/soap/IssueUpdateTest.php b/tests/soap/IssueUpdateTest.php
@@ -510,4 +510,35 @@ public function testUpdateWithTagOperations() {
 		$issue = $this->client->mc_issue_get( $this->userName, $this->password, $issueId );
 		self::assertEquals(0, count ( $issue->tags ) );
 	}
+	
+	public function testUpdateWithMonitors() {
+		
+		// create issue
+		$issueToAdd = $this->getIssueToAdd( 'IssueUpdateTest.testUpdateWithMonitors' );
+		$issueId = $this->client->mc_issue_add( $this->userName, $this->password, $issueToAdd);
+		$this->deleteAfterRun( $issueId );
+		
+		// fresh issue -> no monitors exist
+		$issue = $this->client->mc_issue_get( $this->userName, $this->password, $issueId );
+		self::assertEquals(0, count ( $issue->monitors ));
+		
+		// update with this user as monitor -> should be added
+		$issue->monitors = array ( array ( 'id' => $this->userId));
+		$this->client->mc_issue_update ( $this->userName, $this->password, $issueId, $issue);
+		$issue = $this->client->mc_issue_get( $this->userName, $this->password, $issueId);
+		self::assertEquals( 1, count($issue->monitors) );
+		self::assertEquals($this->userId, $issue->monitors[0]->id );
+		
+		// update with same monitor list -> should be preserved
+		$this->client->mc_issue_update ( $this->userName, $this->password, $issueId, $issue);
+		$issue = $this->client->mc_issue_get( $this->userName, $this->password, $issueId);
+		self::assertEquals( 1, count($issue->monitors) );
+		self::assertEquals($this->userId, $issue->monitors[0]->id );
+		
+		// update with empty monitor list -> should be removed
+		$issue->monitors = array();
+		$this->client->mc_issue_update ( $this->userName, $this->password, $issueId, $issue);
+		$issue = $this->client->mc_issue_get( $this->userName, $this->password, $issueId);
+		self::assertEquals( 0, count($issue->monitors) );
+	}
 }
