Problem/Motivation

The permission with machine name "access user profiles" allows roles permission to view the user account information page but the "title" of this permission "View user profiles" can be confused with other profile modules, such as one which might get added to core (see #1726822: Port Profile2 to D8 and #1668292: Move simplified Profile2 module into core). Some websites attached fields to the Drupal User entity and call this a "Profile", whereas other websites use a contributed module such as Profile 2 to create a "Profile". Clarifying that Drupals "access user profiles" permissions gives access only to viewing the Drupal user entity would be helpful.

Proposed resolution

  • Rename "View user profiles" to "View user account information"
  • Add description to the permission: "View pages displaying user accounts and other user information."

Remaining tasks

Review the latest patch.

User interface changes

Permission is renamed at /admin/people/permissions

API changes

None, unless maintainers also want a patch to change the permission machine name from "access user profiles" to "access user account information".

Original report by jbrauer

The permission "access user profiles" allows roles permission to view the user account page but this is confusing with the core profile module. It's even more confusing with users now having fields themselves. Perhaps "access user account pages" would be more clear. It's important to come up with something accurate and understandable.

Files: 
CommentFileSizeAuthor
#94 658148-user-view-permission-rename-94.patch5.91 KBjhodgdon
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,542 pass(es). View
#92 658148-user-view-permission-rename-92.patch5.11 KBjhodgdon
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,197 pass(es), 1 fail(s), and 0 exception(s). View

Comments

bleen’s picture

Version: 8.x-dev » 7.x-dev
Category: feature » bug
Status: Active » Needs review
FileSize
20.12 KB
Failed on MySQL 5.0 InnoDB, with: 15,991 pass(es), 29 fail(s), and 0 exception(es). View

Let's try to get this into D7 ... if webchick wants to postpone it to D8, that's fine but I don't see why this wouldn't make it in. I agree that the terminology is confusing.

tobiasb’s picture

Status: Needs review » Needs work

remove your search code please :D

bleen’s picture

Status: Needs work » Needs review
FileSize
15.3 KB
Passed on all environments. View

d'oh!! I was playing with something else and forgot.

Status: Needs review » Needs work

The last submitted patch, user-access-perm-658148.patch, failed testing.

bleen’s picture

I suspect this is not actually failing, but rather testbot is still drunk: #667264: [TESTING BROKEN] FileFieldValidateTestCase fails randomly

Status: Needs work » Needs review

Re-test of user-access-perm-658148.patch from comment #3 was requested by tobiasb.

jbrauer’s picture

This is great! I'd love to see some other reviews, comments, thoughts on this.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Yup, this is indeed better "profiles" is really on applicable to those who remember that is on a users page. This indeed is a tiny, bit more clear language.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I support this change, but there should probably be an upgrade path.

jbrauer’s picture

A couple of things were pointed out. In the patch this doesn't make a lot of sense:

-    'access user profiles' => array(
+    'access user account pages' => array(
       'title' => t('View user profiles'),

We need to change the title to something like 'View user account pages'

One option mentioned around the upgrade path is actually to not change the system level permission name and just change the title. This has an advantage in that the upgrade path is null and it prevents someone from implementing this permission in a contrib module and causing problems with third party modules. It does have the drawback of potentially being confusing if we want to add a specific permission to profile module.

bleen’s picture

I think that the impetuous behind the original post is that it was confusing to developers (not just site admins) - maybe thats just how I read it. In any event, I think it should be changed everywhere. I'm happy to work on the upgrade path (and the "DOH!" change that jbrauer pointed out in #10).

Any strong opinions?

RobLoach’s picture

Do we really need to change the internal name here? We could, in theory, just change the "title" to "Access user account pages". Then we wouldn't need an upgrade path as the internal name would stay the same. Ummm, what jbrauer said :-) .

bleen’s picture

FileSize
16.25 KB
Passed on all environments. View

This is the first time I'm writing an update for a core module so if I'm doing things wrong please shout ... want to learn how to do this correctly :)

Assuming its done correctly, does this need to be documented somewhere?

(this patch also fixes #10)

bleen’s picture

Status: Needs work » Needs review
aspilicious’s picture

Nop after some reviews it will become RTBC and then webchick or dries throw it in head.

bleen’s picture

@aspilicious: I should have been clearer :) I meant that this is the first patch I've written that includes an upgrade path of some kind ... thats the part I'm asking about

Dries’s picture

This looks good. I wonder if we need the 'pages' part though, as none of our other permissions use 'pages'. Would 'access user account' be confusing? Thoughts?

bleen’s picture

I have no strong opinion ... I just fond the "profile" confusing

jbrauer’s picture

FileSize
16.08 KB
Passed on all environments. View

Changed to "access user accounts" and "View user accounts"

Bojhan’s picture

Not sure if using accounts, doesn't just put as back at the initial confusion - pages didn't do that. Its the idea that, the confusing part is that people don't know what the object profiles/accounts is - they only know that by the indicator pages. I am not sure, its true that we don't apply this on other permissions. Shouldn't be the deciding factor for this though.

jbrauer’s picture

I guess my initial desire was a little different. It was to make it explicit that what is being viewed is a "User Account" (or User Account Page) if we go there compared to a Profile. The confusing part is that a User Account (including fields added to users) may be viewed elsewhere not just on the User Account Page. However Profile module provides the ability to setup User Profiles and it's quite confusing that one has to deal with the "View User Profiles" permission even when the Profile module is not enabled.

My slight preference for User Accounts over User Account Pages is it covers more situations where the permission may be used to grant access to User Account data in places that aren't the User Account Page. That said if the consensus is User Account Pages it's much better than User Profiles so I'll be happy.

jhodgdon’s picture

So this patch makes the user-facing name of this permission "View user accounts".

I really don't think this is an improvement over "View user profiles" permission name, although I agree that "view user profiles" is confusing, given the Profile core module.

I guess we don't actually have a term for these pages like user/3, do we... sigh.

But to me "View user accounts" sounds like some kind of an administrative permission, since "account" has a connotation of a bank account, and what comes to mind is that I could view the users' account/private information.... Is it possible we can think of a yet better name for this, that isn't "accounts" or "profiles"?

jbrauer’s picture

We currently have View user profiles which with fieldable users is much worse in D7 than the atrocity it's been in the past.

Suggestions so far include:
View user account pages
View user accounts

Other ideas?
View users
View users data
View users information
View information about users

I'd be happy to roll a new patch if there's a consensus on an alternative language.

jhodgdon’s picture

Status: Needs review » Needs work

Just had a quick IRC discussion about this issue.

Apparently this permission is much more comprehensive than just letting you see user pages like user/3. It also governs whether you can see other user information, such as user names displayed as post information, at least according to jbrauer.

So in that case, ksenzee, aspilicious, jbrauer, and I agreed on IRC that the best name for the perm is probably "view user data" or "view user information" (UI-facing name that is), and that on the Permissions page there should be some clarifying information in the description, telling exactly what information is governed, such as "View information such as user names, how long they've had their account, and user profile pages".

jhodgdon’s picture

I took a look at the code to see what this permission actually does:
- Allows access to pages like user/3
- Some kind of file download permission in file_file_download() for the File module, which I don't understand in the least?
- In profile.module, controls access to a bunch of stuff: paths (profile, profile/autocomplete), and the Author Information block. Also allows "browsing" of profile fields (the pages that show you things like "everyone in Canada" etc.).
- In user.module, controls access for searching Users (assuming you have search permission also), and paths user/autocomplete and user/N
- tracker.module also requires this permission to view someone's user/N/track page.

So I am not sure jbrauer is correct about what this permission does. It looks like it's a bunch of stuff in the Profile module (shouldn't that module have its own permission?!?!?), plus searching users and accessing the user/N pages. And that file permission that I'm not sure of.

jhodgdon’s picture

So can we split the permission into two:
- View basic user information page (user/N and user searching permissions)
- View user profile information (profile.module)

jbrauer’s picture

Agreed there was a miscommunication in IRC about this. It does not cover visibility of user names. It does affect the theming of usernames only in so far as whether to link them to the user/3 page. And this variable $variables['profile_access'] is not accounted for in this patch. It is frequently used in other modules but we can definitely have them live with the 'pages' portion.

jbrauer’s picture

The idea in #26 seems good, however, I wonder from a practical standpoint. If a role has "View user profile" but not "View basic user information" it seems like we at least have a number of places where we have to test for both in determining if they can go to user/3. And if I can see the profile but not "basic user information" it's not clear what basic is as username seems like the most basic information but would be awkward.

jhodgdon’s picture

Yeah, I agree the proposal in #26 is awkward.

So the current permission covers user/N pages (and suggests that theme functions not make links to user/N if the viewer has no permission to see them), as well as the ability to search for users (along with some search.module permissions), as well as permissions to view user profile information.

In which case, I do not really think that changing this from "View user profiles" (the current human-readable text; machine-readable is "access user profiles") to something else will really illuminate anything. I don't know of a better name for user/N pages than "user profiles", although I agree that the term "user profile" is confusing, given the profile module.

So that's my vote: leave the permission name as it is.

I would not be against putting some description text in there, such as "View basic user information pages and user profile information", but I'm not sure it really adds a lot.

jbrauer’s picture

The problem with the current state is that it's not accurate and very confusing. Choosing "View User Profiles" grants permissions to view things that aren't associated with the user profile module. For all the same reasons as #26 the current situation really isn't workable. Sounds like "View user information pages" is the way to avoid using 'profile' and 'account'.

jhodgdon’s picture

I'm OK with that solution.

bleen’s picture

Status: Needs work » Needs review
FileSize
15.89 KB
Failed on MySQL 5.0 InnoDB, with: 17,258 pass(es), 6 fail(s), and 6 exception(es). View

here is a patch for:

Computer readable: access user info pages
Human readable: View user information pages

I would not be against putting some description text in there, such as "View basic user information pages and user profile information", but I'm not sure it really adds a lot.

With this change, I do not believe that a description is necessary

jhodgdon’s picture

Ummm. I think we don't normally abbreviate. So the computer readable permission should probably be "access user information pages"?

Status: Needs review » Needs work

The last submitted patch, user_perm_20.patch, failed testing.

Status: Needs work » Needs review

Re-test of user_perm_20.patch from comment #32 was requested by jhodgdon.

Status: Needs review » Needs work

The last submitted patch, user_perm_20.patch, failed testing.

bleen’s picture

Status: Needs work » Needs review

I dont understand why this would fail ... this is same patch as #32 but incorporating jhodgon's comment in #33

If this doesn't pass I'll dig deeper.

bleen’s picture

FileSize
16.12 KB
Failed on MySQL 5.0 InnoDB, with: 17,261 pass(es), 6 fail(s), and 6 exception(es). View

forgot patch

Status: Needs review » Needs work

The last submitted patch, user_perm_21.patch, failed testing.

jhodgdon’s picture

It appears to be consistently failing in the RDF tests. Probably the RDF module tests are trying to use the old-named permission and the test bot is complaining about it. Possibly you missed the rdf.test file when searching for where this permission was used?

Just a guess...

bleen’s picture

Status: Needs work » Needs review
FileSize
16.84 KB
Passed on all environments. View

I think you're right :)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Patch passes tests now. Looks clean to me. And I think we all agreed that access/view user information pages was the best we could come up with, and much better than what's there.

Dries’s picture

Mmm. To me, 'access user information pages' is less clear than 'access user profiles'. In fact, what is the difference? How is a 'user information page' different from a 'user page'. A user information page sounds like something special. I need to sit on this for a bit.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I actually agree with that. I found the old version much clearer.

http://drupal.org/user/1 is a 'user profile page' to me. I haven't been able to read this in depth, but I've skimmed it and don't understand why it's important for the purposes of this permission name whether Profile module or fieldable users is adding stuff to this page?

jhodgdon’s picture

That is what I thought too, but others (see above) overruled me. The consensus above seems to be that the permission, which currently says "View user profiles" is too confusing, given the existence of an optional module called "Profile", and the fact that this permission governs more than just what is coming from the Profile module.

jhodgdon’s picture

Here's an idea for making a different fix that would achieve I think the same purpose: Change the name and/or descriptive text for the Profile module.

Currently (modules/profile/profile.info):
name = Profile
description = Supports configurable user profiles.

Could change to:
name = Profile information
description = Supports adding additional information to user profiles.
(or something like that -- but we might want to avoid the word "fields" in the .info file, because Profile doesn't support Fields with a capital F, although it does has fields with a small f)

Then I think there would be no confusion about what a user profile page is (either the basic page, or the enhanced page with the Profile module, or with information provided by some other contrib module), and the "View user profiles" permission can keep its name.

The thing is, "user profile", in plain English rather than Drupal-ese, I think is pretty clear. The only thing that makes it unclear is that the Profile module makes it sound like you can't have "user profiles" without profile.module, which isn't really true.

Thoughts? Would this make everyone happy?

bleen’s picture

@jhodgdon: I agree that would help solve the confusion. I'm ok with it...

anyone else?

jhodgdon’s picture

Let's get an opinion from webchick/dries before we go forth, since it was mostly the two of them who objected to the proposed change in the permission name.

jhodgdon’s picture

And possibly also from jbrauer and anyone else who commented above...

jhodgdon’s picture

Just as a note: In order to do that patch, the profile.info file needs to change, and also any place in help files that mentions "Profile module" needs to have a name change as well. This is probably limited to profile_help(), but could possibly be mentioned elsewhere.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
5.03 KB
Passed on all environments. View

Here's a patch to change the name and description of profile.module, as described in comment #46 above. I checked, and the only place (I'm pretty sure) that "profile module" was being used in core was in profile_help().

webchick’s picture

Status: Needs review » Needs work

This issue really kind of seems like it's getting out of hand.

Renaming "Profile" to "Profile information" invalidates 8 years' worth of documentation. I know the drop is always moving and all that, and I would be fine making such a change if we were convinced it radically improve usability, but I cannot recall a single instance in all of the years we've been teaching Drupal of someone being confused by this permission name. I also don't recall any forum support requests or bug reports about it, either. I can predict, however, thousands of people getting confused when they're looking for "Profile" module because a tutorial told them to and having to deduce that it's been named something else for no real reason.

Do we have actual evidence that people find this confusing? And if so, is there a reason we have to throw the baby out with the bathwater here, or couldn't we just use the new permission description feature in D7 to clarify what the permission means? That would also spare us the API change of altering the machine-readable name of the permission.

webchick’s picture

Also, if it takes over 50 replies to come to consensus about something as straight-forward as a permission name, it really starts smelling more like D8 to me... though I know Dries said he was in favour above.

jhodgdon’s picture

FileSize
622 bytes
PASSED: [[SimpleTest]]: [MySQL] 17,377 pass(es). View
13.95 KB

OK, here's a simple patch that adds a brief description. Maybe this time it's not controversial? :)

