Suggested UI improvement for field privacy controls

Looks like a great idea. Looking forward to it.

100% in favor of this UI change.

So far I have the form, the theme function to arrange the radios into a table. I moved the theme function and form to a separate file. Submit callback still needs to be done.

+1 for this idea. Just a couple of questions:

Does "Everybody" mean "all site users"?
Is it also necessary to add a column for "public"?

Well currently the columns are the audiences defined using the profile privacy module, if you add more then more columns will appear (at least thats the aim).

izmeez,

Does "Everybody" mean "all site users"?

Maybe, but not necessarily. As discussed in the thread at #299427: Consider changing word "publicly" to distinct registered users and nonregistered users, that depends on permissions. Do anonymous users have the ability to view user profiles? Only a subset of roles? Is there a third party module that is rendering profile information outside of the core Profile page so that the core permissions don't apply? There are a lot of variables in play, but usually this will mean everyone who has permissions to access user profiles.

Thanks,
Chris

EDIT:
Although the image says "Everybody", the text is actually determined by the hook_profile_privacy_access_info() implementations, so the actual text will be "Public" unless it is determined that there is a better alternative. Of course, you can override this string through the normal translation methods.

snufkin,

Great start! UI looks really good. I know you are still working on this, but there are a couple of other things in addition to the items mentioned in #3 that I want to capture in this thread so we can compile a checklist to put in the issue summary.

• Tabs disappear from the privacy page
• Remove the old widget from the profile pages
• Fix alignment of the radio buttons

Well done,
Chris

What is the issue with the radio buttons?

With regards to the tabs I hope to get some suggestions, I tried to copy the way user module implements the tabs, as in using %user_category and the map element in the menu item, but that resulted in getting a 404 for my page callback (even though the tab showed up on the other user/UID/edit pages).

snufkin,

Re: the radios, it's not a big deal but the alignment is a bit off. See the image below. Again, it's a matter of pixels and it doesn't really bother me, but if you have a quick fix it would be a good addition.

D6 menus suck :-). I'll take a hack at it to see if we can come up with something that works.

Thanks,
Chris

hmm I have them in a straight line on chrome + OSX (just checked with skitch too): https://skitch.com/thesnufkin/g3eh2/edit-commons. I'll check it out once the submit callback works.

Interesting. I am on Chrome + OSX as well. I am on Commons 2.3, so that could also be the issue. I will trust your testing on this.

The menu issue was a bit tricky to track down, and it might be even trickier to fix. Listed below is an explanation of the issue and a couple of ways we could attack it.

The Tabs Issue For The tl;dr Crowd

When the tabs are generated, the %map for the load callback is constructed from the current page instead of using each item's path. If you are visiting the privacy page added in the patch, then the load callback will be passed a %map of array('user', $uid, 'edit', 'privacy') for ALL menu items. Therefore none of the tabs will be displayed because the user_category_load() function won't return an object due to "privacy" not being a profile category.

Detailed Tabs Issue

All of the user profile category pages have menu items such as user/%user_category/edit/My Category, where the %user_category is a wildcard loader argument that takes the special load arguments of array('%map', '%index'). The %map is essentially an array containing the path parts that are used by the user_category_load() function to load the user object after checking if any profile category in the path exists. It needs the map so it can use the category ID at the end of the path (My Category in the example above) in addition to the user ID passed through the position of the %user_category wildcard argument loader. This is pretty standard menu item stuff.

Now it gets interesting. When you visit a tab on the user/*/edit pages, the menu system finds all of the other tabs related to the one you are viewing and performs access checks on each item. If you follow the tab generation from theme_menu_local_tasks(), you eventually get to the _menu_translate() function which is where our problem begins. The second thing that function tries to do is load the objects specified by the wildcard argument loaders via the _menu_load_objects() function. The $map variable is the %map passed to the load callbacks, however it is generated from the current path and NOT each tab's path. Therefore all of the tabs use the same map based on the page you are visiting. If you are on a page where the last path part is a valid category, then all of the other tabs will display because user_category_load() will always return an object. However if you are on a the privacy page added in the patch, then none of the tabs will display because _menu_load_objects() will return FALSE due to "privacy" not being a valid category.

Possible Solutions

Nothing easy jumps out at me. Either we override all of the menu items containing the %user_category wildcard argument loader and provide our own that returns an object when the "privacy" page is being viewed, or we fake the "privacy" page being a profile category and simply hide it from the forms where you can add fields to a category page. Are there any other ways we could tackle this?

Looks like a core issue has been filed that attempts to solve the issue mentioned above at #1137052: Argument map of parent router item is used when checking access for local tasks..

The code is now in 1404590-profile-privacy-ui branch, ready to be reviewed. I also removed the form elements from the users profile edit page and all relevant submit and theme calls.

Thanks! Attaching patch to the thread.

Marking as needs work because it seems that a few of the issues merged in from the 6.x-2.x branch are not reflected in the patch. We will have to git cherry-pick the following commits back into the branch so there aren't any regressions when these changed are merged back in.

• d97b0d03
• 5a2c4da
• 5d15295
• f969eab

The changes mentioned above have been cherry-picked ontl the ui branch, so it is now in-sync with the latest 6.x-2.x branch. Changing back to "needs review".

Added the new UI to edit the visibility overrides as well: https://img.skitch.com/20120119-giicaxrj7f7m1wqf1t86a4npg.jpg