chromium-status: automatically linkify usernames/status messages

chromium-status: automatically linkify usernames/status messages

Automatically convert all usernames in the Who col to mailto links.
This makes the field much narrower w/out reducing information which
allows more space for the message field.

There is a large amount of links that can be created based on the status
messages, both from the automatic stuff the bots produce as well as the
common conventions of developers.  Add support for declaring regexes that
in turn create links of the data.

We also add support for doing instance-specific linking (for cases where
source data is ambiguous as to the target, but make sense when considered
in the instance they are shown).  We only use this for ChromeOS atm, but
should be easy to extend if people are interested.

Hopefully this should help out sheriffs by cutting out a few steps.

BUG=chromium:314501
TEST=ran an appengine instance with example statuses and checked output
TEST=`./tests/main_test.py` passes

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=239012

[vapier, 2013-11-25 04:31:50]

what do you think of this approach ?  using a little bit of recursion and lists,
i was able to split the link logic out from the html logic so it's all contained
now.

[cmp, 2013-11-25 05:04:55]

adding people for review

https://codereview.chromium.org/84943003/diff/1/templates/main.html
File templates/main.html (right):

https://codereview.chromium.org/84943003/diff/1/templates/main.html#newcode47
templates/main.html:47: <a href="{% if a_link.email %}mailto:{% endif %}{{
a_link.target }}">{{ a_link.text }}</a>
a_link.email -> a_link.is_email

[vapier, 2013-11-25 15:24:33]

https://codereview.chromium.org/84943003/diff/1/templates/main.html
File templates/main.html (right):

https://codereview.chromium.org/84943003/diff/1/templates/main.html#newcode47
templates/main.html:47: <a href="{% if a_link.email %}mailto:{% endif %}{{
a_link.target }}">{{ a_link.text }}</a>
fixed ... figures the last minute change i made would be slightly broken ;)

[M-A Ruel, 2013-11-25 16:18:14]

