ensure user is actually a member of a course when sending notifications, fixes #2023

Closes #2023

Merge request studip/studip!1317

lib/classes/ModulesNotification.class.php

@@ -79,19 +79,24 @@ class ModulesNotification
 
 
 
-    function getAllNotifications ($user_id = NULL)
+    public function getAllNotifications ($user_id = null)
     {
-
-        if (is_null($user_id)) {
+        if ($user_id === null) {
             $user_id = $GLOBALS['user']->id;
         }
 
         $my_sem = [];
-        $query = "SELECT s.Seminar_id, s.Name, s.chdate, s.start_time, IFNULL(visitdate, :threshold) AS visitdate "
-               . "FROM seminar_user_notifications su "
-               . "LEFT JOIN seminare s USING (Seminar_id) "
-               . "LEFT JOIN object_user_visits ouv ON (ouv.object_id = su.Seminar_id AND ouv.user_id = :user_id AND ouv.plugin_id = 0) "
-               . "WHERE su.user_id = :user_id";
+        $query = "SELECT s.Seminar_id, s.Name, s.chdate, s.start_time, IFNULL(visitdate, :threshold) AS visitdate
+                  FROM seminar_user_notifications su
+                  JOIN seminar_user USING (user_id, seminar_id)
+                  JOIN seminare s USING (Seminar_id)
+                  LEFT JOIN object_user_visits ouv
+                    ON (
+                        ouv.object_id = su.Seminar_id
+                        AND ouv.user_id = :user_id
+                        AND ouv.plugin_id = 0
+                    )
+                  WHERE su.user_id = :user_id";
 
         $statement = DBManager::get()->prepare($query);
         $statement->bindValue(':user_id', $user_id);
@@ -101,21 +106,23 @@ class ModulesNotification
             $seminar_id = $row['Seminar_id'];
             $tools = ToolActivation::findbyRange_id($seminar_id);
             $notification = CourseMemberNotification::find([$user_id, $seminar_id]);
+
+            if (!$notification || count($notification->notification_data) === 0) {
+                continue;
+            }
+
             $my_sem[$seminar_id] = [
-                    'name'       => $row['Name'],
-                    'chdate'     => $row['chdate'],
-                    'start_time' => $row['start_time'],
-                    'tools'    => new SimpleCollection($tools),
-                    'visitdate'  => $row['visitdate'],
-                    'notification'=> $notification ? $notification->notification_data->getArrayCopy() : []
+                'name'         => $row['Name'],
+                'chdate'       => $row['chdate'],
+                'start_time'   => $row['start_time'],
+                'tools'        => new SimpleCollection($tools),
+                'visitdate'    => $row['visitdate'],
+                'notification' => $notification->notification_data->getArrayCopy(),
             ];
         }
         $visit_data = get_objects_visits(array_keys($my_sem), 'sem', null, $user_id, array_keys($this->registered_notification_modules));
         $news = [];
         foreach ($my_sem as $seminar_id => $s_data) {
-            if (!count($s_data['notification'])) {
-                continue;
-            }
             $navigation = MyRealmModel::getAdditionalNavigations($seminar_id, $s_data, null, $user_id, $visit_data[$seminar_id]);
             $n_data = [];
             foreach ($this->registered_notification_modules as $id => $m_data) {
@@ -124,11 +131,11 @@ class ModulesNotification
                     && $navigation[$id]->getImage()
                     && $navigation[$id]->getImage()->getRole() === Icon::ROLE_ATTENTION
                 ) {
-                        $data = $this->getPluginText($navigation[$id], $seminar_id, $id);
-                        if ($data) {
-                            $n_data[] = $data;
-                        }
+                    $data = $this->getPluginText($navigation[$id], $seminar_id, $id);
+                    if ($data) {
+                        $n_data[] = $data;
                     }
+                }
             }
             if (count($n_data)) {
                 $news[$s_data['name']] = $n_data;
@@ -149,6 +156,8 @@ class ModulesNotification
             $template_text->set_attribute('sso', $auth_plugin);
             return ['text' => $template_text->render(), 'html' => $template->render()];
         }
+
+        return null;
     }
 
     function getPluginText($nav, $seminar_id, $id)

lib/cronjobs/send_mail_notifications.class.php

@@ -94,40 +94,46 @@ class SendMailNotificationsJob extends CronJob
      */
     public function execute($last_result, $parameters = [])
     {
-        global $user;
+        $notification = new ModulesNotification();
 
-        $cli_user = $user;
+        $query = "SELECT DISTINCT user_id
+                  FROM seminar_user_notifications
+                  JOIN seminar_user USING (user_id, seminar_id)";
+        DBManager::get()->fetchFirst(
+            $query,
+            [],
+            function ($user_id) use ($parameters, $notification) {
+                $user = User::find($user_id);
+                if (
+                    !$user
+                    || $user->locked
+                    || ($user->config->EXPIRATION_DATE > 0 && $user->config->EXPIRATION_DATE < time())
+                ) {
+                    return;
+                }
 
-        $notification = new ModulesNotification();
+                $ok = false;
+                $mailmessage = $notification->getAllNotifications($user->id);
 
-        $query = "SELECT DISTINCT user_id FROM seminar_user_notifications";
+                if ($mailmessage) {
+                    setTempLanguage('', $user->preferred_language);
 
-        $rs = DBManager::get()->query($query);
-        while($r = $rs->fetch()){
-            $user = new Seminar_User($r["user_id"]);
-            if ($user->locked || ($user->cfg->EXPIRATION_DATE > 0 && $user->cfg->EXPIRATION_DATE < time())) {
-                continue;
-            }
-            setTempLanguage('', $user->preferred_language);
-            $to = $user->email;
-            $title = "[" . Config::get()->UNI_NAME_CLEAN . "] " . _("Tägliche Benachrichtigung");
-            $mailmessage = $notification->getAllNotifications($user->id);
-            $ok = false;
-            if ($mailmessage) {
-                if ($user->cfg->getValue('MAIL_AS_HTML')) {
-                    $smail = new StudipMail();
-                    $ok = $smail->setSubject($title)
-                                ->addRecipient($to)
-                                ->setBodyHtml($mailmessage['html'])
-                                ->setBodyText($mailmessage['text'])
-                                ->send();
-                } else {
-                    $ok = StudipMail::sendMessage($to, $title, $mailmessage['text']);
+                    $ok = StudipMail::sendMessage(
+                        $user->email,
+                        "[" . Config::get()->UNI_NAME_CLEAN . "] " . _('Tägliche Benachrichtigung'),
+                        $mailmessage['text'],
+                        $user->config->MAIL_AS_HTML ? $mailmessage['html'] : null
+                    );
+                }
+
+                // Unset user configuration cache to preserve memory
+                UserConfig::set($user->id, null);
+
+                // Log results
+                if ($ok !== false && $parameters['verbose']) {
+                    echo $user->username . ':' . $ok . "\n";
                 }
             }
-            UserConfig::set($user->id, null);
-            if ($ok !== false && $parameters['verbose']) echo $user->username . ':' . $ok . "\n";
-        }
-        $user = $cli_user;
+        );
     }
 }

lib/models/CourseMemberNotification.php

@@ -20,6 +20,8 @@
  * @property string chdate database column
  * @property User user belongs_to User
  * @property Course course belongs_to Course
+ *
+ * @property JSONArrayObject notification_data
  */
 class CourseMemberNotification extends SimpleORMap implements PrivacyObject
 {

lib/models/User.class.php

@@ -68,6 +68,8 @@
  * @property UserInfo   info   has_one UserInfo
  * @property UserOnline online has_one UserOnline
  * @property Kategorie[]|SimpleORMapCollection $profile_categories has_many Kategorie
+ *
+ * @property UserConfig config
  */
 class User extends AuthUserMd5 implements Range, PrivacyObject
 {