Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-20892 Create Opportunistic locking mechanism in the CiviMail API… #10965

Merged
merged 3 commits into from
Oct 5, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ang/crmMailing/services.js
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@
});
delete params.recipients; // the content was merged in
return qApi('Mailing', 'create', params).then(function(result) {
mailing.modified_date = result.values[result.id].modified_date;
// changes rolled back, so we don't care about updating mailing
return result.values[result.id]['api.Mailing.preview'].values;
});
Expand Down Expand Up @@ -304,6 +305,7 @@
delete params.recipients; // the content was merged in
return qApi('Mailing', 'create', params).then(function (recipResult) {
// changes rolled back, so we don't care about updating mailing
mailing.modified_date = recipResult.values[recipResult.id].modified_date;
return recipResult.values[recipResult.id]['api.MailingRecipients.get'].values;
});
},
Expand All @@ -323,6 +325,7 @@
delete params.recipients; // the content was merged in
return qApi('Mailing', 'create', params).then(function (recipResult) {
// changes rolled back, so we don't care about updating mailing
mailing.modified_date = recipResult.values[recipResult.id].modified_date;
return recipResult.values[recipResult.id]['api.MailingRecipients.getcount'];
});
},
Expand Down Expand Up @@ -354,6 +357,7 @@
mailing.id = result.id;
} // no rollback, so update mailing.id
// Perhaps we should reload mailing based on result?
mailing.modified_date = result.values[result.id].modified_date;
return mailing;
});
},
Expand Down Expand Up @@ -405,6 +409,7 @@
if (result.id && !mailing.id) {
mailing.id = result.id;
} // no rollback, so update mailing.id
mailing.modified_date = result.values[result.id].modified_date;
return result.values[result.id]['api.Mailing.send_test'].values;
});
}
Expand Down
9 changes: 8 additions & 1 deletion api/v3/Mailing.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,17 @@ function civicrm_api3_mailing_create($params) {
else {
$safeParams = $params;
}
$timestampCheck = TRUE;
if (!empty($params['id']) && !empty($params['modified_date'])) {
$timestampCheck = _civicrm_api3_compare_timestamps($safeParams['modified_date'], $safeParams['id'], 'Mailing');
unset($safeParams['modified_date']);
}
if (!$timestampCheck) {
throw new API_Exception("Mailing has not been saved, Content maybe out of date, please refresh the page and try again");
}
$safeParams['_evil_bao_validator_'] = 'CRM_Mailing_BAO_Mailing::checkSendable';
$result = _civicrm_api3_basic_create(_civicrm_api3_get_BAO(__FUNCTION__), $safeParams);
return _civicrm_api3_mailing_get_formatResult($result);

}

/**
Expand Down
15 changes: 15 additions & 0 deletions api/v3/utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -2504,3 +2504,18 @@ function _civicrm_api3_check_edit_permissions($bao_name, $params) {
}
}
}

/**
* Check if an entity has been modified since the last known modified_date
* @param string $modifiedDate Last knowm modified_date
* @param int $id Id of record to check
* @param string $entity API Entity
* @return bool
*/
function _civicrm_api3_compare_timestamps($modifiedDate, $id, $entity) {
$currentDbInfo = civicrm_api3($entity, 'getsingle', array('id' => $id));
if (strtotime($currentDbInfo['modified_date']) <= strtotime($modifiedDate)) {
return TRUE;
}
return FALSE;
}
14 changes: 14 additions & 0 deletions tests/phpunit/api/v3/MailingTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -879,4 +879,18 @@ public function testTrackableURLWithUnicodeSign() {
$this->assertContains($unicodeURL, $url);
}

/**
* CRM-20892 : Test if Mail.create API throws error on update,
* if modified_date less then the date when the mail was last updated/created
*/
public function testModifiedDateMismatchOnMailingUpdate() {
$mail = $this->callAPISuccess('mailing', 'create', $this->_params + array('modified_date' => 'now'));
try {
$this->callAPISuccess('mailing', 'create', $this->_params + array('id' => $mail['id'], 'modified_date' => '2 seconds ago'));
Copy link
Member

Choose a reason for hiding this comment

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

'modified_date' => '2 seconds ago' ==> clever :)

}
catch (Exception $e) {
$this->assertRegExp("/Failure in api call for mailing create: Mailing has not been saved, Content maybe out of date, please refresh the page and try again/", $e->getMessage());
}
}

}
11 changes: 11 additions & 0 deletions tests/phpunit/api/v3/UtilsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -432,4 +432,15 @@ public function testBasicArrayGetReturn() {
), $r3['values']);
}

/**
* CRM-20892 Add Tests of new timestamp checking function
*/
public function testTimeStampChecking() {
CRM_Core_DAO::executeQuery("INSERT INTO civicrm_mailing (id, modified_date) VALUES (25, '2016-06-30 12:52:52')");
$this->assertTrue(_civicrm_api3_compare_timestamps('2017-02-15 16:00:00', 25, 'Mailing'));
$this->callAPISuccess('Mailing', 'create', array('id' => 25, 'subject' => 'Test Subject'));
$this->assertFalse(_civicrm_api3_compare_timestamps('2017-02-15 16:00:00', 25, 'Mailing'));
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, there should more of an integration-test that actually triggers and catches the exception.

$this->callAPISuccess('Mailing', 'delete', array('id' => 25));
}

}