Mentioned in #1364784: Monthly Interval varies by a few days but hived off due to increasing complexity there.

Here's rough code that's dependent on that issue:


 */
function simplenews_scheduler_scheduler_update($newsletter_parent_data, $now_time) {
  // The node id of the parent node.
  $pid = $newsletter_parent_data->nid;

  // Check if we've passed the number of editions if stop_type is edition count.
  if ($newsletter_parent_data->stop_type == 2) {
    // Get the edition count.
    $edition_count = db_query('SELECT COUNT(*) FROM {simplenews_scheduler_editions} WHERE pid = :pid', array(':pid' => $pid))->fetchField();

    if ($edition_count >= $newsletter_parent_data->stop_edition) {
      // If we're up to the number of editions, deactivate the newsletter.
      $newsletter_parent_data->activated = FALSE;
    }
  }

  // Get the run time for the next edition.
  $next_run = simplenews_scheduler_calculate_next_run_time($newsletter_parent_data, $now_time);

  // Compare it to the stop date.
  if ($newsletter_parent_data->stop_date < $next_run) {
    // If the stop date comes before the next run, deactive the newsletter.
    $newsletter_parent_data->activated = FALSE;
  }
  else {
    // Otherwise, set the next run.
    $newsletter_parent_data->next_run = $next_run;
  }

  // Save the record.
  drupal_write_record('simplenews_scheduler', $newsletter_parent_data, 'nid');
}
CommentFileSizeAuthor
#3 do_stop_calculation_in_next_run.patch8.76 KBberdir

Comments

joachim’s picture

joachim commented

That code's fine in itself but we also need:

- remove checking of stop date and edition count from simplenews_scheduler_get_newsletters_due()
- hook_update_N() to update all records to deactivate them if required. *should* be ok to run them through simplenews_scheduler_scheduler_update().

joachim’s picture

joachim commented
Issue tags: +Needs backport to D6

Tagging.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new do_stop_calculation_in_next_run.patch8.76 KB

Ok, here is a first patch.

Added test coverage for both stop calculation methods and moved them to next_run().

update function is not yet there, as that will conflict with the module weight thing, will add that later on.

Haven't touched the php_eval stuff, a) I'm not sure if that is a stop or a skip thing b) will that be removed in favor of a hook in next_run(). Once that is gone, all that's left in the due() function is the query and a return $result->fetchAssoc('nid');

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Oh, actually haven't really looked at your code, you added it to the update function. That probably makes more sense.

Also not yet sure where that hook would live and how he would work exactly. Basically, he should have the possibility to completely override our default behavior, so maybe a drupal_alter('simplenews_scheduler_scheduler_update', $newsletter_parent_data) just before save, but that would mean that he would have to check both next_run and activated to be sure he's not setting next_run on a disabled scheduler.

That's what I like about having this stuff in calculate_next_time(). There is only one variable to worry about that either has a next_run date or not then the newsletter is disabled. We might want to keep the last next_run time, though.

joachim’s picture

joachim commented
Status: Needs review » Needs work

I'd prefer the checking of stop count do be done in simplenews_scheduler_scheduler_update(), or indeed in another helper function.