Page MenuHomeSoftware Heritage

Immediately download archive if already cooked by the vault
ClosedPublic

Authored by anlambert on Jul 11 2019, 4:51 PM.

Details

Summary

When a requested archive has already been cooked by the vault, we can immediately offer its download to users without redirecting to the vault UI page.

That diff introduces that improvement plus some small changes:

  • rename 'Download' Actions menu entries to as tarball and as git
  • add archive extraction instructions in the modals displayed when selecting menu entries cited above

Related T1858

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

shisus created this revision.Jul 11 2019, 4:51 PM
anlambert retitled this revision from References T1858 to [WIP] Immediately download archive if already cooked by the vault.Jul 23 2019, 3:17 PM
anlambert edited the summary of this revision. (Show Details)
anlambert requested changes to this revision.Jul 23 2019, 6:31 PM

Hi @shisus , sorry for the late review but I was in holidays the last weeks and I just get back into my office.

Thanks for your contribution so far but this requires some improvements for the feature to work.

I added some inline comments regarding what needs to be improved.

Please find below a diff that should help you implement the feature properly.

diff --git a/swh/web/assets/src/bundles/vault/index.js b/swh/web/assets/src/bundles/vault/index.js
index 8393c768..ef18a125 100644
--- a/swh/web/assets/src/bundles/vault/index.js
+++ b/swh/web/assets/src/bundles/vault/index.js
@@ -10,4 +10,3 @@
 import './vault.css';
 export * from './vault-ui';
 export * from './vault-create-tasks';
-export * from './vault-utils.js';
diff --git a/swh/web/assets/src/bundles/vault/vault-create-tasks.js b/swh/web/assets/src/bundles/vault/vault-create-tasks.js
index 5a5f3e1d..d4fbbfe4 100644
--- a/swh/web/assets/src/bundles/vault/vault-create-tasks.js
+++ b/swh/web/assets/src/bundles/vault/vault-create-tasks.js
@@ -6,47 +6,31 @@
  */
 
 import {handleFetchError, csrfPost} from 'utils/functions';
-import {checkUrlHandler} from "./vault-utils";
 
