From 08f2c4e2420a7b16f9a975fea7664be71e23e4af Mon Sep 17 00:00:00 2001
From: Renaat Debleu <rdebleu@eWallah.net>
Date: Wed, 10 Apr 2024 08:09:06 +0000
Subject: [PATCH] code review

---
 lib.php                                        | 18 +++++++++---------
 ...ucourse.feature => s3bucket_course.feature} |  6 ++++--
 tests/other_test.php                           |  5 ++++-
 3 files changed, 17 insertions(+), 12 deletions(-)
 rename tests/behat/{s3bucket_ucourse.feature => s3bucket_course.feature} (95%)

diff --git a/lib.php b/lib.php
index 716d0ee..7a71368 100644
--- a/lib.php
+++ b/lib.php
@@ -36,8 +36,8 @@
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class repository_s3bucket extends repository {
-    /** @var _s3client s3 client object */
-    private $_s3client;
+    /** @var s3client s3 client object */
+    private $s3client;
 
     /**
      * Get S3 file list
@@ -63,7 +63,7 @@ public function get_listing($path = '.', $page = 1) {
         $s3 = $this->create_s3();
         try {
             $results = $s3->listObjectsV2($options);
-        } catch (Exception $e) {
+        } catch (\Exception $e) {
             throw new \moodle_exception('errorwhilecommunicatingwith', 'repository', '', $this->get_name(), $e->getMessage());
         }
 
@@ -125,7 +125,7 @@ public function search($q, $page = 1) {
         $s3 = $this->create_s3();
         try {
             $results = $s3->listObjectsV2($options);
-        } catch (Exception $e) {
+        } catch (\Exception $e) {
             throw new \moodle_exception('errorwhilecommunicatingwith', 'repository', '', $this->get_name(), $e->getMessage());
         }
 
@@ -190,7 +190,7 @@ public function send_otherfile($reference, $lifetime) {
             try {
                 $result = $s3->getCommand('GetObject', $options);
                 $req = $s3->createPresignedRequest($result, $lifetime);
-            } catch (Exception $e) {
+            } catch (\Exception $e) {
                 throw new \moodle_exception('errorwhilecommunicatingwith', 'repository', '', $this->get_name(), $e->getMessage());
             }
             $uri = $req->getUri()->__toString();
@@ -352,7 +352,7 @@ public static function instance_form_validation($mform, $data, $errors) {
             try {
                 // Check if the bucket exists.
                 $s3->getCommand('HeadBucket', ['Bucket' => $data['bucket_name']]);
-            } catch (Exception $e) {
+            } catch (\Exception $e) {
                 $errors[] = get_string('errorwhilecommunicatingwith', 'repository');
             }
         }
@@ -383,7 +383,7 @@ public function supported_returntypes() {
      * @return s3
      */
     private function create_s3() {
-        if ($this->_s3client == null) {
+        if ($this->s3client == null) {
             $accesskey = $this->get_option('access_key');
             if (empty($accesskey)) {
                 throw new \moodle_exception('needaccesskey', 'repository_s3');
@@ -392,9 +392,9 @@ private function create_s3() {
                 'credentials' => ['key' => $accesskey, 'secret' => $this->get_option('secret_key')],
                 'use_path_style_endpoint' => true,
                 'region' => $this->get_option('endpoint'), ]);
-            $this->_s3client = \Aws\S3\S3Client::factory($arr);
+            $this->s3client = \Aws\S3\S3Client::factory($arr);
         }
-        return $this->_s3client;
+        return $this->s3client;
     }
 
     /**
diff --git a/tests/behat/s3bucket_ucourse.feature b/tests/behat/s3bucket_course.feature
similarity index 95%
rename from tests/behat/s3bucket_ucourse.feature
rename to tests/behat/s3bucket_course.feature
index 8ba61a7..93666e0 100644
--- a/tests/behat/s3bucket_ucourse.feature
+++ b/tests/behat/s3bucket_course.feature
@@ -2,7 +2,9 @@
 Feature: S3 bucket repository is private in user context
 
   Background:
-    Given the following "courses" exist:
+    # TODO: Why is this not working on github actions?
+    Given the site is running Moodle version 4.3 or lower
+    And the following "courses" exist:
       | fullname | shortname |
       | Course 1 | C1        |
       | Course 2 | C2        |
@@ -43,7 +45,7 @@ Feature: S3 bucket repository is private in user context
 
   @_file_upload
   Scenario: A teacher can add files from the s3 bucket repository in module context
-    Given I am on the "folderB" "folder activity editing" page logged in as teacher
+    And I am on the "folderB" "folder activity editing" page logged in as teacher
     And I click on "Add..." "button" in the "Files" "form_row"
     And I should see "Course 1 Bucket"
     And I follow "Course 1 Bucket"
diff --git a/tests/other_test.php b/tests/other_test.php
index 2d692a2..8aa3185 100644
--- a/tests/other_test.php
+++ b/tests/other_test.php
@@ -225,7 +225,7 @@ public function test_pluginfile(): void {
         $cm = get_coursemodule_from_instance('url', $url->id);
         $this->assertFalse(repository_s3bucket_pluginfile($course, $cm, $coursecontext, 'hr', [], true));
         try {
-             \repository_s3bucket_pluginfile(1, $cm, $systemcontext, 's3', [$systemrepo->id, 'tst.jpg'], true);
+            \repository_s3bucket_pluginfile(1, $cm, $systemcontext, 's3', [$systemrepo->id, 'tst.jpg'], true);
         } catch (\Exception $e) {
             $this->assertStringContainsString($headerf, $e->getMessage());
         }
@@ -244,6 +244,9 @@ public function test_pluginfile(): void {
         } catch (\Exception $e) {
             $this->assertStringContainsString($headerf, $e->getMessage());
         }
+        $systemrepo->set_option(['access_key' => null]);
+        $this->expectException('moodle_exception');
+        \repository_s3bucket_pluginfile(1, $cm, $systemcontext, 's3', [$systemrepo->id, 'tst.jpg'], true);
     }
 
     /**