Page MenuHomeSoftware Heritage
Differential D5570

Display visit status information in the save request information
ClosedPublic
Actions

Authored by ardumont on Apr 21 2021, 3:27 PM.

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Maniphest Tasks
T3266: Improve save code now failed/uneventful status reporting
Commits
rDWAPPSe6cce7a595b8: Display visit status information in the save request information
rDWAPPS85253bc88f6e: Drop redundant `Task` prefix in row title in save code now detail view
Summary

As demonstrated in the following snapshots, just placed prior to arguments [1] [2]. So this
is read soon enough by users who wants more details.

Another dedicated commit also drops the redundant Task prefix in row title in save
code now detail view (included in the diff for simplicity and avoiding future gazillions
of rebases)

This is populated from now on. There is no backtracking existing visit_status into the db for prior save code now request.

[1] full

[2] not_found

Depends on D5569
Related to T3266

Test Plan

tox & frontend tests still happy as well

Diff Detail

Repository
rDWAPPS Web applications
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20914
Build 32455: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 32454: arc lint + arc unit

Event Timeline

ardumont created this revision.Apr 21 2021, 3:27 PM
swh-public-ci added a comment.Apr 21 2021, 3:35 PM

Build is green

Patch application report for D5570 (id=19901)

Could not rebase; Attempt merge onto 015ddb8964...

Merge made by the 'recursive' strategy.
 assets/src/bundles/save/index.js                   | 28 +++++++++++--------
 cypress/integration/origin-save.spec.js            | 17 ++++++++----
 requirements-test.txt                              |  1 +
 .../0009_saveoriginrequest_visit_status.py         | 32 ++++++++++++++++++++++
 swh/web/common/models.py                           | 18 ++++++++++++
 swh/web/common/origin_save.py                      | 12 ++++++--
 swh/web/tests/common/test_origin_save.py           |  7 +++++
 swh/web/tests/test_migrations.py                   | 23 ++++++++++++++++
 8 files changed, 119 insertions(+), 19 deletions(-)
 create mode 100644 swh/web/common/migrations/0009_saveoriginrequest_visit_status.py
 create mode 100644 swh/web/tests/test_migrations.py
Changes applied before test
commit 1477ad7b695f4083159f8e7618f3a5267dc1f024
Merge: 015ddb89 29bb9a4f
Author: Jenkins user <jenkins@localhost>
Date:   Wed Apr 21 13:27:39 2021 +0000

    Merge branch 'diff-target' into HEAD

commit 29bb9a4f0716958db8914c9189c4b06a7e187962
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Apr 21 15:03:02 2021 +0200

    Drop redundant `Task` prefix in row title in save code now detail view
    
    Related to T3266

commit bedbd2f2d0ea935cf5dc4c2c7b210a483015bf6c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Apr 21 14:58:08 2021 +0200

    Display visit status information in the save request information
    
    As demonstrated in the following snapshot, just placed prior to arguments [1]. So this
    is read soon enough by users who wants more details.
    
    [1] https://forge.softwareheritage.org/F4420335
    
    Related to T3266

commit 1e7e00b3cab0749ed72d7a6800594beb1b6f4ee5
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Apr 21 13:59:19 2021 +0200

    Populate the visit status value in save code now request
    
    Related to T3266

commit fb1f5bbd85137f0754435c10784d03aa41efd20e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Apr 19 18:26:33 2021 +0200

    Add visit_status to SaveOriginRequest model
    
    Related to T3266

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/707/ for more details.

Harbormaster completed remote builds in B20907: Diff 19901.Apr 21 2021, 3:35 PM
ardumont requested review of this revision.Apr 21 2021, 3:35 PM
ardumont edited the summary of this revision. (Show Details)Apr 21 2021, 3:43 PM
ardumont edited the summary of this revision. (Show Details)
ardumont edited the summary of this revision. (Show Details)
ardumont edited the summary of this revision. (Show Details)
ardumont edited the summary of this revision. (Show Details)Apr 21 2021, 3:47 PM
anlambert requested changes to this revision.Apr 21 2021, 3:51 PM
anlambert added inline comments.
assets/src/bundles/save/index.js
393–413

