Page MenuHomeSoftware Heritage
Differential D3162

admin/deposit: Extract origin from swh_anchor_id according to latest change
ClosedPublic
Actions

Authored by ardumont on May 18 2020, 1:57 PM.

Details

Reviewers
None
Group Reviewers
Reviewers
Maniphest Tasks
T2398: Simplify/update the SWHIDs returned from a SWORD deposit
T2409: Remove trailing SWHID qualifiers from origin URL column in deposit admin page
Commits
rDWAPPSd5c4aead493a: admin/deposit: Extract origin from swh_anchor_id according to latest change
Summary

In the deposit, the swh_anchor_id is in the new format:

swh:1:dir:<hash>;?origin=<origin>;visit=<snp-hash>;...

When, previously in the format:

swh:1:dir:<hash>;?origin=<origin>

This adapts the extraction to take into account the new format and provides the
end bound the data.slice call.

Related to T2398
Related to T2409

Test Plan

No idea how to test this

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

ardumont created this revision.May 18 2020, 1:57 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptMay 18 2020, 1:57 PM
ardumont edited the summary of this revision. (Show Details)May 18 2020, 1:59 PM
ardumont edited the summary of this revision. (Show Details)
ardumont edited the summary of this revision. (Show Details)May 18 2020, 2:07 PM
ardumont added a task: T2398: Simplify/update the SWHIDs returned from a SWORD deposit.
ardumont removed a task: T1398: npm incremental lister.
swh-public-ci added a comment.May 18 2020, 2:07 PM

Build is green

Patch application report for D3162 (id=11224)

Rebasing onto 3bbc62e227...

Current branch diff-target is up to date.
Changes applied before test
commit 7a80ad927b55897d77a851763061acb80210cc8d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 18 13:55:27 2020 +0200

    admin/deposit: Extract origin from swh_anchor_id according to latest change
    
    Related to T1398
    Related to T2409

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

Harbormaster completed remote builds in B12411: Diff 11224.May 18 2020, 2:07 PM
ardumont edited the summary of this revision. (Show Details)May 18 2020, 2:13 PM
ardumont added projects: Web app, SWORD deposit.
rdicosmo added a subscriber: rdicosmo.May 18 2020, 2:18 PM

It would be better to make the code robust w.r.t. future potential changes in qualifier order.
Suggested pseudo-code attached (to be checked :-))

swh/web/assets/src/bundles/admin/deposit.js
48–49

This is kind of fragile, as it assumes that the visit qualifiers will appear immediately after the origin qualifier.
It would be better to do something like:

if (originPatternIdx !== -1) {
           let visitPattern = ';visit=';
             let fromorigin = data.slice(originPatternIdx + originPattern.length);
             let nextsep = ';'
             let nextsepIdx = data.indexOf(nextsep)
             if (nextsepIdx !== -1) {
                 originUrl = fromorigin.slice(0,nextsepIdx)
                 } else
             {originUrl = fromorigin}
ardumont added inline comments.May 18 2020, 2:22 PM
swh/web/assets/src/bundles/admin/deposit.js
48–49

fragile, indeed.
I like this approach better. Thanks.

I will adapt accordingly.

What'd be even greater is to test this and actually add the relevant tests.
At least adding a test on a function which does this extraction.

rdicosmo added a comment.May 18 2020, 2:35 PM

Great, thanks. Dont think you need a test on a function as basic as this :-)

ardumont updated this revision to Diff 11231.May 18 2020, 4:36 PM

Adapt implementation according to review

rdicosmo added a comment.May 18 2020, 4:46 PM
In D3162#76908, @ardumont wrote:

Adapt implementation according to review

LGTM

swh-public-ci added a comment.May 18 2020, 4:46 PM

Build is green

Patch application report for D3162 (id=11231)

Rebasing onto 3bbc62e227...

Current branch diff-target is up to date.
Changes applied before test
commit b663dbf7764f188e7afd1b45a8d48c78d9a0bfa2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 18 13:55:27 2020 +0200

    admin/deposit: Extract origin from swh_anchor_id according to latest change
    
    Related to T1398
    Related to T2409

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

Harbormaster completed remote builds in B12418: Diff 11231.May 18 2020, 4:46 PM
ardumont updated this revision to Diff 11234.May 18 2020, 5:49 PM
  • Respect current naming patterns
  • Update copyright range

(Still did not find how to test this)

swh-public-ci added a comment.May 18 2020, 5:57 PM

Build is green

Patch application report for D3162 (id=11234)

Rebasing onto 3bbc62e227...

Current branch diff-target is up to date.
Changes applied before test
commit d5c4aead493ab6d13908a00ffef3c2da66042b29
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 18 13:55:27 2020 +0200

    admin/deposit: Extract origin from swh_anchor_id according to latest change
    
    Related to T1398
    Related to T2409

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

Harbormaster completed remote builds in B12421: Diff 11234.May 18 2020, 5:58 PM
ardumont added a comment.EditedMay 19 2020, 9:03 AM

Ok, so I started to understand how to test this. However, I did not
successfully make it green yet [1]. We actually need cypress tests on the
deposit admin page which does not exist.

Also, I could not simply patch the production as there are some minification
and anonymisation taking place somewhere during deployment. So I'll just land
this to unblock the current issue.

Some tests will be added later to that page then.

[1] The following is green so the function is fine. I did not find how to test
only the function.

