Tag suggestions #4034

sean-roberts · 2016-11-07T23:01:10Z

Work for: https://trello.com/c/glm03Us8/1-add-tag-suggestions-to-suggestion-box

Having a component need both static data and dynamic data, I opted to set a global object window.h to namespace anything that we need like that. This is meant to make it available and not assume that the script is loaded already. Once the script is loaded, it will pick it up and render the list items. I did not see an example for which to base a solution for dynamic data - so if there is a pattern that we would prefer, let me know.

Also, I figured there would be a nice way jinja template handle to pull in a script file on demand. I looked and tried a lot of different things based on what our templates already do, and I could not come out with something that wasn't hacky as hell. So, I opted to just put the script tag in the template and we will figure out a more elegant solution if this pattern/need is more common than just this one use case.

The tests and the unicode pattern that is used is pulled straight from unicode.coffee in the client repo.

robertknight · 2016-11-08T11:29:35Z

I did not see an example for which to base a solution for dynamic data - so if there is a pattern that we would prefer, let me know.

We have a couple of existing approaches to getting dynamic data into the rendered HTML:

Using HTML attributes (eg. with either base-64 or quote-escaped markup) on the controller's root element (eg. data-suggestions="<blob of escaped data, eg in JSON format>")
Using <script> tags containing JSON data. See how app_config is rendered in app.html.jinja2. The <script> tag is given a known class which the consumer of the data then uses to find and load it (see settings.js in the client)

For the second approach, there are two reasons for using a script tag containing JSON data rather than an inline JS script:

It makes correct escaping easy. You can either generate a string of JSON in Python code and mark it as escaped using variable | safe in a template, or just pipe a variable (dict, list etc.) into the to_json filter in a template using variable | to_json.
It enables us to block inline JS scripts using Content Security Policy on the production website as a security measure.

robertknight · 2016-11-08T11:33:09Z

Also, I figured there would be a nice way jinja template handle to pull in a script file on demand.
I looked and tried a lot of different things based on what our templates already do, and I could
not come out with something that wasn't hacky as hell.

You didn't miss anything - we don't do any lazy loading of scripts in the client or on the site yet, although we have several use cases for it (eg. on the client only loading KaTeX and unorm when actually needed).

robertknight · 2016-11-08T11:34:06Z

h/static/scripts/controllers/search-bar-controller.js

@@ -4,6 +4,12 @@ var Controller = require('../base/controller');
 var LozengeController = require('./lozenge-controller');
 var AutosuggestDropdownController = require('./autosuggest-dropdown-controller');
 var SearchTextParser = require('../util/search-text-parser');
+var stringUtil = require('../util/string');
+var escapeHtml = require('escape-html');


In both the Python and JS code, we follow a pattern of putting vendor imports at the top of the file, followed by a blank line, then local requires.

robertknight · 2016-11-08T11:36:13Z

h/static/scripts/controllers/search-bar-controller.js

+
+    this._suggestionsHandler = new AutosuggestDropdownController( this._input, {
+
+      list: [].concat(explanationList, tagsList),


concat returns a copy of the array rather than mutating the original, so explanationList.concat(tagsList) would be equivalent here.

I can update. But I did this intentionally because I think it reads more easily and leaves the slight lack of tribal knowledge question "is tagsList is being added to the explanationList?" out of the way

robertknight · 2016-11-08T11:42:28Z

h/static/scripts/tests/controllers/search-bar-controller-test.js

@@ -83,6 +88,79 @@ describe('SearchBarController', function () {
        });
    });

+    it('it shows suggestions for tags when face is selected', function (done) {


Typo: face => "tags facet"?

robertknight · 2016-11-08T11:45:38Z

h/static/scripts/tests/util/string-test.js

+
+  describe('stringUtil helpers', function(){
+
+    it('removes hungarian marks', function(){


Note to self: These were ported across from the tests in unicode-test.coffee in the client.

robertknight · 2016-11-08T11:48:06Z

h/static/scripts/util/string.js

+  // This require is coming from a vendor bundle
+  // that is loaded globally on the running webpage
+  // not a require in the node context
+  if(!require || !require('unorm') || !String.prototype.normalize){


Couple of issues:

If require is not defined, this would result in a ReferenceError exception because of strict mode. Given that we are using CommonJS, I think we can just assume the existence of require as a function here, so we can just remove that check.

If require() fails here, I think it would throw an exception rather than returning false (but please check, I may be mistaken), so you'd need to use try..catch.

robertknight · 2016-11-08T11:57:51Z

h/templates/panels/navbar.html.jinja2

 {% if feature('activity_pages') %}
+
+<script src="/assets/scripts/unorm.bundle.js"></script>


The preferred way to get asset paths is using the asset_urls(bundle_name) function. The bundle names are defined in h/assets.ini. See

h/h/templates/layouts/base.html.jinja2

Line 75 in 2409e15

{% for url in asset_urls("header_js") %}

and

h/h/assets.ini

Line 22 in 2409e15

header_js =

I get that the idea here is to only load unorm on pages that actually display search. However since this template is rendered as part of the navbar which is loaded on every page, what this actually does is add a blocking script at the top of the HTML.

There are a couple of ways you could approach this:

The simplest thing would just be to add unorm.bundle.js to the site_js list in assets.ini - it will then be loaded on every page in the new site.

You could add the bundle just in search.html.jinja2 since only search pages will use tag suggestions initially - but we'd then have to change this again if we implement some kind of tag or user or group suggestions on other pages.

I suggest that (1) would probably be the simplest thing for now.

robertknight · 2016-11-08T12:02:18Z

h/static/scripts/controllers/autosuggest-dropdown-controller.js

@@ -84,6 +84,10 @@ class AutosuggestDropdownController extends Controller {
    });

    this._setList(configOptions.list);
+
+    return {


We use a pattern a bit like this in some of the legacy (pre-ES6) code in the client, but I would suggest that we avoid returning custom objects from class constructors. The reason being that the actual runtime interface of the object returned by new SomeClassName() won't match what you'd expect by looking at the code.

Could we just make setHeader a public method here?

The pattern of binding a method using this.someMethod = this.someMethod.bind(this) is fine and quite useful if you need to pass this.someMethod to an event handler, but I don't think we need to do that here?

robertknight · 2016-11-08T12:10:11Z

h/static/scripts/controllers/search-bar-controller.js

+    let tagsList = (tagSuggestions || []).map((item)=>{
+      return Object.assign(item, {
+        type: TAG_TYPE,
+        title: escapeHtml(item.tag), // make safe


This is fine, although I'm concerned that there will be a mixup with escaping in future, where something escaped for HTML is incorrectly used in a different context or some transformation gets applied to the string which breaks the escaping (on the Python side, this hazard is avoided by wrapping escaped strings using a special Markup class).

robertknight

The UX of tag suggestion and selection works great. However when I submit the search, the search terms added via accepted suggestions get lost.

Steps to reproduce:

Focus input field
Type "tag:"
Select suggestion from facet dropdown
Press Enter to submit search

Expected: Selected tag is submitted with query
Actual: Empty "tag:" query is submitted

The decision not to implement lazy loading in this PR makes sense - I agree that can be considered separately.

Aside from that, the main feedback I had is to consider using the same mechanism that we use in the client for passing dynamic data to the application, or a "data-*" attribute. My guess is that the former will be easier.

codecov-io · 2016-11-08T15:02:14Z

Current coverage is 82.40% (diff: 100%)

Merging #4034 into master will increase coverage by <.01%

@@             master      #4034   diff @@
==========================================
  Files           158        158          
  Lines          5967       5968     +1   
  Methods           0          0          
  Messages          0          0          
  Branches        673        673          
==========================================
+ Hits           4917       4918     +1   
+ Misses          981        979     -2   
- Partials         69         71     +2

Powered by Codecov. Last update 1fb8917...5866bf4

sean-roberts · 2016-11-08T15:31:31Z

As mentioned in slack the losing of search term values was a regression in master which got merged into this branch. I fixed it in this branch though

robertknight · 2016-11-08T16:23:59Z

h/static/scripts/controllers/search-bar-controller.js

+      try{
+        tagSuggestions = JSON.parse(tagSuggestions.innerHTML.trim());
+      }catch(e){
+        console.error('Could not parse .js-tag-suggestions JSON content', e);


If finding the tag succeeds but parsing the JSON fails this will lead to some confusing errors below because .map will be called on an Element. I would suggest pulling this out into a helper function which takes the selector for the suggestions tag and returns Object|Array|null

I just changed the logic to not reuse a variable. I don't like doing that for this reason- didn't think about it while jumping around though.

robertknight · 2016-11-08T16:38:16Z

h/static/scripts/util/string.js

+ */
+function normalize(str){
+
+  // This require is coming from a vendor bundle


I think this comment is obsolete now?

It's still used to articulate that it's being required from outside the site.js bundle and pulled in from the

Probably worth adding a brief comment that "unorm" provides a String.prototype.normalize polyfill

robertknight · 2016-11-08T16:38:55Z

h/static/scripts/util/string.js

+  // that is loaded globally on the running webpage
+  // not a require in the node context
+  try{
+    require('unorm');


Great, this is much clearer and paves the way to loading unorm only if needed.

robertknight · 2016-11-08T16:49:34Z

h/templates/activity/search.html.jinja2

@@ -82,6 +82,14 @@

  {{ panel('navbar') }}

+  {% for url in asset_urls("search_js") %}
+    <script src="{{ url }}"></script>


Unfortunately this is still going to block HTML parsing during download. If we add the defer attribute here and on the script tags which depend on it (in base.html.jinja2) this will avoid the problem since deferred scripts execute after HTML parsing and in-order.

Perhaps it would be best to revisit this after this PR is merged though.

I will add defer to this script as it is not required that we have it immediately

robertknight · 2016-11-08T16:52:58Z

h/static/scripts/tests/controllers/search-bar-controller-test.js

@@ -83,6 +92,82 @@ describe('SearchBarController', function () {
        });
    });

+    it('it shows suggestions for tags when facet is selected', function (done) {


Hmm... we're testing several behaviors in a single test case here. We generally try to only check one behavior in a particular it() test, which tends to make debugging easier when tests fail. Where several test cases share common setup, that can be grouped into beforeEach() within a nested describe() block.

robertknight · 2016-11-08T16:53:43Z

h/static/scripts/controllers/autosuggest-dropdown-controller.js

+
+
+    // Public API
+    this.setHeader = this._setHeader;


Any reason not to just rename _setHeader to setHeader?

I'd prefer to not mix internals and public api. Or rather, make it super obvious which is which

robertknight · 2016-11-08T16:54:12Z

h/static/scripts/controllers/search-bar-controller.js

@@ -1,9 +1,15 @@
 'use strict';

+var escapeHtml = require('escape-html');


Need a new line after the vendor bundle list.

lol I did that and then removed it thinking you would say not to have two lines :P

sean-roberts · 2016-11-08T22:39:38Z

@robertknight updated. Should be good to go now

robertknight · 2016-11-09T10:39:12Z

I'm still seeing an issue where the hidden "q" input is not always updated after accepting a suggestion.

Steps to reproduce:

Create an annotation with the tag "aaa"
Go to /search, click in the search box and type "tag:a"
Press [down], then [enter]

Expected: Hidden "q" input has value "tag:aaa"
Actual: Hidden "q" input has value "tag:a"

My initial efforts to write a failing test for this were frustrated by the fact that 'syn' triggers the "input" event in cases where real browsers do not (See bitovi/syn#119 and bitovi/syn#120). In both cases that causes the "input" event handler in SearchBarController to be called after accepting a tag suggestion when it isn't in real browsers.

robertknight · 2016-11-09T10:51:54Z

h/static/scripts/tests/controllers/search-bar-controller-test.js

@@ -83,6 +134,61 @@ describe('SearchBarController', function () {
        });
    });

+    describe('it allows tag value suggestions', function () {


This is easier to follow and extend now that the individual test cases have been split up 👍

robertknight · 2016-11-09T10:52:45Z

h/static/scripts/tests/controllers/search-bar-controller-test.js

+        sinon.stub(testEl.querySelector('form'), 'submit');
+      });
+
+      it('shows tag suggestsions', function(done){


Typo: 'suggestsions'

robertknight · 2016-11-09T11:16:14Z

h/templates/activity/search.html.jinja2

@@ -82,6 +82,14 @@

  {{ panel('navbar') }}

+  {% for url in asset_urls("search_js") %}
+    <script src="{{ url }}" defer></script>


If you use defer here you'll also need to add it to other scripts which depend on this one - ie. the "site_js" bundle. The browser guarantees to execute deferred scripts in the order that it encounters them, but only once parsing of all HTML (including parsing and evaluation of any "ordinary" blocking <script> tags) is complete.

Hence the console error message about require('unorm') failing in this branch.

You're right. Throwing a console.error in combination with the compromise that the module might not actually be there is not a good plan. I am just going to back out the defer approach now and move on here.

robertknight · 2016-11-09T17:04:33Z

Populating the hidden input with accepted suggestions and loading unorm now works, however handling of Unicode normalization and folding when typing in the search box isn't.

Steps to reproduce:

Create an annotation with the tag "éffort" (note accented "é")
In the Hypothesis client, search for either "tag:effort" or "tag:éffort" - both should filter the annotations in the sidebar to the annotation created in step (1)
On /search, type in "tag:é" (note use of accented "é").

Expected: The tag "effort" is suggested and available to select (note that the server currently does not include accents in the returned tags)
Actual: The tag "effort" disappears from the suggestion list after typing "e"

robertknight

LGTM

sean-roberts added the Needs Review label Nov 7, 2016

robertknight self-assigned this Nov 8, 2016

robertknight added In Review and removed Needs Review labels Nov 8, 2016

robertknight reviewed Nov 8, 2016

View reviewed changes

robertknight requested changes Nov 8, 2016

View reviewed changes

sean-roberts force-pushed the tag-suggestions branch from aef9671 to 62a1cc4 Compare November 8, 2016 15:21

robertknight reviewed Nov 8, 2016

View reviewed changes

sean-roberts force-pushed the tag-suggestions branch from 0801955 to e5ea7ba Compare November 8, 2016 22:39

robertknight reviewed Nov 9, 2016

View reviewed changes

sean-roberts force-pushed the tag-suggestions branch from e5ea7ba to 374bfbb Compare November 9, 2016 16:06

Tag suggestions after tag: facet input

5866bf4

sean-roberts force-pushed the tag-suggestions branch from ad0d805 to 5866bf4 Compare November 9, 2016 17:34

robertknight approved these changes Nov 9, 2016

View reviewed changes

robertknight merged commit 52df077 into master Nov 9, 2016

robertknight deleted the tag-suggestions branch November 9, 2016 17:46


		this._suggestionsHandler = new AutosuggestDropdownController( this._input, {

		list: [].concat(explanationList, tagsList),


		describe('stringUtil helpers', function(){

		it('removes hungarian marks', function(){

		{% if feature('activity_pages') %}

		<script src="/assets/scripts/unorm.bundle.js"></script>

		@@ -1,9 +1,15 @@
		'use strict';

		var escapeHtml = require('escape-html');

Tag suggestions #4034

Tag suggestions #4034

Conversation

sean-roberts commented Nov 7, 2016

robertknight commented Nov 8, 2016

robertknight commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

codecov-io commented Nov 8, 2016 • edited Loading

Current coverage is 82.40% (diff: 100%)

sean-roberts commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean-roberts commented Nov 8, 2016

robertknight commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight commented Nov 9, 2016 • edited Loading

robertknight left a comment

Choose a reason for hiding this comment

codecov-io commented Nov 8, 2016 •

edited

Loading

robertknight commented Nov 9, 2016 •

edited

Loading