The view link was showing for all users regardless of the 'access user profiles' permission. If a user that didn't have the proper permision clicked the view link, they were brought to permission denied page.

Patch fixes that and removes extraneous column headers for view & edit if viewer doesn't have appropriate permissions.

Sorry if duplicate post - attachment had been dropped, but here it is.

CommentFileSizeAuthor
site_user_list_patch_3.txt1.62 KBscafmac

Comments

pukku’s picture

pukku commented

I agree with half of this patch, but don't all users have the ability to view their own profiles? Is it possible to set it up so that users can't see their own profile data? I don't think it is. So the 'view' column is always needed. Users can always edit their own data as well, so the edit column should always be needed. I'm not sure it hurts anything to have the columns there even if the users aren't in the filtered set.

I do agree, however, that the 'view' option should be tested against user permissions.

scafmac’s picture

scafmac commented

The way the patch works, both the view and edit links will appear for user's own account, but not for ones they do not have permission to.

The anonymous user has no account and might not always have permission to users' accounts. If an anonymous user has access to the site user list page but not access to user accounts, then they should not see any view or edit links.

It just seems like poor form to have empty columns that can be easily avoided with an if statement.

pukku’s picture

pukku commented

However, at the time of your first chunk (365-ish?), you don't know if the user will be in the result set. Actually, at that time, $contact isn't properly defined, and the test devolves just to the user access checks, which aren't sufficient for the case where the user is in the result set.

I've committed the fix while in the loop, which is correct. But I don't think that conditional suppressing of the two extra columns can be done without knowing something about the results.

I'm not saying it isn't possible, but it would be complicated.

Also, I just realized that the test for 'access user profiles' should also have 'administer users' in it, because if you can administer the users, I think you can also see their profiles...

scafmac’s picture

scafmac commented

Yeah - sorry about that; I cut & pasted that if statement without reading it carefully.

I just downloaded the latest from head - it looks great.

You are absolutely right on all count. I can think of a couple of simple options and am trying out my favorite. It is to remove the view link and instead make the first column (username by default) a link to view the user profile; it's intuitive and allows for a user by user check on access. I added a column title to the edit column figuring that the column looks more intentional with a title.

Lots of options, each with plenty of pros & cons... What do you think, are you interested in a patch?

pukku’s picture

pukku commented

Hi! I just submitted a patch which would allow for themeing of individual columns, which you could use to make the links to the user profile. All that would be needed then is an option to suppress the 'view' and 'edit' links (separate options) that the admin could set.

I'm perfectly happy to add these as options, if you think they're useful.

scafmac’s picture

scafmac commented

Sure that makes sense. Any thought on column headings for view & edit? Doesn't matter, just curious. Thanks for the responsiveness.

pukku’s picture

pukku commented
Status: Needs review » Fixed

I just committed a version that should address this. Note that you should run update.php and update site_user_list to version 3; I discovered that the maximum length of a variable name is 48 characters, and I had one that was 49, so I renamed a number of variables. update_3 will copy old values over and delete the incorrect variables.

pukku’s picture

pukku commented
Assigned: Unassigned » pukku
Status: Fixed » Closed (fixed)