Improve logging, text adjustments and test fixes.

Author: AaronGilMartinez

config/install/collabora_online.settings.yml

@@ -13,5 +13,5 @@ cool:
   wopi_proof: true
   # Allow fullscreen - Enabled by default for functionality.
   allowfullscreen: true
-  # New File interval on save.
+  # New file interval on save.
   new_file_interval: 0

config/schema/collabora_online.schema.yml

@@ -29,7 +29,7 @@ collabora_online.settings:
           label: 'Allow full-screen.'
         new_file_interval:
           type: integer
-          label: 'New File interval.'
+          label: 'New file interval.'
 
 key.type.collabora_jwt_hs:
   # The key type has no actual configurable settings, but it still needs a

src/Controller/WopiController.php

@@ -164,16 +164,19 @@ protected function wopiPutFile(MediaInterface $media, FileInterface $file, UserI
       );
 
       // Entity didn't change but file has been replaced.
-      $file->setChangedTime($request_time);
       $file->save();
 
       $this->logger->info(
-        'File entity @file_id source @file_uri was replaced with Collabora.<br>
-  Save reason: @reason<br>',
+        'The file contents for media @media_id were overwritten with Collabora.<br>
+Save reason: @reason<br>
+File: @file_id / @file_uri<br>
+User ID: @user_id',
         [
+          '@media_id' => $media->id(),
+          '@reason' => $save_reason,
           '@file_id' => $file->id(),
           '@file_uri' => $file->getFileUri(),
-          '@reason' => $save_reason,
+          '@user_id' => $user->id(),
         ],
       );
 
@@ -198,14 +201,16 @@ protected function wopiPutFile(MediaInterface $media, FileInterface $file, UserI
       'Media entity @media_id was updated with Collabora.<br>
 Save reason: @reason<br>
 Old file: @old_file_id / @old_file_uri<br>
-New file: @new_file_id / @new_file_uri',
+New file: @new_file_id / @new_file_uri<br>
+User ID: @user_id',
       [
         '@media_id' => $media->id(),
         '@reason' => $save_reason,
         '@old_file_id' => $file->id(),
         '@old_file_uri' => $file->getFileUri(),
         '@new_file_id' => $new_file->id(),
         '@new_file_uri' => $new_file->getFileUri(),
+        '@user_id' => $user->id(),
       ],
     );
 

src/Form/ConfigForm.php

