274906 - Replies garner extra [edit] attachment links

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[[e-mail]]

If you reply to a message that references an attachment the [edit] is captured
into the reply, then an extra [edit] is generated when the reply is displayed.

Comment 1

[Gervase Markham [:gerv]]

Attachment: Here is an attachment

Comment 2

[Gervase Markham [:gerv]]

(In reply to comment #1)
> Created an attachment (id=168901) [edit]
> Here is an attachment

Here is a reply to the auto-generated message featuring the attachment.

Gerv

Comment 3

[Gervase Markham [:gerv]]

We have two choices. Either:

- Get the reply link to filter out the [edit], either specifically or all
matches of that string, or
- Don't add a second one if we detect that one is already there, but linkify it
instead.

Gerv

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
> - Get the reply link to filter out the [edit], either specifically or all
> matches of that string, or

So here we hack getText() in the JS script...


> - Don't add a second one if we detect that one is already there, but linkify it
> instead.

And here we modify GetAttachmentLink() in globals.pl.


Which solution is better? What if I write "toto [edit]"? The JS script would
remove it even if [edit] means nothing special here?

Personally, I would prefer to remove it when clicking the "reply" link, that
means to consider this in the JS script.

Or we could even imagine not to linkify bug numbers and/or attachments in
replies, but only in new texts.

Comment 5

[Max Kanat-Alexander]

I don't think that this is really blocking2.20+, it's pretty minor. It can be
fixed in 2.20.1 if we have to, if it's not fixed by 2.20...

Comment 6

[Stephen Lee]

The underlying problem is that a function that is only supposed to linkify text 
is changing the actual content of the text.

One idea I just had (which perhaps belongs in another bug if it's plausible): 
Why not make the [Edit] into a real button, rather than the word "Edit" in 
square brackets? ... or would that be unworkable e.g. due to nested forms?


Failing that (or in order to cope with historical data), the problem is created 
by the linkification code, so IMHO should be fixed by the linkification code 
too...

perhaps something like (globals.pl - by inspection, untested):
- $text =~ s~((?:^Created\ an\ |\b)attachment\s*\(id=(\d+)\))
+ $text =~ s~((?:^Created\ an\ |\b)attachment\s*\(id=(\d+)\)(\ \[Edit\])?)

- $text =~ s~\b(attachment\s*\#?\s*(\d+))
+ $text =~ s~\b(attachment\s*\#?\s*(\d+)(\ \[Edit\])?)

Comment 7

[Jacob Steenhagen]

I like the idea of a button, but it has to either be a nested form (which seems
to be a bad idea) or use a JavaScript onclick event... and because it's
JavaScript, we'd need to have a non-JavaScript alternative and that would
probably put us in the same boat we're in.

Comment 8

[Stephen Lee]

(In reply to comment #7)
> I like the idea of a button, but it has to either be a nested form (which
> seems to be a bad idea) or use a JavaScript onclick event...

Shortly after I submitted my comment here about a button I had (IMHO) a better 
idea - see bug 286427.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

We should stop appending [edit] to attachment links and start linking the
attachments directly to the edit page by default.  From that page, users who
want to see the raw attachment can click a link to take them to it.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

"If it's not a regression from 2.18 and it's not a critical problem with
something that's already landed, let's push it off." - Dave

Comment 11

[Christian Reis]

Agree 100% with Myk. It may be a bit painful now, but the edit page is the most
useful place to link to (the raw view page doesn't let you go _anywhere_, at
least not without hacking the URL).

Comment 12

[Jacob Steenhagen]

Attachment: Patch v1

I agree that the easiest way to solve this problem is to change the link to be
to the edit page instead of the attachment itself when in a comment.  There is
still a direct link to the attachment in the table above the comments and one
is available from the edit page. Bug 286427 can be about expanding the UI to
provide a popup <div/> with more info links in context.

Comment 13

[Marc Schumann [:Wurblzap]]

Comment on attachment 181872 [details] [diff] [review]
Patch v1

Happiness-flavoured r=wurblzap by inspection.

Comment 14

[Dave Miller [:justdave]]

wooot!  by all means! :)  That extra link always annoyed me :)

Comment 15

[Jacob Steenhagen]

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.321; previous revision: 1.320
done