PR#1920: Searching by attributes - copr/copr

		`@@ -715,3 +715,38 @@`
		`if not new_args:`
		`return flask.request.path`
		`return '{}?{}'.format(flask.request.path, url_encode(new_args))`
		`+`
		`+`
		`+ def parse_fullname(full_name):`
		`+ """`
		+ Take a string in a `ownername/projectname` format and return them in a tuple
		+ `(ownername, projectname)`. If a string without a forward-slash is passed,
		`+ it is considered to be a projectname without ownername.`
		`+ """`
		`+ if "/" in full_name:`
		`+ return full_name.split("/", maxsplit=1)`
		`+ return None, full_name`
		`+`
		`+`
		`+ def format_search_string(params):`
		`+ """`
		`+ Takes a dict of parameters that were specified for searching and return`
		`+ them in a formatted, human-readable string.`
		`+`
		`+ {"ownername": "@copr", "projectname": "copr-dev"}`
		`+ => "ownername: @copr, projectname: copr-dev"`
		`+`
		`+ {"ownername": "frostyx", "fulltext": "foo"}`
		`+ => "ownername: frostyx, foo"`
		`+ """`
		`+ params_copy = dict(params.copy())`
		`+ fulltext = params_copy.pop("fulltext", None)`
		`+`
		`+ params_list = ["{0}: {1}".format(k, v) for k, v in params_copy.items()]`
		`+ if not params_list:`
		`+ return fulltext`
		`+`
		`+ result = ", ".join(params_list)`
		`+ if not fulltext:`
		`+ return result`
		`+ return ", ".join([result, fulltext])`

frontend/coprs_frontend/coprs/logic/coprs_logic.py

file modified

+33 -22

		`@@ -8,9 +8,8 @@`

		`import flask`

		`- from sqlalchemy import and_, not_`
		`- from sqlalchemy.sql import func`
		`- from sqlalchemy import asc, desc`
		`+ from sqlalchemy import not_`
		`+ from sqlalchemy import desc`
		`from sqlalchemy.event import listens_for`
		`from sqlalchemy.orm.attributes import NEVER_SET`
		`from sqlalchemy.orm.exc import NoResultFound`
		`@@ -197,27 +196,39 @@`
		`"User is not a system admin")`

		`@classmethod`
		`- def get_multiple_fulltext(cls, search_string):`
		`+ def get_multiple_fulltext(cls, fulltext=None, projectname=None,`
		`+ ownername=None, packagename=None):`
		`+`
		`+ if fulltext and "/" in fulltext:`
		`+ ownername, projectname = helpers.parse_fullname(fulltext)`
		`+ fulltext = None`
		`+`
		`query = (models.Copr.query.order_by(desc(models.Copr.created_on))`
		`- .join(models.User)`
		`.filter(models.Copr.deleted == False))`
		`- if "/" in search_string: # copr search by its full name`
		`- if search_string[0] == '@': # searching for @group/project`
		`- group_name = "%{}%".format(search_string.split("/")[0][1:])`
		`- project = "%{}%".format(search_string.split("/")[1])`
		`- query = query.filter(and_(models.Group.name.ilike(group_name),`
		`- models.Copr.name.ilike(project),`
		`- models.Group.id == models.Copr.group_id))`
		`- query = query.order_by(asc(func.length(models.Group.name)+func.length(models.Copr.name)))`
		`- else: # searching for user/project`
		`- user_name = "%{}%".format(search_string.split("/")[0])`
		`- project = "%{}%".format(search_string.split("/")[1])`
		`- query = query.filter(and_(models.User.username.ilike(user_name),`
		`- models.Copr.name.ilike(project),`
		`- models.User.id == models.Copr.user_id))`
		`- query = query.order_by(asc(func.length(models.User.username)+func.length(models.Copr.name)))`
		`- else: # fulltext search`
		`- query = query.whooshee_search(search_string, whoosheer=CoprWhoosheer, order_by_relevance=100)`
		`+`
		`+ if projectname:`
		`+ value = "%{}%".format(projectname)`
		`+ query = query.filter(models.Copr.name.ilike(value))`
		`+`
		`+ if ownername and ownername[0] != "@":`
		`+ query = query.join(models.User)`
		`+ value = "%{}%".format(ownername)`
		`+ query = query.filter(models.User.username.ilike(value))`
		`+`
		`+ if ownername and ownername[0] == "@":`
		`+ query = query.join(models.Group)`
		`+ value = "%{}%".format(ownername[1:])`
		`+ query = query.filter(models.Group.name.ilike(value))`
		`+`
		`+ if packagename:`
		`+ query = query.join(models.Package)`
		`+ value = "%{}%".format(packagename)`
		`+ query = query.filter(models.Package.name.ilike(value))`
		`+`
		`+ if fulltext:`
		`+ query = query.whooshee_search(`
		`+ fulltext, whoosheer=CoprWhoosheer, order_by_relevance=100)`
		`+`
		`return query`

		`@classmethod`