And a screen shot of the User module's permissions.

jhodgdon’s picture

Status: Needs work » Needs review
Dries’s picture

Version: 7.x-dev » 8.x-dev

I don't think this is a big UX problem so I'd be OK to postpone to D8.

fago’s picture

Oh sry, I completely missed that issue till now.

>I cannot recall a single instance in all of the years we've been teaching Drupal of someone being confused by this permission name.

Oh there were nodeprofile/contentprofile users that were confused of that as those permission doesn't relate to nodeprofiles - but given the name it wasn't obvious for users.

As we noted in #301071: Remove profile module from core we need to better separate the account from the user profile (-> 8.x). User modules has not only that permission (which with that naming would belong to the profile module) it also has quite some profile-* theme functions that in the end theme information that is display on the user page.

To move on I think a rename of the permission to *access user pages* would help.

So where's the difference between user pages and user profiles? Each module may put information about the user on the user page - that might include the users profile's data but not only. I see that this poses for end-users having a look at the page the "user-profile", however site-builders need to be able to distinct between "show profile data" and "show user access history", "show user points" or whatever.

Thus the *access user page* would pose the kill switch to access the page at all, still modules might add additional permissions for their stuff. Thus a hypothetical permission *access profile information* would belong to the profile module then.

jhodgdon’s picture