Looking at this code I wrote at the time, I should have written it like that instead:

const taskData = {
  "Type": "type",
  "Log": "message",
   ...
};
for (const [title, property] of Object.entries(taskData)) {
  if (saveRequestTaskInfo.hasOwnProperty(property)) {
    saveRequestInfo.push({
      key: title,
      value: saveRequestTaskInfo[property]
    });
  }  
}

You should change it to remove the code duplication.

This revision now requires changes to proceed.Apr 21 2021, 3:51 PM
ardumont added inline comments.Apr 21 2021, 4:05 PM
assets/src/bundles/save/index.js
393–413

that will be a tad more complicated than that, we'll need types as well.

There is json (arguments), dates (scheduled, started, ended), duration (duration i think), and raw values, etc...
They all have some values to rework prior to just push in the array.

I'll try something...

anlambert added inline comments.Apr 21 2021, 4:21 PM
assets/src/bundles/save/index.js
393–413

You could do something like that then:

const taskData = {
  "Type": {"property": "type", "transform": x => x},
  "Start date": {"property": "started", "transform": x => new Date(x).toLocaleString()},
   ...
};
for (const [title, data] of Object.entries(taskData)) {
  if (saveRequestTaskInfo.hasOwnProperty(data.property)) {
    saveRequestInfo.push({
      key: title,
      value: data.transform(saveRequestTaskInfo[data.property])
    });
  }  
}
ardumont added inline comments.Apr 21 2021, 4:21 PM
assets/src/bundles/save/index.js
393–413

oh no... ;)

No idea if that would work, just thrown out there... just to show it'd be more complicated than entertained.

There is potential issues... with the following.

First with the destructuring, which i've no idea if that would work.
Second, the lambdas i've defined... no idea either if those are ok...

export function formatValuePerType(type, value) {
  const mapFunction = {
    "json": function(v) { return JSON.stringify(v, null, 2); },
    "date": function(v) { return new Date(v).toLocaleString(); },
    "raw": function(v) { return v; },
    "duration": function(v) { return v +' seconds'; },
  }
  return mapFunction[type](value)
}

...
        const taskData = {
          "Type": ["raw", "type"],
          "Visit status": ["raw", "visit_status"],
          "Arguments": ["json", "arguments"],
          "Id": ["raw", "id"],
          "Backend id": ["raw", "backend_id"],
          "Scheduling date": ["date", "scheduled"],
          "Start date": ["date", "started"],
          "Termination date": ["date", "ended"],
          "Duration": ["duration", "duration"],
          "Executor": ["raw", "worker"],
          "Log": ["raw", "message"],
        };
        for (const [title, [type, property]] of Object.entries(taskData)) {
          if (saveRequestTaskInfo.hasOwnProperty(property)) {
            saveRequestInfo.push({
              key: title,
              value: formatValuePerType(saveRequestTaskInfo[property])
            });
          }
...
ardumont added inline comments.Apr 21 2021, 4:24 PM
assets/src/bundles/save/index.js
393–413

simpler, more readable.
awesome, thanks.

anlambert added inline comments.Apr 21 2021, 4:24 PM
assets/src/bundles/save/index.js
393–413

Looks good to me.

anlambert added inline comments.Apr 21 2021, 4:25 PM
assets/src/bundles/save/index.js
393–413

Your solution is quite equivalent, pick the one you prefer.

ardumont updated this revision to Diff 19909.Apr 21 2021, 5:32 PM

Adapt according to review (rework existing code drop duplicated code)

ardumont added inline comments.Apr 21 2021, 5:38 PM
assets/src/bundles/save/index.js
393–413

I kept my implementation but i reused the syntactic sugar on the lambdas ;).

I somehow preferred it slightly to avoid repeating the lambdas per dict entry.

In any case i like our new version is quite cleaner, it's more functional ;)

Thanks for the heads up.

anlambert accepted this revision.Apr 21 2021, 5:38 PM

Great, thanks !

