Paths

Table of Contentst

Differential D5905

Search inside the origin branches with name
ClosedPublic
Actions

Authored by jayeshv on Jun 22 2021, 11:33 AM.

Details

Reviewers

anlambert

Group Reviewers

Reviewers

Maniphest Tasks

T3157: Add a search feature to snapshot branches view

Summary

This code enables the possibility to search for branch names inside
the browse origin branches page. A newly added search input is connected
to a JS function and that reloads the page on press of return.
Filtering with name is happening at the storage level.

Related to T3157

Test Plan

search for a branch, eg: master
- page will reload with only matching pages
- url will have search string as query param
- the search text will be pre-populated with the search term
Search for a non existing branch name
- "No branch names containing 'search term' have been found!" message will appear
Search with empty branch name (or delete the text in searech box and enter)
- Page with all the branches will appear

Diff Detail

Repository

rDWAPPS Web applications

Branch

T3157

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 22186
Build 34525: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 34524: arc lint + arc unit

Event Timeline

jayeshv created this revision.Jun 22 2021, 11:33 AM

Herald added a reviewer: Reviewers. · View Herald TranscriptJun 22 2021, 11:33 AM

Build is green

Patch application report for D5905 (id=21196)

Rebasing onto 1bd55d031e...

Current branch diff-target is up to date.

Changes applied before test

commit 23c98445b77be2083ad64d85500493db182bedf9
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Tue Jun 22 11:25:30 2021 +0200

    Search inside the origin branches with name
    
    This code enables the possibility to search for branch names inside
    the browse origin branches page.A newly added search input is connected
    to a JS function and that reloads the page on press of return.
    Filtering with name is happening at the storage level.
    
    Related to T3157

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

Harbormaster completed remote builds in B22183: Diff 21196.Jun 22 2021, 11:42 AM

jayeshv requested review of this revision.Jun 22 2021, 11:42 AM

jayeshv edited the summary of this revision. (Show Details)Jun 22 2021, 11:58 AM

jayeshv edited the test plan for this revision. (Show Details)

jayeshv added a reviewer: anlambert.

Neat. lgtm.

Couple of remarks inline.

Ensure the indentation from existing code is respected in the template.

l'll let @anlambert double check

assets/src/bundles/browse/browse-utils.js
66	No need to make it a "variable", let's keep it an immutable value by default (either let or const should be good enough here).
67	isn't there some keycode named so the code is more readable than a 13 code?
swh/web/common/archive.py
1071
swh/web/templates/browse/branches.html
16	indentation is a bit off.
94

Suggested changes by @ardumont

Thanks for working on that task ! Feature seems to work as expected but some improvements can be added before landing this (see my inline comments).

assets/src/bundles/browse/browse-utils.js
65	You should rather listen for the `submit` event of the form here, see comment in `swh/web/templates/browse/branches.html`.
70	Please use underscore to separate the works for consistency with other query parameters of `swh-web` endpoints.
cypress/integration/origin-browse.spec.js
97	should search inside branches
108	should search non existing branch name
swh/web/templates/browse/branches.html
14–21	Could you turn this to a real form DOM element with a search button on the right ? You can inspire from the origins search form at the top right of the main HTML layout. It will simplify JS code as you will simply need to bind the submit event instead of listening for key up event.
91–97	It will be great to display a message like `No branch names containing {search_string} have been found !` instead when search returns no results. You can check for the `branch_name_include` query parameter in the template code and display such message. Something like: {% elif "branch_name_include" in request.GET %}
swh/web/tests/common/test_archive.py
1072–1075	`revision` parameter is already in hexadecimal representation, you can remove the call to `hash_to_*` Could you also add a test checking that searching for a non existing branch name returns without raising an exception ?

This revision now requires changes to proceed.Jun 22 2021, 12:42 PM

Couple of remarks inline.

@thanks. Updated the version

anlambert added inline comments.Jun 22 2021, 12:43 PM

assets/src/bundles/browse/browse-utils.js
70	s/works/words/

Build is green

Patch application report for D5905 (id=21199)

Rebasing onto 1bd55d031e...

Current branch diff-target is up to date.

Changes applied before test

commit 01eeb6c92cc6665963ba668b57ac961a1c059e34
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Tue Jun 22 12:35:26 2021 +0200

    Search inside the origin branches with name
    
    This code enables the possibility to search for branch names inside
    the browse origin branches page.A newly added search input is connected
    to a JS function and that reloads the page on press of return.
    Filtering with name is happening at the storage level.
    
    Related to T3157

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

Harbormaster completed remote builds in B22186: Diff 21199.Jun 22 2021, 12:50 PM

Requested changes

jayeshv edited the test plan for this revision. (Show Details)Jun 25 2021, 6:55 PM

Build is green

Patch application report for D5905 (id=21297)

Rebasing onto 0de60f0f4d...

Current branch diff-target is up to date.

Changes applied before test

commit 80ed9762de8936e50fbcb6edf5f07b1a2630da91
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Jun 25 18:47:23 2021 +0200

    Search inside the origin branches with name
    
    This code enables the possibility to search for branch names inside
    the browse origin branches page.A newly added search input is connected
    to a JS function and that reloads the page on press of return.
    Filtering with name is happening at the storage level.
    
    Related to T3157

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

Harbormaster completed remote builds in B22277: Diff 21297.Jun 25 2021, 7:05 PM

I added a couple of comments to handle and we should be able to land this afterwards.

swh/web/browse/snapshot_context.py
1217	You should rename `branchname_include` parameter to `branch_name_include` for consistency.
swh/web/common/archive.py
1071	This should be formatted as follow and moved under the `Args` section: Raises: NotFoundExc if the given snapshot_id is missing
swh/web/templates/browse/branches.html
18–25	Indentation is not right here, sibling elements should be at the same level.
swh/web/tests/common/test_archive.py
1089	Can you be more specific about the inconsistency in the comment ?

This revision now requires changes to proceed.Jun 28 2021, 11:35 AM

jayeshv added inline comments.Jun 28 2021, 12:00 PM

swh/web/templates/browse/branches.html
18–25	Isn't <i> the child element of <button>? I'm using jinja2 mode in emacs and that formats like this. What is the mode/plugin you are in?. Maybe we can use the same one for consistency.

anlambert added inline comments.Jun 28 2021, 12:10 PM

swh/web/templates/browse/branches.html

18–25

This should be indented as follow:

<div class="form-group row float-right">
  <form class="form-horizontal d-none d-md-flex input-group" id="swh-branch-search-form">
    <input class="form-control"
      placeholder="Search in branch names"
      id="swh-branch-search-string"
      value="{{search_string}}"
      type="text" />
    <div class="input-group-append">
      <button class="btn btn-primary" type="submit" id="swh-branch-serach-button">
        <i class="swh-search-icon mdi mdi-24px mdi-magnify" aria-hidden="true"></i>
      </button>
    </div>
  </form>
</div>

I am not using any HTML formatting plugin as I could not find one that plays well with django template language.

Review changes

Build has FAILED

Patch application report for D5905 (id=21299)

Rebasing onto 0de60f0f4d...

Current branch diff-target is up to date.

Changes applied before test

commit 52e9762f8da2eecd59b72465ee0729e9ce1d81c6
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon Jun 28 12:37:39 2021 +0200

    Search inside the origin branches with name
    
    This code enables the possibility to search for branch names inside
    the browse origin branches page.A newly added search input is connected
    to a JS function and that reloads the page on press of return.
    Filtering with name is happening at the storage level.
    
    Related to T3157

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

Harbormaster failed remote builds in B22279: Diff 21299!Jun 28 2021, 12:55 PM

Build is green

Patch application report for D5905 (id=21299)

Rebasing onto 0de60f0f4d...

Current branch diff-target is up to date.

Changes applied before test

commit 52e9762f8da2eecd59b72465ee0729e9ce1d81c6
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon Jun 28 12:37:39 2021 +0200

    Search inside the origin branches with name
    
    This code enables the possibility to search for branch names inside
    the browse origin branches page.A newly added search input is connected
    to a JS function and that reloads the page on press of return.
    Filtering with name is happening at the storage level.
    
    Related to T3157

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

Harbormaster completed remote builds in B22279: Diff 21299.Jun 28 2021, 1:30 PM

Looks good to me, thanks ! There is still a typo remaining on an exception name, see inline comment.