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

[Ryan J Ollos]

Test failure in automated builds, but passing for me locally with LXML 3.3.5. I'm upgrading to 3.4.0 and testing.

======================================================================
FAIL: runTest (trac.ticket.tests.functional.RegressionTestTicket10984)
Test for regression of http://trac.edgewall.org/ticket/10984
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/build/edgewall/trac/trac/ticket/tests/functional.py", line 2143, in runTest
self._tester.logout()
File "/home/travis/build/edgewall/trac/trac/tests/functional/tester.py", line 66, in logout
tc.submit('logout', 'logout')
File "/home/travis/build/edgewall/trac/trac/tests/functional/better_twill.py", line 192, in better_submit
b.submit(fieldname, formname)
File "/home/travis/build/edgewall/trac/trac/tests/functional/better_twill.py", line 188, in better_browser_submit
old_submit(fieldname)
File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/twill/browser.py", line 467, in submit
self._journey('open', request)
File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/twill/browser.py", line 549, in _journey
callable(func_name, *args, **kwargs)
File "/home/travis/build/edgewall/trac/trac/tests/functional/better_twill.py", line 126, in _validate_xhtml
_format_error_log(page, e.error_log))

TwillAssertionError:

# IDREF attribute for references an unknown ID "field-cc"
# URL: http://127.0.0.1:8999/ticket/65
# Line 530, column 3
By <a href="http://www.edgewall.org/">Edgewall Software</a>.</p>
<p class="right">Visit the Trac open source project at<br /><a href="http://trac.edgewall.org/">http://trac.edgewall.org/</a></p>
</div>
</body>
</html>

[Jun Omae]

Same here with lxml 3.3.5. See 786c3e0d5f4222e766dad1cd4b8d6a291696eeff.tar.xz​.

I guess that id="field-cc" doesn't exist and that occurs if unauthenticated user without TICKET_EDIT_CC and no email set in prefs, at branches/1.0-stable/trac/ticket/templates/ticket.html@13355:308-313#L299​.

[Ryan J Ollos]

Thanks, not sure why it still doesn't fail for me. How does this patch look?

trac/ticket/templates/ticket.html

@@ -284 +284 @@
                             <span py:when="field.name == 'cc'">
                               <py:choose>
                                 <!--! Unauthenticated user without TICKET_EDIT_CC and with email set in prefs -->
-                                <py:when test="'cc_entry' in field and field.cc_action is not None">
-                                  <em>$field.cc_entry</em>
+                                <py:when test="'cc_entry' in field" py:choose="">
+                                  <em py:when="field.cc_action is None" i18n:msg="">
+                                    Set your email in <a href="${href.prefs()}" class="trac-target-new">Preferences</a>
+                                  </em>
+                                  <em py:otherwise="">$field.cc_entry</em>
                                   <input type="checkbox" id="field-cc" name="cc_update"
                                          title="This checkbox allows you to add or remove yourself from the CC list."
-                                         checked="${field.cc_update or None}" />
+                                         disabled="${field.cc_action is None or None}" checked="${field.cc_update or None}" />
                                 </py:when>
                                 <!--! Unauthenticated user without TICKET_EDIT_CC and no email set in prefs -->
-                                <py:when test="'cc_entry' in field">
-                                  <span  class="hint" i18n:msg="">
-                                    Set your email in <a href="${href.prefs()}" class="trac-target-new">Preferences</a>
-                                  </span>
+                                <py:when test="field.cc_action is None">
+
                                 </py:when>
                                 <!--! TICKET_EDIT_CC is allowed -->
                                 <py:otherwise>

[Jun Omae]

Okay, all unit and functional tests pass with the patch.

Trying the patch, if anonymous has email and no TICKET_EDIT_CC, the email would be shown. But the email text is not clickable to toggle Cc checkbox. I think it would be user-friendly to allow to click it.

-                                  <em py:otherwise="">$field.cc_entry</em>
+                                  <label for="field-cc" py:otherwise=""><em>$field.cc_entry</em></label>

[Ryan J Ollos]

Replying to jomae:

Trying the patch, if anonymous has email and no TICKET_EDIT_CC, the email would be shown. But the email text is not clickable to toggle Cc checkbox. I think it would be user-friendly to allow to click it.

Thanks, I've felt similarly and considered the same change. I just was never sure if two labels were allowed for an input per HTML specifications. I'll apply both patches now to get the tests passing again.

[Ryan J Ollos]

Fixed in [13362:13363]. Wrapped e-mail address with label in [13364:13365].