1040816 - Linkify bug numbers shown in the Treestatus UI

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[David Burns :automatedtester]

I tried to click on a link that only had the bug number as a link but it kept making my phone want to dial the number.

To do this we need to add

<meta name="format-detection" content="telephone=no">

see 

https://developer.apple.com/library/safari/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/MetaTags.html#//apple_ref/doc/uid/TP40008193-SW5

Comment 1

[Ed Morley [:emorley]]

I'm not really a fan of adding iOS specific markup - a better solution would be to just linkify the bug numbers in treestatus (they are currently not links) and then presumably they wouldn't be made into phone numbers - plus we'd actually be able to click them.

We'll need to process the closure reason strings on the tree overview page (https://github.com/mozilla/treestatus/blob/master/treestatus/templates/index.html), as well as the closure reason strings and the MOTD on the single-tree pages (https://github.com/mozilla/treestatus/blob/master/treestatus/templates/tree.html). I imagine we should just add the links via the templates (possibly with some Flask template inclusion, so we don't end up copy-pasting everywhere) rather than modifying what the app returns.

If someone wants to work on this, then:
1) Clone https://github.com/mozilla/treestatus
2) Have a look at https://github.com/mozilla/treestatus/blob/master/README
3) Using those instructions get a local instance running
4) Add some repositories and add some closure reasons of form "Closed for bug 123456"
5) Add support for replacing "bug [0-9]+" with the link to Bugzilla, on the template pages linked above (we'll need to process variables tree.reason and tree.message_of_the_day). TBPL does this using:

  _linkBugs: function UserInterface__linkBugs(text, addOrangefactorLink, shouldEscape) {
    var bzlink = '<a href="https://bugzilla.mozilla.org/show_bug.cgi?id=$2" ' +
      'data-bugid="$2" onmouseover="UserInterface.loadBug(this);">$1$2</a>';
...
    if (shouldEscape)
      text = text.escapeContent();
    return text.replace(/(bug\s*|b=)([1-9][0-9]*)(?!\s*<\/a>)\b/ig, bzlink));
...
  },

...but the regex there could do with a cleanup. Also I don't believe Jonja2 has support for for regex, so you'll likely need to create a custom filter (see http://jinja.pocoo.org/docs/api/#custom-filters and http://stackoverflow.com/questions/12791216/how-do-i-use-regular-expressions-in-jinja2).

6) After making the changes locally, test again.
7) For bonus points also use http://jinja.pocoo.org/docs/templates/#urlize so that URLs inserted in the reason/MOTD work too (iirc they don't currently).

If you have any questions, don't hesitate to ask! :-)

Comment 2

[Ed Morley [:emorley]]

s/Jonja2/Jinja2/

Comment 3

[David Burns :automatedtester]

It would always have to be <a href='<somewhere>'>Bug \d+</a> otherwise iOS will think its a number.

The other thing is that any link that is <a>\d+</a> could be seen that way so we need to just be aware of it

Comment 4

[Ed Morley [:emorley]]

