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

Query from url #254

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/annotator/config.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

var annotationIDs = require('./util/annotation-ids');
var annotationQuery = require('./util/extract-annotation-query');
var settings = require('../shared/settings');

var docs = 'https://h.readthedocs.io/en/latest/embedding.html';
Expand Down Expand Up @@ -33,17 +33,17 @@ function config(window_) {
}
}

// Extract the direct linked ID from the URL.
// Extract the default query from the URL.
//
// The Chrome extension or proxy may already have provided this config
// via a tag injected into the DOM, which avoids the problem where the page's
// JS rewrites the URL before Hypothesis loads.
//
// In environments where the config has not been injected into the DOM,
// we try to retrieve it from the URL here.
var directLinkedID = annotationIDs.extractIDFromURL(window_.location.href);
var directLinkedID = annotationQuery.extractAnnotationQuery(window_.location.href);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this variable needs an update.

if (directLinkedID) {
options.annotations = directLinkedID;
Object.assign(options, directLinkedID);
}
return options;
}
Expand Down
2 changes: 1 addition & 1 deletion src/annotator/sidebar.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ module.exports = class Sidebar extends Host
super
this.hide()

if options.openSidebar || options.annotations
if options.openSidebar || options.annotations || options.query
this.on 'panelReady', => this.show()

if @plugins.BucketBar?
Expand Down
27 changes: 0 additions & 27 deletions src/annotator/util/annotation-ids.js

This file was deleted.

32 changes: 32 additions & 0 deletions src/annotator/util/extract-annotation-query.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
'use strict';