frontend/coprs_frontend/coprs/rest_api/resources/project.py

file modified

+3 -1

		`@@ -4,7 +4,7 @@`
		`from flask import url_for, make_response`
		`from flask_restful import Resource`

		`- from ... import db`
		`+ from ... import db, models`
		`from ...logic.builds_logic import BuildsLogic`
		`from ...logic.complex_logic import ComplexLogic`
		`from ...logic.helpers import slice_query`
		`@@ -76,9 +76,11 @@`
		`query = CoprsLogic.set_query_order(query)`

		`if req_args["owner"]:`
		`+ query = query.join(models.User, models.Copr.user_id == models.User.id)`
		`query = CoprsLogic.filter_by_user_name(query, req_args["owner"])`

		`if req_args["group"]:`
		`+ query = query.join(models.Group)`
		`query = CoprsLogic.filter_by_group_name(query, req_args["group"])`

		`if req_args["name"]:`

frontend/coprs_frontend/coprs/static/copr.js

file modified

+14

		`@@ -32,3 +32,17 @@`
		`}`
		`);`
		`});`
		`+`
		`+ function search_by_attribute(attribute) {`
		`+ event.preventDefault();`
		`+ var value = $("input[name=fulltext]").val()`
		`+`
		`+ // When searching by group but omitting the starting @`
		`+ var group = $(event.target).attr("id") == "search-groupname"`
		`+ if (group && value[0] != "@") {`
		`+ value = "@" + value`
		`+ }`
		`+`
		`+ var url = "/coprs/fulltext/?" + attribute + "=" + value`
		`+ window.location.href = url`
		`+ }`

frontend/coprs_frontend/coprs/templates/coprs/show/fulltext.html

file modified

+2 -2

		`@@ -9,12 +9,12 @@`
		`<a href="{{ url_for('coprs_ns.coprs_show') }}">Home</a>`
		`</li>`
		`<li class="active">`
		`- Searching: {{fulltext}}`
		`+ Searching: {{ search_string }}`
		`</li>`
		`</ol>`
		`{% endblock %}`
		`{% block show_top %}`


		`- <h1> Search Results <small> {{fulltext}} </small></h1>`
		`+ <h1> Search Results <small> {{ search_string }} </small></h1>`
		`{% endblock %}`

frontend/coprs_frontend/coprs/templates/layout.html

file modified

+37 -4

		`@@ -49,11 +49,44 @@`
		`<form role="search" method="get" action="{{ url_for('coprs_ns.coprs_fulltext_search') }}">`
		`<div class="input-group menu-search">`
		`<input name="fulltext" type="text" class="form-control" placeholder="Search projects by name, os or arch" value="{% block search_box %}{%endblock%}">`
		`- <span class="input-group-btn">`
		`+`
		`+ <div class="input-group-btn">`
		`<button type="submit" class="btn btn-default">`
		`- <span class="glyphicon glyphicon-search"></span>`
		`+ <span class="glyphicon glyphicon-search"></span>`
		`+ </button>`
		`+`
		`+ <button type="button" class="btn btn-default dropdown-toggle"`
		`+ data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">`
		`+ <span class="caret"></span>`
		`+ <span class="sr-only">Toggle Dropdown</span>`
		`</button>`
		`- </span>`
		`+ <ul class="dropdown-menu">`
		`+ <li>`
		`+ <a id="search-username" onClick="search_by_attribute('ownername')" href="#">`
		`+ Search by user name`
		`+ </a>`
		`+ </li>`
		`+`
		`+ <li>`
		`+ <a id="search-groupname" onClick="search_by_attribute('ownername')" href="#">`
		`+ Search by group name`
		`+ </a>`
		`+ </li>`
		`+ <li role="separator" class="divider"></li>`
		`+`
		`+ <li>`
		`+ <a id="search-projectname" onClick="search_by_attribute('projectname')" href="#">`
		`+ Search by project name`
		`+ </a>`
		`+ </li>`
		`+`
		`+ <li>`
		`+ <a id="search-packagename" onClick="search_by_attribute('packagename')" href="#">`
		`+ Search by package name`
		`+ </a>`
		`+ </li>`
		`+ </ul>`
		`+ </div>`
		`</div>`
		`</form>`
		`</div>`
		`@@ -99,7 +132,7 @@`
		`<dl>`
		`{% include 'contact_us.html' %}`
		`</dl>`
		`- </div>`
		`+ </div>`
		`<div class="col-sm-3">`
		`<dl>`
		`{% include 'project_info.html' %}`

