Add tests for ResourceImporter and sample_content (#4276)

.circleci/config.yml

@@ -29,7 +29,7 @@ commands:
         default: '8.2'
         type: string
       addon_branch:
-        description: 'Repo branch name for the dkan-ddev-addon you want to test against.'
+        description: 'Repo branch name for the DKAN DDev add-on you want to test against.'
         default: 'main'
         type: string
       dkan_recommended_branch:

modules/common/src/Util/DrupalFiles.php

@@ -53,7 +53,10 @@ public function __construct(FileSystemInterface $filesystem, StreamWrapperManage
   }
 
   /**
-   * Getter.
+   * Get the Drupal file_system service.
+   *
+   * @returns FileSystemInterface
+   *   The file_system service.
    */
   public function getFilesystem(): FileSystemInterface {
     return $this->filesystem;
@@ -84,6 +87,7 @@ public function retrieveFile($url, $destination) {
       throw new \Exception("Only moving files to Drupal's public directory (public://) is supported");
     }
 
+    // Handle file:// URIs.
     if (substr_count($url, "file://") > 0) {
 
       $src = str_replace("file://", "", $url);
@@ -93,15 +97,17 @@ public function retrieveFile($url, $destination) {
 
       return $this->fileCreateUrl("{$destination}/{$filename}");
     }
-    else {
-      return system_retrieve_file($url, $destination, FALSE, FileSystemInterface::EXISTS_REPLACE);
-    }
+    // Handle http(s):// URIs.
+    return system_retrieve_file($url, $destination, FALSE, FileSystemInterface::EXISTS_REPLACE);
   }
 
   /**
-   * Given a URI like public:// retrieve the URL.
+   * Given a URI like public://, retrieve the http URL.
+   *
+   * @returns string
+   *   The URL.
    */
-  public function fileCreateUrl($uri) {
+  public function fileCreateUrl($uri) : string {
     if (substr_count($uri, 'http') > 0) {
       return $uri;
     }

modules/harvest/src/Transform/ResourceImporter.php

@@ -3,28 +3,48 @@
 namespace Drupal\harvest\Transform;
 
 use Drupal\Core\File\FileSystemInterface;
+use Drupal\Core\File\FileUrlGeneratorInterface;
+use Drupal\common\Util\DrupalFiles;
 use Harvest\ETL\Transform\Transform;
 
 /**
  * Moves local files to public:// and alters the downloadUrl field.
  *
- * @codeCoverageIgnore
+ * Used by the sample_content harvest.
+ *
+ * @see modules/sample_content/harvest_plan.json
  */
 class ResourceImporter extends Transform {
 
   /**
-   * Drupal files.
+   * DKAN's Drupal files service.
    *
    * @var \Drupal\common\Util\DrupalFiles
    */
-  private $drupalFiles;
+  private DrupalFiles $drupalFiles;
+
+  /**
+   * File URL generator service.
+   *
+   * @var \Drupal\Core\File\FileUrlGeneratorInterface
+   */
+  private FileUrlGeneratorInterface $fileUrlGenerator;
+
+  /**
+   * Drupal's file system service.
+   *
+   * @var \Drupal\Core\File\FileSystemInterface
+   */
+  private FileSystemInterface $fileSystem;
 
   /**
    * Constructor.
    */
   public function __construct($harvest_plan) {
     parent::__construct($harvest_plan);
     $this->drupalFiles = \Drupal::service('dkan.common.drupal_files');
+    $this->fileUrlGenerator = \Drupal::service('file_url_generator');
+    $this->fileSystem = \Drupal::service('file_system');
   }
 
   /**
@@ -96,10 +116,6 @@ protected function updateDownloadUrl($dataset, $dist) {
   /**
    * Pulls down external file and saves it locally.
    *
-   * If this method is called when PHP is running on the CLI (e.g. via drush),
-   * `$settings['file_public_base_url']` must be configured in `settings.php`,
-   * otherwise 'default' will be used as the hostname in the new URL.
-   *
    * @param string $url
    *   External file URL.
    * @param string $dataset_id
@@ -108,12 +124,10 @@ protected function updateDownloadUrl($dataset, $dist) {
    * @return string|bool
    *   The URL for the newly created file, or FALSE if failure occurs.
    */
-  public function saveFile($url, $dataset_id) {
-
+  public function saveFile(string $url, string $dataset_id) {
     $targetDir = 'public://distribution/' . $dataset_id;
 
-    $this->drupalFiles
-      ->getFilesystem()
+    $this->fileSystem
       ->prepareDirectory($targetDir, FileSystemInterface::CREATE_DIRECTORY);
 
     // Abort if file can't be saved locally.
@@ -122,11 +136,9 @@ public function saveFile($url, $dataset_id) {
     }
 
     if (is_object($path)) {
-      return \Drupal::service('file_url_generator')->generateAbsoluteString($path->uri->value);
-    }
-    else {
-      return $this->drupalFiles->fileCreateUrl($path);
+      return $this->fileUrlGenerator->generateAbsoluteString($path->uri->value);
     }
+    return $this->drupalFiles->fileCreateUrl($path);
   }
 
 }

modules/harvest/tests/src/Functional/Transform/ResourceImporterTest.php

@@ -0,0 +1,47 @@
+<?php
+
+namespace Drupal\Tests\harvest\Functional\Transform;
+
+use Drupal\harvest\Transform\ResourceImporter;
+use Drupal\Tests\BrowserTestBase;
+
+/**
+ * @covers \Drupal\harvest\Transform\ResourceImporter
+ * @coversDefaultClass \Drupal\harvest\Transform\ResourceImporter
+ *
+ * @group dkan
+ * @group harvest
+ * @group functional
+ * @group btb
+ *
+ * @todo Turn this into a kernel test when we have refactored
+ *   DrupalFiles::retrieveFile to allow for vfsStream.
+ */
+class ResourceImporterTest extends BrowserTestBase {
+
+  protected static $modules = [
+    'datastore',
+    'metastore',
+  ];
+
+  protected $defaultTheme = 'stark';
+
+  /**
+   * @covers ::saveFile
+   */
+  public function testSaveFileHappyPath() {
+    $sample_content_module_path = $this->container->get('extension.list.module')
+      ->getPath('sample_content');
+
+    $importer = new ResourceImporter('harvest_plan_id');
+
+    $this->assertStringContainsString(
+      'distribution/dataset/Bike_Lane.csv',
+      $importer->saveFile(
+        'file://' . $sample_content_module_path . '/files/Bike_Lane.csv',
+        'dataset'
+      )
+    );
+  }
+
+}

modules/harvest/tests/src/Kernel/Transform/ResourceImporterTest.php

@@ -0,0 +1,101 @@
+<?php
+
+namespace Drupal\Tests\harvest\Kernel\Transform;
+
+use Drupal\common\Util\DrupalFiles;
+use Drupal\KernelTests\KernelTestBase;
+use Drupal\harvest\Transform\ResourceImporter;
+
+/**
+ * @covers \Drupal\harvest\Transform\ResourceImporter
+ * @coversDefaultClass \Drupal\harvest\Transform\ResourceImporter
+ *
+ * @group dkan
+ * @group harvest
+ * @group kernel
+ */
+class ResourceImporterTest extends KernelTestBase {
+
+  protected static $modules = [
+    'common',
+  ];
+
+  /**
+   * @covers ::updateDistributions
+   */
+  public function testUpdateDistributions() {
+    // Calling with an empty dataset should result in the same empty dataset.
+    $dataset = (object) [];
+    $importer = new ResourceImporter('harvest_plan_id');
+    $this->assertIsObject($result = $importer->run($dataset));
+    $this->assertEquals($dataset, $result);
+
+    // Calling with a distribution with no download url should result in the
+    // same object being returned.
+    $dataset = (object) [
+      'distribution' => [
+        (object) [
+          'title' => 'my title',
+        ],
+      ],
+    ];
+    $this->assertIsObject($result = $importer->run($dataset));
+    $this->assertEquals($dataset, $result);
+  }
+
+  /**
+   * @covers ::updateDownloadUrl
+   */
+  public function testUpdateDownloadUrl() {
+    // Mock saveFile so we don't actually have to worry about the file system.
+    $importer = $this->getMockBuilder(ResourceImporter::class)
+      ->setConstructorArgs(['harvest_plan_id'])
+      ->onlyMethods(['saveFile'])
+      ->getMock();
+    $importer->expects($this->any())
+      ->method('saveFile')
+      // Adds '_saved' to whatever URL was passed in.
+      ->willReturnCallback(function ($url, $dataset_id): string {
+        return $url . '_saved';
+      });
+
+    // Prepare a dataset with a downloadURL.
+    $dataset = (object) [
+      'identifier' => 'identifier',
+      'distribution' => [
+        (object) [
+          'downloadURL' => 'my_url',
+          'title' => 'my title',
+        ],
+      ],
+    ];
+    $this->assertIsObject($result = $importer->run($dataset));
+    $this->assertEquals(
+      'my_url_saved',
+      $result->distribution[0]->downloadURL ?? 'nope'
+    );
+  }
+
+  /**
+   * @covers ::saveFile
+   */
+  public function testSaveFile() {
+    // When DrupalFiles::retrieveFile fails, saveFile will return FALSE.
+    $drupal_files = $this->getMockBuilder(DrupalFiles::class)
+      ->setConstructorArgs([
+        $this->container->get('file_system'),
+        $this->container->get('stream_wrapper_manager'),
+      ])
+      ->onlyMethods(['retrieveFile'])
+      ->getMock();
+    $drupal_files->expects($this->once())
+      ->method('retrieveFile')
+      ->willReturn(FALSE);
+
+    $this->container->set('dkan.common.drupal_files', $drupal_files);
+
+    $importer = new ResourceImporter('harvest_plan_id');
+    $this->assertFalse($importer->saveFile('any_url', 'any_dataset'));
+  }
+
+}

modules/metastore/metastore.info.yml

@@ -7,3 +7,4 @@ dependencies:
   - dkan:common
   - drupal:basic_auth
   - drupal:content_moderation
+  - drupal:node

modules/sample_content/src/SampleContentService.php

@@ -21,7 +21,7 @@ class SampleContentService {
   private string $modulePath;
 
   /**
-   * Constructor for the Sample Content commands.
+   * Constructor for the Sample Content service.
    *
    * @param \Drupal\Core\Extension\ModuleExtensionList $moduleExtensionList
    *   Extension list.