@@ -93,11 +93,13 @@ public function buildForm(array $form, FormStateInterface $form_state): array {
 
     $form['new_file_interval'] = [
       '#type' => 'number',
-      '#title' => $this->t('New File interval'),
+      '#title' => $this->t('Create new file on save after…'),
       '#default_value' => $cool_settings['new_file_interval'] ?? 0,
       '#field_suffix' => $this->t('seconds'),
       '#min' => 0,
-      '#description' => $this->t('Minimum interval of time required to create a new File upon save. A value of 0 will not create any entity.'),
+      '#description' => $this->t('Period during which a save operation will overwrite the existing file, rather than creating a new file.<br>
+If the value is 0, the existing file will always be overwritten, and no new file will be created.<br>
+This applies equally to autosave, the editor\'s save button, and the close button.'),
       '#required' => TRUE,
     ];
 

tests/src/Functional/ConfigFormTest.php

@@ -60,7 +60,7 @@ public function testConfigForm(): void {
     $assert_session->fieldValueEquals('WOPI host URL', 'https://localhost/');
     $assert_session->fieldValueEquals('JWT private key', '');
     $assert_session->fieldValueEquals('Access Token Expiration', '86400');
-    $assert_session->fieldValueEquals('New File interval', '0');
+    $assert_session->fieldValueEquals('Create new file on save after…', '0');
     $assert_session->checkboxNotChecked('Disable TLS certificate check for COOL.');
     $assert_session->checkboxChecked('Verify proof header and timestamp in incoming WOPI requests.');
     $assert_session->checkboxChecked('Allow COOL to use fullscreen mode.');
@@ -94,8 +94,8 @@ public function testConfigForm(): void {
       ->setValue('collabora_test');
     $assert_session->fieldExists('Access Token Expiration')
       ->setValue('3600');
-    $assert_session->fieldExists('New File interval')
-      ->setValue(value: '300');
+    $assert_session->fieldExists('Create new file on save after…')
+      ->setValue('300');
     $assert_session->fieldExists('Disable TLS certificate check for COOL.')
       ->check();
     $assert_session->fieldExists('Verify proof header and timestamp in incoming WOPI requests.')
@@ -112,7 +112,7 @@ public function testConfigForm(): void {
     $assert_session->fieldValueEquals('WOPI host URL', 'http://wopihost.com/');
     $assert_session->fieldValueEquals('JWT private key', 'collabora_test');
     $assert_session->fieldValueEquals('Access Token Expiration', '3600');
-    $assert_session->fieldValueEquals('New File interval', '300');
+    $assert_session->fieldValueEquals('Create new file on save after…', '300');
     $assert_session->checkboxChecked('Disable TLS certificate check for COOL.');
     $assert_session->checkboxNotChecked('Verify proof header and timestamp in incoming WOPI requests.');
     $assert_session->checkboxNotChecked('Allow COOL to use fullscreen mode.');
@@ -123,15 +123,15 @@ public function testConfigForm(): void {
     $assert_session->fieldExists('WOPI host URL')->setValue('');
     $assert_session->fieldExists('JWT private key')->setValue('');
     $assert_session->fieldExists('Access Token Expiration')->setValue('');
-    $assert_session->fieldExists('New File interval')->setValue('');
+    $assert_session->fieldExists('Create new file on save after…')->setValue('');
     $assert_session->fieldExists('Disable TLS certificate check for COOL.')->uncheck();
     $assert_session->fieldExists('Allow COOL to use fullscreen mode.')->uncheck();
     $assert_session->buttonExists('Save configuration')->press();
     $assert_session->statusMessageContains('Collabora Online server URL field is required.', 'error');
     $assert_session->statusMessageContains('WOPI host URL field is required.', 'error');
     $assert_session->statusMessageContains('JWT private key field is required.', 'error');
     $assert_session->statusMessageContains('Access Token Expiration field is required.', 'error');
-    $assert_session->statusMessageContains('New File interval', 'error');
+    $assert_session->statusMessageContains('Create new file on save after… field is required.', 'error');
 
     // Test validation of bad form values.
     $this->drupalGet(Url::fromRoute('collabora-online.settings'));
@@ -140,12 +140,12 @@ public function testConfigForm(): void {
     $assert_session->fieldExists('WOPI host URL')->setValue('any-other-value');
     // Set invalid values for numeric field.
     $assert_session->fieldExists('Access Token Expiration')->setValue('text');
-    $assert_session->fieldExists('New File interval')->setValue('text');
+    $assert_session->fieldExists('Create new file on save after…')->setValue('text');
     $assert_session->buttonExists('Save configuration')->press();
     $assert_session->statusMessageContains('The URL /internal is not valid.', 'error');
     $assert_session->statusMessageContains('The URL any-other-value is not valid.', 'error');
-    $assert_session->statusMessageNotContains('Access Token Expiration must be a number.', 'status');
-    $assert_session->statusMessageNotContains('New File interval', 'status');
+    $assert_session->statusMessageContains('Access Token Expiration must be a number.', 'error');
+    $assert_session->statusMessageContains('Create new file on save after… must be a number.', 'error');
 
     // Test form with no configuration.
     \Drupal::configFactory()->getEditable('collabora_online.settings')->setData([])->save();
@@ -154,7 +154,7 @@ public function testConfigForm(): void {
     $assert_session->fieldValueEquals('WOPI host URL', '');
     $assert_session->fieldValueEquals('JWT private key', '');
     $assert_session->fieldValueEquals('Access Token Expiration', '0');
-    $assert_session->fieldValueEquals('New File interval', '0');
+    $assert_session->fieldValueEquals('Create new file on save after…', '0');
     $assert_session->checkboxNotChecked('Disable TLS certificate check for COOL.');
     $assert_session->checkboxChecked('Verify proof header and timestamp in incoming WOPI requests.');
     $assert_session->checkboxNotChecked('Allow COOL to use fullscreen mode.');

tests/src/Kernel/Controller/WopiControllerTest.php

@@ -143,13 +143,13 @@ public function testWopiPutFile(): void {
   }
 
   /**
-   * Tests new File creation on successful requests.
+   * Tests new file creation on successful requests.
    *
    * @covers ::wopiPutFile
    */
   public function testWopiPutFileNewFile(): void {
     // Change configuration to 300 seconds and attempt a save immediately after.
-    // File was created just before, so no new File.
+    // File was created just before, so no new file.
     $wopi_settings = \Drupal::configFactory()->getEditable('collabora_online.settings');
     $wopi_settings->set('cool.new_file_interval', 300)->save();
     $this->doTestWopiPutFile();
@@ -158,12 +158,12 @@ public function testWopiPutFileNewFile(): void {
     \Drupal::time()->setTime('+100 seconds');
     $this->doTestWopiPutFile();
 
-    // Wait more than 300 seconds to trigger the File creation.
-    \Drupal::time()->setTime('+301 seconds');
-    $this->doTestWopiPutFile(new_file: TRUE);
+    // Wait more than 300 seconds to trigger the file creation.
+    \Drupal::time()->setTime('+305 seconds');
+    $this->doTestWopiPutfile(new_file: TRUE);
 
     // Multiple sequential calls under the configured time does not prevent the
-    // creation of the File.
+    // creation of the file.
     \Drupal::time()->setTime('+295 seconds');
     $this->doTestWopiPutFile();
     \Drupal::time()->setTime('+295 seconds');
@@ -173,12 +173,12 @@ public function testWopiPutFileNewFile(): void {
     \Drupal::time()->setTime('+295 seconds');
     $this->doTestWopiPutFile(new_file: TRUE);
 
-    // Configured interval of 0 won't create any File.
+    // Configured interval of 0 won't create any file.
     $wopi_settings->set('cool.new_file_interval', 0)->save();
     \Drupal::time()->setTime('+5 seconds');
     $this->doTestWopiPutFile();
 
-    // Empty value on the configuration won't create any File.
+    // Empty value on the configuration won't create any file.
     $wopi_settings->set('cool.new_file_interval', '')->save();
     \Drupal::time()->setTime('+5 seconds');
     $this->doTestWopiPutFile();
@@ -193,7 +193,7 @@ public function testWopiPutFileNewFile(): void {
    *   Request headers.
    * @param string $reason_message
    *   Reason message expected to appear in the log and in the revision log.
-   * @param string $new_file
+   * @param bool $new_file
    *   New file is expected.
    */
   protected function doTestWopiPutFile(
@@ -203,6 +203,8 @@ protected function doTestWopiPutFile(
   ): void {
     $new_file_content = "File content " . str_repeat('m', rand(0, 999)) . '.';
     $old_file = $this->loadCurrentMediaFile();
+    $old_file_id = $old_file->id();
+    $old_file_uri = $old_file->getFileUri();
     $this->logger->reset();
     $request = $this->createRequest(
       '/contents',
@@ -230,18 +232,22 @@ protected function doTestWopiPutFile(
     $this->assertSame(strlen($new_file_content), $file->getSize());
 
     if (!$new_file) {
-      // The URI remains the same, and no File entity has been created.
-      $this->assertSame($old_file->id(), $file->id());
-      $this->assertSame($file->getFileUri(), $file->getFileUri());
+      // The URI remains the same, and no file entity has been created.
+      $this->assertSame($old_file_id, $file->id());
+      $this->assertSame($old_file_uri, $file->getFileUri());
 
       $this->assertLogMessage(
         RfcLogLevel::INFO,
-        'File entity @file_id source @file_uri was replaced with Collabora.<br>
-  Save reason: @reason<br>',
+        'The file contents for media @media_id were overwritten with Collabora.<br>
+Save reason: @reason<br>
+File: @file_id / @file_uri<br>
+User ID: @user_id',
         [
+          '@media_id' => $media->id(),
+          '@reason' => $reason_message,
           '@file_id' => $file->id(),
           '@file_uri' => $file->getFileUri(),
-          '@reason' => $reason_message,
+          '@user_id' => $this->user->id(),
         ],
       );
 
@@ -250,7 +256,7 @@ protected function doTestWopiPutFile(
 
     $i = $this->getCounterValue();
     // Assert that a new file was created.
-    $this->assertGreaterThan((int) $old_file->id(), (int) $file->id());
+    $this->assertGreaterThan((int) $old_file_id, (int) $file->id());
     // The file uri is fully predictable in the context of this test.
     // Each new file version gets a new number suffix.
     // There is no repeated suffix like "test_0_0_0_0.txt".
@@ -268,14 +274,16 @@ protected function doTestWopiPutFile(
       'Media entity @media_id was updated with Collabora.<br>
 Save reason: @reason<br>
 Old file: @old_file_id / @old_file_uri<br>
-New file: @new_file_id / @new_file_uri',
+New file: @new_file_id / @new_file_uri<br>
+User ID: @user_id',
       [
         '@media_id' => $this->media->id(),
         '@reason' => $reason_message,
         '@old_file_id' => $old_file->id(),
         '@old_file_uri' => $old_file->getFileUri(),
         '@new_file_id' => $file->id(),
         '@new_file_uri' => $file->getFileUri(),
+        '@user_id' => $this->user->id(),
       ],
     );
   }