Details

Reviewers

Group Reviewers

Commits

rDWAPPS36b589f9337f: browse/origin-search: To Use proper URL building in origin-search.js

Summary

Previously we did it by appending fragments of strings and manually escaping data, but now we are using dedicated JavaScript interfaces must be used to build search URLs instead of manually building them.

Related to T2286

Diff Detail

Repository

rDWAPPS Web applications

Branch

url-building

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 10731
Build 16107: Cypress tests for swh-web diffs	Jenkins
Build 16106: tox-on-jenkins	Jenkins
Build 16105: arc lint + arc unit

Event Timeline

Iamshankhadeep created this revision.Feb 21 2020, 4:26 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptFeb 21 2020, 4:26 PM

Iamshankhadeep retitled this revision from To Use proper URL building in origin-search.js related to T2286 to To Use proper URL building in origin-search.js.Feb 21 2020, 4:30 PM

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

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

Build has FAILED

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

Harbormaster failed remote builds in B10726: Diff 9659!Feb 21 2020, 4:44 PM

Fixed cypress tests

anlambert requested changes to this revision.Feb 21 2020, 5:57 PM

anlambert added a subscriber: anlambert.

anlambert added inline comments.

swh/web/assets/src/bundles/browse/origin-search.js
8	Remove the added white spaces. I will update our eslint config and add a precommit hook to ensure code style consistency.
100	Use `baseSearchUrl.toString()` instead
194	Use `queryParameters.toString()` instead

This revision now requires changes to proceed.Feb 21 2020, 5:57 PM

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

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

Harbormaster completed remote builds in B10728: Diff 9662.Feb 21 2020, 6:08 PM

We can not use queryParameters.toString()
because it returns a string omitting initial '?'. To get the '?' we need to update
window.location.search with queryParameters

swh/web/assets/src/bundles/browse/origin-search.js
194	We can not use queryParameters.toString() because it returns a string omitting initial '?'. To get the '?' we need to update window.location.search ``` with queryParameters

removed spaces and added baseSearchUrl.toString()

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

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

Harbormaster completed remote builds in B10730: Diff 9664.Feb 21 2020, 6:38 PM

Iamshankhadeep marked 2 inline comments as done.Feb 21 2020, 6:47 PM

anlambert requested changes to this revision.Feb 21 2020, 7:36 PM

anlambert added inline comments.

swh/web/assets/src/bundles/browse/origin-search.js
194	It seems to work without prepending the `?` in modern browsers. Anyway, use the following instead: window.location.search = `?${queryParameters.toString()}`; I truly doubt that older browsers support setting an `URLSearchParams` directly and the documentation is pretty clear that a string should be used.