Edgewall Software
Modify

Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#10984 closed enhancement (fixed)

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

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.3
Component: ticket system Version: 1.0
Severity: normal Keywords:
Cc: Branch:
Release Notes:

The milestone select is hidden from the ticket form if the user doesn't have MILESTONE_VIEW. Previously an empty select would be rendered.

API Changes:

Description

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.

Attachments (5)

t10984-r11483-1.patch (4.2 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 7 years ago.
Patch again r11483 of the trunk.
#modify.png (14.4 KB ) - added by Ryan J Ollos 5 years ago.
#properties.png (23.2 KB ) - added by Ryan J Ollos 5 years ago.
786c3e0d5f4222e766dad1cd4b8d6a291696eeff.tar.xz (114.9 KB ) - added by Jun Omae 5 years ago.
SetYourEmailInPrefs.png (10.0 KB ) - added by Ryan J Ollos 5 years ago.

Download all attachments as: .zip

Change History (24)

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

Attachment: t10984-r11483-1.patch added

Patch again r11483 of the trunk.

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

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.

comment:2 by Christian Boos, 7 years ago

Milestone: 1.0.2

Looks reasonable.

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

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?

in reply to:  3 comment:4 by Christian Boos, 7 years ago

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.

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

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.

comment:6 by Ryan J Ollos, 6 years ago

Owner: set to Ryan J Ollos
Status: newassigned

in reply to:  3 comment:7 by Ryan J Ollos, 6 years ago

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.

comment:8 by Ryan J Ollos, 6 years ago

Milestone: 1.0.2next-stable-1.0.x

comment:9 by Ryan J Ollos, 6 years ago

Milestone: next-stable-1.0.x1.1.3

comment:10 by Ryan J Ollos, 5 years ago

Reporter: changed from Ryan J Ollos <ryan.j.ollos@…> to Ryan J Ollos

by Ryan J Ollos, 5 years ago

Attachment: #modify.png added

by Ryan J Ollos, 5 years ago

Attachment: #properties.png added

comment:11 by Ryan J Ollos, 5 years ago

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.

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

in reply to:  11 comment:12 by Christian Boos, 5 years ago

Replying to rjollos:

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

Looks good to me.

comment:13 by Ryan J Ollos, 5 years ago

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

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

comment:14 by Ryan J Ollos, 5 years ago

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>

comment:15 by Jun Omae, 5 years ago

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.

Last edited 5 years ago by Jun Omae (previous) (diff)

comment:16 by Ryan J Ollos, 5 years ago

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

  • trac/ticket/templates/ticket.html

    diff --git a/trac/ticket/templates/ticket.html b/trac/ticket/templates/ticket.html
    index b65f6d8..b07da05 100644
    a b ${value}</textarea>  
    284284                            <span py:when="field.name == 'cc'">
    285285                              <py:choose>
    286286                                <!--! Unauthenticated user without TICKET_EDIT_CC and with email set in prefs -->
    287                                 <py:when test="'cc_entry' in field and field.cc_action is not None">
    288                                   <em>$field.cc_entry</em>
     287                                <py:when test="'cc_entry' in field" py:choose="">
     288                                  <em py:when="field.cc_action is None" i18n:msg="">
     289                                    Set your email in <a href="${href.prefs()}" class="trac-target-new">Preferences</a>
     290                                  </em>
     291                                  <em py:otherwise="">$field.cc_entry</em>
    289292                                  <input type="checkbox" id="field-cc" name="cc_update"
    290293                                         title="This checkbox allows you to add or remove yourself from the CC list."
    291                                          checked="${field.cc_update or None}" />
     294                                         disabled="${field.cc_action is None or None}" checked="${field.cc_update or None}" />
    292295                                </py:when>
    293296                                <!--! Unauthenticated user without TICKET_EDIT_CC and no email set in prefs -->
    294                                 <py:when test="'cc_entry' in field">
    295                                   <span  class="hint" i18n:msg="">
    296                                     Set your email in <a href="${href.prefs()}" class="trac-target-new">Preferences</a>
    297                                   </span>
     297                                <py:when test="field.cc_action is None">
     298
    298299                                </py:when>
    299300                                <!--! TICKET_EDIT_CC is allowed -->
    300301                                <py:otherwise>

by Ryan J Ollos, 5 years ago

Attachment: SetYourEmailInPrefs.png added

comment:17 by Jun Omae, 5 years ago

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>

in reply to:  17 comment:18 by Ryan J Ollos, 5 years ago

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.

comment:19 by Ryan J Ollos, 5 years ago

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

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 as closed 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.