Closed Bug 891199 Opened 11 years ago Closed 10 years ago

clicking on needinfo flag/text should scroll you to the comment which set the flag

Categories

(bugzilla.mozilla.org :: Extensions, enhancement, P3)

Production
enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: k0scist, Assigned: dylan)

References

Details

Attachments

(1 file, 3 obsolete files)

For NEEDINFO bugs, there is text after the comment box:

"""
 	I am providing the requested information for xyz@mozilla.com (clears the needinfo request). 
"""

and at the top of the bug:

"""
Status: 	ASSIGNED (edit) [NEEDINFO]
...
Flags: 	someguy: 	needinfo 		[?][xyz@mozilla.com]
"""

However, neither of these link to the outstanding needinfo request.  It would be nice to be able to navigate to the extant needinfo request.
Assignee: ui → nobody
Component: User Interface → Extensions: Needinfo
Product: Bugzilla → bugzilla.mozilla.org
QA Contact: default-qa
Version: unspecified → Production
Component: Extensions: Needinfo → Extensions: InlineHistory
Summary: NEEDINFO links → clicking on needinfo flag/text should scroll you to the comment which set the flag
glob: Why was this moved from
   Extensions: Needinfo
to
   Extensions: InlineHistory
?
(In reply to Andre Klapper from comment #1)
> glob: Why was this moved from
>    Extensions: Needinfo
> to
>    Extensions: InlineHistory
> ?

The issue is not specific to needinfo flags so it is more an issue with InlineHistory.

dkl
(In reply to David Lawrence [:dkl] from comment #2)
> The issue is not specific to needinfo flags so it is more an issue with
> InlineHistory.

I disagree. This deals with the 'there is text after the comment box' and '[NEEDINFO] at the top of the bug', which only appear if the needinfo extension is available. There is already code in the InlineHistory extension that highlights the entry in the Flags section of the page (with a pending bug fix by me).

You shouldn't need to have the InlineHistory extension enabled to have what the OP has requested. YMMV, so I'm not changing the component back.

  -- simon
the element the link needs to point to is possibly one created by inline-history -- it's possible to set needinfo without adding a comment.

so we either tell needinfo about inline-history elements, or tell inline-history about needinfo elements.  given inline-history already creates links for the flags in other places of the ui, the fix should go there.
simon convinced me on irc that this belongs in the needinfo extension, and it should be taught about inline-history divs where required.

the primary reason for this is if someone has disabled inline-history, we should still link to the comment which set the flag.  if the flag was set without a corresponding comment, then no link for you.
Component: Extensions: InlineHistory → Extensions: Needinfo
Assignee: nobody → dylan
Status: NEW → ASSIGNED
Priority: -- → P3
Any news here?  This is really annoying on more comment-active bugs and sometimes I simply miss stuff...
This is definitely on my todo list, but it is a lower priority at the moment.
Good news! I've been working on this bug.
Less-good news: It is a bit more difficult to do, but I'm pushing ahead with a solution.

There are two parts:

Turn [NEEDINFO] in the bug status heading to a link, which jumps to the most recent inline history item that is needinfo? current_user.

Add links in the area below the comment editor, e.g. around "I am providing..."

It would be simple, but as inline history is still just an extension that fiddles with the html in javascript, everything about is a bit messier.
Attached patch bug-891199-v1.patch (obsolete) — Splinter Review
This patch adds a link to the [NEEDINFO] in the top. It doesn't add any more links to the button, per the discussion with glob this morning.
Attachment #8486007 - Flags: review?(glob)
Comment on attachment 8486007 [details] [diff] [review]
bug-891199-v1.patch

Review of attachment 8486007 [details] [diff] [review]:
-----------------------------------------------------------------

as per comment 5, the changes to the needinfo extension need to work without the inline history extension installed or enabled.

it's ok to check for the inline-history extension and call new code from there (as per this patch); however we need to accommodate:
- inline-history extension not installed
- user has disabled inline-history
- bug has too many activity entries, resulting in inline-history disabling itself
Attachment #8486007 - Flags: review?(glob) → review-
Handling the case of inline history being off seems pretty difficult, any suggestions on that?
Eventually, when native inline history lands, this point will be moot...

Handling it when it turns itself off by reaching MAXIMUM_ACTIVITY_COUNT is easy; since we've already called ->get_activity() it is simple to just use the template to set a JS var with the last needinfo entry in that perl array.
Flags: needinfo?(glob)
(In reply to Dylan William Hardison [:dylan] from comment #13)
> Handling the case of inline history being off seems pretty difficult, any
> suggestions on that?

ih_activity won't be defined/true if inline-history is not installed or pref'ed off.
to find the right comment to link to when !ih_activity .. well, i guess that's the crux of your question :)

you could borrow code from inline-history: iterate bz_comment_time elements to find the comment which matches the last element in the needinfo_flags array, and link to that comment (match on timestamp and flag.requester==comment.author).  if a matching comment cannot be found, use the current unlinked text.

> Handling it when it turns itself off by reaching MAXIMUM_ACTIVITY_COUNT is easy..

because ih_activity will be a false value when MAXIMUM_ACTIVITY_COUNT is hit, it may be easier to let the above not-available logic deal with it.  it means you wouldn't have to explicitly deal with needinfo being set without a comment being added (in this scenario there won't be a corresponding div to link to).
Flags: needinfo?(glob)
Attached patch bug-891199-v2.patch (obsolete) — Splinter Review
I consider this to be about as good as we can get until inline history is no longer an extension.

It will fallback to looking for comments at the time "time" as a needinfo flag if any of the following are true:

1) The inline history extension is disabled by the user.
2) The inline history extension disabled itself (>500 items)
3) The inline history extension is not installed.

Additionally, it does not use javascript to scroll to the view, because it always has the anchor id when it builds the [NEEDINFO] link.
Attachment #8486007 - Attachment is obsolete: true
Attachment #8490407 - Flags: review?(glob)
Comment on attachment 8490407 [details] [diff] [review]
bug-891199-v2.patch

Review of attachment 8490407 [details] [diff] [review]:
-----------------------------------------------------------------

this looks good; just a few minor things to fix up.

::: extensions/Needinfo/template/en/default/bug/needinfo.html.tmpl
@@ +25,5 @@
> +  BLOCK needinfo_comment_div;
> +    match = needinfo_flags.last.creation_date.match('^(\d{4})\.(\d{2})\.(\d{2})(.+)$');
> +    date  = "$match.0-$match.1-$match.2$match.3";
> +    c     = 0;
> +    FOREACH comment IN bug.comments;

need to skip private comments if the current user isn't an insider

@@ +26,5 @@
> +    match = needinfo_flags.last.creation_date.match('^(\d{4})\.(\d{2})\.(\d{2})(.+)$');
> +    date  = "$match.0-$match.1-$match.2$match.3";
> +    c     = 0;
> +    FOREACH comment IN bug.comments;
> +        IF comment.creation_ts == date;

incorrect indentation

@@ +27,5 @@
> +    date  = "$match.0-$match.1-$match.2$match.3";
> +    c     = 0;
> +    FOREACH comment IN bug.comments;
> +        IF comment.creation_ts == date;
> +        result = "c$c";

you don't need to do the counting; use comment.count.
you should also LAST out of that loop as soon as the corresponding comment is found.