I don't think "access user pages" is a good idea. For instance, the admin page that lists and manages users to me is a "user page. So I think "access user pages" is confusing. Anyway, this is now bumped to D8...

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Novice

On account of #301071: Remove profile module from core, I don't think confusion with profile.module is an issue anymore.

Personally I think the approach of simply adding a more helpful description is a good middle ground.

Note that this patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). ...And, well, this one also needs to be rerolled for the git migration yet. :)

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC. :) Tagging as novice for the task of rerolling this patch.

BBommarito’s picture

Assigned: Unassigned » BBommarito
BBommarito’s picture

Assigned: BBommarito » Unassigned
Status: Needs work » Needs review
FileSize
497 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,256 pass(es). View
jhodgdon’s picture

Looks reasonable to me, but I think this was a reroll of my patch, so I can't probably mark it reviewed. And I didn't test it.

xjm’s picture

Let's keep the novice tag for someone to test the patch, and maybe post some before/after screenshots.

jhodgdon’s picture

Cauliflower’s picture

Patch works as expected but missed a small dot at the end of the description. I attached a new patch.

Before:
Schermafbeelding 2012-08-24 om 16.51.55.png

After:
Schermafbeelding 2012-08-24 om 16.57.02.png

WebDevDude’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
13.15 KB
8.07 KB

