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

Conversation

seamuslee001
Copy link
Contributor

… and angular content to ensure that if content is editied in a separate tab it alerts the user

Overview

Previously CiviCRM Mailings could be edited from multiple tabs at the same time and Users wouldn't have any indication that the content may have changed. Now there is a check to see if the Mailing has become out of date and Alerts the user.

Before

Mailing could be edited in multiple tabs by different people without no notice

After

Message is displayed if the database has been altered since the mailing was last saved from that specific window

Technical Details

This adds opportunistic locking using the API. This is pedant on the PR #10953

Comments

@JKingsnorth @eileenmcnaughton @totten @awzilkie I believe this will solve the problem in just a similar method to what was being proposed in #10864 However using the modifed_date which is added via PR #10953

This also resolves a concern that was raised by Tim by not using Javascript time and dates and just uses information gained from the DAO.

@seamuslee001
Copy link
Contributor Author

Jenkins test this please

$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);
}
Copy link
Member

@totten totten Sep 11, 2017

Choose a reason for hiding this comment

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

Non-blocking style nitpick: the last three lines are nested a level deeper (inside the else {... } block). This is unnecessary.

Consider how the code would look if we added another validation step with strict else{} style. You might get:

if (! valid criterion 1) {
  throw new Exception("failed criterion 1");
} else {
  if (! valid criterion 2) {
    throw new Exception("failed criterion 2");
  }
  else {
    doTheNormalThing();
    doTheOtherNormalThing();
  }
}

IMHO, the code is more malleable (and a bit easier to read) if you just think of exceptions as exceptional -- the exceptional condition can disrupt flow, but the flow otherwise continues normally, eg

if (! valid criterion 1) {
  throw new Exception("failed criterion 1");
}
if (! valid criterion 2) {
  throw new Exception("failed criterion 2");
}

doTheNormalThing();
doTheOtherNormalThing();

In the second case, it's "malleable" because you're free to rearrange the validation/logic steps separately -- without munging with the formatting or SCM records.

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.

@monishdeb
Copy link
Member

@seamuslee001 @totten I have made additional fixes as per comment, here's my patch https://gist.github.com/monishdeb/99b336238198d5b67ce8820f57b823bc

Tested on my local, works fine.

@seamuslee001
Copy link
Contributor Author

@monishdeb thanks for the suggestions, have now updated the PR @totten @eileenmcnaughton

$result = _civicrm_api3_basic_create(_civicrm_api3_get_BAO(__FUNCTION__), $safeParams);
return _civicrm_api3_mailing_get_formatResult($result);
}
$safeParams['_evil_bao_validator_'] = 'CRM_Mailing_BAO_Mailing::checkSendable';
Copy link
Contributor

Choose a reason for hiding this comment

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

was that one Tim or me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure but it was there before

@eileenmcnaughton
Copy link
Contributor

@totten this one seems good to me but I feel you need to be the merger on it

@monishdeb
Copy link
Member

Yup final changes work for me

@eileenmcnaughton
Copy link
Contributor

@totten keen to get your input on this as it is a critical bug. Looking at it the review so far probably is enough but since you have been tagged on it it feels like some small piece of the world will break if we merge without your OK

@JoeMurray
Copy link
Contributor

As a process suggestion I think we need to do more with less @totten ;)

I want more @totten, of course. I'm sure we'd all love to replicate him. But he's overburdened and backlogged. So let's figure out more ways to do things without bottlenecking on his time. In other words, let's merge this without needing him to okay it.

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 :)

@totten
Copy link
Member

totten commented Oct 5, 2017

This seems ready to merge. Notes:

  • It includes multiple tests, including an integration test.
  • There are several supportive comments, including a positive test result from Monish.
    • In the comments, it wasn't clear to me what form of testing was done (eg manual API calls, UI-based testing, both, or something else). Rather than do a round of discussion to clarify, I did little extra testing in the UI (eg by creating a mailing and editing it in two tabs), and that worked out well.
  • I was momentarily concerned that there was a problem because the new tests did not mention the opposite scenario (eg the cases where oplocking isn't around), but there should already by a large body of tests in api_v3_MailingTest, api_v3_JobProcessMailingTest, etal which follow older dataflows.

@totten totten merged commit 8d45518 into civicrm:master Oct 5, 2017
@JKingsnorth
Copy link
Contributor

Yippee, thanks everyone

@eileenmcnaughton eileenmcnaughton deleted the CRM-20892-lock branch October 5, 2017 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants