Hide the milestone select from the ticket form if the user doesn't have MILESTONE_VIEW

[Ryan J Ollos]

If the user doesn't have MILESTONE_VIEW the milestone select is shown with no options. It might as well be hidden and not clutter up the ticket form.

Hiding it also exposes less information because the user without MILESTONE_VIEW doesn't know whether there are any milestones defined. After the patch, the behavior is the same whether there are milestones defined but the user doesn't have MILESTONE_VIEW or there are no milestones defined. In both cases there are no mentions of milestone on the ticket form.

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

Patch again r11483 of the trunk.

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

t10984-r11483-1.patch​ includes a functional test case, but I'm not very satisfied with how it is structured (same concerns as in comment:6:ticket:10957), so I'd appreciate feedback on how I can write a better test case.

I stuck with the presumed convention of naming these test cases RegressionTestTicket#### (e.g. #8861 & test case), even though I wouldn't consider this to be a regression.

[Christian Boos]

Looks reasonable.

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

Following #10957 and #11028, I proceeded to rebase this patch against the current 1.0-stable and discovered a few issues.

Should a user without MILESTONE_VIEW be able to see the name of the ticket's milestone in the ticket properties box? The user can view the ticket's milestone on the query page, so if the answer is no, we have other problems to solve as well.

Here are some potential problems:

• t10984-r11483-1.patch​ causes the milestone field to be removed from the ticket properties box if the user doesn't have MILESTONE_VIEW.
• The current 1.0-stable allows the user to unset the milestone, even if the user doesn't have MILESTONE_VIEW. This is because the ticket's current milestone and an empty option are always added to the select, regardless of whether the user has MILESTONE_VIEW.

Another behavior that feels a bit inconsistent is that when the user doesn't have MILESTONE_VIEW the milestone filter on the query page is empty except for the group labels, however, as mentioned, the ticket milestones can still be viewed as a column in the table.

I'm not sure what the right thing to do is here, but I think the answer lies in defining what the MILESTONE_VIEW permission should allow. Is its primary purpose to grant viewing of the Milestone pages, or should it also grant viewing what milestone a ticket is associated with?

[Christian Boos]

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

I'm not sure what the right thing to do is here, but I think the answer lies in defining what the MILESTONE_VIEW permission should allow. Is its primary purpose to grant viewing of the Milestone pages,

Yes.

or should it also grant viewing what milestone a ticket is associated with?

If you can't see that milestone and just have the name, it doesn't do much "harm", it's just a kind of dead link. Consider a WikiPageName link pointing to a view protected page, in this case we don't remove the page name from the text, we just don't show it as a link. I suppose we can do something similar here.

The fact that it's possible to modify the ticket's milestone field even though there's no milestone view permission is indeed a problem, but more a consistency issue than a violation of the view permission itself.

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

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

• The current 1.0-stable allows the user to unset the milestone, even if the user doesn't have MILESTONE_VIEW. This is because the ticket's current milestone and an empty option are always added to the select, regardless of whether the user has MILESTONE_VIEW.

Another behavior that feels a bit inconsistent is that when the user doesn't have MILESTONE_VIEW the milestone filter on the query page is empty except for the group labels, however, as mentioned, the ticket milestones can still be viewed as a column in the table.

It is also possible to batch modify the milestone field to set the milestone to the empty value even when the user doesn't have MILESTONE_VIEW permission.

Another inconsistency is that, while a ticket's milestone can't be set to a closed milestone from the ticket page if the user doesn't have TICKET_ADMIN, it is possible to perform that action from the batch modify page without having TICKET_ADMIN.

The latter issue appears difficult to fix because the dictionary of ticket fields is shared for the query filter and batch modify, and we don't want to impose the same restrictions on the query filter that we do on the batch modify.

[Ryan J Ollos]

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

Here are some potential problems:

• t10984-r11483-1.patch​ causes the milestone field to be removed from the ticket properties box if the user doesn't have MILESTONE_VIEW.
• The current 1.0-stable allows the user to unset the milestone, even if the user doesn't have MILESTONE_VIEW. This is because the ticket's current milestone and an empty option are always added to the select, regardless of whether the user has MILESTONE_VIEW.

I'm going to focus on the ticket page for this ticket, and deal with the query page elsewhere.

Currently the 'skip' entry in the dictionary is used to control rendering in the ticket properties display box and ticket properties modify box. I'm considering replacing skip with viewable and editable so that we can independently control what is rendered in each box. This might also allow localizing more of the logic in the Python code, such as not in ('type', 'owner') from fields = [f for f in fields if not f.skip and f.name not in ('type', 'owner')]. This would also make it more straightforward in the future to do VIEW and MODIFY permission checks in _prepare_fields for other fields.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t10984.1.

When the user doesn't have MILESTONE_VIEW, the milestone can still be seen in the ticket properties box:

However, the Milestone select won't be rendered:

For reference, this issue had also been discussed previously in #8778 and ​gmessage:trac-users:1itmIo9Obq8/dzMDNn6gS80J.

Changes in this ticket are applicable to future work in #1233, where Components and Versions will become resources. In the future we might also consider adding modify_perm and view_perm attributes to TracTicketsCustomFields.

[Christian Boos]

Replying to rjollos:

Proposed changes in log:rjollos.git:t10984.1.

Looks good to me.

[Ryan J Ollos]

Thanks for the review. Committed to 1.0-stable in [13355], merged to trunk in [13356].