Edgewall Software
Modify

Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#10742 closed enhancement (fixed)

Comment TracLinks without a ticket id should not be shown in strikethrough

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: rendering Version: 0.12-stable
Severity: minor Keywords: bitesized, traclinks
Cc: Branch:
Release Notes:

Ticket comment TracLinks are only rendered as followable when the resource exists and the user has permission to view the resource. Comment TracLinks are not rendered with strikethrough font when the context is the resource to which they refer.

API Changes:

Description

As originally discussed in ticket:9981, TracLinks within a ticket such as comment:1 should not be shown in strike-through font. Links pointing to other tickets, such as comment:1ticket:9981, should be shown in strike-through font, which was the intent of #9981.

Attachments (3)

t10742-r11086-1.patch (4.8 KB ) - added by Ryan J Ollos <ryano@…> 7 years ago.
Patch again r11086 of the trunk to address items (1) through (3).
t10742-r11086-2.patch (7.6 KB ) - added by Ryan J Ollos <ryano@…> 7 years ago.
Patch again r11086 of the trunk to address items (1) through (5).
t10742-r12011-1.patch (7.2 KB ) - added by Ryan J Ollos 6 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 by Ryan J Ollos <ryano@…>, 7 years ago

The patch attempts to resolve a couple of issues:

  1. TracLinks of the form comment:N are no longer shown in strike-through font when the enclosing ticket is closed (and neither are TracLinks of the form comment:N:ticket:M, when present in ticket M).
  2. Possible error in #9981: class_=status was added to the HTML generated for TracLinks in the comment realm, but that was inconsistent with class_='%s ticket' % status that is applied in _format_link to the HTML generated for TracLinks in the ticket realm. It doesn't appear this presented any immediate problems.
  3. Possible error in #9981: The functional test added in [10643] was expecting a link with class closed ticket, but the test was still passing despite issue (2) because it was testing the markup ticket:%s#comment:1, which was actually testing a code pathway through _format_link, not _format_comment_link.

I added and modified some tests in trunk/trac/ticket/tests/wikisyntax.py. As best I could determine though, without significant modifications to trunk/trac/wiki/tests/formatter.py it would not be possible to add a test for a comment:N link in wikisyntax.py since the tests are setup and executed in a wikipage. The comment here might be relevant, but I don't understand it.

Here are some issues I'm considering addressing in a follow-on patch:

  1. TracLinks of the form comment:N:ticket:M should display with class missing ticket when ticket M does not exist or the user doesn't have permission to view ticket M. For example, compare: ticket:100000 vs comment:1:ticket:100000 - the latter should display as a missing link.
  2. comment:N and comment:N:ticket:M should display with class missing comment when ticket M does exist and the user has permission to view ticket M, but comment N does not exist. For example, compare: comment:1 vs comment:100000 and comment:1:ticket:1 vs comment:10000:ticket:1 - the latter in both comparisons the latter example should display as a missing link.

by Ryan J Ollos <ryano@…>, 7 years ago

Attachment: t10742-r11086-1.patch added

Patch again r11086 of the trunk to address items (1) through (3).

comment:2 by Ryan J Ollos <ryano@…>, 7 years ago

The second patch I'm posting addresses items (1) through (5). I plan to spend some more time with it tomorrow evening, but it is currently passing the unit tests, so I thought it would be worthwhile to post it for feedback.

by Ryan J Ollos <ryano@…>, 7 years ago

Attachment: t10742-r11086-2.patch added

Patch again r11086 of the trunk to address items (1) through (5).

comment:3 by Ryan J Ollos <ryano@…>, 7 years ago

Keywords: bitesized added; bitsized removed

comment:4 by Christian Boos, 7 years ago

Milestone: next-stable-1.0.x

Patch looks good, we'll have a look.

comment:5 by Christian Boos, 6 years ago

Milestone: next-stable-1.0.x1.0.2
Owner: set to Christian Boos
Status: newassigned

comment:6 by Ryan J Ollos, 6 years ago

Keywords: traclinks added
Owner: changed from Christian Boos to Ryan J Ollos
Reporter: changed from Ryan J Ollos <ryano@…> to Ryan J Ollos

by Ryan J Ollos, 6 years ago

Attachment: t10742-r12011-1.patch added

in reply to:  1 comment:7 by Ryan J Ollos, 6 years ago

Replying to Ryan J Ollos <ryano@…>:

I added and modified some tests in trunk/trac/ticket/tests/wikisyntax.py. As best I could determine though, without significant modifications to trunk/trac/wiki/tests/formatter.py it would not be possible to add a test for a comment:N link in wikisyntax.py since the tests are setup and executed in a wikipage. The comment here might be relevant, but I don't understand it.

It seems that TracLinks of the form comment:N can easily be tested by using the context parameter with formatter.suite.

in reply to:  1 ; comment:8 by Ryan J Ollos, 6 years ago

Replying to Ryan J Ollos <ryano@…>:

  1. comment:N and comment:N:ticket:M should display with class missing comment when ticket M does exist and the user has permission to view ticket M, but comment N does not exist. For example, compare: comment:1 vs comment:100000 and comment:1:ticket:1 vs comment:10000:ticket:1 - the latter in both comparisons the latter example should display as a missing link.

The comment class can't be used for TracLinks since it is already used on the ticket page for the div that encloses the comment text. I've the class ticket-comment instead. I wonder though whether there is any value in adding this class, or whether I should just use the ticket class for TracLinks prefixed with ticket: or comment:.

I plan to give this another thorough review in the next day or so, but the latest proposed changes can be found in rjollos.git:t10742.

Version 0, edited 6 years ago by Ryan J Ollos (next)

comment:9 by Jun Omae, 6 years ago

LGTM except 416c4e86/rjollos.git. Many suite.addTest() lines in functionalSuite are commented out.

comment:10 by Ryan J Ollos, 6 years ago

Thanks. Someday maybe I will investigate a way to allow running a single test in a module without resulting to commenting out all of the other tests. I'm bound to make this mistake again!

comment:11 by Ryan J Ollos, 6 years ago

Unrelated change committed to 1.0-stable in [12117] and merged to trunk in [12118].

in reply to:  8 comment:12 by Ryan J Ollos, 6 years ago

Replying to rjollos:

I've added the class ticket-comment instead. I wonder though whether there is any value in adding this class, or whether I should just use the ticket class for TracLinks prefixed with ticket: or comment:.

The latest proposed changes can be found in log:rjollos.git:t10742.2. The ticket-comment class has been removed and a test case has been fixed.

comment:13 by Ryan J Ollos, 6 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed with minor changes to 1.0-stable in [12126:12135] and merged to trunk in [12136]

comment:14 by Ryan J Ollos, 6 years ago

In case anyone is able to cleanup the commit log messages, [12126] should be prefixed with 1.0.2dev, not 0.0.2dev.

comment:15 by Ryan J Ollos, 5 years ago

Keywords:

comment:16 by Ryan J Ollos, 5 years ago

Keywords: traclink added; traclinks removed

comment:17 by Ryan J Ollos, 5 years ago

Keywords: traclinks added; traclink removed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.