Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 05f7c33

Browse files
rombertrombert
committedAug 10, 2012
Fix #14558: Monitors get lost when calling mc_issue_update via SOAP
·
release-1.2.20release-1.2.12
1 parent 2fe465b commit 05f7c33

File tree

2 files changed

+46
-11
lines changed

2 files changed

+46
-11
lines changed
 

‎api/soap/mc_issue_api.php

+15-11
Original file line numberDiff line numberDiff line change
@@ -341,44 +341,48 @@ function mci_issue_get_notes( $p_issue_id ) {
341341
* @param array $p_monitors An array of arrays with the <em>id</em> field set to the id
342342
* of the users which should monitor this issue.
343343
*/
344-
function mci_issue_set_monitors( $p_issue_id , $p_user_id, $p_monitors ) {
344+
function mci_issue_set_monitors( $p_issue_id , $p_requesting_user_id, $p_monitors ) {
345345
if ( bug_is_readonly( $p_issue_id ) ) {
346-
return mci_soap_fault_access_denied( $p_user_id, "Issue '$p_issue_id' is readonly" );
346+
return mci_soap_fault_access_denied( $p_requesting_user_id, "Issue '$p_issue_id' is readonly" );
347347
}
348348

349-
$t_existing_monitors = bug_get_monitors( $p_issue_id );
349+
// 1. get existing monitor ids
350+
$t_existing_monitor_ids = bug_get_monitors( $p_issue_id );
350351

351-
$t_monitors = array();
352+
// 2. build new monitors ids
353+
$t_new_monitor_ids = array();
352354
foreach ( $p_monitors as $t_monitor )
353-
$t_monitors[] = $t_monitor['id'];
355+
$t_new_monitor_ids[] = $t_monitor['id'];
354356

355-
foreach ( $t_monitors as $t_user_id ) {
357+
// 3. for each of the new monitor ids, add it if it does not already exist
358+
foreach ( $t_new_monitor_ids as $t_user_id ) {
356359

357-
if ( $p_user_id == $t_user_id ) {
360+
if ( $p_requesting_user_id == $t_user_id ) {
358361
if ( ! access_has_bug_level( config_get( 'monitor_bug_threshold' ), $p_issue_id ) )
359362
continue;
360363
} else {
361364
if ( !access_has_bug_level( config_get( 'monitor_add_others_bug_threshold' ), $p_issue_id ) )
362365
continue;
363366
}
364367

365-
if ( in_array( $p_user_id, $t_existing_monitors) )
368+
if ( in_array( $t_user_id, $t_existing_monitor_ids) )
366369
continue;
367370

368371
bug_monitor( $p_issue_id, $t_user_id);
369372
}
370373

371-
foreach ( $t_existing_monitors as $t_user_id ) {
374+
// 4. for each of the existing monitor ids, remove it if it is not found in the new monitor ids
375+
foreach ( $t_existing_monitor_ids as $t_user_id ) {
372376

373-
if ( $p_user_id == $t_user_id ) {
377+
if ( $p_requesting_user_id == $t_user_id ) {
374378
if ( ! access_has_bug_level( config_get( 'monitor_bug_threshold' ), $p_issue_id ) )
375379
continue;
376380
} else {
377381
if ( !access_has_bug_level( config_get( 'monitor_delete_others_bug_threshold' ), $p_issue_id ) )
378382
continue;
379383
}
380384

381-
if ( in_array( $p_user_id, $t_monitors) )
385+
if ( in_array( $t_user_id, $t_new_monitor_ids) )
382386
continue;
383387

384388
bug_unmonitor( $p_issue_id, $t_user_id);

‎tests/soap/IssueUpdateTest.php

+31
Original file line numberDiff line numberDiff line change
@@ -510,4 +510,35 @@ public function testUpdateWithTagOperations() {
510510
$issue = $this->client->mc_issue_get( $this->userName, $this->password, $issueId );
511511
self::assertEquals(0, count ( $issue->tags ) );
512512
}
513+
514+
public function testUpdateWithMonitors() {
515+
516+
// create issue
517+
$issueToAdd = $this->getIssueToAdd( 'IssueUpdateTest.testUpdateWithMonitors' );
518+
$issueId = $this->client->mc_issue_add( $this->userName, $this->password, $issueToAdd);
519+
$this->deleteAfterRun( $issueId );
520+
521+
// fresh issue -> no monitors exist
522+
$issue = $this->client->mc_issue_get( $this->userName, $this->password, $issueId );
523+
self::assertEquals(0, count ( $issue->monitors ));
524+
525+
// update with this user as monitor -> should be added
526+
$issue->monitors = array ( array ( 'id' => $this->userId));
527+
$this->client->mc_issue_update ( $this->userName, $this->password, $issueId, $issue);
528+
$issue = $this->client->mc_issue_get( $this->userName, $this->password, $issueId);
529+
self::assertEquals( 1, count($issue->monitors) );
530+
self::assertEquals($this->userId, $issue->monitors[0]->id );
531+
532+
// update with same monitor list -> should be preserved
533+
$this->client->mc_issue_update ( $this->userName, $this->password, $issueId, $issue);
534+
$issue = $this->client->mc_issue_get( $this->userName, $this->password, $issueId);
535+
self::assertEquals( 1, count($issue->monitors) );
536+
self::assertEquals($this->userId, $issue->monitors[0]->id );
537+
538+
// update with empty monitor list -> should be removed
539+
$issue->monitors = array();
540+
$this->client->mc_issue_update ( $this->userName, $this->password, $issueId, $issue);
541+
$issue = $this->client->mc_issue_get( $this->userName, $this->password, $issueId);
542+
self::assertEquals( 0, count($issue->monitors) );
543+
}
513544
}

8 commit comments

Comments
 (8)

atrol commented on Aug 11, 2012

@atrol
atrol
on Aug 11, 2012
Member

I like the // line comments, but not our conventions

http://www.mantisbt.org/guidelines.php

Use the # symbol for line commenting, not //

rombert commented on Aug 11, 2012

@rombert
rombert
on Aug 11, 2012
MemberAuthor

atrol commented on Aug 12, 2012

@atrol
atrol
on Aug 12, 2012
Member

Don't get me wrong.
Maybe we should change some of the rules because they don't make sense.
There are hundreds of // in existing code.

At the moment it's even possible to introduce a runtime syntax error
in a stable version by a wrong commit.
So introducing a "wrong" comment isn't really a problem compared to this.

Rules should be checked as early as possible when developing

  1. when writing the code (IDE)
  2. when commiting (hooks)
  3. when pushing
  4. in build process
  5. in delivery process
  6. in installation process
  7. during runtime

As long as we don't check anything, there will always occur commits that
brake rules.

rombert commented on Aug 21, 2012

@rombert
rombert
on Aug 21, 2012
MemberAuthor

atrol commented on Sep 18, 2012

@atrol
atrol
on Sep 18, 2012
Member

Do you have any suggestions on how to improve that? I'm all ears.

Suggestions: yes, enough time to implement: No :-(

A very very small start: A script which could run in nightly build to avoid syntax errors in delivery: https://github.com/atrol/Tools/blob/master/bin/check_php_syntax.sh

This is the right tool to check violations of coding standards: http://pear.php.net/package/PHP_CodeSniffer

dregad commented on Sep 19, 2012

@dregad
dregad
on Sep 19, 2012
Member

The smartest way of doing this, would be to use a pre-commit hook that would take care of e.g. automatic syntax check of php files, and any other tasks we might want to define.

However, the hooks are not distributed and installed automatically - it is the individual responsibility of each developer to set things up (quote from git-scm book: "hooks aren’t transferred with a clone of a project, you must distribute these scripts some other way and then have your users copy them to their .git/hooks directory and make them executable.")

For even more strict control, a pre-receive hook would be even better, but that is not supported by github.

Setting up a job on the server may also be an option, making the checks a part of the nightly build script would be best I think.

git hooks reference: http://git-scm.com/book/en/Customizing-Git-Git-Hooks

dregad commented on Sep 19, 2012

@dregad
dregad
on Sep 19, 2012
Member

I had a look at CodeSniffer docs and it looks like a good tool, but seems like a lot of work to set up the "sniffs" to match our standards, especially considering that "Most of the sniffs written for PHP_CodeSniffer do not support the usage of tabs for indentation and alignment. You can write your own sniffs that check for tabs instead of spaces" (see http://pear.php.net/manual/en/package.php.php-codesniffer.advanced-usage.php)

Maybe we should consider to revise our standards ;-)

And maybe we should move this discussion to the mailing list.

bieli commented on Sep 21, 2012

@bieli
bieli
on Sep 21, 2012

Yes, I'll recommend CodeSniffer for PHP language. I'm using this solution since three years in four companies - we need use consequent coding method.

Please sign in to comment.