Tested patch and works as expected:

Before:
before

After:
after

P.S. This is my first patch review, so please tell me if I've done something incorrectly.

fago’s picture

This still clashes with profile modules like profile2. The user module does not implement profiles, it implements a user page. If a profile module implements profiles the "view profile" permission would tell me whether the profile is visible on the page, not whether the whole user page is visible.

The description of the permission actually states that it is not really the profile, but a page with lots of different stuff related to the user. So what about using "View user pages" ?

fago’s picture

Status: Reviewed & tested by the community » Needs work

On account of #301071: Remove profile module from core, I don't think confusion with profile.module is an issue anymore.

This does not mean no profile modules exist any more, and there is #1668292: Move simplified Profile2 module into core.

jbrauer’s picture

Re #68 and #67 I could not agree more. Now that core does not have a profile at all it is even more imperative that we reduce the possible confusion of calling something by an incorrect name, especially with a term like profile that implies a significant functionality to many people administering sites.

Renee S’s picture

Status: Needs review » Needs work

@fago I'm not sure about "View user account pages", as that implies pages belonging to the user, which could be any content they've created. Perhaps "View user account information", a variant on what was suggested way up above?

cferthorney’s picture

Status: Needs work » Needs review
FileSize
574 bytes
PASSED: [[SimpleTest]]: [MySQL] 48,208 pass(es). View

