Page MenuHomeSoftware Heritage
Differential D2611

Resolve search query which does not look like PID
ClosedPublic
Actions

Authored by Iamshankhadeep on Jan 31 2020, 1:41 PM.

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Commits
rDWAPPS911f82d23424: assets/origin-search: Resolve swh PID only when required
Required Signatures
L3 Software Heritage Contributor License Agreement, version 1.0
Summary

In the web app search box, we send all search queries to the PID resolve endpoint, regardless of whether they look like a PID.
But in tis commit written a script to check if the search query does look like PID but does not start with 'swh' then the app does not send search queries to the PID resolve endpoint.

Related to T1850

Diff Detail

Repository
rDWAPPS Web applications
Branch
resolve-search-query
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10442
Build 15552: Cypress tests for swh-web diffsJenkins
Build 15551: tox-on-jenkinsJenkins
Build 15550: arc lint + arc unit

Event Timeline

Iamshankhadeep created this revision.Jan 31 2020, 1:41 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJan 31 2020, 1:41 PM
Herald added a required legal document: L3 Software Heritage Contributor License Agreement, version 1.0. · View Herald Transcript
swh-public-ci added a comment.Jan 31 2020, 1:44 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/914/ for more details.

anlambert added a subscriber: anlambert.Jan 31 2020, 1:46 PM

@Iamshankhadeep , thanks for the contribution. Please sign our contributor license agreement as we can not review your diff without that action.

swh-public-ci added a comment.Jan 31 2020, 1:50 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/522/ for more details.

Harbormaster completed remote builds in B10401: Diff 9340.Jan 31 2020, 1:51 PM
ardumont added a subscriber: ardumont.Jan 31 2020, 1:58 PM

Also there is a CONTRIBUTOR file at the top-level where you could add yourself to.

I'll let @anlambert discuss the implementation detail with you now ;)

swh/web/assets/src/bundles/browse/origin-search.js
2

update headers to 2018-2020 now

anlambert edited the summary of this revision. (Show Details)Jan 31 2020, 2:08 PM
anlambert requested changes to this revision.Jan 31 2020, 2:13 PM

Unfortunately, this does not resolve the issue explained in T1850.

What we want to obtain here is the following: if the input search query looks like a swh pid (meaning it starts with swh:)
then try to resolve it by querying the associated api endpoint, otherwise issue a search request.

You should rather modify the doSearch function to implement that behavior.

swh/web/assets/src/bundles/browse/origin-search.js
8

please remove the leading and trailing space

This revision now requires changes to proceed.Jan 31 2020, 2:13 PM
Iamshankhadeep updated this revision to Diff 9348.Jan 31 2020, 7:49 PM
  • Update script to not send request to PID resolve endpoint if search query does not look like a pid
swh-public-ci added a comment.Jan 31 2020, 7:53 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/915/ for more details.

swh-public-ci added a comment.Jan 31 2020, 7:59 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/523/ for more details.

Harbormaster completed remote builds in B10409: Diff 9348.Jan 31 2020, 7:59 PM
ardumont added a comment.Feb 3 2020, 9:43 AM

I'm guessing it's missing the corresponding cypress test scenario.

"1 query with a pid not starting with swh: should trigger an origin search" or some such.

Look for cypress pattern file, those are frontend specification tests.
Try and add a new one testing your adaptations.