It's not an end-to-end cypress test though. It's using mocha testing framework
(loaded with cypress). I just wanted to test the one function. I just did not
find how to properly "require" the library defining parseOrigin (thus why the
function is defined in the test ¯\_(ツ)_/¯).

describe('Test admin deposit page', function() {

    it('Does not do much!', function () {
        expect(true).to.equal(true);
    });

    it('parseOrigin should parse correctly origin', function () {
        function parseOrigin(data) {
            "Extract origin if pattern found, null otherwise"
            let originPattern = ';origin=';
            let originPatternIdx = data.indexOf(originPattern);
            if (originPatternIdx !== -1) {
                let originUrl = data.slice(originPatternIdx + originPattern.length);
                let nextSepPattern = ';';
                let nextSepPatternIdx = originUrl.indexOf(nextSepPattern);
                if (nextSepPatternIdx !== -1) { /* Remove extra context */
                    originUrl = originUrl.slice(0, nextSepPatternIdx);
                }
                return originUrl;
            }
            return null;
        }
        let data = "swh:1:rev:abc;origin=https://g.c/user/repo;visit=swh:1:snp:dev;path=/"
        let actual_origin = parseOrigin(data)
        expect(actual_origin).to.equal("https://g.c/user/repo")

        let data2 = "swh:1:rev:abc;origin=something;path=/some/other"
        let actual_origin2 = parseOrigin(data2)
        expect(actual_origin2).to.equal("something")

        let data3 = "swh:1:rev:abc;origin=https://f.s.o/repositories/swh-web"
        let actual_origin3 = parseOrigin(data3)
        expect(actual_origin3).to.equal("https://f.s.o/repositories/swh-web")
    });

});

Related to P676

This revision was not accepted when it landed; it landed in state Needs Review.May 19 2020, 9:41 AM
Closed by commit rDWAPPSd5c4aead493a: admin/deposit: Extract origin from swh_anchor_id according to latest change (authored by ardumont). · Explain Why
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDWAPPSd5c4aead493a: admin/deposit: Extract origin from swh_anchor_id according to latest change.
ardumont mentioned this in T2416: swh-web: Add coverage to deposit admin page.May 20 2020, 9:38 AM

Revision Contents
Changeset List

PathSize
swh/
web/
assets/
src/
bundles/
admin/
7 lines

Diff 11237

View Options

swh/web/assets/src/bundles/admin/deposit.js

/** /**
* Copyright (C) 2018-2019 The Software Heritage developers * Copyright (C) 2018-2020 The Software Heritage developers
* See the AUTHORS file at the top-level directory of this distribution * See the AUTHORS file at the top-level directory of this distribution
* License: GNU Affero General Public License version 3, or any later version * License: GNU Affero General Public License version 3, or any later version
* See top-level LICENSE file for more information * See top-level LICENSE file for more information
*/ */
function genSwhLink(data, type) { function genSwhLink(data, type) {
if (type === 'display') { if (type === 'display') {
if (data && data.startsWith('swh')) { if (data && data.startsWith('swh')) {
Show All 24 Lines depositsTable = $('#swh-admin-deposit-list')
data: 'swh_id_context', data: 'swh_id_context',
name: 'swh_id_context', name: 'swh_id_context',
render: (data, type, row) => { render: (data, type, row) => {
if (data && type === 'display') { if (data && type === 'display') {
let originPattern = ';origin='; let originPattern = ';origin=';
let originPatternIdx = data.indexOf(originPattern); let originPatternIdx = data.indexOf(originPattern);
if (originPatternIdx !== -1) { if (originPatternIdx !== -1) {
let originUrl = data.slice(originPatternIdx + originPattern.length); let originUrl = data.slice(originPatternIdx + originPattern.length);
let nextSepPattern = ';';
let nextSepPatternIdx = originUrl.indexOf(nextSepPattern);
if (nextSepPatternIdx !== -1) { /* Remove extra context */
originUrl = originUrl.slice(0, nextSepPatternIdx);
}
return `<a href="${originUrl}">${originUrl}</a>`; return `<a href="${originUrl}">${originUrl}</a>`;
} }
rdicosmoUnsubmitted
Not Done
Inline Actions

This is kind of fragile, as it assumes that the visit qualifiers will appear immediately after the origin qualifier.
It would be better to do something like:

if (originPatternIdx !== -1) {
           let visitPattern = ';visit=';
             let fromorigin = data.slice(originPatternIdx + originPattern.length);
             let nextsep = ';'
             let nextsepIdx = data.indexOf(nextsep)
             if (nextsepIdx !== -1) {
                 originUrl = fromorigin.slice(0,nextsepIdx)
                 } else
             {originUrl = fromorigin}
rdicosmo: This is kind of fragile, as it assumes that the visit qualifiers will appear immediately after…
ardumontAuthorUnsubmitted
Done
Inline Actions

fragile, indeed.
I like this approach better. Thanks.

I will adapt accordingly.

What'd be even greater is to test this and actually add the relevant tests.
At least adding a test on a function which does this extraction.

ardumont: fragile, indeed. I like this approach better. Thanks. I will adapt accordingly. What'd be…
} }
return data; return data;
} }
}, },
{ {
data: 'reception_date', data: 'reception_date',
name: 'reception_date', name: 'reception_date',
render: (data, type, row) => { render: (data, type, row) => {
▲ Show 20 Lines • Show All 83 Lines • Show Last 20 Lines
About · Developer information · Legal