Edgewall Software
Modify

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#11152 closed enhancement (fixed)

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

Reported by: Ryan J Ollos <ryan.j.ollos@…> Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: query system Version: 1.0-stable
Severity: normal Keywords: permissions report query navigation
Cc: Branch:
Release Notes:

The View Tickets mainnav item links to the /query page when the user has TICKET_VIEW and either the ReportModule is disabled or the user doesn't have REPORT_VIEW.

API Changes:

Description (last modified by Christian Boos)

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.

Attachments (0)

Change History (11)

comment:1 by Christian Boos, 6 years ago

Milestone: next-stable-1.0.x

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

comment:2 by Christian Boos, 6 years ago

Description: modified (diff)

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

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.

in reply to:  3 comment:4 by Ryan J Ollos <ryan.j.ollos@…>, 6 years ago

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.

comment:5 by Ryan J Ollos, 6 years ago

Milestone: next-stable-1.0.x1.0.2
Owner: set to Ryan J Ollos
Status: newassigned

comment:6 by Ryan J Ollos, 6 years ago

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

Committed to 1.0-stable in [11824].

comment:7 by Ryan J Ollos, 5 years ago

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

    commit 295c6c3df54981499155e263f265ab81643e62b8
    Author: rjollos <ryan.j.ollos@gmail.com>
    Date:   Wed Jul 30 08:55:33 2014 -0700
    
        More readable expression.
    
    diff --git a/trac/ticket/query.py b/trac/ticket/query.py
    index 44c225c..0132abd 100644
    a b class QueryModule(Component):  
    882882    def get_navigation_items(self, req):
    883883        from trac.ticket.report import ReportModule
    884884        if 'TICKET_VIEW' in req.perm and \
    885                 not (self.env.is_component_enabled(ReportModule) and
    886                      'REPORT_VIEW' in req.perm):
     885                (not self.env.is_component_enabled(ReportModule) or
     886                 'REPORT_VIEW' not in req.perm):
    887887            yield ('mainnav', 'tickets',
    888888                   tag.a(_('View Tickets'), href=req.href.query()))

comment:8 by Ryan J Ollos, 5 years ago

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.

comment:9 by Ryan J Ollos, 5 years ago

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.

in reply to:  7 comment:10 by Ryan J Ollos, 5 years ago

Replying to rjollos:

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

  • trac/ticket/query.py

    commit 295c6c3df54981499155e263f265ab81643e62b8
    Author: rjollos <ryan.j.ollos@gmail.com>
    Date:   Wed Jul 30 08:55:33 2014 -0700
    
        More readable expression.
    
    diff --git a/trac/ticket/query.py b/trac/ticket/query.py
    index 44c225c..0132abd 100644
    a b class QueryModule(Component):  
    882882    def get_navigation_items(self, req):
    883883        from trac.ticket.report import ReportModule
    884884        if 'TICKET_VIEW' in req.perm and \
    885                 not (self.env.is_component_enabled(ReportModule) and
    886                      'REPORT_VIEW' in req.perm):
     885                (not self.env.is_component_enabled(ReportModule) or
     886                 'REPORT_VIEW' not in req.perm):
    887887            yield ('mainnav', 'tickets',
    888888                   tag.a(_('View Tickets'), href=req.href.query()))

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

in reply to:  8 comment:11 by Ryan J Ollos, 5 years ago

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].

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.