This revision is now accepted and ready to land.Apr 21 2021, 5:38 PM
swh-public-ci added a comment.Apr 21 2021, 5:45 PM

Build is green

Patch application report for D5570 (id=19909)

Could not rebase; Attempt merge onto 015ddb8964...

Merge made by the 'recursive' strategy.
 assets/src/bundles/save/index.js                   | 93 ++++++++--------------
 cypress/integration/origin-save.spec.js            | 17 ++--
 requirements-test.txt                              |  1 +
 swh/web/api/views/origin_save.py                   |  2 +
 .../0009_saveoriginrequest_visit_status.py         | 32 ++++++++
 swh/web/common/models.py                           | 18 +++++
 swh/web/common/origin_save.py                      | 12 ++-
 swh/web/tests/common/test_origin_save.py           |  7 ++
 swh/web/tests/test_migrations.py                   | 23 ++++++
 9 files changed, 137 insertions(+), 68 deletions(-)
 create mode 100644 swh/web/common/migrations/0009_saveoriginrequest_visit_status.py
 create mode 100644 swh/web/tests/test_migrations.py
Changes applied before test
commit 48b54f62a53a4c0a935978af69b6cb95058f99d9
Merge: 015ddb89 fcd74383
Author: Jenkins user <jenkins@localhost>
Date:   Wed Apr 21 15:38:19 2021 +0000

    Merge branch 'diff-target' into HEAD

commit fcd74383f75915d4233b754f30f9e01ea972e407
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Apr 21 15:03:02 2021 +0200

    Drop redundant `Task` prefix in row title in save code now detail view
    
    Related to T3266

commit 6aeaad45cce4cb02b98b0d988955a7b3a5f4cad2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Apr 21 14:58:08 2021 +0200

    Display visit status information in the save request information
    
    As demonstrated in the following snapshot, just placed prior to arguments [1]. So this
    is read soon enough by users who wants more details.
    
    [1] https://forge.softwareheritage.org/F4420335
    
    Related to T3266

commit 80f35d1ef288a8f8dad8edcf61e2bdebe839c504
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Apr 21 13:59:19 2021 +0200

    Populate the visit status value in save code now request
    
    Related to T3266

commit fb1f5bbd85137f0754435c10784d03aa41efd20e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Apr 19 18:26:33 2021 +0200

    Add visit_status to SaveOriginRequest model
    
    Related to T3266

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/709/ for more details.

Harbormaster completed remote builds in B20914: Diff 19909.Apr 21 2021, 5:45 PM
ardumont updated this revision to Diff 19914.Apr 21 2021, 6:39 PM

Rebase

ardumont edited the summary of this revision. (Show Details)Apr 21 2021, 6:40 PM
ardumont added a parent revision: D5569: Populate the visit status value in save code now request.
swh-public-ci added a comment.Apr 21 2021, 7:00 PM

Build has FAILED

Patch application report for D5570 (id=19914)

Could not rebase; Attempt merge onto 015ddb8964...

Updating 015ddb89..85253bc8
Fast-forward
 assets/src/bundles/save/index.js                   | 93 ++++++++--------------
 cypress/integration/origin-save.spec.js            | 17 ++--
 requirements-test.txt                              |  1 +
 swh/web/api/views/origin_save.py                   |  6 ++
 .../0009_saveoriginrequest_visit_status.py         | 32 ++++++++
 swh/web/common/models.py                           | 18 +++++
 swh/web/common/origin_save.py                      | 12 ++-
 swh/web/tests/api/views/test_origin_save.py        | 11 ++-
 swh/web/tests/common/test_origin_save.py           |  7 ++
 swh/web/tests/test_migrations.py                   | 23 ++++++
 10 files changed, 150 insertions(+), 70 deletions(-)
 create mode 100644 swh/web/common/migrations/0009_saveoriginrequest_visit_status.py
 create mode 100644 swh/web/tests/test_migrations.py
Changes applied before test
commit 85253bc88f6ea7d625e041b3dfaf49c5096f353c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Apr 21 15:03:02 2021 +0200

    Drop redundant `Task` prefix in row title in save code now detail view
    
    Related to T3266

