Modify

Opened 3 years ago

Closed 20 months ago

Last modified 18 months ago

#9212 closed enhancement (fixed)

Closed tickets should display with strike-through

Reported by: rjollos Owned by: rjollos
Priority: high Component: BookmarkPlugin
Severity: normal Keywords:
Cc: jun66j5 Trac Release: 0.11

Description

As in normal wiki markup, it would be nice if the TracLinks to tickets that are displayed through the BookmarkPlugin would display with strike-through font when the ticket is closed.

I may be able to provide a patch for this soon. See #3610 for a related discussion.

Attachments (0)

Change History (21)

comment:1 Changed 3 years ago by rjollos

  • Cc jun66j5 added; anonymous removed

comment:2 follow-up: Changed 2 years ago by rjollos

  • Owner changed from saigon to rjollos
  • Status changed from new to assigned

We need to pass the ticket status to the template, and the same will be true for milestone status. Here is a quick and dirty version:

  • bookmarkplugin/trunk/tracbookmark/templates/list.html

     
    99    <div style="margin-bottom: 3em;">
    1010    <ul class="bookmarks">
    1111      <py:for each="bookmark in bookmarks">
    12         <li class="${bookmark.realm}">
    13             <a href="${bookmark.url}">${bookmark.linkname}</a>
    14                 ${bookmark.name} (<a href="${bookmark.delete}">delete</a>)
    15         </li>
     12        <li><a href="${bookmark.url}" class="${bookmark.realm}">${bookmark.linkname}</a>
     13            ${bookmark.name} (<a href="${bookmark.delete}">delete</a>)</li>
    1614      </py:for>
    1715    </ul>
    1816    </div>
  • bookmarkplugin/trunk/tracbookmark/__init__.py

     
     1# -*- coding: utf-8 -*-
     2
    13import re
    24from trac.core import *
    35from trac.config import ListOption
     
    212214                if realm == 'ticket':
    213215                    linkname = get_resource_shortname(self.env, resource)
    214216                    name = get_resource_summary(self.env, resource)
     217                    from trac.ticket.model import Ticket
     218                    realm = Ticket(self.env, resource.id)['status'] + ' ' + realm
    215219                elif realm == 'milestone':
    216220                    linkname = get_resource_shortname(self.env, resource)
    217221                elif realm == 'wiki':

I propose we add class to the template's data dictionary, modify the template as shown in the patch and populate the class variable within the if realm == conditional.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 2 years ago by jun66j5

Replying to rjollos:

I propose we add class to the template's data dictionary, modify the template as shown in the patch and populate the class variable within the if realm == conditional.

Looks good!

Additionally, it would be better to add missing to the class attribute when a ticket doesn't exist. And, we should check the existence of bookmarked resources for other than tickets...

  • tracbookmark/__init__.py

    diff --git a/tracbookmark/__init__.py b/tracbookmark/__init__.py
    index 342ea90..1bfa9fa 100644
    a b from trac.env import IEnvironmentSetupParticipant 
    77from trac.web.api import IRequestFilter, IRequestHandler, Href
    88from trac.web.chrome import ITemplateProvider, add_stylesheet, \
    99                            add_script, add_notice
    10 from trac.resource import Resource, get_resource_description, get_resource_shortname, get_resource_summary
     10from trac.resource import (
     11    Resource, ResourceNotFound, get_resource_description,
     12    get_resource_shortname, get_resource_summary)
    1113from trac.db import DatabaseManager, Table, Column
    1214from trac.perm import IPermissionRequestor
    1315from trac.util import get_reporter_id
    class BookmarkSystem(Component): 
    213215            if resource:
    214216                if realm == 'ticket':
    215217                    linkname = get_resource_shortname(self.env, resource)
    216                     name = get_resource_summary(self.env, resource)
    217                     from trac.ticket.model import Ticket
    218                     realm = Ticket(self.env, resource.id)['status'] + ' ' + realm
     218                    try:
     219                        name = get_resource_summary(self.env, resource)
     220                    except ResourceNotFound, e:
     221                        name = ''
     222                        realm = 'missing ' + realm
     223                    else:
     224                        from trac.ticket.model import Ticket
     225                        realm = Ticket(self.env, resource.id)['status'] + ' ' + realm
    219226                elif realm == 'milestone':
    220227                    linkname = get_resource_shortname(self.env, resource)
    221228                elif realm == 'wiki':

comment:4 Changed 2 years ago by rjollos

Thanks for the feedback and additional suggestions (and pushing patches for those other tickets just now). I'm almost finished implementing this and will push the changes later today.

comment:5 Changed 2 years ago by rjollos

  • Priority changed from normal to high

comment:6 in reply to: ↑ 3 Changed 20 months ago by rjollos

Replying to jun66j5:

And, we should check the existence of bookmarked resources for other than tickets...

