Page MenuHomeSoftware Heritage

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

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

Details

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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12421
Build 18850: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 18849: arc lint + arc unit

Event Timeline

ardumont edited the summary of this revision. (Show Details)

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.

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}
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.

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

Adapt implementation according to review

Adapt implementation according to review

LGTM

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.

  • Respect current naming patterns
  • Update copyright range

(Still did not find how to test this)

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.

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
This revision was automatically updated to reflect the committed changes.