https://codereview.chromium.org/84943003/diff/60001/status.py
File status.py (right):

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode26
status.py:26: """Simple object to hold text that might be linked"""
"""Holds text ..

No need to state a relative quality of the object in the docstring, here that it
is simple.

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode28
status.py:28: def __init__(self, text, target=None, is_email=False):
I'd prefer to not use default arguments.

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode38
status.py:38: """Turn arbitrary text into a set of links"""
Turns
(imperative everywhere)

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode41
status.py:41: 'chrome': 'https://chrome-internal-review.googlesource.com',
Very hardcoded. :/ Could these be entities?

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode88
status.py:88: (re.compile(r'("cbuildbot" on "([^"]+ canary)")',
These should be stated as static lists. There isn't much dynamic in there,
unless you properly create entities to store the instance-specific rules, which
I prefer.

You can work around re.I by using r'(?i)' as a prefix to your regexp.

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode130
status.py:130: # Normalize newlines otherwise the DB store barfs.
I'm fine with adding multiline=True instead. Actually,  I prefer that. The
constructor may not always be used.

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode352
status.py:352: LinkableText.bootstrap(self.APP_NAME)
This is a bad idea to do this there. It creates a memory leak as
LinkableText._CONVERT grows on each root page rendering. There's a bootstrap
function at the end to manage on-startup issues like that.

https://codereview.chromium.org/84943003/diff/60001/templates/main.html
File templates/main.html (right):

https://codereview.chromium.org/84943003/diff/60001/templates/main.html#newco...
templates/main.html:37: <a href="{% if a_link.is_email %}mailto:{% endif %}{{
a_link.target }}">{{ a_link.text }}</a>
could you wrap at 80 cols?

https://codereview.chromium.org/84943003/diff/60001/templates/main.html#newco...
templates/main.html:37: <a href="{% if a_link.is_email %}mailto:{% endif %}{{
a_link.target }}">{{ a_link.text }}</a>
An option: this mailto: condition should probably be a library function.

[vapier, 2013-11-25 17:51:26]

https://codereview.chromium.org/84943003/diff/60001/status.py
File status.py (right):

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode26
status.py:26: """Simple object to hold text that might be linked"""
i agree it's simple, but i find scanning a one line docstring is a lot faster
than scanning the content

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode28
status.py:28: def __init__(self, text, target=None, is_email=False):
any reason why ?  the intention is for email to be uncommon.

if you're really set on there not being defaults, then i might as well use the
python collections module and not create my own.

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode38
status.py:38: """Turn arbitrary text into a set of links"""
i dont see the point, but i also dont really care to debate

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode41
status.py:41: 'chrome': 'https://chrome-internal-review.googlesource.com',
i have no idea what "entities" are.  plus this file already hardcodes gerrit
URLs, and the idea is to support known link styles across multiple instances (so
that CL:12345 works both in chromium-status and chromiumos-status and so on).

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode88
status.py:88: (re.compile(r'("cbuildbot" on "([^"]+ canary)")',
this was needed in a previous CL where i used a function callback.  i no longer
use that, so i no longer need to bootstrap these rules on the fly.

the re.compile/re.flags stuff aren't a problem to do at class decl time (as i'm
already doing that above).

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode130
status.py:130: # Normalize newlines otherwise the DB store barfs.
i don't think we want people to insert newlines into their messages.  i'm not
sure the various parts of the system (or the clients) are ready to handle it,
and it doesn't make sense to even bother supporting it.

https://codereview.chromium.org/84943003/diff/60001/status.py#newcode352
status.py:352: LinkableText.bootstrap(self.APP_NAME)
i'm not terribly familiar with the app engine model.  i could make it so the
func itself handles being called more than once.

i also had it here because i thought the app_name stuff was initialized later,
and to use the api as available.  but i guess that is setup in the basepage's
bootstrap phase, and is set on the class level, so i can just use that directly.

[cmp, 2013-12-03 15:16:16]

Adding Vadim and Robbie this may have time.  Aaron or Stefan, feel free to
finish this review if you have cycles, too.

[cmp, 2013-12-03 15:53:46]

On 2013/12/03 15:16:16, cmp wrote:
> Adding Vadim and Robbie this may have time.  Aaron or Stefan, feel free to
> finish this review if you have cycles, too.

That should be: Adding Vadim and Robbie *who* may have time.

[Vadim Sh., 2013-12-03 20:06:36]

https://codereview.chromium.org/84943003/diff/70001/status.py
File status.py (right):

https://codereview.chromium.org/84943003/diff/70001/status.py#newcode25
status.py:25: class Link(object):
I'd convert it to collections.namedtuple. Shorter, immutable, __repr__ for free.

Up to you, class is also OK.

https://codereview.chromium.org/84943003/diff/70001/status.py#newcode25
status.py:25: class Link(object):
Also, 'Link' is a misleading name. This object can represent a chunk of a plain
text, not necessary a link.

https://codereview.chromium.org/84943003/diff/70001/status.py#newcode50
status.py:50: # Automatically linkify convert known strings for the user.
nit: 'linkify convert'. Keep only one?)

https://codereview.chromium.org/84943003/diff/70001/status.py#newcode51
status.py:51: _CONVERTS = [
Is is possible to get rid of 're.compile' repetition?

For instance,
_CONVERTERS = [(re.compile(x[0], flags=re.I),) + x[1:] for x in [
   (r'\b((http://)?((crbug|crosbug)(\.com)?(/(p/)?[0-9]+)))\b', ...),
   ...
]]

It also looks ugly :(

Another suggestion is to actually use 'bootstrap'.

@classmethod
def register_converter(cls, regex, target, pretty, is_email):
  cls._CONVERTERS.append((re.compile(regex, ...), ...))

@classmethod
def bootstrap(cls):
  cls.register_converter(....)
  cls.register_converter(....)

I actually like it more:
a) It adds a proper documentation to format of _CONVERTERS tuples.
b) It gives an example of what subclasses should do in 'bootstrap'.

It's optional. Current approach is also fine.

https://codereview.chromium.org/84943003/diff/70001/status.py#newcode66
status.py:66: r'\1', r'\2', True),
Why not put 'mailto:' into target (i.e. r'mailto:\1') and remove is_email
completely (from _CONVERTERS and Link and html).

https://codereview.chromium.org/84943003/diff/70001/status.py#newcode110
status.py:110: return left_links + [link] + right_links
This looks nice and elegant :)

https://codereview.chromium.org/84943003/diff/70001/status.py#newcode135
status.py:135: self.username_links = LinkableText(self.username)
__init__ in db.Model subclass is not very conventional. Afaik, there's no
guarantee that db.get(...) or Query.fetch(...) is actually calling it with
**kwargs set properly (and not modifying it afterwards).

Also Status object is mutable, 'message' may change after constructor, and
'message_links' will become out of date.

I propose the following change:
1) Store 'message' as is (by adding multiline=True to db.StringProperty). Remove
'\n' right before displaying it (or when converting it to LinkableText).
2) Convert username_links & message_links to @property, compute it on the fly.
3) Remove __init__.

What do you think?

https://codereview.chromium.org/84943003/diff/70001/templates/main.html
File templates/main.html (right):

https://codereview.chromium.org/84943003/diff/70001/templates/main.html#newco...
templates/main.html:35: {% for a_link in a_status.username_links.links %}
This can be converted to jinja2 macro, to reduce copy-pasting
(http://jinja.pocoo.org/docs/templates/#macros):

{% macro linkified_text(text) -%}
  {% for a_link in text.links %}
    ....
  {% endfor %}
{%- endmacro %}

.....

<td class="username">
  {{ linkified_text(a_status.username_links) }}
</td>

[vapier, 2013-12-04 07:54:32]

https://codereview.chromium.org/84943003/diff/70001/status.py
File status.py (right):

https://codereview.chromium.org/84943003/diff/70001/status.py#newcode25
status.py:25: class Link(object):
we discussed collections in the previous patch.  afaik, you can't do default
values using the collections module.  i was waiting for maruel to answer a
question and then based on that, either keep this code or switch to collections.

i can rename it to TextFragments.

https://codereview.chromium.org/84943003/diff/70001/status.py#newcode50
status.py:50: # Automatically linkify convert known strings for the user.
done

https://codereview.chromium.org/84943003/diff/70001/status.py#newcode51
status.py:51: _CONVERTS = [
it is a bit cleaner, and it would provide self documentation.  i'm not sure
about the overhead difference, but i think that's generally a non-issue as
appspot will warmup things ahead of time when load gets to a certain point, and
the bootstrap code only runs once then.

i'm a fan of giant static data lists, but i'll see about converting to your
version.

https://codereview.chromium.org/84943003/diff/70001/status.py#newcode66
status.py:66: r'\1', r'\2', True),
"mailto:" is part of the rendering side (i.e. HTML) not part of the data side
(i.e. this) and people wanted me to keep them completely sep

https://codereview.chromium.org/84943003/diff/70001/status.py#newcode135
status.py:135: self.username_links = LinkableText(self.username)
for the message issue, i think this would fix it:
 def __init__(self, message=None, *args, **kwargs):

i'm not sure you can change the db model on the fly (there will already be
entities with these in there).  also, i don't think we want to allow multiline
... see the comments in the previous patchset where we discussed this issue.

the @property idea SGTM

https://codereview.chromium.org/84943003/diff/70001/templates/main.html
File templates/main.html (right):

https://codereview.chromium.org/84943003/diff/70001/templates/main.html#newco...
templates/main.html:35: {% for a_link in a_status.username_links.links %}
thanks, i've never done templating code before, so i have no idea what's even
available

[vapier, 2013-12-05 07:23:32]

PTAL; i think all the status.py feedback is addressed now.

i couldn't do the template suggestion because this AE uses django templating,
and from what i can find via google, django doesn't support macros itself. 
you'd have to install a custom templatetags module and i don't think specific AE
instances can do it.

i think converting to jinja2 from django templating is a discussion i don't want
to get into ;).

[Vadim Sh., 2013-12-05 18:27:24]

On 2013/12/05 07:23:32, vapier wrote:
> PTAL; i think all the status.py feedback is addressed now.
> 
> i couldn't do the template suggestion because this AE uses django templating,
> and from what i can find via google, django doesn't support macros itself. 
> you'd have to install a custom templatetags module and i don't think specific
AE
> instances can do it.
> 
> i think converting to jinja2 from django templating is a discussion i don't
want
> to get into ;).

lgtm

Sorry about misleading macros suggestion, I forgot that this app is using
django.

[commit-bot: I haz the power, 2013-12-05 19:36:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vapier@chromium.org/84943003/90001

[commit-bot: I haz the power, 2013-12-05 19:36:18]

Change committed as 239012