Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review for search v0.1 release candidate 1 #138

Merged
merged 144 commits into from
Apr 17, 2018
Merged

Review for search v0.1 release candidate 1 #138

merged 144 commits into from
Apr 17, 2018

Conversation

erickpeirson
Copy link
Contributor

This PR is for search-0.1rc1. This incorporates significant revisions to the UI and functionality since https://github.com/cul-it/arxiv-search/releases/tag/0.1-beta (see also previous dev team review), based on end-user feedback.

Release team (to answer your questions):

How to review

What to look for

  • Clarity on code structure and ability to understand how the feature is built.
  • Clarity on how this feature fits into documented architecture.
  • Consistency of feature with decisions made in other parts of the project.
  • Parity with current milestone: see milestone doc

Feedback collection and format

  • Code review: GitHub review comments
    • Inline comments are great to use for context on specific issues
    • “Approve” means “passes muster, ready to go to beta”
    • “Request changes” is exactly that; a bug or fix worth making now to avoid having obvious mistakes released to the wild
    • If neither option seems applicable: make comment to indicate that you have reviewed and are finished offering feedback
    • Avoid a non-response (no feedback at all), even if it’s just “I pass on this review and defer to others”

Giving effective feedback

  • Keep feedback within the scope of the target milestone.
  • If something is unclear, assume that there is a good explanation and ask for clarification.
  • As always, keep it civil, respectful, and supportive.

Getting started

See https://github.com/cul-it/arxiv-search/blob/develop/README.md for instructions on running search in your local environment, and running tests/quality checks.

Major changes

To facilitate review, an overview of changes is provided below.

ARXIVNG-338 | Replace requirements/... with pipenv

You should now be able to use pipenv to install Python dependencies and run the search application in a virtual environment. The README has been updated accordingly.

ARXIVNG-324 | Generate UI test criteria and execute manually

Given our accessibility quality goals, some UI testing must still be performed manually. This process is documented here. There is room to further explore automated regression tests for the UI, however.

ARXIVNG-402 | Document search UI with screenshots

Screenshots to assist with user interface testing can be found in docs/ui/screenshots.

ARXIVNG-381 | MathJax should only target title and abstract in search results

MathJax is configured to render TeXisms in elements with the CSS class .mathjax. This is enabled for title, abstract, and comments in the search results.

ARXIVNG-370 | Hit highlighting for search & ARXIVNG-371 | Add DOI, Record number, Jref, abstract to search results

We could see from the beta testing feedback that users were often confused about why certain results were returned. On investigation, we found that many of the scenarios that users described involved hits on the abstract field (which was not shown). We decided to add an expandable snippet from the abstract, and to add hit highlighting to all fields so that it would be clearer why a result was shown.

We also added DOI and JREF to the search results, as users wanted to be able to quickly ascertain whether or not a version of record was available. The DOI links to the dx.doi.org resolver, so that users can quickly locate the VoR.

image

We add highlighting options to queries here, and process the highlighting in the search results here.

  ARXIVNG-380 | Default results per page should be 50 & ARXIVNG-379 | Some parameters should propagate between searches

We increased the options for number of results per page by 100%, and set the default to 50. Note that sort order and number of results now propagate among views, per user requests. This is achieved by including the corresponding GET parameters in all links/forms; this was preferred over a cookie for the sake of simplicity.

image

ARXIVNG-369 | Default option for search to be chronological most recent -> least recent

Many users were thrown off by the mysterious-seeming "relevance" search. Some of the problem stemmed from unclear hits (see ARXIVNG-370, above). But given many explicit appeals for chronological sorting to be the default, we capitulated. Sort should now default to the announcement date (year + month), in descending order.

image

ARXIVNG-378 | Add toggle for whether to include all versions

In the beta release, we included all versions of papers in the advanced search results. Many users saw this as unnecessary and confusing, and so we disabled this behavior by default. Users can include previous versions by checking a box in the advanced search interface (unchecked by default).

image

ARXIVNG-382 | Date range filter in advanced search should support variable precision

Some users asked for the ability to enter only year, or year and month, into the date range filter in the advanced search. This is now supported.

image

ARXIVNG-401 | Implement health check endpoint

In order to support liveness probes in Kubernetes, we needed an endpoint that could be monitored for service health.

The /status endpoint (see https://github.com/cul-it/arxiv-search/blob/develop/search/controllers/__init__.py#L16) accepts GET and HEAD requests, and exercises the connection with Elasticsearch by issuing a real query. If results are returned, the endpoint responds with a status code of 200; otherwise, status 500 is returned.

ARXIVNG-368 | Update help text to reflect Journal Reference field behavior & ARXIVNG-385 | Term1 term2* in journal reference field produces no results

There was some confusion about the behavior of the journal reference field in search.

image

Note that this did lead us to find a regression in wildcard handling, which we fixed. We handle wildcards by using a "simple_query_string" query; see https://github.com/cul-it/arxiv-search/blob/develop/search/services/index/prepare.py#L79..L84

ARXIVNG-373 | Search service log output should be consistent with arXiv standard logging format, and free of tracebacks

This was mostly about configuring uWSGI to output logs in the classic format (see https://github.com/cul-it/arxiv-search/blob/develop/Dockerfile#L56), so not particularly relevant in the short term and subject to further revision.

One minor annoyance was that the elasticesearch-py package was including tracebacks in warning messages, so we fixed that.

  ARXIVNG-372 | Refactor author name search | DONE

Author name search is no longer a separate interface. Instead, author name search has been improved and standardized for both the simple and advanced search interfaces. Here are a few examples:

http://127.0.0.1:5000/?query=smith&searchtype=author&order=-announced_date_first&size=50

image

http://127.0.0.1:5000/?query=r+j+smith&searchtype=author&order=-announced_date_first&size=50

image

http://127.0.0.1:5000/?query=rowan+smith&searchtype=author&order=-announced_date_first&size=50

image

http://127.0.0.1:5000/?query=smith%2C+rowan&searchtype=author&order=-announced_date_first&size=50

image

http://127.0.0.1:5000/?query=%22smith%2C+rowan%22&searchtype=author&order=-announced_date_first&size=50

image

http://127.0.0.1:5000/?searchtype=author&query=%22Smith%2C+Rowan+J%22

image

ARXIVNG-367 | Supporting proceedings index page links to report numbers

Per #75, search should continue to support proceedings index pages, which rely on the report number field in article metadata. This is fully supported.

image

Other minor UI improvements

  • ARXIVNG-383 | Add "all fields" as option in advanced search
  • ARXIVNG-384 | Add comma for thousands separator
  • ARXIVNG-377 | Label the + box on advanced search page

mhl10 and others added 30 commits March 21, 2018 16:30
ARXIVNG-377 ARXIVNG-368 form label adjustments and help text adjustments
Add "mathjax" class to title in search results template
ARXIVNG-372 refacted author search as part of simple and advanced search interfaces
Add comma separator to thousands place in title
e-prints holding id and others added 2 commits April 15, 2018 20:35
@coveralls
Copy link

coveralls commented Apr 16, 2018

Coverage Status

Coverage increased (+2.5%) to 84.578% when pulling db35043 on develop into bf3aa74 on master.

erickpeirson and others added 26 commits April 16, 2018 08:42
ARXIVNG-418 fixed wildcard search, changed default querystring behavior
…re; changed tie-breaker for sort to paper_id_v
ARXIVNG-416 return a 400 Bad Request if the pagination parameters are funky
@erickpeirson erickpeirson merged commit 9025d41 into master Apr 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants