Edgewall Software
Modify

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#11938 closed enhancement (fixed)

Improve error message from TracAdmin remove when permission doesn't exist

Reported by: Ryan J Ollos Owned by: walty <walty8@…>
Priority: normal Milestone: 1.2
Component: admin/console Version:
Severity: normal Keywords: permission
Cc: Branch:
Release Notes:

Improved error message when adding and removing an invalid action with TracAdmin.

API Changes:
Internal Changes:

Description

When attempting to assign an undefined permission a helpful error is echoed.

$ trac-admin ../tracdev permission add anonymous NOT_A_PERM
TracError: NOT_A_PERM is not a valid action.

The error message seen when attempting to remove an undefined permission is:

$ trac-admin ../tracdev permission remove anonymous NOT_A_PERM
Error: Cannot remove permission NOT_A_PERM for user anonymous. The user has not been granted the permission.

It would probably be more helpful to check if the permission exists before trying to remove it from a user, in order to echo a more helpful error message in the case that the user makes a typo or a Component isn't loaded.

Attachments (2)

11938-permission-remove-error.v1.patch (659 bytes ) - added by walty <walty8@…> 6 years ago.
11938-permission-remove-error.v2.patch (2.4 KB ) - added by walty 6 years ago.
added unit test

Download all attachments as: .zip

Change History (12)

by walty <walty8@…>, 6 years ago

comment:1 by walty <walty8@…>, 6 years ago

pls let me know if unit test is needed..

comment:2 by Ryan J Ollos, 6 years ago

Unit test would be good. You can add a test in tags/trac-1.0.7/trac/admin/tests/console.py#L307. Expected output goes in tags/trac-1.0.7/trac/admin/tests/console-tests.txt#L96.

by walty, 6 years ago

added unit test

comment:3 by walty, 6 years ago

While I try to add the unit test to the patch, I got one minor issue. The original test suites did not get passed (9 cases failed), I tried to revert all my patches, and set up a clean test environment, and it's still the same.

Later I found that it seems to be related with [13853], which removes BATCH_TICKET_MODIFY by default, so the result file for permission list could not match with the actual output.

I finally manually added back the permission in order to test my patch (but the corresponding change is not included in the patch file).

comment:4 by Ryan J Ollos, 6 years ago

Thank you for finding the cause of the test failures! I had noticed those a few times, but never investigated. Fixed on 1.0-stable in [14204], merged in [14205].

comment:5 by Ryan J Ollos, 6 years ago

Milestone: next-dev-1.1.x1.0.9
Owner: set to Ryan J Ollos
Status: newassigned

comment:6 by Ryan J Ollos, 6 years ago

Milestone: 1.0.91.2

Patch looks good. I added a few additional minor changes in log:rjollos.git:t11938.

comment:7 by walty, 6 years ago

Thanks, I checked the code, and it did make more sense with elif :)

comment:8 by Ryan J Ollos, 6 years ago

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

Committed to trunk in [14210:14211].

comment:9 by Ryan J Ollos, 6 years ago

Owner: changed from Ryan J Ollos to walty

comment:10 by Ryan J Ollos, 6 years ago

Owner: changed from walty to walty <walty8@…>

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain walty <walty8@…>.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from walty <walty8@…> 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.