frontend/coprs_frontend/coprs/views/coprs_ns/coprs_general.py

file modified

+13 -3

		`@@ -128,8 +128,16 @@`
		`@coprs_ns.route("/fulltext/", defaults={"page": 1})`
		`@coprs_ns.route("/fulltext/<int:page>/")`
		`def coprs_fulltext_search(page=1):`
		`- fulltext = flask.request.args.get("fulltext", "")`
		`+ params = flask.request.args`
		`+ allowed_keys = ["fulltext", "projectname", "ownername", "packagename"]`
		`+ for key in params.keys():`
		`+ if key in allowed_keys:`
		`+ continue`
		`+ flask.flash("Cannot search by: {}".format(key), "error")`
		`+ return flask.redirect(flask.request.referrer or`
		`+ flask.url_for("coprs_ns.coprs_show"))`

		`+ fulltext = params.get("fulltext", "")`
		`if fulltext.isnumeric():`
		`build = builds_logic.BuildsLogic.get(int(fulltext)).first()`
		`if build:`
		`@@ -138,14 +146,15 @@`
		`return flask.redirect(url)`

		`try:`
		`- query = coprs_logic.CoprsLogic.get_multiple_fulltext(fulltext)`
		`+ query = coprs_logic.CoprsLogic.get_multiple_fulltext(`
		`+ **flask.request.args)`
		`except ValueError as e:`
		`flask.flash(str(e), "error")`
		`return flask.redirect(flask.request.referrer or`
		`flask.url_for("coprs_ns.coprs_show"))`

		`paginator = helpers.Paginator(query, query.count(), page,`
		`- additional_params={"fulltext": fulltext})`
		`+ additional_params=params)`

		`data = builds_logic.BuildsLogic.get_small_graph_data('30min')`

		`@@ -155,6 +164,7 @@`
		`pinned=[],`
		`paginator=paginator,`
		`fulltext=fulltext,`
		`+ search_string=helpers.format_search_string(params),`
		`tasks_info=ComplexLogic.get_queue_sizes(),`
		`graph=data)`

frontend/coprs_frontend/tests/test_logic/test_coprs_logic.py

file modified

+25

		`@@ -391,3 +391,28 @@`
		`assert self.c1.upvotes == 1`
		`assert self.c1.downvotes == 2`
		`assert self.c1.score == -1`
		`+`
		`+`
		`+ class TestCoprSearchLogic(CoprsTestCase):`
		`+`
		`+ @pytest.mark.usefixtures("f_users", "f_coprs", "f_group_copr", "f_builds",`
		`+ "f_db")`
		`+ def test_search_by_attributes(self):`
		`+ result = CoprsLogic.get_multiple_fulltext(ownername="user2")`
		`+ assert set(result) == {self.c2, self.c3}`
		`+`
		`+ result = CoprsLogic.get_multiple_fulltext(ownername="@group1")`
		`+ assert set(result) == {self.gc1, self.gc2}`
		`+`
		`+ result = CoprsLogic.get_multiple_fulltext(projectname="foo")`
		`+ assert set(result) == {self.c1, self.c2}`
		`+`
		`+ result = CoprsLogic.get_multiple_fulltext(packagename="goodbye")`
		`+ assert set(result) == {self.c3}`
		`+`
		`+ result = CoprsLogic.get_multiple_fulltext(`
		`+ ownername="user2",`
		`+ projectname="foo",`
		`+ packagename="world",`
		`+ )`
		`+ assert set(result) == {self.c2}`

frontend/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_general.py

file modified

+31

		`@@ -4,6 +4,7 @@`

		`from unittest import mock`

		`+ from lxml import html`
		`import pytest`
		`import flask`
		`from sqlalchemy import desc, and_`
		`@@ -966,6 +967,36 @@`
		`# qargs, qkwargs = mc_render_template.call_args`
		`# assert qkwargs["paginator"].total_count == 5`

		`+ @pytest.mark.usefixtures("f_users", "f_coprs", "f_group_copr", "f_builds",`
		`+ "f_db")`
		`+ def test_search_by_attributes(self):`
		`+ # We will be searching a lot, so let's make a small helper for that`
		`+ def search(url):`
		`+ response = self.tc.get(url)`
		`+ tree = html.fromstring(response.data)`
		`+ results = [x.find(".//h3") for x in`
		`+ tree.xpath("//a[@class='list-group-item']")]`
		`+ return [x.text for x in results if x is not None]`
		`+`
		`+ # Search by username`
		`+ results = search("/coprs/fulltext/?ownername=user2")`
		`+ assert len(results) == 2`
		`+`
		`+ # Search by packagename`
		`+ results = search("/coprs/fulltext/?packagename=world")`
		`+ assert len(results) == 3`
		`+`
		`+ # Search by multiple attributes at once`
		`+ params = "?ownername=user2&projectname=foo&packagename=world"`
		`+ results = search("/coprs/fulltext/" + params)`
		`+ assert len(results) == 1`
		`+`
		`+ # Make sure all found results contain the searched username`
		`+ # and projectname`
		`+ for result in results:`
		`+ assert "user2" in result`
		`+ assert "foo" in result`
		`+`

		`class TestRepo(CoprsTestCase):`
		`def test_repo_renders_http(self, f_users, f_coprs, f_mock_chroots, f_db):`

frostyx commented 2 years ago

It starts to look that we are going to put search bar to the center of
the homepage, see #1890. The design mockup suggests that it should be
available to search by using specific attributes such as project name,
owner name, or package name. I think it is important otherwise the
searching returns too many false-positives.

I rewrote the whole get_multiple_fulltext function so it may not be
clear on the first sight but it is possible to use searching exactly
as before. Searching the following strings return the exact same
results as previously:

frostyx
@copr
@copr/copr
frostyx/tracer
copr-cli
2679386

But on top of that, it now recognizes the following GET parameters -
ownername, projectname, packagename, and their combinations.
Also, I didn't do any measuring but the searching feels a bit faster
now.

We didn't have any RFE for this so I wasn't sure what way of
specifying attributes we preferred, so I chose the one requiring the
least magic possible. But if we want to implement some custom
searching language, I think this PR would be a good step towards it.

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 04s

frostyx commented 2 years ago

Here is a screenshot of the dropdown menu

praiskup commented 2 years ago

UI looks OK.. I have no problem overlooking the missing non-JS alternative if the default behavior is not changed.

praiskup commented 2 years ago

(tests are broken)

rebased onto 67bc88af316af4e129e6167fc58afa91c569ca25

2 years ago

rebased onto 0c632be85bdf00523df4542f978e97a0f852adc2

2 years ago

zuul commented 2 years ago

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

rebased onto 16c393fc331403a456edc3b2329c358700bdb85d

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 11s

frostyx commented 2 years ago

[copr-build]

frostyx commented 2 years ago

[copr-build]

frostyx commented 2 years ago

Meh, for some reason it shows all the builds, not just the latest one. And some of them are stuck in a pending state while they already succeeded ... never mind.

I fixed the tests and resolved merge conflicts from main.

praiskup commented 2 years ago

Meh, for some reason it shows all the builds, not just the latest one. And some of them are stuck in a pending state while they already succeeded ... never mind.

This is bug in Copr, we do not always deliver the status badge to Pagure (we should retry
if some http problem happens on delivery).

[copr-build]

praiskup commented 2 years ago

Meh, for some reason it shows all the builds

This started happening several months back, and I neither like the new variant.

praiskup commented 2 years ago

I'm almost +1, but when I try to search for praiskup/ping, and I hit "Search by multiple parameters", the header starts printing "Search Results ownername: frostyx, packagename: copr-cli"?

praiskup commented on line 21 of frontend/coprs_frontend/coprs/templates/layout.html 2 years ago

Perhaps this ... is it OK to have there some default?

Metadata Update from @praiskup:
- Pull-request tagged with: wip

2 years ago

2 new commits added

frontend: test searching by attributes
frontend: search by attributes using the input value

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 02s

frostyx commented 2 years ago

I'm almost +1, but when I try to search for praiskup/ping, and I hit "Search by multiple parameters", the header starts printing "Search Results ownername: frostyx, packagename: copr-cli"?

This should work properly now.
And I also added some unit tests, so we can be at least somewhat sure that it works.

praiskup commented 2 years ago