(In reply to David Burns :automatedtester from comment #3)
> It would always have to be <a href='<somewhere>'>Bug \d+</a> otherwise iOS
> will think its a number.
> 
> The other thing is that any link that is <a>\d+</a> could be seen that way
> so we need to just be aware of it

I doubt that iOS would override <a href='<somewhere>'>\d+</a> with a dialer link, and if it does then that's stupid (and WONTFIX imo). However, similar to all other bug linkification (TBPL, Bugzilla, wiki.m.o) we'll be using "bug N" not "N" so this should be a non-issue.

Side note: there's a whole raft of google results about people having issues with Safari's feature - seems like it's less helpful than useful (given people could just use "tel:N")

Comment 5

[Ed Morley [:emorley]]

To make this bug clearer:

1) Safari on iOS inserts links for any text that it believes to be a phone number.
2) This behaviour can be overridden by adding iOS specific markup, however this would not be my preferred approach, given that:
  a) Browser specific markup is not ideal
  b) This markup would mean the bug numbers revert back to text, which is still not clickable (there are no links in the current markup, Safari inserts them entirely by it's own doing)
3) I've long since wanted for the links to be clickable anyway, so we should do this either way.
4) By inserting our own link, it is my understanding that Safari will no longer try and override the markup and insert it's own dialer link, thereby fixing the issue described in comment 0 as an added bonus. If it doesn't, then I think the Safari specific issue is likely WONTFIX (due to #2a) [this is what I meant by WONTFIX in comment 4], however we should still take the fix for #3.

Comment 6

[sandip.chatterjee]

Hi Ed,

I would like to work on this bug if it is still available. 

Pretty new to this so might need some help - I've cloned the repository and have a local instance running, and I added some trees and modified their status (steps 1-4 in your instructions above).

Sandip

Comment 7

[Ed Morley [:emorley]]

Sorry for the delay replying - this got mixed into my other bugmail - when you ask a question use "needinfo?" on people and it means that the request doesn't get missed :-)

(In reply to sandip.chatterjee from comment #6)
> Hi Ed,
> 
> I would like to work on this bug if it is still available. 

Yup it is :-)

> Pretty new to this so might need some help - I've cloned the repository and
> have a local instance running, and I added some trees and modified their
> status (steps 1-4 in your instructions above).

That's great :-)

Have you had a look at:
http://stackoverflow.com/questions/12791216/how-do-i-use-regular-expressions-in-jinja2
and
http://jinja.pocoo.org/docs/api/#custom-filters ?

You'll need to create a custom filter, added to app.py, which you then refer to in the Jinja markup in templates/index.html and templates/tree.html

Comment 8

[shaktimaan]

Attachment: Linkify bugs

Assigning the changes for review.

Comment 9

[Ed Morley [:emorley]]

Comment on attachment 8478809 [details] [review]
Linkify bugs

Thank you for the patch. 

I've tested it locally & it successfully linkifies the bug numbers, however unfortunately the use of '|safe' means that everything is unescaped. ie someone can set a reason of '<script>alert(1);</script>' and cause mischief.

Reading up about this more, it seems like you might need to use:
http://flask.pocoo.org/docs/0.10/api/#flask.Markup

There's also one extra place to add the linkification - to the reason column in the history table in tree.html (class "tableReason").

When updating the commits in your PR, can you prefix the bug number at the front of the commit message - eg:
Bug 1040816 - Foo bar; r=reviewers-name

Thanks! :-)

Comment 10

[shaktimaan]

Attachment: Bug 1040816 - Linkify bug numbers shown in the Treestatus UI; r=[e-mail]

Comment 11

[Ed Morley [:emorley]]

Comment on attachment 8479221 [details] [review]
Bug 1040816 - Linkify bug numbers shown in the Treestatus UI; r=[e-mail]

Thank you for those changes - almost there! :-)
I've left some comments on the PR for the last few things needed.

Once you've made those changes, could you squash some of the followup commits - so that the adjustments are not made separately (more than one commit in the PR is great, it just needs to be the final version split up logically, rather than the iterations :-)).

I also had a play around to see if using re.sub() directly would work (since it would avoid the initial .search() - however I couldn't get Markup() to work properly on it at all - I can only think due to some quirk in the implementation of re.sub() and Markup(). I spent some time searching for examples to work around it, but to no avail.

Comment 12

[Ed Morley [:emorley]]

Thank you for updating the commit message - it's almost there - except the "r=" normally uses the person's IRC nick rather than their email address - so in my case r=edmorley :-)

Comment 13

[shaktimaan]

Hi Ed,
.
I am ready with the final changes. However, I am not sure how to go about squashing up follow up commits. One way I could think of was to delete my personal forked repo, fork mozilla/treestatus again and then make the required changes. But wouldn't doing that, result in losing your comments in the pull request?

Comment 14

[Ed Morley [:emorley]]

There's an easier way than that - use |git rebase -i| to squash and then force push to your github branch and the pull request will update :-)