Good point. I'll make sure to do that as part of this ticket.

comment:7 Changed 20 months ago by rjollos

(In [12985]) Refs #9212:

  • Closed tickets are displayed with strike-through font on the /bookmark page.
  • Missing tickets have the missing class, are rendered as light-grey links with no href attribute on the /bookmark page and are not displayed on the bookmark menu.
  • Resolved issue in which all pages on the Trac site would display a TracError if a non-existent ticket was bookmarked. This could occur in cases that the ticket was bookmarked and then deleted.

Thanks to Jun Omae for a major contributions to this changeset.

comment:8 Changed 20 months ago by rjollos

(In [12986]) Refs #9212: Organized imports.

comment:9 Changed 20 months ago by rjollos

It looks like the assignment name = '' can be dropped in the except clause. I'll make that change in a follow-up commit.

comment:10 Changed 20 months ago by rjollos

I also just noticed that I broke the unit tests with [12985]. I'll fix the existing unit tests and add cases to cover the issues fixed in this ticket.

comment:11 Changed 20 months ago by rjollos

After fixing the unit tests, I'm still seeing one failure with Trac 1.0.2dev, which is probably related to a change in what the Trac API returns rather than any of the changes associated with this ticket:

======================================================================
FAIL: test_format_name_changeset (tracbookmark.tests.BookmarkSystemTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/Workspace/trac-hacks.git/bookmarkplugin/trunk/tracbookmark/tests/__init__.py", line 112, in test_format_name_changeset
    self.assertEquals('Changeset 42', data['name'])
AssertionError: 'Changeset 42' != 'Changeset 42 in trunk'

----------------------------------------------------------------------
Ran 9 tests in 0.047s

FAILED (failures=1)

comment:12 Changed 20 months ago by rjollos

Bookmarking a file results in the following:

How to Reproduce

While doing a GET operation on /browser, Trac issued an internal error.

(please provide additional details here)

Request parameters:

{'path': '/'}

User agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.68 Safari/537.17

System Information

Trac 1.0.2dev
Genshi 0.6 (without speedups)
pysqlite 2.6.0
Python 2.7.3 (default, Jan 2 2013, 13:56:14)
[GCC 4.7.2]
pytz 2012c
setuptools 0.6
SQLite 3.7.13
Subversion 1.6.17 (r1128011)
jQuery 1.7.2

Enabled Plugins

TracBookmark 0.1dev-r12986

Python Traceback

Traceback (most recent call last):
  File "/home/user/Workspace/th9212-r12986/trac.git/trac/web/main.py", line 497, in _dispatch_request
    dispatcher.dispatch(req)
  File "/home/user/Workspace/th9212-r12986/trac.git/trac/web/main.py", line 224, in dispatch
    self._post_process_request(req, *resp)
  File "/home/user/Workspace/th9212-r12986/trac.git/trac/web/main.py", line 338, in _post_process_request
    resp = f.post_process_request(req, *resp)
  File "/home/user/Workspace/bookmarkplugin/trunk/tracbookmark/__init__.py", line 172, in post_process_request
    self.render_bookmarker(req)
  File "/home/user/Workspace/bookmarkplugin/trunk/tracbookmark/__init__.py", line 302, in render_bookmarker
    menu = self._get_bookmarks_menu(req)
  File "/home/user/Workspace/bookmarkplugin/trunk/tracbookmark/__init__.py", line 310, in _get_bookmarks_menu
    params = self._format_name(req, url)
  File "/home/user/Workspace/bookmarkplugin/trunk/tracbookmark/__init__.py", line 241, in _format_name
    name = get_resource_summary(self.env, resource)
  File "/home/user/Workspace/th9212-r12986/trac.git/trac/resource.py", line 347, in get_resource_summary
    return get_resource_description(env, resource, 'summary')
  File "/home/user/Workspace/th9212-r12986/trac.git/trac/resource.py", line 333, in get_resource_description
    return manager.get_resource_description(resource, format, **kwargs)
  File "/home/user/Workspace/th9212-r12986/trac.git/trac/versioncontrol/api.py", line 387, in get_resource_description
    node = repos.get_node(resource.id, resource.version)
AttributeError: 'NoneType' object has no attribute 'get_node'

comment:13 follow-up: Changed 20 months ago by rjollos

I started out thinking that the task of handling missing resources wouldn't be too much work, but it quickly spiraled into a significant amount of work. There are a lot of issues with bookmarking repository resources, and I'm sure my forthcoming changeset won't cover all the corner cases. I hope that it is at least a step in the right direction, and we can dedicate specific tickets to any of the corner cases.

I've attempted to handle all of the cases for which TracLinks might point to missing resources. TracLinks have two distinct behaviors. In the case of a TracLink to a non-existent wiki page, the link has an href attribute, giving the user the opportunity create the page. Milestones behave the same as wiki pages. Ticket links on the other hand do not have an href attribute. Most or all other TracLinks behave like ticket links.

Links to non-existent reports don't have the missing class, which I think is a bug (see t:#11166).

comment:14 follow-up: Changed 20 months ago by rjollos

It would be nice if we could delegate some of the work in BookmarkSystem._format_name to the Trac API. I did an experiment to see if it was possible to accomplish this using the Trac 0.11 API (tested with Trac 1.0.2dev, but I didn't attempt to use the latest API functions such as web_context since we are trying to keep the plugin backward compatible):

from trac.resource import render_resource_link
from trac.mimeview.api import Context
context = Context.from_request(req, resource)
link = render_resource_link(self.env, context, resource)

This gives us what we want for existing and non-existing milestones.

Existing milestone:

<a class="milestone" href="/tracdev/milestone/milestone2">Milestone milestone2</a>

Non-existing milestone:

<a class="missing milestone" href="/tracdev/milestone/milestone1" rel="nofollow">Milestone milestone1</a>

However, for wiki pages, the missing class isn't added.

Existing wiki page:

<a href="/tracdev/wiki/TracBrowser">TracBrowser</a>

Non-existing wiki page:

<a href="/tracdev/wiki/TracBrowserOne">TracBrowserOne</a>

WikiSystem doesn't use the context parameter to generate get_resource_description's return value, and therefore doesn't return a link element. However, render_resource_link could still call resource_exists to determine whether the missing class should be added to the link that it generates (see t:#11169).

Additionally, many of the Components, such as ReportModule, ChangesetModule, ... don't implement IResourceManager, so it's not possible to utilize the functions in the resource module to get a compact (TracLink) description of a resource, or a summary of the resource. This seems like a hole in the Trac API that we might be able to improve on later, but I can't see any way to simplify BookmarkSystem._format_name at this time.

comment:15 in reply to: ↑ 13 Changed 20 months ago by rjollos

Replying to rjollos:

In the case of a TracLink to a non-existent wiki page, the link has an href attribute, giving the user the opportunity create the page. Milestones behave the same as wiki pages.

TracLinks to missing Tickets and Milestones also have the rel="nofollow" attribute, so we should take care to add that as well for consistency.

comment:16 in reply to: ↑ 14 Changed 20 months ago by rjollos

Replying to rjollos:

Additionally, many of the Components, such as ReportModule, ChangesetModule, ... don't implement IResourceManager, so it's not possible to utilize the functions in the resource module to get a compact (TracLink) description of a resource, or a summary of the resource.

The IResourceManager for the changeset realm appears to be RepositoryManager, however its get_resource_description method doesn't use a context object to return an Element object, and it won't return a compact format for the resource.

comment:17 Changed 20 months ago by rjollos

I have some refactoring in mind:

  • The bookmarks list that is passed to the template will become a generator.
  • The generator will return a tuple of (resource_link, resource_name, delete_href), with resource_link being an a element.

In cases that render_resource_link returns rich content with the compact representation we want, we can just call that function to get resource_link. It looks like this will currently only work for the milestone realm. However, we can continue to push patches to the core to enhance render_resource_link for various realms, and that will simplify the BookmarkPlugin code in the future when we drop support for older versions of Trac.

The immediate advantage of creating the resource_link in the Python code as opposed to the template is that we can add additional attributes to the link elements, such as rel=nofollow, without passing more parameters in the template's dictionary. This should keep the code a bit simpler.

I will finish up the unit tests for this ticket, close this ticket, make a new one for the other issues notes here, and begin working from a branch on GitHub so that the repository history for this plugin can be kept a bit cleaner.

comment:18 Changed 20 months ago by rjollos

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [13007]) Fixes #9212:

  • Added and modified unit test cases to cover changes in [12985].
  • Accounted for differences in get_resource_description's output for the changeset realm pre and post Trac 0.12.
  • Moved instantiation of BookmarkSystem into the fixture's setUp method.
  • Removed unused imports from test case module.
  • Removed unnecessary variable assignment that was part of [12985].

comment:19 Changed 20 months ago by rjollos

Also, please let me know if you have any feedback or suggestions on the proposed or implemented changes here. Thanks!

comment:20 Changed 19 months ago by rjollos

As noted in comment:12:ticket:10942, with Trac 0.11 we get:

10:34:40 AM Trac[loader] DEBUG: Loading tracbookmark from /home/user/Workspace/trachacks.git/bookmarkplugin/trunk
10:34:40 AM Trac[loader] ERROR: Skipping "tracbookmark = tracbookmark": (can't import "cannot import name resource_exists")

comment:21 Changed 19 months ago by rjollos

(In [13226]) Refs #9212: Added compatibility code to restore compatibility for Trac <= 0.11.7.

Add Comment

Modify Ticket

Action
as closed The owner will remain rjollos.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.