Edgewall Software
Modify

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#11169 closed enhancement (fixed)

`render_resource_link` should add the "missing" class if the resource doesn't exist

Reported by: Ryan J Ollos <ryan.j.ollos@…> Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: rendering Version: 1.0-stable
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:

When get_resource_description doesn't return an Element (e.g. when the IResourceManager doesn't implement get_resource_description), render_resource_link adds the missing class and the rel=nofollow attribute if the resource has a ResourceManager and the resource doesn't exist. The realm is always added as a class.

Added assertion with error message to check that the first argument to ComponentMeta is a ComponentManager.

Internal Changes:

Description

In the case that get_resource_description doesn't return an Element, render_resource_link should check whether the resource exists and add the missing class if it doesn't.

This is mainly useful for plugins at the moment, since render_resource_link is only utilized once in the Trac source. In this one case that it is used, a context parameter is passed, so an Element will be returned by render_resource_link and the pathway that this ticket proposes to modify is not being executed in that case.

I've prepared a patch in ff9c10ec. I don't like how the test case adds a dependency on trac.wiki.model.WikiPage, but I didn't have any better ideas at the moment and struggled to figure out the right way to code a "dummy" ResourceManager. I'll iterate on the patch if anyone can provide suggestions on how to improve.

Attachments (0)

Change History (8)

comment:1 by Ryan J Ollos <ryan.j.ollos@…>, 7 years ago

  • 53e03e02 Added rel='nofollow' attribute to the 'missing link'.

comment:2 by Ryan J Ollos, 7 years ago

Milestone: 1.1.2
Owner: set to Ryan J Ollos
Status: newassigned

comment:3 by Ryan J Ollos, 7 years ago

The test cases have been improved. Revised changes proposed in rjollos.git:t11169.2:

  • adds the realm as a class for the link.
  • adds the missing class and the rel=nofollow attribute to the link if the resource has a ResourceManager and the resource does not exist.

comment:4 by Ryan J Ollos, 7 years ago

API Changes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [12308].

comment:5 by Ryan J Ollos, 7 years ago

While implementing this ticket, I noticed a small improvement that might help plugin developers. I was seeing this traceback due to an improper call:

======================================================================
ERROR: test_resource_missing (__main__.RenderResourceLinkTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/Workspace/t11169/teo-rjollos.git/trac/tests/resource.py", line 76, in setUp
    self.resource_manager = FakeResourceManager()
  File "trac/core.py", line 128, in __call__
    compmgr = args[0]
IndexError: tuple index out of range

----------------------------------------------------------------------

Proposed patch is:

  • trac/core.py

    diff --git a/trac/core.py b/trac/core.py
    index 4b93ca3..6385f4f 100644
    a b class ComponentMeta(type):  
    125125            return self
    126126
    127127        # The normal case where the component is not also the component manager
     128        assert len(args) >= 1 and isinstance(args[0], ComponentManager), \
     129               "First argument must be a ComponentManager instance"
    128130        compmgr = args[0]
    129131        self = compmgr.components.get(cls)
    130132        # Note that this check is racy, we intentionally don't use a
  • trac/tests/core.py

    diff --git a/trac/tests/core.py b/trac/tests/core.py
    index 862de27..71ddddc 100644
    a b class ComponentTestCase(unittest.TestCase):  
    341341        instance = ComponentA(mgr)
    342342        self.assertIsNone(mgr[ComponentA])
    343343
     344    def test_invalid_argument_raises(self):
     345        """
     346        AssertionError is raised when first argument to initializer is not a
     347        ComponentManager instance.
     348        """
     349        class ComponentA(Component):
     350            pass
     351        self.assertRaises(AssertionError, Component)
     352
    344353
    345354def suite():
    346355    return unittest.makeSuite(ComponentTestCase, 'test')

comment:6 by Ryan J Ollos, 7 years ago

There were a few errors when running the full test suite, such as:

======================================================================
FAIL: test_explicit_failure (__main__.WithTransactionTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/Workspace/t11390/teo-rjollos.git/trac/db/tests/api.py", line 122, in test_explicit_failure
    @with_transaction(env, db)
  File "trac/db/api.py", line 77, in with_transaction
    dbm = DatabaseManager(env)
  File "trac/core.py", line 129, in __call__
    "First argument must be a ComponentManager instance"
AssertionError: First argument must be a ComponentManager instance

I've improved the patch in log:rjollos.git:t11169.3 to avoid the test failures. Does it look okay?

comment:7 by Jun Omae, 7 years ago

LGTM.

comment:8 by Ryan J Ollos, 7 years ago

API Changes: modified (diff)

Thanks for reviewing. Committed to 1.0-stable in [12311]; merged to trunk in [12312].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
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.