-function existCookingTask(cookingTask) {
-  let vaultCookingTasks = JSON.parse(localStorage.getItem('swh-vault-cooking-tasks'));
-  if (!vaultCookingTasks) {
-    vaultCookingTasks = [];
+function requestArchiveCookingIfNeeded(objectType, objectId) {
+  let vaultUrl;
+  if (objectType === 'directory') {
+    vaultUrl = Urls.api_1_vault_cook_directory(objectId);
+  } else {
+    vaultUrl = Urls.api_1_vault_cook_revision(objectId);
   }
-
-  return vaultCookingTasks.find(val => {
-    return val.object_type === cookingTask.object_type &&
-        val.object_id === cookingTask.object_id;
-  })
-}
-
-export function checkDirectoryArchive(id) {
-  checkVaultCookingTask('directory', id);
+  fetch(vaultUrl)
+    .then(response => response.json())
+    .then(data => {
+      if (data.status !== 'done') {
+        $(`#vault-cook-${objectType}-modal`).modal('show');
+      } else {
+        window.location = data.fetch_url;
+      }
+    });
 }
 
-export function checkRevisionArchive(id) {
-  checkVaultCookingTask('revision', id);
+export function requestDirectoryCookingIfNeeded(directoryId) {
+  requestArchiveCookingIfNeeded('directory', directoryId);
 }
 
-function checkVaultCookingTask(type, id) {
-  const cookingTask = {
-    object_type: type,
-    object_id: id
-  };
-
-  const exists_task = existCookingTask(cookingTask);
-
-  if (!exists_task) {
-    let vaultUrl = checkUrlHandler[cookingTask.object_type](cookingTask.object_id);
-    fetch(vaultUrl)
-    .then(({ body }) => {
-      if (body.status !== 'done') {
-        $(`#vault-cook-${cookingTask.object_type}-modal`).modal('show');
-      } else {
-        window.location = Urls.browse_vault();
-      }
-    });
-  }
+export function requestRevisionCookingIfNeeded(revisionId) {
+  requestArchiveCookingIfNeeded('revision', revisionId);
 }
 
 function addVaultCookingTask(cookingTask) {
diff --git a/swh/web/assets/src/bundles/vault/vault-utils.js b/swh/web/assets/src/bundles/vault/vault-utils.js
deleted file mode 100644
index 277de2fd..00000000
--- a/swh/web/assets/src/bundles/vault/vault-utils.js
+++ /dev/null
@@ -1,5 +0,0 @@
-
-export const checkUrlHandler = {
-  directory: (object_id) => Urls.api_1_vault_cook_directory(object_id),
-  revision: (object_id) => Urls.api_1_vault_cook_revision_gitfast(object_id),
-};
diff --git a/swh/web/templates/includes/vault-create-tasks.html b/swh/web/templates/includes/vault-create-tasks.html
index 952489c3..c47d1e00 100644
--- a/swh/web/templates/includes/vault-create-tasks.html
+++ b/swh/web/templates/includes/vault-create-tasks.html
@@ -16,12 +16,12 @@ See top-level LICENSE file for more information
     </a>
     <div class="dropdown-menu">
       {% if vault_cooking.directory_context %}
-        <button class="dropdown-item" type="button" tabindex="-1" onclick="swh.vault.checkDirectoryArchive('{{ vault_cooking.directory_id }}')">
+        <button class="dropdown-item" type="button" tabindex="-1" onclick="swh.vault.requestDirectoryCookingIfNeeded('{{ vault_cooking.directory_id }}')">
           <i class="{{ swh_object_icons.directory }} fa-fw" aria-hidden="true"></i>Directory
         </button>
       {% endif %}
       {% if vault_cooking.revision_context %}
-        <button class="dropdown-item" type="button" tabindex="-1" onclick="swh.vault.checkRevisionArchive('{{ vault_cooking.revision_id }}')">
+        <button class="dropdown-item" type="button" tabindex="-1" onclick="swh.vault.requestRevisionCookingIfNeeded('{{ vault_cooking.revision_id }}')">
           <i class="{{ swh_object_icons.revision }} fa-fw"></i>Revision
         </button>
       {% endif %}

I also noticed that webpack did not correctly report linting errors for js and css files, I fixed it today.

I invite you to rebase your feature branch on origin/master before integrating the changes detailed above.

Next step: Add a test for that new feature using the cypress tool. This will require mocking some calls to the vault API, I need to think on how to do that first.

swh/web/assets/src/bundles/vault/index.js
13 ↗(On Diff #5804)

This export is not needed

swh/web/assets/src/bundles/vault/vault-create-tasks.js
11–21

In order to check if an object has already been cooked, you need to make a call to the vault/directory or the vault/revision/gitfast endpoints of the Software Heritage public API.

The local storage is only used to track the cooking progress of objects not available in the vault cache.

41–42

This will not work as the fetch response needs to be read and converted to JSON. The correct syntax is:

fetch(vaultUrl)
  .then(response => response.json())
  .then(data => {
  })
46

Here, you must redirect to the URL contained in the fetch_url field from the fetched JSON response.

swh/web/assets/src/bundles/vault/vault-utils.js
1–5 ↗(On Diff #5804)

Considering its size, the content of this file should be merged in the vault-create-task.js one and it should be removed.

This revision now requires changes to proceed.Jul 23 2019, 6:31 PM

Thanks for your insights @anlambert.
I'll continue working on this.

swh/web/assets/src/bundles/vault/index.js
13 ↗(On Diff #5804)

removed

swh/web/assets/src/bundles/vault/vault-create-tasks.js
11–21

I thought when a new cooking task is being created it's added to the local storage. That's why I was checking if the cooking task is already there.

I've created that function to refactor some repeated parts of the code but It might not be a good idea to add it in this Diff.

So, I'll change it to make the API call instead of checking the cooking task existence in the local storage.

41–42

Fixed

46

Fixed

swh/web/assets/src/bundles/vault/vault-utils.js
1–5 ↗(On Diff #5804)

The main reason to create this file was to add some functions that are repeated in the code, so we can easily refactor them. Again, I think it is not necessary to do that in this Diff.

Moved this function to vault-create-task.js file.

@shisus, do not forget to rebase to origin/master before submitting an update to that Diff.

Once you have integrated your changes, you can update that diff using arcanist the following way:

$ arc diff --update D1727
swh/web/assets/src/bundles/vault/vault-utils.js
1–5 ↗(On Diff #5804)

Except you only use these functions in one place so there is no interest to put them in a separate file.

anlambert commandeered this revision.Oct 10 2019, 4:52 PM
anlambert edited reviewers, added: shisus; removed: anlambert.

Commandeering this as the diff has not been updated by its original author since two months.

This revision now requires review to proceed.Oct 10 2019, 4:52 PM
anlambert retitled this revision from [WIP] Immediately download archive if already cooked by the vault to Immediately download archive if already cooked by the vault.Oct 10 2019, 4:57 PM
anlambert edited the summary of this revision. (Show Details)
anlambert edited reviewers, added: Reviewers; removed: shisus.
anlambert updated this revision to Diff 7097.Oct 10 2019, 4:59 PM

Implement new vault UI feature after commandeering the diff.

anlambert edited the summary of this revision. (Show Details)Oct 10 2019, 5:01 PM
anlambert updated this revision to Diff 7098.Oct 10 2019, 5:16 PM

Update: Fix vault cypress tests

anlambert updated this revision to Diff 7099.Oct 10 2019, 5:47 PM

Update: Really fix vault tests and add new ones to validate the new feature

anlambert edited the summary of this revision. (Show Details)Oct 11 2019, 1:24 PM
ardumont accepted this revision.Oct 14 2019, 3:54 PM
ardumont added a subscriber: ardumont.

Looks good.

This revision is now accepted and ready to land.Oct 14 2019, 3:54 PM
anlambert updated this revision to Diff 7233.Oct 15 2019, 10:47 AM

Update: Add task reference in commit message