/**
* Extracts an annotation selection or default filter from a url.
*
* @param {string} url - The URL which may contain a '#annotations:<ID>'
* fragment.
* @return {Object} - An object with either an annotation ID or a filter string.
*/
function extractAnnotationQuery(url) {
var filter = {};
try {
// Annotation IDs are url-safe-base64 identifiers
// See https://tools.ietf.org/html/rfc4648#page-7
var annotFragmentMatch = url.match(/#annotations:([A-Za-z0-9_-]+)$/);
var queryFragmentMatch = url.match(/#annotations:(query|q):(.+)$/i);
if (queryFragmentMatch) {
filter.query = queryFragmentMatch[2];
} else if (annotFragmentMatch) {
filter.annotations = annotFragmentMatch[1];
} else {
filter = null;
}
} catch (err) {
filter = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What specific error are you trying to guard against here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just following the previous codebase (function extractURLFromID formerly) I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was looking at the code and the only possible error is a URIError which would not be caught by this code. I'll remove the try/catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two tests that fail if the try-catch code is not included, so I kept it in.

}
return filter;
}

module.exports = {
extractAnnotationQuery: extractAnnotationQuery,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually if a module exports a single function then we use module.exports = <function name> rather than exporting an object with one field.

};
35 changes: 35 additions & 0 deletions src/annotator/util/test/extract-annotation-query-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
'use strict';

var annotationIds = require('../extract-annotation-query');

describe('annotation queries', function () {
var annotation = annotationIds.extractAnnotationQuery('http://localhost:3000#annotations:alphanum3ric_-only');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a utility called unroll for writing data-driven tests which I think would be useful here. See this test for an example.

var queryVarA = annotationIds.extractAnnotationQuery('http://localhost:3000#annotations:q:user:USERNAME');
var queryVarB = annotationIds.extractAnnotationQuery('http://localhost:3000#annotations:QuerY:user:USERNAME');
var invalid = annotationIds.extractAnnotationQuery('http://localhost:3000#annotations:\"TRYINGTOGETIN\";EVILSCRIPT()');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A general comment about these tests is that it is easier to follow if the preparation/arrangement is part of the test case instead of being a separate step that happens in beforeEach. The exception is if you're using a particular fixture across several test cases. We generally try to follow the 'Arrange Act Assert' formula for structuring individual test cases.


it ('accepts regular annotation id', function () {
assert.equal(annotation.annotations, 'alphanum3ric_-only');
});

it ('returns null for query when annotation id exists', function() {
assert.equal(annotation.query, null);
});

it ('returns null on invalid query / id', function() {
assert.equal(invalid, null);
});

it ('produces a null annotation when valid query exists', function () {
assert.equal(queryVarA.annotations, null);
});

it ('accepts query style A ("q:")', function () {
assert.equal(queryVarA.query, 'user:USERNAME');
});

it ('accepts query style B ("query:")', function () {
assert.equal (queryVarB.query, 'user:USERNAME');
});

});
3 changes: 3 additions & 0 deletions src/sidebar/host-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ function hostPageConfig(window) {
var paramWhiteList = [
// Direct-linked annotation ID
'annotations',

// Default query passed by url
'query',

// Config param added by the extension, Via etc. indicating how Hypothesis
// was added to the page.
Expand Down
7 changes: 5 additions & 2 deletions src/sidebar/reducers/selection.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ var uiConstants = require('../ui-constants');

var util = require('./util');

var validQuery = require('../util/validate-query');

/**
* Default starting tab.
*/
Expand All @@ -37,9 +39,10 @@ TAB_SORTKEYS_AVAILABLE[uiConstants.TAB_ANNOTATIONS] = ['Newest', 'Oldest', 'Loca
TAB_SORTKEYS_AVAILABLE[uiConstants.TAB_NOTES] = ['Newest', 'Oldest'];
TAB_SORTKEYS_AVAILABLE[uiConstants.TAB_ORPHANS] = ['Newest', 'Oldest', 'Location'];


function initialSelection(settings) {
var selection = {};
if (settings.annotations) {
if (settings.annotations && !validQuery(settings.query)) {
selection[settings.annotations] = true;
}
return freeze(selection);
Expand Down Expand Up @@ -81,7 +84,7 @@ function init(settings) {
// IDs of annotations that should be highlighted
highlighted: [],

filterQuery: null,
filterQuery: validQuery(settings.query),

selectedTab: TAB_DEFAULT,

Expand Down
38 changes: 38 additions & 0 deletions src/sidebar/util/test/validate-query-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';

var queryUrl = require('../validate-query');

describe('queryUrl', function () {
var qURL;
var longqURL;
var trickyqURL;
var upperURL;

beforeEach(function () {
qURL = queryUrl('user:user_name');
longqURL = queryUrl('user:user_nameany:hello');
trickyqURL = queryUrl('user_bob__helloany:hello');
upperURL = queryUrl('something');
});

it ('returns false on a non-query', function () {
assert.equal(queryUrl({foo:'bar'}), null);
});

it('returns an annotation string as a query', function () {
assert.equal(qURL, 'user:user_name');
});

it('accepts longer queries', function () {
assert.equal(longqURL, 'user:user_name any: hello');
});

it ('is not tricked by confounding usernames or queries', function() {
assert.equal(trickyqURL, 'user_bob__hello any: hello');
});

it ('accepts upper and lower case values', function () {
assert.equal(upperURL, 'something');
});

});
35 changes: 35 additions & 0 deletions src/sidebar/util/validate-query.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
'use strict';

// A set of functions to prepare a query from a url request
//
// These functions take values from var annotations
// produced in a url fragment
// ( e.g. http://www.example.com/path/to/file
// #annotations:query__user__username)
// in settings
// and converts it to a search query, detecting
// tags like "user:", "any:" and "tag:".
//

function returnQueryFromAnnotationFragment (query){
var result = null;
try {
if (query) {
result = query.replace(/(user|any|tag|text):/gi,
function (tag) {
// temporarily fix bug where
// any:term does not work
if (tag === 'any:') {
return ' ' + tag + ' ';
} else {
return ' ' + tag;
}
}).trim();
}
} catch (e) {
result = null;
}
return (result);
}

module.exports = returnQueryFromAnnotationFragment;