CONTRIB-9569: Fix dismissed recordings (#627)

* Added check that recording was successfully retrieved from BBB server before marking it as dismissed because no metadata exists

classes/local/proxy/recording_proxy.php

@@ -131,8 +131,8 @@ public static function update_recording(string $recordid, array $params): bool {
      * @return null|array
      */
     public static function fetch_recording(string $recordingid): ?array {
-        $data = self::fetch_recordings([$recordingid]);
-
+        $result = self::fetch_recordings([$recordingid]);
+        $data = $result['recordings'];
         if (array_key_exists($recordingid, $data)) {
             return $data[$recordingid];
         }
@@ -179,17 +179,18 @@ public static function fetch_recordings(array $keyids = []): array {
 
         // If $ids is empty return array() to prevent a getRecordings with meetingID and recordID set to ''.
         if (empty($keyids)) {
-            return $recordings;
+            return ['recordings' => $recordings, 'unfetchedids' => []];
         }
         $cache = cache::make('mod_bigbluebuttonbn', 'recordings');
         $currentfetchcache = cache::make('mod_bigbluebuttonbn', 'currentfetch');
         $recordings = array_filter($cache->get_many($keyids));
         $missingkeys = array_diff(array_values($keyids), array_keys($recordings));
 
-        $recordings += self::do_fetch_recordings($missingkeys);
+        $result = self::do_fetch_recordings($missingkeys);
+        $recordings += $result['recordings'];
         $cache->set_many($recordings);
         $currentfetchcache->set_many(array_flip(array_keys($recordings)));
-        return $recordings;
+        return ['recordings' => $recordings, 'unfetchedids' => $result['unfetchedids']];
     }
 
     /**
@@ -203,10 +204,9 @@ public static function fetch_recording_by_meeting_id(array $keyids = []): array
 
         // If $ids is empty return array() to prevent a getRecordings with meetingID and recordID set to ''.
         if (empty($keyids)) {
-            return $recordings;
+            return ['recordings' => $recordings, 'unfetchedids' => []];
         }
-        $recordings = self::do_fetch_recordings($keyids, 'meetingID');
-        return $recordings;
+        return self::do_fetch_recordings($keyids, 'meetingID');
     }
 
     /**
@@ -218,61 +218,58 @@ public static function fetch_recording_by_meeting_id(array $keyids = []): array
      */
     private static function do_fetch_recordings(array $keyids = [], string $key = 'recordID'): array {
         $recordings = [];
+        $unfetchedids = [];
         $pagesize = 25;
         while ($ids = array_splice($keyids, 0, $pagesize)) {
-            $fetchrecordings = self::fetch_recordings_page($ids, $key);
-            $recordings += $fetchrecordings;
+            $result = self::fetch_recordings_page($ids, $key);
+            if (!$result['success']) {
+                $unfetchedids = array_merge($unfetchedids, $ids);
+                continue; // Skip this page as the response is not valid but we will keep record of all unfetched ids.
+            }
+            $recordings += $result['recordings'];
         }
-        return $recordings;
+        return ['recordings' => $recordings, 'unfetchedids' => $unfetchedids];
     }
     /**
      * Helper function to fetch a page of recordings from the remote server.
      *
      * @param array $ids
      * @param string $key
-     * @return array
+     * @return array an associative array containing recordings list and fetch success.
      */
     private static function fetch_recordings_page(array $ids, $key = 'recordID'): array {
         // The getRecordings call is executed using a method GET (supported by all versions of BBB).
         $xml = self::fetch_endpoint_xml('getRecordings', [$key => implode(',', $ids), 'state' => 'any']);
 
-        if (!$xml) {
-            return [];
-        }
-
-        if ($xml->returncode != 'SUCCESS') {
-            return [];
-        }
-
-        if (!isset($xml->recordings)) {
-            return [];
-        }
-
-        $recordings = [];
-        // If there were recordings already created.
-        foreach ($xml->recordings->recording as $recordingxml) {
-            $recording = self::parse_recording($recordingxml);
-            $recordings[$recording['recordID']] = $recording;
-            // Check if there are any child.
-            if (isset($recordingxml->breakoutRooms->breakoutRoom)) {
-                $breakoutrooms = [];
-                foreach ($recordingxml->breakoutRooms->breakoutRoom as $breakoutroom) {
-                    $breakoutrooms[] = trim((string) $breakoutroom);
-                }
-                if ($breakoutrooms) {
-                    $xml = self::fetch_endpoint_xml('getRecordings', ['recordID' => implode(',', $breakoutrooms)]);
-                    if ($xml && $xml->returncode == 'SUCCESS' && isset($xml->recordings)) {
-                        // If there were already created meetings.
-                        foreach ($xml->recordings->recording as $subrecordingxml) {
-                            $recording = self::parse_recording($subrecordingxml);
-                            $recordings[$recording['recordID']] = $recording;
+        // Response from the server must be valid.
+        if ($xml && $xml->returncode == 'SUCCESS' && isset($xml->recordings)) {
+            $recordings = [];
+            // If there were recordings already created.
+            foreach ($xml->recordings->recording as $recordingxml) {
+                $recording = self::parse_recording($recordingxml);
+                $recordings[$recording['recordID']] = $recording;
+                // Check if there are any child.
+                if (isset($recordingxml->breakoutRooms->breakoutRoom)) {
+                    $breakoutrooms = [];
+                    foreach ($recordingxml->breakoutRooms->breakoutRoom as $breakoutroom) {
+                        $breakoutrooms[] = trim((string) $breakoutroom);
+                    }
+                    if ($breakoutrooms) {
+                        $xml = self::fetch_endpoint_xml('getRecordings', ['recordID' => implode(',', $breakoutrooms)]);
+                        if ($xml && $xml->returncode == 'SUCCESS' && isset($xml->recordings)) {
+                            // If there were already created meetings.
+                            foreach ($xml->recordings->recording as $subrecordingxml) {
+                                $recording = self::parse_recording($subrecordingxml);
+                                $recordings[$recording['recordID']] = $recording;
+                            }
                         }
                     }
                 }
             }
+            return ['recordings' => $recordings, 'success' => true];
         }
-
-        return $recordings;
+        debugging('getRecordings API call failed', DEBUG_DEVELOPER);
+        return ['recordings' => [], 'success' => false];
     }
 
     /**

classes/recording.php

@@ -700,14 +700,16 @@ protected static function fetch_records(array $selects, array $params): array {
         }, $recordings));
 
         // Fetch all metadata for these recordings.
-        $metadatas = recording_proxy::fetch_recordings($recordingids);
+        $result = recording_proxy::fetch_recordings($recordingids);
+        $metadatas = $result['recordings'];
+        $failedids = $result['unfetchedids'];
 
         // Return the instances.
-        return array_filter(array_map(function ($recording) use ($metadatas, $withindays) {
+        return array_filter(array_map(function ($recording) use ($metadatas, $withindays, $failedids) {
             // Filter out if no metadata was fetched.
             if (!array_key_exists($recording->recordingid, $metadatas)) {
-                // Mark it as dismissed if it is older than 30 days.
-                if ($withindays > $recording->timecreated) {
+                // If the recording was successfully fetched, mark it as dismissed if it is older than 30 days.
+                if (!in_array($recording->recordingid, $failedids) && $withindays > $recording->timecreated) {
                     $recording = new self(0, $recording, null);
                     $recording->set_status(self::RECORDING_STATUS_DISMISSED);
                 }
@@ -795,7 +797,8 @@ public static function sync_pending_recordings_from_server($dismissedonly = fals
 
         // Fetch all metadata for these recordings.
         mtrace("=> Fetching recording metadata from server");
-        $metadatas = recording_proxy::fetch_recordings($recordingids);
+        $result = recording_proxy::fetch_recordings($recordingids);
+        $metadatas = $result['recordings'];
 
         $foundcount = 0;
         foreach ($metadatas as $recordingid => $metadata) {

classes/task/upgrade_recordings_task.php

@@ -74,7 +74,8 @@ protected function process_bigbluebuttonbn_logs(string $meetingid, bool $isimpor
             return false;
         }
         // Retrieve recordings from the servers for this meeting.
-        $recordings = recording_proxy::fetch_recording_by_meeting_id([$meetingid]);
+        $result = recording_proxy::fetch_recording_by_meeting_id([$meetingid]);
+        $recordings = $result['recordings'];
         // Sort recordings by meetingId, then startTime.
         uasort($recordings, function($a, $b) {
             return $b['startTime'] - $a['startTime'];

classes/test/testcase_helper_trait.php

@@ -309,7 +309,8 @@ protected function create_legacy_log_entries(
                 $baselogdata['meetingid'] = $instance->get_meeting_id();
                 if ($importedrecordings) {
                     // Fetch the data.
-                    $data = recording_proxy::fetch_recordings([$recording->recordingid]);
+                    $result = recording_proxy::fetch_recordings([$recording->recordingid]);
+                    $data = $result['recordings'];
                     $data = end($data);
                     if ($data) {
                         $metaonly = array_filter($data, function($key) {

cli/update_dismissed_recordings.php

@@ -105,7 +105,8 @@
     $recordingkeys = array_map(function($rec) {
         return $rec->get('recordingid');
     }, $recordings);
-    $recordingmeta = \mod_bigbluebuttonbn\local\proxy\recording_proxy::fetch_recordings($recordingkeys);
+    $result = \mod_bigbluebuttonbn\local\proxy\recording_proxy::fetch_recordings($recordingkeys);
+    $recordingmeta = $result['recordings'];
     if (empty($recordings)) {
         cli_writeln("\t->No dimissed recordings found ...");
     } else {

tests/local/proxy/recording_proxy_test.php

@@ -46,7 +46,8 @@ public function test_fetch_recordings() {
         $recordingsid = array_map(function ($r) {
             return $r->recordingid;
         }, $recordings);
-        $recordings = recording_proxy::fetch_recordings($recordingsid);
+        $result = recording_proxy::fetch_recordings($recordingsid);
+        $recordings = $result['recordings'];
         $this->assertCount(2, $recordings);
     }
 
@@ -90,7 +91,8 @@ public function test_fetch_recordings_breakoutroom() {
         $recordingsid = array_map(function ($r) {
             return $r->recordingid;
         }, $recordings);
-        $recordings = recording_proxy::fetch_recordings([$recordingsid[0]]);
+        $result = recording_proxy::fetch_recordings([$recordingsid[0]]);
+        $recordings = $result['recordings'];
         $this->assertCount(3, $recordings);
     }
 }