Edgewall Software
Modify

Opened 7 years ago

Closed 7 years ago

Last modified 3 years ago

#10752 closed enhancement (fixed)

On the WebAdmin Permissions panel, invalid permissions should be styled differently

Reported by: Ryan J Ollos <ryano@…> Owned by: Ryan J Ollos <ryano@…>
Priority: low Milestone: 1.0
Component: admin/web Version: 1.0dev
Severity: normal Keywords: bitesized
Cc: Branch:
Release Notes:

In permission admin panel, gray out actions which are no longer defined.

API Changes:

Description

An issue I've encountered when administering a Trac project is that permissions remain assigned to users and groups after the plugin that registers the permission has been removed, which presents a confusing view and makes manual cleanup a bit difficult. I've seen one user on the mailing list get confused by the permission being defined for a user and therefore they expected the permission to have its effect, even though the plugin was no longer loading.

The proposal is to use the light grey color that is used for a missing link, to indicate that a permission is not valid. It will make cleanup after a plugin uninstall easier.

Attachments (7)

ExampleOfPatch.png (12.5 KB ) - added by Ryan J Ollos <ryano@…> 7 years ago.
t10752-r11100-1.patch (1.2 KB ) - added by Ryan J Ollos <ryano@…> 7 years ago.
Patch against r11100 of the trunk.
t10752-r11103-1.patch (1.2 KB ) - added by Ryan J Ollos <ryano@…> 7 years ago.
Patch against r11103 of the trunk, which addresses comment:2.
t10752-r11103-2.patch (1.2 KB ) - added by Ryan J Ollos <ryano@…> 7 years ago.
Patch against r11103 of the trunk, which better addresses comment:2.
t10752-r11103-3.patch (958 bytes ) - added by Ryan J Ollos <ryano@…> 7 years ago.
Patch against r11103 of the trunk, with better Genshi and dropping the CSS changes entirely.
t10752-r11103-4.patch (860 bytes ) - added by Ryan J Ollos <ryano@…> 7 years ago.
Patch against r11103 of the trunk, with improved Genshi using py:strip.
t10752-r11103-4.2.patch (849 bytes ) - added by Ryan J Ollos <ryano@…> 7 years ago.
Patch against r11103 of the trunk, which uses only a single class 'missing'.

Download all attachments as: .zip

Change History (25)

by Ryan J Ollos <ryano@…>, 7 years ago

Attachment: ExampleOfPatch.png added

comment:1 by Ryan J Ollos <ryano@…>, 7 years ago

Here is an example of the change:

If the patch is accepted, a sentence should be added to the TracPermissions to describe the new behavior.

by Ryan J Ollos <ryano@…>, 7 years ago

Attachment: t10752-r11100-1.patch added

Patch against r11100 of the trunk.

in reply to:  1 comment:2 by Christian Boos, 7 years ago

Replying to Ryan J Ollos <ryano@…>:

If the patch is accepted, a sentence should be added to the TracPermissions to describe the new behavior.

The feature looks pertinent. A few remarks about the patch however:

  • <text>? surely you meant <span>
  • #permlist .missing should be enough for the styling (no need to add two classes, as anyway you can't write a CSS rule targeting only elements having both classes wrong)
  • a title attribute saying something like "invalid permission" would be useful for explaining the different styling
Last edited 7 years ago by Christian Boos (previous) (diff)

comment:3 by Ryan J Ollos <ryano@…>, 7 years ago

Thanks for the pointers! New patch is attached. I doubt the Genshi is stylistically correct, but since there is no py:else, I wasn't sure how to code it better. Looking forward to seeing how it can be done better though!

by Ryan J Ollos <ryano@…>, 7 years ago

Attachment: t10752-r11103-1.patch added

Patch against r11103 of the trunk, which addresses comment:2.

by Ryan J Ollos <ryano@…>, 7 years ago

Attachment: t10752-r11103-2.patch added

Patch against r11103 of the trunk, which better addresses comment:2.

comment:4 by Ryan J Ollos <ryano@…>, 7 years ago

I had meant to entirely delete the CSS rule in the second patch.

Inspector in Firefox tells me that the following rule in trac.css applies for the span.missing element:

