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

[Ryan J Ollos <ryano@…>]

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.

[Ryan J Ollos <ryano@…>]

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.

[Ryan J Ollos <ryano@…>]

Patch against r11100 of the trunk.

[Christian Boos]

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)
• a title attribute saying something like "invalid permission" would be useful for explaining the different styling

[Ryan J Ollos <ryano@…>]

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!

[Ryan J Ollos <ryano@…>]

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

[Ryan J Ollos <ryano@…>]

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

[Ryan J Ollos <ryano@…>]

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?

[Ryan J Ollos <ryano@…>]

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

[Ryan J Ollos <ryano@…>]

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.

[Christian Boos]

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.

[Ryan J Ollos <ryano@…>]

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

[Ryan J Ollos <ryano@…>]

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 ;)

[Christian Boos]

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

[Ryan J Ollos <ryano@…>]

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

[Ryan J Ollos <ryano@…>]

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

[Ryan J Ollos <ryano@…>]

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.

[Christian Boos]

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.

[Ryan J Ollos <ryano@…>]

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.

[Christian Boos]

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

Applied in r11111.

[Ryan J Ollos]

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

[Ryan J Ollos]

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

[Ryan J Ollos]

See also #12595.