Periodic notifications don't seem to work

live627

  • Should five per cent appear too small / Be thankful I don't take it all / 'Cause I'm the taxman, yeah I'm the taxman
  • Posts: 1,670
A confident man keeps quiet.whereas a frightened man keeps talking, hiding his fear.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: Periodic notifications don't seem to work
« Reply #1, on June 16th, 2013, 02:54 AM »
My bet would be the scheduled task didn't use to fire because of not having the right dependencies (and thus becoming disabled because the function wasn't loaded and therefore not callable)... check that your scheduled_tasks table has the right content as per install.sql and check it's enabled.
When we unite against a common enemy that attacks our ethos, it nurtures group solidarity. Trolls are sensational, yes, but we keep everyone honest. | Game Memorial

live627

  • Should five per cent appear too small / Be thankful I don't take it all / 'Cause I'm the taxman, yeah I'm the taxman
  • Posts: 1,670
Re: Periodic notifications don't seem to work
« Reply #2, on June 16th, 2013, 04:09 AM »
I actually meant here. I should be more clear. :P

@Nao> Please ensure the task has the right parameters in the DB. Also, the task itself must return true in order to not get disabled. :^^;:

Code: [Select]
Index: Sources/Notifications.php
===================================================================
--- Sources/Notifications.php (revision 2166)
+++ Sources/Notifications.php (working copy)
@@ -670,7 +765,7 @@
  continue;
 
  // Assemble the notifications into one huge text.
- $body = template_notification_email($m['notifications']);
+ $body = template_notification_email($m['notifications'], $m['id']);
  $subject = sprintf($txt['notification_email_periodical_subject'], $m['name'], $m['unread']);
 
  sendmail($m['email'], $subject, $body, null, null, true);
Index: Themes/default/Notifications.template.php
===================================================================
--- Themes/default/Notifications.template.php (revision 2166)
+++ Themes/default/Notifications.template.php (working copy)
@@ -64,46 +64,14 @@
 }
 
-function template_notification_email($notifications)
+function template_notification_email($notifications, $member_id)
 {
  global $txt;
 
@@ -111,7 +79,7 @@
 
  foreach ($notifications as $notifier => $notifs)
  {
- list ($title) = weNotif::getNotifiers($notifier)->getProfile();
+ list ($title) = weNotif::getNotifiers($notifier)->getProfile($member_id);
 
  $str .= '
  <h3>' . $title . '</h3>

@Dragooon> Why is this template so complicated? Ideally, the variables should be put into $context in the source file. I think templates/views should never do any logic work, only presentation.

Dragooon

  • I can code! Really!
  • polygon.com has to be one of the best sites I've seen recently.
  • Posts: 1,841
The way it's meant to be

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: Periodic notifications don't seem to work
« Reply #7, on August 1st, 2013, 05:23 PM »
Missed this topic... (As I did many others, obviously!)

Yes, the database structure had yet to be updated; including the size for 'task'.
If it still doesn't work, then... It's something else, I guess.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278

live627

  • Should five per cent appear too small / Be thankful I don't take it all / 'Cause I'm the taxman, yeah I'm the taxman
  • Posts: 1,670
Re: Periodic notifications don't seem to work
« Reply #9, on August 2nd, 2013, 07:06 AM »
Fatal error: Call to undefined method Notification::getEmail() in I:\www\wedge\trunk\Themes\default\Notifications.template.php on line 119


That's when trying to run the schd task.

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: Periodic notifications don't seem to work
« Reply #10, on August 2nd, 2013, 06:36 PM »
I had a quick look, and to be honest with you, I'm not exactly sure where the thing goes wrong... getEmail is a Notifier method, not a Notification one, but then as I said before, Notifiers in general are somehow quite unclear to me, and I tend to easily forget the difference, so... Maybe Dragooon, or someone else, can fix this for me..? :^^;:

live627

  • Should five per cent appear too small / Be thankful I don't take it all / 'Cause I'm the taxman, yeah I'm the taxman
  • Posts: 1,670
Re: Periodic notifications don't seem to work
« Reply #11, on August 3rd, 2013, 12:16 AM »
Is that someone else me? :P

I'm also finding it hard to understand. I've been doing my head in trying to make it work. The emails send, but the data isn't quite right. :^^;:

Nao

  • Dadman with a boy
  • Posts: 16,079

live627

  • Should five per cent appear too small / Be thankful I don't take it all / 'Cause I'm the taxman, yeah I'm the taxman
  • Posts: 1,670
Re: Periodic notifications don't seem to work
« Reply #13, on August 3rd, 2013, 07:39 AM »
Ok, so I've been playing around some more. The biggest issue is that the email body is duplicated since it is HTML. II have yet to understand why.

The above error is because it is calling on the Notification object and wants the getEmail() method from a Notifier.

Code: [Select]
Index: I:/repos/wedge/trunk/Themes/default/Notifications.template.php
===================================================================
--- I:/repos/wedge/trunk/Themes/default/Notifications.template.php (revision 2194)
+++ I:/repos/wedge/trunk/Themes/default/Notifications.template.php (working copy)
@@ -116,9 +116,10 @@
  $str .= "\n" . $title . "\n" . str_repeat('=', strlen($title)) . "\n";
 
  foreach ($notifs as $n)
- $str .= $n->getEmail();
-
- $str .= "\n\n";
+ {
+ list (, $n) = $n->getNotifier()->getEmail($n, array());
+ $str .= $n . "\n\n";
+ }
  }
 
  return $str;

Let's save some memory! $valid_notifiers is only checked for existence, so why not use a bool instead of an object?

Code: [Select]
Index: I:/repos/wedge/trunk/Sources/Notifications.php
===================================================================
--- I:/repos/wedge/trunk/Sources/Notifications.php (revision 2194)
+++ I:/repos/wedge/trunk/Sources/Notifications.php (working copy)
@@ -609,7 +608,7 @@
  {
  $status = isset($data['email_notifiers'][$notifier->getName()]) ? $data['email_notifiers'][$notifier->getName()] : 0;
  if ($status < 2 && (empty($data['disabled_notifiers']) || !in_array($notifier, $data['disabled_notifiers'])))
- $valid_notifiers[$notifier->getName()] = $notifier;
+ $valid_notifiers[$notifier->getName()] = true;
  }
 
  if (empty($valid_notifiers))


Objects shouldn't be this hard to understand. I think it's the duplication that's the problem. Perhaps Notification can go, and its leftovers merged with weNotif. Also, shouldn't Notifier be abstract?

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: Periodic notifications don't seem to work
« Reply #14, on August 3rd, 2013, 07:59 AM »
Last point first, Nao didn't want the Notifier to be completely abstract so that anything common to all notifiers could be extended easily. Quite what would be common to all notifiers, I don't know, I can't think of anything but that's the theory behind it.

The notification system is hard. When I wrote the likes notification I started from one of the others that was done and just hacked it about until it worked. I actually don't understand all the guts of it, I don't think I ever have, actually...