a.missing:link, a.missing:visited, a.missing, span.missing,
a.forbidden, span.forbidden { color: #998 }

So maybe we don't need to add a rule in admin.css at all?

by Ryan J Ollos <ryano@…>, 7 years ago

Attachment: t10752-r11103-3.patch added

Patch against r11103 of the trunk, with better Genshi and dropping the CSS changes entirely.

comment:5 by Ryan J Ollos <ryano@…>, 7 years ago

I think I may have just been repeating in comment:4 what you already said in comment:2. Anyway, I experimented with Genshi for a while and studied some other code, which led me to t10752-r11103-3.patch, which I think is probably an improvement stylistically.

The thing I question is whether it is desirable to have span elements wrapping every permission, since they have no attributes in the case that it is a valid permission. I guess we could always give them a class, such as valid permission. Eventually, if the permissions documentation became part of the code base, then we could show the permission description as a title.

comment:6 by Christian Boos, 7 years ago

You can use a py:strip="action in actions" for that <span>, and then have plain class="missing" and title="Invalid permission" attributes. If the action is a valid action, that wrapping <span> will be stripped but not its content.

by Ryan J Ollos <ryano@…>, 7 years ago

Attachment: t10752-r11103-4.patch added

Patch against r11103 of the trunk, with improved Genshi using py:strip.

comment:7 by Ryan J Ollos <ryano@…>, 7 years ago

That seems to result a much more readable statement. Changes applied in t10752-r11103-4.patch. I must be close to the record for number of iterations on a one statement patch ;)

comment:8 by Christian Boos, 7 years ago

… and we're not there yet ;-) I don't see why you insist on using two classes, missing and permission, as you don't seem to use the second.

by Ryan J Ollos <ryano@…>, 7 years ago

Attachment: t10752-r11103-4.2.patch added

Patch against r11103 of the trunk, which uses only a single class 'missing'.

in reply to:  8 comment:9 by Ryan J Ollos <ryano@…>, 7 years ago

Replying to cboos:

… and we're not there yet ;-) I don't see why you insist on using two classes, missing and permission, as you don't seem to use the second.

Sorry, I had misunderstood your earlier comment. The only reason I did that was that I saw classes such as missing ticket, missing changeset, … throughout the codebase, primarily associated with TracLinks, and, I don't know any better ;) Issue resolved in t10752-r11103-4.2.patch.

comment:10 by Ryan J Ollos <ryano@…>, 7 years ago

I'm fairly happy with the missing styling, but also considered using a strike-through (INVALID_PERM), in order to be a bit more obvious. The strike-through seems to be going out of style with Trac though.

comment:11 by Christian Boos, 7 years ago

Milestone: 1.0

Also, I've been thinking a bit more about the tooltip: it should be rather something like "Action no longer effective". As you can't set an invalid permission ("TracError: XXX is not a valid action"), it must have existed at some point.

in reply to:  11 comment:12 by Ryan J Ollos <ryano@…>, 7 years ago

Replying to cboos:

Also, I've been thinking a bit more about the tooltip: it should be rather something like "Action no longer effective".

That makes sense. I've been thinking about the wording and came up with Action no longer registered/active/defined. No of those particularly jump out at me as being the most clear, but I tend to prefer Action is no longer defined.

comment:13 by Christian Boos, 7 years ago

Resolution: fixed
Status: newclosed

Fine, let's go for Action is no longer defined then.

Applied in r11111.

comment:14 by Christian Boos, 7 years ago

Owner: set to Ryan J Ollos <ryano@…>

comment:15 by Christian Boos, 7 years ago

Release Notes: modified (diff)

comment:16 by Ryan J Ollos, 6 years ago

Some additional minor changes to this feature and a test case will be added in #11069 (see comment:19:ticket:11069 and later comments).

in reply to:  16 comment:17 by Ryan J Ollos, 6 years ago

Replying to rjollos:

Some additional minor changes to this feature and a test case will be added in #11069 (see comment:19:ticket:11069 and later comments).

Regression test added in [12159] and title attribute changed in [12160] so that the full name of the action in shown (#11095).

comment:18 by Ryan J Ollos, 3 years ago

See also #12595.

Modify Ticket

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