Page MenuHomeSoftware Heritage

Resolve search query which does not look like PID
ClosedPublic

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

Details

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 , thanks for the contribution. Please sign our contributor license agreement as we can not review your diff without that action.

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

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
  • Update script to not send request to PID resolve endpoint if search query does not look like a pid

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

@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
  • Written cypress test to testPID resolve endpoint and updated CONTRIBUTORS

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 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
  • Added newline in CONTRIBUTORS

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

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