In the README, there are mentions of test-frontend makefile targets so that should help trigger those (make test-frontend, check swh-web's readme for more info).

anlambert requested changes to this revision.Feb 3 2020, 2:02 PM

@Iamshankhadeep, sorry for the late reply but I am sick since a couple of days and it is complicated for me to work in an efficient way.

Your updated diff looks good ! But as @ardumont explained, we need to add a frontend test to validate the changes.

We use cypress to implement frontend tests. You should add new tests in the following file: [[ https://forge.softwareheritage.org/source/swh-web/browse/master/cypress/integration/origin-search.spec.js | cypress/integration/origin-search.spec.js ]].

Those tests should consist in:

  • if the input search query looks like a PID, check that a request is sent to the /api/1/resolve/ endpoint but also check that no request is sent to the /api/1/origin/search/ endpoint
  • if the input search query does not look like a PID, check the opposite of what is described above

To check that a request has been correctly sent with cypress, you can inspire from this code snippet.
To check that a request has not been sent, the better option seems to raise an error if it is the case: see this code snippet on stackoverflow.

In order to ease cypress invocation, two make targets are available:

  • test-frontend: run tests in headless mode (without browser window)
  • test-frontend-ui: launch the cypress GUI to ease tests development, you should use this one
This revision now requires changes to proceed.Feb 3 2020, 2:02 PM
Iamshankhadeep updated this revision to Diff 9379.Feb 4 2020, 8:27 PM
  • Written cypress test to testPID resolve endpoint and updated CONTRIBUTORS
swh-public-ci added a comment.Feb 4 2020, 8:35 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/918/ for more details.

swh-public-ci added a comment.Feb 4 2020, 8:41 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/526/ for more details.

Harbormaster completed remote builds in B10440: Diff 9379.Feb 4 2020, 8:41 PM
anlambert requested changes to this revision.Feb 4 2020, 10:44 PM

This diff still needs some improvements. I have added a couple of inline comments that should help you to improve the tests implementation.

CONTRIBUTORS
1

Please insert your name in the lexicographic ordering of the contributors first name.

cypress/integration/origin-search.spec.js
2

(C) 2019-2020

107

Use ${this.Urls.api_1_resolve()}** here instead of harcoding the url

109

I have executed the test without your modifications in the swh/web/assets/src/bundles/browse/origin-search.js file. There is indeed an assertion raised but cypress does not report the test as failed.

So we will have to use another approach to check the request is not sent.
By adding this custom cypress command in the cypress/support/index.js, I managed to write that test the following way:

it('should not send request to the resolve endpoint', function() {
    cy.server();

    cy.route({
      method: 'GET',
      url: `${this.Urls.api_1_resolve()}**`,
    }).as('resolvePid');

    cy.route({
      method: 'GET',
      url: `${this.Urls.api_1_origin_search()}**`,
    }).as('searchOrigin');

    cy.get('#origins-url-patterns')
      .type(origin.url);
    cy.get('.swh-search-icon')
      .click();

    cy.wait('@searchOrigin');

    cy.xhrShouldBeCalled('resolvePid', 0);
    cy.xhrShouldBeCalled('searchOrigin', 1);
  });

The other test should also be rewritten this way.

113

Use single quotes for string litterals

372

it('should not send request to the search endpoint', function()

377

Use `${this.Urls.api_1_origin_search()}**` here instead of harcoding the url

This revision now requires changes to proceed.Feb 4 2020, 10:44 PM
Iamshankhadeep updated this revision to Diff 9380.Feb 5 2020, 7:47 AM
Iamshankhadeep marked 9 inline comments as done.
  • updated CONTRIBUTORS in lexicographic order, updated index.js to support custom cypress command and updated origin-search.spec.js as asked
swh-public-ci added a comment.Feb 5 2020, 7:56 AM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/919/ for more details.

swh-public-ci added a comment.Feb 5 2020, 8:02 AM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/527/ for more details.

Harbormaster completed remote builds in B10441: Diff 9380.Feb 5 2020, 8:02 AM
Iamshankhadeep updated this revision to Diff 9381.Feb 5 2020, 8:04 AM
  • Added newline in CONTRIBUTORS
swh-public-ci added a comment.Feb 5 2020, 8:12 AM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/920/ for more details.

swh-public-ci added a comment.Feb 5 2020, 8:18 AM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/528/ for more details.

Harbormaster completed remote builds in B10442: Diff 9381.Feb 5 2020, 8:18 AM
anlambert requested changes to this revision.Feb 5 2020, 12:57 PM

This looks good to me. Just a couple of modifications to add before I accept the diff:

  • update copyright years in one file (see inline comment)
  • commits must be squashed and commit message should be updated as followed
assets/origin-search: Resolve swh PID only when required

When a search query does not look like a PID, do not try to resolve it by
sending a request to the dedicated endpoint and directly search for origins.

Closes T1850
cypress/support/index.js
2

(C) 2019-2020

This revision now requires changes to proceed.Feb 5 2020, 12:57 PM
Iamshankhadeep updated this revision to Diff 9389.Feb 5 2020, 2:49 PM

Squashed commit messages

swh-public-ci added a comment.Feb 5 2020, 2:58 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/921/ for more details.

swh-public-ci added a comment.Feb 5 2020, 3:04 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/529/ for more details.

Harbormaster completed remote builds in B10449: Diff 9389.Feb 5 2020, 3:04 PM
anlambert accepted this revision.Feb 5 2020, 4:45 PM

All good ! Before pushing the commit to the master branch, please rebase the diff to the origin/master branch (you will have to update the diff again in order for Phabricator to automatically close it) as we like to have a clean linear history. Then merge your feature branch to master and push it to the remote repository.

This revision is now accepted and ready to land.Feb 5 2020, 4:45 PM
Iamshankhadeep updated this revision to Diff 9394.Feb 5 2020, 5:37 PM

rebase to origin/master

swh-public-ci added a comment.Feb 5 2020, 5:45 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/922/ for more details.

swh-public-ci added a comment.Feb 5 2020, 5:51 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/530/ for more details.

Harbormaster completed remote builds in B10454: Diff 9394.Feb 5 2020, 5:51 PM
Closed by commit rDWAPPS911f82d23424: assets/origin-search: Resolve swh PID only when required (authored by Iamshankhadeep). · Explain WhyFeb 5 2020, 5:59 PM
This revision was automatically updated to reflect the committed changes.
Iamshankhadeep added a commit: rDWAPPS911f82d23424: assets/origin-search: Resolve swh PID only when required.

Revision Contents
Changeset List

PathSize
1 line
cypress/
integration/
57 lines
support/
7 lines
swh/
web/
assets/
src/
bundles/
browse/
42 lines

Diff 9381

View Options

CONTRIBUTORS

Loading...
View Options

cypress/integration/origin-search.spec.js

Loading...
View Options

cypress/support/index.js

Loading...
View Options

swh/web/assets/src/bundles/browse/origin-search.js

Loading...
About · Developer information · Legal