The "View Tickets" mainnav entry should be a link to the query page when the user possesses TICKET_VIEW but not REPORT_VIEW

[Ryan J Ollos <ryan.j.ollos@…>]

The View Tickets mainnav entry links to the /report page, both of which are only displayed if the user has the REPORT_VIEW permission. TICKET_VIEW is the only permission required to view the /query page, however if the user doesn't have REPORT_VIEW, there is no direct way to navigate to the /query page.

I think it would make sense to have the View Tickets mainnav entry link to the /query page when the user has TICKET_VIEW, but does not have REPORT_VIEW.

[Christian Boos]

Right, that's already what happens when the report module is disabled.

[Ryan J Ollos <ryan.j.ollos@…>]

​293d59cf implements the behavior discussed in comment:description, followed by a minor refactoring ​8531b6a8.

It looks to me that Component.config is an in-memory object that is not recreated as the result of a Configuration.set or even a Configuration.save method call. Therefore, I can't see a reason to call Configuration.save within a test in which we only want the configuration to persist within the scope of the test case. If that makes sense, it would suggest that in the following we could drop the "save".

env = self._testenv.get_trac_environment()
env.config.set(...)
env.config.save()
try:
    ...
finally:
    env.config.remove(...)

It also seems unnecessary to call self._testenv.restart()​ after a configuration change.

I'll wait for feedback on those two issues before making the changes.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

It looks to me that Component.config is an in-memory object that is not recreated as the result of a Configuration.set or even a Configuration.save method call.

I think I was wrong about this, since additional testing has shown that the save calls can't be removed. I'll have to look at it in more detail later to understand it better.

[Ryan J Ollos]

Committed to 1.0-stable in [11824].

[Ryan J Ollos]

In [13053], fixed incorrect test name from [11824]. Merged in [13054].

Also, I'm thinking that the following is equivalent, but the intention is more clear:

trac/ticket/query.py

@@ -882 +882 @@
     def get_navigation_items(self, req):
         from trac.ticket.report import ReportModule
         if 'TICKET_VIEW' in req.perm and \
-                not (self.env.is_component_enabled(ReportModule) and
-                     'REPORT_VIEW' in req.perm):
+                (not self.env.is_component_enabled(ReportModule) or
+                 'REPORT_VIEW' not in req.perm):
             yield ('mainnav', 'tickets',
                    tag.a(_('View Tickets'), href=req.href.query()))

[Ryan J Ollos]

I've replaced the functional test with unit tests and added the refactoring from comment:7 in log:rjollos.git:t11152. The unit tests might then be extended to test fine-grained permission checks for the navigation contributor proposed in #11697. However, I'll wait until milestone:1.0.3 before committing these changes. Please let me know if any reasons are seen for keeping the functional test rather than the unit tests.

[Ryan J Ollos]

It's pretty ugly having to check whether another navigation item has been added by checking if the component is enabled and the user has the appropriate permission. I wonder how generally applicable a Priority value would be. For the case shown in comment:7, QueryModule would return a navigation item named ticket that has less priority than the one return by ReportModule (i.e. won't be present when the navigation item from ReportModule is present). Another case of overriding can be found with the ​ExtendedVersionPlugin.

[Ryan J Ollos]

Replying to rjollos:

Also, I'm thinking that the following is equivalent, but the intention is more clear:

trac/ticket/query.py

@@ -882 +882 @@
     def get_navigation_items(self, req):
         from trac.ticket.report import ReportModule
         if 'TICKET_VIEW' in req.perm and \
-                not (self.env.is_component_enabled(ReportModule) and
-                     'REPORT_VIEW' in req.perm):
+                (not self.env.is_component_enabled(ReportModule) or
+                 'REPORT_VIEW' not in req.perm):
             yield ('mainnav', 'tickets',
                    tag.a(_('View Tickets'), href=req.href.query()))

Committed to 1.0-stable in [13233], merged to trunk in [13234].

[Ryan J Ollos]

Replying to rjollos:

I've replaced the functional test with unit tests and added the refactoring from comment:7 in log:rjollos.git:t11152.

Committed to 1.0-stable in [13241], merged to trunk in [13242].