Document link refactor

[seanh]

No description provided.

[seanh]

TODO:

• Fix patchers, see Nick's comment
• A couple of comments from Nick about tests: https://github.com/hypothesis/h/pull/2667/files#r43128239, https://github.com/hypothesis/h/pull/2667/files#r43128472
• Add 10px padding to the .group-form div
• Test on more production data.
• It's almost certainly possible to make this code crash by throwing random types of junk into our API

[seanh]

TODO:

• Fix patchers, see Nick's comment
• A couple of comments from Nick about tests: https://github.com/hypothesis/h/pull/2667/files#r43128239, https://github.com/hypothesis/h/pull/2667/files#r43128472
• Add 10px padding to the .group-form div
• Test on more production data.
• It's almost certainly possible to make this code crash by throwing random types of junk into our API

[seanh]

Add a list of up to most recently annotated (by the group) URLs to the group page

[seanh]

Add a list of up to most recently annotated (by the group) URLs to the group page

[seanh]

One thing I've noticed is that since the text in the <input> on the group page is automatically selected to make copying easier, and the text is at the bottom of the page, the browser window will scroll to the bottom of the page on page load. This only really matters if the list of documents is long enough to make a scrollbar appear of course, but with our current length (max 25 documents) it can be.

[seanh]

One thing I've noticed is that since the text in the <input> on the group page is automatically selected to make copying easier, and the text is at the bottom of the page, the browser window will scroll to the bottom of the page on page load. This only really matters if the list of documents is long enough to make a scrollbar appear of course, but with our current length (max 25 documents) it can be.

[seanh]

Ok, I've refactored this, and done some more work to make it more bullet proof against unexpected data.

@nickstenning I'll do the patcher thing in a separate pr, since we have that patcher problem all over our code

[seanh]

Ok, I've refactored this, and done some more work to make it more bullet proof against unexpected data.

@nickstenning I'll do the patcher thing in a separate pr, since we have that patcher problem all over our code

[nickstenning]

The second half of these conditionals isn't needed... it's implied by the fact we're in an elif clause and we'd never of got here if it weren't true.

[nickstenning]

This whole function would be a fair bit clearer if you just assigned the format strings and did the .format once at the end:

if href and hostname:
    tpl = '<a href="{href}" title="{title}">{link_text}</a> ({hostname})'
elif hostname:
    tpl = '...'
elif href:
   ...

link = tpl.format(href=href, title=title, link_text=link_text, hostname=hostname)
return jinja2.Markup(link)

[nickstenning]

There's a bunch of places you've got comments like this. I don't think this is the right approach. The entire point of a taintedness approach to string escaping is that you don't need to examine the whole call stack to understand if what you're doing is safe. If this function returns something that needs to be escaped, it should escape it. Using jinja2.escape(self.uri.split('/')[-1]) will not double escape anything.

[nickstenning]

s/not p ==/p !=/

[nickstenning]

As I mentioned the other day, there's really no need to use PropertyMock here. @patch.object(models.Annotation, 'uri', 'http://example.com/example.html') would work just fine and would be less to understand.

[nickstenning]

I have a bunch of comments, but none of them are blockers to merging. Take a chance to read them over and feel free to either submit PRs addressing the comments or roll that work into whatever else we do with this later.

[nickstenning]

I have a bunch of comments, but none of them are blockers to merging. Take a chance to read them over and feel free to either submit PRs addressing the comments or roll that work into whatever else we do with this later.