Currently we use these permissions:
'create polls', 'delete polls', 'view polls', 'vote on polls', 'cancel own vote', 'administer polls', 'inspect all votes'

I have a strong feeling they don't even do what they say. Anyway they are pretty non-standard and I suggest these instead (might need work):

'create polls', 'edit polls', 'edit own polls', 'edit polls', 'vote on polls', 'cancel own vote', 'reset votes', 'inspect all votes', 'access electoral list'

Input welcomed.

CommentFileSizeAuthor
#17 advpoll_perm_2.patch6.4 KBanders.fajerson
#13 advpoll_perm_1.patch5.75 KBanders.fajerson
#6 advpoll_perm_0.patch5.02 KBanders.fajerson
#5 advpoll_perm.patch5.09 KBanders.fajerson

Comments

anders.fajerson’s picture

anders.fajerson commented

New list. Added back 'administer polls' and removed 'reset votes' (like we have it now, I looked at the code source this time):

'create polls', 'edit polls', 'edit own polls', 'edit polls', 'vote on polls', 'cancel own vote', 'administer polls', 'inspect all votes', 'access electoral list'

I also had an idea to move more of the node edit settings into the 'administer polls' permission, to deal with the settings bloat. Or maybe all of them? "Maximum choices" should probably be left out, i.e. users with the 'create polls' permission will still see it. That would actually mimic how other modules use the 'administer' permission like node module and comment module. Together with the option to set default content type settings (see http://drupal.org/node/162146#comment-279710) this might be a winner.

ChrisKennedy’s picture

ChrisKennedy commented

1. You have "edit polls" on there twice - do you mean for one of them to be "delete polls"?
2. Would "access electoral list" be to both view it and add to it, just view it, or what?
3. I think the best way to deal with settings bloat would be to collapse the settings field by default and also allow per content-type settings. Requiring "administer polls" permission might be too restrictive, but it seems to depend on how people tend to use the module on live sites.
4. What about moving the "reset votes" button to the votes tab? That might be a minor improvement.

anders.fajerson’s picture

anders.fajerson commented

1. A typo. Delete content is handled by the 'administer nodes' permission (as for all other content types, I see no reason why we should be different)
2. 'access electoral list' is only for viewing. "Access" is the standard term used by Drupal: 'access content', 'access user profiles' etc. 'administer polls' handles "add to it".
3. We can start with that :)
4. The votes tab is only visible if "Show individual votes" is checked. I'd like to have it after the delete button, but I don't think it's possible in Drupal 5 (Drupal 6 fixed this).

anders.fajerson’s picture

anders.fajerson commented

More thoughts on 3.: I say we do both. Collapse it/default content type settings AND add *some* settings to the 'administer poll' permission. I vote for "Status", "Restrict voting to electoral list" and "Show individual votes".

anders.fajerson’s picture

anders.fajerson commented
Status: Active » Needs review
StatusFileSize
new advpoll_perm.patch5.09 KB

1. Removed access checking from latest poll block, that's usually handled by the block
2. Moved "Restrict voting to electoral list" and "Show individual votes" inside 'administer polls' permission, together with "reset votes". I left "status" out as it doesn't make much sense to have it there but not the date settings.
3. To sum up, these are now our permissions: 'create polls', 'edit polls', 'edit own polls', 'vote on polls', 'cancel own vote', 'administer polls', 'inspect all votes', 'access electoral list'

anders.fajerson’s picture

anders.fajerson commented
StatusFileSize
new advpoll_perm_0.patch5.02 KB

Re-rolled.

ChrisKennedy’s picture

ChrisKennedy commented

Can you give me a permission "diff" - relative to our current permissions, what is being removed and what is being added?

anders.fajerson’s picture

anders.fajerson commented

What do you mean? :)

ChrisKennedy’s picture

ChrisKennedy commented

Something simple like this would be handy for future reference:
* Changed X to Y1 and Y2
* Deleted Z
* Added permission ASDF

It's kind of a pain to have to look through the permission list to try to find exactly what is changing.

Anyway, on to a more substantive comment: I like the idea of using "administer nodes" to hold some of the settings, but think for non-administer-nodes users I think it might not use the content type's default settings unless we add extra code to advpoll_insert. For example, if the default is showvotes=1, but the user doesn't have "administer votes" permission, we won't define that form element so in the INSERT call the variable will be undefined and will default to 0 (plus it's an E_NOTICE since that variable isn't defined).

ChrisKennedy’s picture

ChrisKennedy commented

Let's put "close poll" inside of "administer nodes" too - that is what poll.module does.

ChrisKennedy’s picture

ChrisKennedy commented

Oops, I see your comment in #5 regarding the status option... good point.

ChrisKennedy’s picture

ChrisKennedy commented

Another thought: perhaps we should add some sort of label or fieldset (uncollapsible) to clarify which settings are only for administrators.

anders.fajerson’s picture

anders.fajerson commented
Title: Do permissions the Drupal way » Improve use of permissions
StatusFileSize
new advpoll_perm_1.patch5.75 KB

As always, great input.

1. The settings are now passed as '#value" for users without the admin permission.
2. Added a "note": The settings below are only available for users with the <em>administer polls</em> permission.. It's styled as a description. I dislike fieldsets within fieldsets so this is an alternative to that.

An attempt to summarize the the changes (could go in a commit message):
* Removed 'delete polls' permission. As for other content types delete is handled by the 'administer nodes' permission.
* Added 'edit polls' and 'edit own polls' permission. It replaces the 'create polls' permission when editing polls.
* Removed 'view polls' permission.
* 'access content' permission is used instead of 'view polls' permission' for the result tab and the polls page.
* Added 'access electoral list' which is used instead of 'view polls' permission for accessing the electoral list tab.
* Access to the "Latest poll block" which used the 'view polls' permission should be handled on the block edit page.
* The settings "Show individual votes" and "Restrict voting to electoral list" on the edit poll page are now only available for users with the 'administer polls' permission.

ChrisKennedy’s picture

ChrisKennedy commented
Priority: Minor » Normal

Admin note looks good (I agree that nested fieldsets are ugly) but should say "administer polls" (plural instead of singular). Is that #description an old attempt?

The spacing is a bit off on the reset votes form definition.

When we make a new release we need to note that the permissions have changed and will need to be reassigned to roles. This should be noted in the commit message too.

On the electoral list tab, an error is generated on the "add user" autocomplete if the user has "access electoral list" and "administer nodes" permissions but not "access user profiles". We should disable the autocomplete functionalty if they don't have "access user profiles" permission.
I admit that it would be strange to use this combination of permissions on a real site though. And it's kind of a bug in Drupal core to not let you autocomplete names just because you can't access profiles.

Otherwise I think this is good to go.

ChrisKennedy’s picture

ChrisKennedy commented
Status: Needs review » Needs work

CNW

anders.fajerson’s picture

anders.fajerson commented
Status: Needs work » Needs review

I must have uploaded an old patch. Fixed that and also the (old?) bug you found.

anders.fajerson’s picture

anders.fajerson commented
StatusFileSize
new advpoll_perm_2.patch6.4 KB

Patch.

ChrisKennedy’s picture

ChrisKennedy commented
Status: Needs review » Reviewed & tested by the community

Woo cleaner permissions!

anders.fajerson’s picture

anders.fajerson commented
Status: Reviewed & tested by the community » Fixed

http://drupal.org/cvs?commit=79780

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)