See:
http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-commits-in-a-github-pull-request
(or for more detail, https://gun.io/blog/how-to-github-fork-branch-and-pull-request/)

Comment 15

[shaktimaan]

Attachment: Bug 1040816 - Linkify bug numbers shown in the Treestatus UI; r=edmorley

Assigning for review. Again.

Turns out I had already deleted my fork, so I could not attach the final changes to issue 43. And then I messed up once more by committing my virtualenv. Creating multiple issues/pull requests for the same bug was a mistake. Hope it is okay.

Comment 16

[Ed Morley [:emorley]]

Comment on attachment 8482500 [details] [review]
Bug 1040816 - Linkify bug numbers shown in the Treestatus UI; r=edmorley

This looks great - thank you for the changes. I've tested locally and it correctly linkifies the bug links and is now immune to things like |<script>alert(1);</script>|.

Chris, could you take a quick glance to double check you're ok with this, given you wrote treestatus? :-)

Comment 17

[Ed Morley [:emorley]]

(In reply to Varun from comment #15)
> Creating multiple issues/pull requests for the same bug was a
> mistake. Hope it is okay.

Of course it's ok! Don't worry you've done a great job here :-)

Comment 18

[Chris AtLee [:catlee]]

Comment on attachment 8482500 [details] [review]
Bug 1040816 - Linkify bug numbers shown in the Treestatus UI; r=edmorley

lgtm, thanks!

Comment 19

[Ed Morley [:emorley]]

After merging the PR, I'm getting ISE 500s on https://treestatus-dev.allizom.org/ when I didn't prior to the merge.

Guessing an older Python/Flask version or something, since it worked when testing locally :-/

I've filed bug 1061719 for seeing what the exception is, since I don't have access to logs.

Comment 20

[shaktimaan]

Thank you for mentoring me on this bug Ed. Please let me know if there are any other bugs I could work on.

Comment 21

[Ed Morley [:emorley]]

You're most welcome! I'll have a look tomorrow for another bug for you :-) (It's almost 11pm here so I'll have more time then)

Turns out the ISE 500 on treestatus-dev in bug 1061719 was due to the cluster running python 2.6 which doesn't support the "{}".format(foo) pattern:

https://docs.python.org/2/library/string.html#format-string-syntax
"Changed in version 2.7: The positional argument specifiers can be omitted, so '{} {}' is equivalent to '{0} {1}'."

I've landed a followup to make it python 2.6 compatible:
https://github.com/mozilla/treestatus/commit/fb5a73958edf10663e7c4679d9cac32e8e8b0b09

Yey for us using 4 year old versions of Python :-(

Once the cron pushes that to https://treestatus-dev.allizom.org/ we can see if there is anything else lurking :-)

Comment 22

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley] from comment #21)
> You're most welcome! I'll have a look tomorrow for another bug for you :-)

I've filed a bug for making treestatus pyflakes/PEP8 warning free - bug 1062267, if you are interested?
There's also bug 846868 and bug 823627.

> I've landed a followup to make it python 2.6 compatible:
> https://github.com/mozilla/treestatus/commit/
> fb5a73958edf10663e7c4679d9cac32e8e8b0b09
> 
> Yey for us using 4 year old versions of Python :-(
> 
> Once the cron pushes that to https://treestatus-dev.allizom.org/ we can see
> if there is anything else lurking :-)

Ok so unfortunately testing more I see that using a reason of "Bug 123456 <script>alert("bad!");</script>" still gets through, so I've had to revert this for now :-(

https://github.com/mozilla/treestatus/commit/aa2cda55826448e86a7f1c9d461fd876783e4cdb

The Flask docs on this are really bad, so I'm not overly sure of the best way forwards - since all the examples I can find are simplistic replaces of markup like [b] rather than a bug linkification that uses re.sub() (which is what seems to break the Flask Markup() handling). Perhaps it's worth contacting the upstream project and seeing how they would recommend doing this?

Comment 23

[shaktimaan]

Hi Ed,
.
I tried a few things and the following seems to block <script> tags in the reason/MOTD and yet linkify bug numbers:

Using `return re.sub(bug_re, Markup(bug_url_anchor.format(m.group('bug_id'), m.group('bug_text'))), s)` in the template filter definition. And,

Using `|e|linkbugs` in the HTML templates. 

More on manual escaping 'e' filter here - http://jinja.pocoo.org/docs/dev/templates/#working-with-manual-escaping

Comment 24

[SUMIT RAJAN]

hii Ed Morley [:edmorley],

I am new to all this things and i am really interested in doing this stuff.

So can you please help me in how to start working on this bug and having a complete understanding of this bug!
-Sumit

Comment 25

[Ed Morley [:emorley]]

(In reply to SUMIT RAJAN from comment #24)
> So can you please help me in how to start working on this bug and having a
> complete understanding of this bug!
> -Sumit

Hi! Unfortunately this bug is already taken - the "Status" is "ASSIGNED" rather than "NEW" and the "Assignee" is "Varun" rather than "[e-mail]". Comment 22 in this bug does have some suggestions for other bugs though, if that helps? :-)

