Skip to content

Commit

Permalink
Fix #14558: Monitors get lost when calling mc_issue_update via SOAP
Browse files Browse the repository at this point in the history
  • Loading branch information
@rombert
rombert committed Aug 10, 2012
1 parent 2fe465b commit 05f7c33
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 11 deletions.
26 changes: 15 additions & 11 deletions api/soap/mc_issue_api.php
View file
Expand Up @@ -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);
Expand Down
31 changes: 31 additions & 0 deletions tests/soap/IssueUpdateTest.php
View file
Expand Up @@ -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) );
}
}

8 comments on commit 05f7c33

@atrol
Copy link
Member

@atrol atrol commented on 05f7c33 Aug 11, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Use the # symbol for line commenting, not //

@rombert
Copy link
Member Author

@rombert rombert commented on 05f7c33 Aug 11, 2012 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atrol
Copy link
Member

@atrol atrol commented on 05f7c33 Aug 12, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

@rombert rombert commented on 05f7c33 Aug 21, 2012 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atrol
Copy link
Member

@atrol atrol commented on 05f7c33 Sep 18, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@dregad dregad commented on 05f7c33 Sep 19, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@dregad dregad commented on 05f7c33 Sep 19, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

@bieli bieli commented on 05f7c33 Sep 21, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.