commit e6cce7a595b8dbd3b8c089d3e7e018eae8cb6b65
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Apr 21 14:58:08 2021 +0200

    Display visit status information in the save request information
    
    As demonstrated in the following snapshot, just placed prior to arguments [1]. So this
    is read soon enough by users who wants more details.
    
    [1] https://forge.softwareheritage.org/F4420335
    
    Related to T3266

commit 83f93270101e2de20bf65bf829c421230b88e3fc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Apr 21 13:59:19 2021 +0200

    Populate the visit status value in save code now request
    
    Related to T3266

commit 1042f73c181d722e681a562088afcdcc2ee0af69
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Apr 19 18:26:33 2021 +0200

    Add visit_status to SaveOriginRequest model
    
    Related to T3266

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/714/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/714/console

Harbormaster failed remote builds in B20919: Diff 19914!Apr 21 2021, 7:00 PM
swh-public-ci added a comment.Apr 21 2021, 7:07 PM

Build is green

Patch application report for D5570 (id=19914)

Could not rebase; Attempt merge onto 015ddb8964...

Updating 015ddb89..85253bc8
Fast-forward
 assets/src/bundles/save/index.js                   | 93 ++++++++--------------
 cypress/integration/origin-save.spec.js            | 17 ++--
 requirements-test.txt                              |  1 +
 swh/web/api/views/origin_save.py                   |  6 ++
 .../0009_saveoriginrequest_visit_status.py         | 32 ++++++++
 swh/web/common/models.py                           | 18 +++++
 swh/web/common/origin_save.py                      | 12 ++-
 swh/web/tests/api/views/test_origin_save.py        | 11 ++-
 swh/web/tests/common/test_origin_save.py           |  7 ++
 swh/web/tests/test_migrations.py                   | 23 ++++++
 10 files changed, 150 insertions(+), 70 deletions(-)
 create mode 100644 swh/web/common/migrations/0009_saveoriginrequest_visit_status.py
 create mode 100644 swh/web/tests/test_migrations.py
Changes applied before test
commit 85253bc88f6ea7d625e041b3dfaf49c5096f353c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Apr 21 15:03:02 2021 +0200

    Drop redundant `Task` prefix in row title in save code now detail view
    
    Related to T3266

commit e6cce7a595b8dbd3b8c089d3e7e018eae8cb6b65
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Apr 21 14:58:08 2021 +0200

    Display visit status information in the save request information
    
    As demonstrated in the following snapshot, just placed prior to arguments [1]. So this
    is read soon enough by users who wants more details.
    
    [1] https://forge.softwareheritage.org/F4420335
    
    Related to T3266

commit 83f93270101e2de20bf65bf829c421230b88e3fc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Apr 21 13:59:19 2021 +0200

    Populate the visit status value in save code now request
    
    Related to T3266

commit 1042f73c181d722e681a562088afcdcc2ee0af69
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Apr 19 18:26:33 2021 +0200

    Add visit_status to SaveOriginRequest model
    
    Related to T3266

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/715/ for more details.

Harbormaster completed remote builds in B20919: Diff 19914.Apr 21 2021, 7:07 PM
Closed by commit rDWAPPS85253bc88f6e: Drop redundant `Task` prefix in row title in save code now detail view (authored by ardumont). · Explain WhyApr 21 2021, 7:07 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDWAPPS85253bc88f6e: Drop redundant `Task` prefix in row title in save code now detail view.
ardumont added a commit: rDWAPPSe6cce7a595b8: Display visit status information in the save request information.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
assets/
src/
bundles/
save/
93 lines--
cypress/
integration/
17 lines--
swh/
web/
common/
3 lines93%
Loading...
tests/
common/
1 line100%
Loading...

Diff 19909

View Options

assets/src/bundles/save/index.js

Loading...
View Options

cypress/integration/origin-save.spec.js

Loading...
View Options

swh/web/common/origin_save.py

Loading...
View Options

swh/web/tests/common/test_origin_save.py

Loading...
About · Developer information · Legal