I quite like "View user account information" It's clear, to the point and doesn't use language that is too ambiguous. Patch attached.

Status: Needs work » Needs review
bleen’s picture

your comment in #71 does not match your patch?

ShaunDychko’s picture

Issue summary: View changes
FileSize
709 bytes
586 bytes
FAILED: [[SimpleTest]]: [MySQL] 59,689 pass(es), 1 fail(s), and 0 exception(s). View

Updated #71 to adjust the 'title' to 'View user account information'.

(Added an interdiff just because I'm practicing for the upcoming code sprint.)

Status: Needs review » Needs work

The last submitted patch, 74: access_user_profiles_rename-658148-74.patch, failed testing.

ShaunDychko’s picture

FileSize
4.04 KB
ShaunDychko’s picture

Had to update the permission in the test, and also found a reference to the permission in the search module.

ShaunDychko’s picture

FileSize
4.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch access_user_profiles_rename-658148-77.patch. Unable to apply patch. See the log in the details link for more information. View

Also updated a comment referring to the "View user profiles" permission.

Do we want to also update the permission machine name? This is a more ambitious task. Currently the machine name for this permission is:

access user profiles

which could be changed to

access user account information

let me know if maintainers want a patch for that.

ShaunDychko’s picture

Status: Needs work » Needs review
ShaunDychko’s picture

Issue summary: View changes
jhodgdon’s picture

I think this patch is a good idea. +1 for marking it RTBC.

greggillingham’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
8.3 KB

tested and verified changes

webchick’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Usability

Since the idea is to improve the usability of this permission through this naming, tagging for the UX team to eyeball first.

Status: Needs review » Needs work

The last submitted patch, 78: access_user_profiles_rename-658148-77.patch, failed testing.

g3r4’s picture

Status: Needs work » Needs review
FileSize
3.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch access_user_profiles_rename-658148-86.patch. Unable to apply patch. See the log in the details link for more information. View

Looks like core/modules/user/lib/Drupal/user/Tests/Views/HandlerFieldPermissionTest.php no longer exists, I've rerolled the patch accordingly I hope.

edit: grammar

Status: Needs review » Needs work

The last submitted patch, 86: access_user_profiles_rename-658148-86.patch, failed testing.

g3r4’s picture

Sorry I just realize this is not about a direct reroll, I'll wait for comments from someone of the UX team to work on changes accordingly if any.

Bojhan’s picture

Isn't it a bit misleading to say it allows acces that "display" certain data? Our permission system doesn't work that way, it gives acces to a specific concept, which could be repeated anywhere.

jhodgdon’s picture

Well, the previous was still view:

   'access user profiles' => array(
-      'title' => t('View user profiles'),
+      'title' => t('View user account information'),
+      'description' => t('View pages displaying user accounts and other user information.')

It doesn't grant access for actions other than "view", such as "edit" or "delete"... I think "view" is accurate here? There are other permissions that give access to other actions.

This is also similar to node_permission():

   'access content' => array(
      'title' => t('View published content'),
    ),

Do you think we should change that too?

Bojhan’s picture

Well the "pages" part is the confusing thing, I think. If it says View user accounts and other user information it would be more accurate, because permissions are used to disclose concepts not pages, perse.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
5.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,197 pass(es), 1 fail(s), and 0 exception(s). View

Ah, the "pages" is the problem, not the "View". OK, how about this? I didn't make an interdiff, because it's the same exact size as the patch (every line is different from the last patch).

Status: Needs review » Needs work

The last submitted patch, 92: 658148-user-view-permission-rename-92.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
5.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,542 pass(es). View

Ah, apparently a test has the UI version of the permission in it now.

scottrigby’s picture

Assigned: Unassigned » scottrigby
scottrigby’s picture

Status: Needs review » Reviewed & tested by the community

5 years and almost 100 comments later by Drupal project lead, core maintainers, documentation maintainer and others… this patch seems to address everyone's concerns, applies cleanly and passes tests (notes for context during the jersey shore D8 sprint). Looks good!

scottrigby’s picture

Assigned: scottrigby » Unassigned
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b96e823 and pushed to 8.x. Thanks!

  • alexpott committed b96e823 on 8.x
    Issue #658148 by bleen18, ShaunDychko, jhodgdon, g3r4, cferthorney,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.