(In reply to Varun from comment #23)
> Hi Ed,
> .
> I tried a few things and the following seems to block <script> tags in the
> reason/MOTD and yet linkify bug numbers:
> 
> Using `return re.sub(bug_re, Markup(bug_url_anchor.format(m.group('bug_id'),
> m.group('bug_text'))), s)` in the template filter definition. And,
> 
> Using `|e|linkbugs` in the HTML templates. 
> 
> More on manual escaping 'e' filter here -
> http://jinja.pocoo.org/docs/dev/templates/#working-with-manual-escaping

Sorry I didn't see this - best way to make sure I don't miss something is to needinfo me (unless it's a review or feedback request, in which case they are already a form of needinfo). Could you post a new pull request and we can work from there? :-)

Sorry this bug is turning out to be more of a pain than it first appeared!

Comment 26

[shaktimaan]

Attachment: Bug 1040816 - Linkify bug numbers shown in the Treestatus UI; r=edmorley

Sorry, I was MIA for a long time. Assigning the changes for review.

Comment 27

[Ed Morley [:emorley]]

(In reply to Varun from comment #26)
> Sorry, I was MIA for a long time. Assigning the changes for review.

No problem! It's been quite a while since I've looked at this code now, so I'll need a bit longer to go through it - and things are pretty busy with regressions after the rollout of https://wiki.mozilla.org/Auto-tools/Projects/Treeherder - but I'll make sure I have a look by the end of the week :-)

Comment 28

[Ed Morley [:emorley]]

Comment on attachment 8503870 [details] [review]
Bug 1040816 - Linkify bug numbers shown in the Treestatus UI; r=edmorley

Thank you for persevering with this! :-)

Unfortunately it isn't rebased, and even rolling back to prior commits, I can't get it to apply (it seems to be based on local changes, perhaps as the result of a rebase -i somewhere).

We also need to include the Python 2.6 breakage fix that I landed in https://github.com/mozilla/treestatus/commit/fb5a73958edf10663e7c4679d9cac32e8e8b0b09 prior to the previous PR being reverted.

When rebasing, could you include the bug number in the commit message, eg:
"Bug 1040816 - Use linkbugs filter for reason and message of the day"

Many thanks! :-)

Comment 29

[shaktimaan]

Attachment: Bug 1040816 - Linkify bug numbers shown in the Treestatus UI; r=edmorley

Comment 30

[Ed Morley [:emorley]]

Comment on attachment 8515691 [details] [review]
Bug 1040816 - Linkify bug numbers shown in the Treestatus UI; r=edmorley

Sorry was away last week. Thank you for the rebased PR - I've tested locally and works well. Nice one! :-)

The only changes remaining are:

(In reply to Ed Morley [:edmorley] from comment #28)
> We also need to include the Python 2.6 breakage fix that I landed in
> https://github.com/mozilla/treestatus/commit/
> fb5a73958edf10663e7c4679d9cac32e8e8b0b09 prior to the previous PR being
> reverted.
> 
> When rebasing, could you include the bug number in the commit message, eg:
> "Bug 1040816 - Use linkbugs filter for reason and message of the day"

If you update the PR and then comment in the bug when you've done so, hopefully we can merge straight away.

Many thanks :-)

Comment 31

[shaktimaan]

I did a rebase and 'edit'ed the two commits for changing the commit message. I then 'amend'ed the commit messages to include the bugzilla bug number and the description. When I force pushed my changes, the pull request was updated automatically. So I am assuming the changes are ready to be merged now?

Thanks

Comment 32

[Ed Morley [:emorley]]

Comment on attachment 8515691 [details] [review]
Bug 1040816 - Linkify bug numbers shown in the Treestatus UI; r=edmorley

To save too much more churn, I've added the Python 2.6 compatibility fix (missed review comment from comment 28 + comment 30), tweaked the commit message (since they were lengthened since the last review to ~115-120 chars, and <70-80 is recommended) and merged to master:
https://github.com/mozilla/treestatus/commit/1256c1f81998e87808b26978801f9a92a43fad56
https://github.com/mozilla/treestatus/commit/fef6bdb32c0c02b70047b98323d3ef91739e4fcb

Many thanks for working on this :-)

Comment 33

[Ed Morley [:emorley]]

Looks great on https://treestatus-dev.allizom.org/ - bug linkification + escaping of other content working. I'll file a bug to push to prod :-)

Comment 34

[shaktimaan]

Thank you for being patient through my git n00bity :P

I think issues #46 and #47 on the github project page can be closed too. Those were the issues created by duplicate pull requests for my bugs.

Comment 35

[Ed Morley [:emorley]]

(In reply to Varun from comment #34)
> Thank you for being patient through my git n00bity :P

No problem, Git is not straightforwards to start with - I'm only just getting familiar with it (more used to Hg) :-)

> I think issues #46 and #47 on the github project page can be closed too.
> Those were the issues created by duplicate pull requests for my bugs.

Ah indeded, done.

Comment 36

[Ed Morley [:emorley]]

Deployed and looks great in production:
https://treestatus.mozilla.org/

Thank you for your perseverance with this! :-)

If you're looking for other bugs to work on there are some more for TreeStatus:
https://bugzilla.mozilla.org/buglist.cgi?component=TreeStatus&product=Tree+Management&bug_status=__open__

Or Treeherder is looking for contributors:
https://wiki.mozilla.org/Auto-tools/Projects/Treeherder#Contributing