@@ +45,3 @@
>          var summary_container = document.getElementById('static_bug_status');
> +        if (document.getElementById('inline-history-ext')) {
> +            needinfo_comment_div = inline_history.getNeedinfoDiv();

incorrect indentation
Attachment #8490407 - Flags: review?(glob) → review-
Attached patch bug-891199-v3.patch (obsolete) — Splinter Review
(In reply to Byron Jones ‹:glob› from comment #16)
> Comment on attachment 8490407 [details] [diff] [review]
> bug-891199-v2.patch
> 
> Review of attachment 8490407 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this looks good; just a few minor things to fix up.
Thanks.

> need to skip private comments if the current user isn't an insider
Fixed.
 
> incorrect indentation
Fixed.

> you don't need to do the counting; use comment.count.
> you should also LAST out of that loop as soon as the corresponding comment
> is found.
Fixed.

> incorrect indentation
Fixed.
Attachment #8490407 - Attachment is obsolete: true
Attachment #8491254 - Flags: review?(glob)
Comment on attachment 8491254 [details] [diff] [review]
bug-891199-v3.patch

Review of attachment 8491254 [details] [diff] [review]:
-----------------------------------------------------------------

looks like you crossed the streams -- this patch contains changes unrelated to this bug.
Attachment #8491254 - Flags: review?(glob) → review-
Ah, forgot to rebase off of master, but still generated the diff from master..HEAD.
Attachment #8491254 - Attachment is obsolete: true
Attachment #8491495 - Flags: review?(glob)
Comment on attachment 8491495 [details] [diff] [review]
bug-891199-v4.patch

Review of attachment 8491495 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob
Attachment #8491495 - Flags: review?(glob) → review+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   7b055b9..e2a3c8d  master -> master
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 1072105
No longer blocks: 1072105
I find the fix for this bug insufficient.  There are bugs having ni? set for different people on different comments.  It happened to me several times (using the [NEEDINFO] clickable link at the top) that I've arrived to a wrong comment.

This makes peoples' life much harder and sometimes it blocks important work as I may answer to a wrong comment.

I was expecting the real solution here would be the email next to the flag (at the top-right section) would be clickable.  Each name would have a link to its proper comment number.

Having a single global link is simply not a good enough solution, sorry.

File a new bug?
Flags: needinfo?(glob)
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #22)
> Having a single global link is simply not a good enough solution, sorry.
> File a new bug?

the new ui (enabled via preferences, look for 'enable experimental ui') deals with this better.
https://globau.wordpress.com/2015/03/31/bmo-new-look/

as the plan is for that ui to replace the existing one instead of filing a bug to change the current ui it would be better if you switch to the new ui instead :)
Flags: needinfo?(glob)
(In reply to Byron Jones ‹:glob› from comment #23)
> the new ui (enabled via preferences, look for 'enable experimental ui')
> deals with this better.

It's near the bottom of the "General Preferences" page:
                                   ______________________
  Use experimental user interface: [ Site default (Off) ]
                                   | On                 |
                                   | Off                |
                                   ¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯
Took me some time to find it.
N.B. "Experimental UI" and "Classic" skin can't be used together (selecting "experimental UI" switches skin from "Classic" to "Mozilla"). "T'aint a bug, it's a feature", I suppose? Then I'm too old-fashioned for it.
(In reply to Tony Mechelynck [:tonymec] from comment #25)
> N.B. "Experimental UI" and "Classic" skin can't be used together (selecting
> "experimental UI" switches skin from "Classic" to "Mozilla"). "T'aint a bug,
> it's a feature", I suppose? Then I'm too old-fashioned for it.

correct - the new ui requires the mozilla skin (and eventually we'll only have a single supported skin on bmo; there's maintenance overheads for having multiple skins which are hard to justify).
Thanks for all the responses.


Switching to Mozilla theme + the exp-UI (having a bunch of dis+advantages), beside my name there is no clickable link, only for anyone else also ni?'ed on the bug.  So I cannot reach the comment where someone ni? me.  It's actually worse, since when I'm the only one ni?'ed I no longer can reach the comment easily.

Note that I'm using BugzillaTweaks.

Note2: when I click [Edit] (is there a pref to always edit, please) I cannot see the list of ni? people anymore..
Flags: needinfo?(glob)
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #27)
> Switching to Mozilla theme + the exp-UI (having a bunch of dis+advantages),
> beside my name there is no clickable link, only for anyone else also ni?'ed
> on the bug.  So I cannot reach the comment where someone ni? me.

ah, that's an issue.  filed as bug 1211891.

> It's actually worse, since when I'm the only one ni?'ed I no longer can reach the
> comment easily.

there's a link at the top of the bug before you switch to edit mode.

> Note that I'm using BugzillaTweaks.

i strongly suggest disabling/uninstalling that addon.
 
> Note2: when I click [Edit] (is there a pref to always edit, please)

there isn't a pref for always-edit, nor are there any plans to do so.

> I cannot see the list of ni? people anymore..

rather than duplicate the list of flags in two locations, all needinfo requests are shown only below new comment textarea.  there's a button in the flag header section which scrolls you to the comment box.
Flags: needinfo?(glob)
Component: Extensions: Needinfo → Extensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: