Opened 11 years ago
Closed 9 years ago
#5192 closed enhancement (fixed)
User roles with differents profile fields
Reported by: | _DorsVenabili | Owned by: | |
---|---|---|---|
Milestone: | 2.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Extended Profile | Keywords: | has-patch |
Cc: | tanner@…, wordpress@… |
Description
This is a ticket for a enhancement of the Extended Profile module.
It would be very useful to be able to create a group of profile fields only for a user role (or some of them).
This way, it would be possible to create custom roles with different fields and to have different registration forms.
Attachments (12)
Change History (72)
This ticket was mentioned in Slack in #buddypress by mamaduka. View the logs.
10 years ago
This ticket was mentioned in Slack in #buddypress by boone. View the logs.
10 years ago
This ticket was mentioned in Slack in #buddypress by boone. View the logs.
10 years ago
This ticket was mentioned in Slack in #buddypress by boone. View the logs.
10 years ago
#7
@
10 years ago
A while ago I wrote a plugin to assign XProfile field(groups) to BuddyPress groups. Since BP 2.2 my thought was along the lines of this topic to implement the same logic, but for member types. I did not realize it yet, but how would an eventual core implementation differ from the logic present in that plugin?
My plugin distinguishes two implications:
- applicability - who has to provide data for the field(group), at the Edit Profile page?
- visibility - who can see the entered data for the field(group), at the View Profile page?
The group assignment is done on the edit page of the field(group) and group ids are stored as profile field(group) meta.
To be clear: I'd like to help out with some of the code to make this happen. Yet, I'm not sure how it is envisioned by the core devs.
#8
@
10 years ago
Offereins - Thanks so much for chiming in. I didn't know about your plugin, and it looks like there is indeed some conceptual overlap. (Plugin code looks great, btw.)
First off, I wasn't envisioning any "visibility" implications for this first revision. I can certainly imagine a future enhancement that would allow member types to appear in visibility levels, but I don't think this ticket depends on it. I was imagining it purely in terms of "applicability".
Another conceptual difference I'd imagined is that I'd been imagining this to be a per-field setting, instead of per-group. I think we'd need to go with one or the other - trying to mix the two seems like it would cause weird issues.
At a very basic level, I'd imagined the following:
- When creating/editing a profile field in the Dashboard, admins would have a Member Type metabox. It'd be nice to have three-way toggles for each member type: 'Required', 'Optional', 'Disabled', or something like that. In this way, maybe it would replace the current "required" field.
- Perhaps a simpler (though less flexible) implementation would be a "Hide from Member Types" metabox, with simple toggles for each member type.
- Then we filter fields on the front end, in Edit mode. Similar in concept to your
filter_fieldgroups()
, though we could do it directly in the class instead of unsetting inside of a filter callback. - Ideally, we'd also interface with the registration process. This is perhaps too big a task. It'd mean providing a member type selector at registration, and then dynamically showing the proper xprofile fields based on the member type selected.
I'd value your thoughts on the above, given your experience building and using bp-xprofile-for-user-groups.
#9
@
10 years ago
@boonebgorges Thank you for your kind words and your clear response. Keeping it to applicability for now, is fine with me. I'm all for the metabox direction, as I've done in my plugin.
Could you elaborate on the foreseen issues with applying this to both fields and fieldgroups? Because I can see great value in it being implemented for both, and my implementation with groups didn't see much/any trouble.
Your first two bullets touch a decision we have to make on what the best user experience would be, I think:
- selecting member types that the field applies to
- selecting member types that the field does not apply to
- per-member-type selecting how the field applies (either optional, required or disabled)
My personal preference goes to option 1, since I imagine fields are more often specific to one member type than to all-but-one. Additionally I'm having difficulties finding a use case where a field would be required for one member type, but optional for another. Note that my plugin wasn't user tested, so I couldn't really tell which road would be best for an implementation in core.
@ Bullet 3: sure!
Concerning your fourth bullet, going beyond my experience, so thinking out loud: either 1) paged registration, 2) AJAX, 3) conditionally showing a member type's field(groups) with CSS: field containers would have a member-type-{all/$type}
class and JS would listen for a selected member type, then switch the page's <body>
member-type parent class.
I hope these are some constructive thoughts.
#10
@
10 years ago
Could you elaborate on the foreseen issues with applying this to both fields and fieldgroups? Because I can see great value in it being implemented for both, and my implementation with groups didn't see much/any trouble.
I'm concerned about UX when field and fieldgroup settings overlap. If you have group A with fields 1, 2, and 3; and if you set group A so that it's applicable only to member type 'foo', but set field 2 so that it's applicable only to member type 'bar', who actually sees field 2? In other words: how do settings cascade downward, and how do we show that in the UI? It's not impossible to do this, of course - we can dynamically change settings, and give warnings to users, etc etc - but it's much more complicated than simply having a single layer of settings.
A related idea: member-type-applicability applies only at the field level (not group), but we have a bulk-editing interface that allows admins to apply changes to entire groups at once. I think this might be easier than dealing with the cascading logic described above, and it accomplishes much the same thing.
My personal preference goes to option 1, since I imagine fields are more often specific to one member type than to all-but-one
This sounds sensible, and I think it makes sense to go with something simpler for the first version of the feature - we can build it out to be more robust later on if there is demand.
For registration, something like your option (3) is probably the best for the first version of the feature.
Offereins, if you are interested in taking the first swing at a patch for this - especially given your experience building a sorta-similar plugin - please let me know, and I will give you room to work before diving in myself :-D
#11
@
10 years ago
The following issues arise when developing this for a multiple-member-types-per-field situation:
- When multiple member types can be selected per field: how should this be stored in the database?
- When multiple member types can be selected per field: how does this translate to field types (optional/required), when different member types should have different field types?
For both issues counts: storing an array as data (either member types or field types) is bad when we would want to query profile fields per member type (see registration page suggestion).
Any suggestions on this or is it advised to discontinue the multiple-member-types track for now?
#12
@
10 years ago
I agree that storing an array is a bad idea.
I'm thinking something like this. Use bp_xprofile_add_meta()
to add a member type to a given field. This allows you to have multiple member types for a given field:
bp_xprofile_add_meta( $field_id, 'field', 'show_to_member_type', 'student' ); bp_xprofile_add_meta( $field_id, 'field', 'show_to_member_type', 'teacher' );
This means that when you query for a single field's member types, you need to be sure to fetch the whole array ($single = false
):
$member_types_for_this_field = bp_xprofile_get_meta( $field_id, 'field', 'show_to_member_type', false ); // array( 0 => 'student', 1 => 'teacher' )
In the loop, you will have access to a meta_query. (Not yet fully implemented; see [9722] and #6347.) So if you want to fetch all fields that need to be shown to the current user, it'll look something like:
$current_user_member_type = bp_get_member_type( bp_loggedin_user_id(), false ); // single=false bp_has_profile( array( // ... 'meta_query' => array( array( 'object' => 'field', 'key' => 'show_to_member_type', 'value' => $current_user_member_type, 'compare' => 'IN', ), ), // ... ) );
The API might look a little different here - you might, eg, pass a 'member_type' param to bp_has_profile()
, and then prepare the 'meta_query' clause internally, but this gives you the idea.
When multiple member types can be selected per field: how does this translate to field types (optional/required), when different member types should have different field types?
At a glance, I'd think that assigning a field to multiple member types would mean that a user with at least one of the member type would then see the field. So, in the above situation, a user with member type 'student' would see the field in question. We'll have to make this as clear as possible in the UI, and we'll have to come up with a trick to make sure that fields with *no* member type associations are shown to *all* users.
#13
@
10 years ago
Thank you very much for the metadata lecture. How wrong of me to never consider storing array meta values as separate meta instances. Finally using *_add_meta()
makes sense to me :)
Considering the last part of your reply, I don't think my point was clear: it concerns field requirement. Yes, only one member type should have to match, but how can we differentiate between field requirement types per member type? For example, how do we mark fields optional for teachers and required for students?
#14
@
10 years ago
Thank you very much for the metadata lecture. How wrong of me to never consider storing array meta values as separate meta instances. Finally using *_add_meta() makes sense to me :)
Awesome! :)
For example, how do we mark fields optional for teachers and required for students?
I have no idea. If we were going to do this, it should probably be part of the existing "Requirement" metabox, or perhaps it would replace it altogether. Maybe something like:
Setting Member Type [optional] Teachers [required] Students [none] Foo
where the part in [brackets] is a dropdown, and the Member Type is a list of all member types? This would probably also need a button that'd quickly enable "require for all" or "optional for all" or something like that, with a single click. You'd also need a row that represents users with *no* member types, I guess?
If any UX mavens have brilliant ideas....
#15
@
10 years ago
So I ran into a UX maven the other day... Made me decide to split the requirement from the member-type assignment and maybe, just maybe, have at least the latter finished in time for inclusion in 2.3 (I know the deadline sorta passed).
On the member-type/requirement split: after giving it some thought, it makes sense that in the future, fields may get more member-type specific attributes than just Field Requirement, like Field Visibility for example. This means that tying member-type assignment and requirement together is restricting these future enhancements.
So with this in mind, I think it is best to setup member types for fields first. All other member-type specific field attributes can be added through plugins or later inclusion.
All said, here is the feature plugin for field member-types: https://github.com/lmoffereins/bp-xprofile-field-for-member-types
Details:
- Works with BP 2.2
- Assign member types through checkboxes in a metabox on the Edit Field page
- Metabox only shows up when there is any member type registered
- Profile fields are removed on a member's page through the
bp_xprofile_get_hidden_fields_for_user
filter, where other access restrictions are checked too. - Plugin has it's own implementation of
bp_has_member_type()
#16
@
10 years ago
- Milestone changed from Future Release to 2.4
Offereins - Thanks for working on this! The general strategy - splitting the metabox from Requirement - does make sense.
Unfortunately, we're past the due date for 2.3 and there's a fair amount of work to be done here (even just in converting it to a BP patch), but let's definitely do something along these lines for 2.4.
A couple of thoughts and questions:
- When this is turned into a BP patch, should we continue to implement it as a filter on 'bp_xprofile_get_hidden_fields_for_user'? As a filter, it can be easily removed by a developer, which is both a good thing and a bad thing :)
- The proposed UX doesn't address the case where you want a field to "apply" to users who don't have any member type at all. To be honest, I don't know how to handle this. Maybe an additional checkbox called "[no type]" or something like that?
- Maybe instead of your "When no member type is selected..." description, all checkboxes could be checked by default? Then we could get rid of the gloss, I think.
- We probably need some helper text here. Something like: "Make this field available only to the following member types". It's very hard to come up with phrasing that makes this clear, especially because it's easy to confuse with the idea of visibility. IMO, "apply" is not a clear enough word - we need something that means "only users of the following types will have the field appear on their profiles", but obviously that sentence is terrible :)
#17
@
10 years ago
Boone - Thanks for your fast reply and feedback. Too bad about the non-inclusion for 2.3, but the feature plugin will be available for those who need it, untill 2.4.
Concerning your questions:
- I'm not aware of any WP/BP Core policy on internally applied filters. It may also be hardcoded into the function logic before the filter, as is done with visibility levels. I guess using a hook would at least either count for both the metabox and the filter or for none.
- Missed that one. Will create an additional checkbox as you suggest.
- Checking all items by default seems a little off for me. It has consequences for a) obligatory storing of field meta for all available member types on field creation, and b) new member type registration, which leads to confusing scenarios (do all fields immediately apply for the new member type?). Perhaps the gloss may seem to be a necessity.
- Valid point. Will come up with alternatives, but perhaps others within the community may also offer their suggestions.
#18
@
9 years ago
Just shipped version 1.0.0 of my plugin: https://github.com/lmoffereins/bp-xprofile-field-for-member-types. Implemented are points 2 and 4-ish. Feedback is much appreciated :)
This ticket was mentioned in Slack in #buddypress by offereins. View the logs.
9 years ago
#20
@
9 years ago
Very cool, Offereins!
I just pushed a mod to my fork of your repo that changes the wording of the description and the "null" option to read like in Screenshot_2015-07-02_14-41-09.png. The shorter description seems clearer to me; I like putting the "null" option at the end of the list; and I think that "users with no member type" is less ambiguous (to me, "none" could mean "no users").
I'm still concerned about my point 3 above. A few thoughts:
- In BP 2.3, all fields are shown to all member types. When upgrading to 2.4, the default behavior should be the same. Otherwise, existing fields will disappear until the admin takes action. So either (i) we'll need a migration routine during 2.4 upgrade, or (ii) when
get_xprofile_member_types()
returns an empty array, we'll need to interpret it as "all" rather than "none", or (iii) we'll need to change the way the data is stored so that a field that has never had the "member types" value set (ie, legacy fields) will be distinguishable from those that have had the "member types" value set to "no types". Each of these options is unpleasant in its own way, but I think we have to do one of them. - I think it's correct behavior that all boxes would be checked when creating new fields. It's likely that, for most sites, most new fields will be intended for all member types. That is: limited-access fields will be the exception, rather than the rule.
- It might be nice to have JS-powered "Select All"/"Select None" links, or maybe a checkbox column header for a similar purpose.
I'm particularly concerned about item (a) here. Offereins, do you have thoughts about which of these techniques is the least unpleasant? Or maybe you have a better idea :)
#21
@
9 years ago
Yup. I can see how your labeling clearifies the interaction with it. It just makes me think whether the xprofile meta table would be overloaded with irrelevant data when, as you say, fields will be intended for all member types most of the time. Besides that, I wonder what it means to have none of the boxes checked. Would the field become lost? Don't get me wrong, I like your implementation of the layout. I'm just not sure how it plays out data-wise.
As for point a/1), while that issue wouldn't exist with my implementation (*wink*), I'd like to go with option i
, since it would be the least hacky and could also account for installations that have the plugin already installed. It was the first solution that came to my mind as well.
Option ii
would be counterintuitive regarding the meaning of the (un)checked status of the member type boxes. This sort-of counts for option i as well.
For option iii
I have no idea why/how that would be a good idea.
#22
@
9 years ago
Just so I'm clear: On your implementation, having no boxes checked means that the field shows for everyone? (I guess that's what your inline comment "no member types were assigned, so user validates" means.) I agree that this makes the technical implementation much simpler, but I find it pretty confusing from a user's point of view. I agree that there's not much purpose to having a field that's available to *no* member types, but at least it makes for a consistent interface.
#24
@
9 years ago
@Offereins just tested your plugin, really interesting work, thank you.
About registrations:
1) i think we need to adapt core, your plugin is filtering bp_xprofile_get_hidden_fields_for_user
but when no user id is set, this filter is never fired. So we should adapt bp_xprofile_get_hidden_fields_for_user()
, else all "restricted to a member type" fields will always be displayed into the signup form.
see the patch attached to this ticket (5192.bp_xprofile_get_hidden_fields_for_user.patch)
But after applying this patch, you'll need to edit your plugin, else when on the xProfile Administration screen "the restricted to a member type" fields won't display.
see https://gist.github.com/imath/2360ad6c2b48a5bde160
2) Back on the ticket's description :
and to have different registration forms.
i think this can be done pretty easily and what i'm thinking of would not require css/js manipulation.
Let's imagine this: site.url/register/member_type_slug
See https://gist.github.com/imath/60c736673cc3edc804b9
(FYI ^^
is containing https://gist.github.com/imath/2360ad6c2b48a5bde160)
This ticket was mentioned in Slack in #buddypress by tanner. View the logs.
9 years ago
#26
@
9 years ago
- Cc tanner@… added
Thanks for all your hard work here @Offereins! The code looks fantastic and is very clear to follow.
After looking this over I would like to propose expanding this functionality to support restrictions in general. I would love to see this functionality be written so that it can be expanded to support user role restrictions, membership type restrictions, etc.
With that in mind I have forked @Offereins plugin and updated to support a more general approach. You can find the fork here: https://github.com/tannerm/bp-xprofile-field-for-member-types.
I have updated the verbiage in the admin to use "Restrictions" instead of member types and I think this will make a clearer UI.
Let me know what you all think.
#27
@
9 years ago
The more i think of it, the more i think this feature is really really interesting for 2.4 !
Again, @Offereins did a great work.
Maybe i'm overthinking it, but i think the logic used is improvable. What the plugin's doing is it waits for the visibility to be set on *all* fields before querying again *all* fields to check if some needs to be removed because they don't match with the member's member type.
I think my brain would feel more comfortable (maybe it's working in a strange way :) ) if we would reverse the order of the logic. First remove the fields that don't match the member types and then for *these remaining fields*, check for their visibility.
More globally the registration thing in my previous comment shows that we should also be able to fetch the fields for a given member type *without* needing any user id. Ideally there should be a new argument in bp_has_profile()
so it would be possible to use it in specific loops such as the registration one. Maybe something like :
function bp_has_profile( $args = '' ) { // Parse arguments $r = bp_parse_args( $args, array( 'user_id' => bp_displayed_user_id(), /* other arguments */ 'member_type' => bp_get_current_member_type(), ), 'has_profile' ); if ( ! empty( $r['user_id'] ) ) { $r['member_type'] = bp_get_member_type( $r['user_id'] ); }
In BP_XProfile_Group::get(), we could remove the fields that are not of the requested member types, and then apply the visibility to the remaining fields.
#28
@
9 years ago
@tanner - Thanks for your thoughts. It looks like your dropbox link has expired - I saw your screenshot the other day, but it's no longer available. Can you upload it directly to Trac?
I think I'm a fan of your UI innovation of separating out "This field is available to all members" from the type-specific selections. This clears up a fair amount of the confusion of the interfaces that Offereins and I had been working on. We'll need some JS that disables fields as appropriate, of course.
As for the question of a more general "Restrictions" field, I am a bit wary. Adding the ability to display content based on more than one criterion means that we're going to have AND/OR confusion. For example, if you have member types Student and Teacher alongside the WP roles, and you check the boxes Student and Author, does that mean that the field should be shown to all Students and all Authors, or only to those members who are both Students and Authors? Presumably the former, but what if one wants to do the latter? The UI here is complex enough when we're only worried about member types. My inclination is that, for 2.4, we should focus on a clear UI for that use case only, and think about making it more general (either in the same metabox, or with additional metaboxes) in future releases.
#29
@
9 years ago
@boonbgorges, I agree about the confusion that could be introduced with a more general approach to "Restrictions". I am not in any way saying that we should add support for anything other than Member Types. What I am proposing is that as we build it in a way so that plugin developers can extend it how they wish. They will then need to deal with clearing up that And/Or scenario but at least there would be a way for them to hook in and add that functionality.
Really all this means for us is that we store the information in a way that is friendly to other restriction types and add hooks and filters to allow others to do what they need to.
I think what we are doing here really has a lot of potential and I keep hearing from more and more people how they want to setup these kinds of custom restrictions.
Of course I'll be happy to contribute whichever direction we go, just my two cents. :)
In my fork of @Offereins plugins I have adjusted the the way the data is handled and have added the hooks and actions that I think would be needed to make this feature extensible.
#30
@
9 years ago
- Keywords has-patch added
Hi everyone,
Thanks for your continued work on this.
What I am proposing is that as we build it in a way so that plugin developers can extend it how they wish.
As things stand, developers are welcome to introduce new metaboxes for additional types of restrictions. Then they can handle data validation, building UI, etc in a way that makes sense for their specific restriction. Let's go with the more limited UI for now, and if there's huge demand for a broader system after the initial release, let's talk about it then.
5192.diff is a first attempt at converting this feature to a BP patch. It's based heavily on work done by tanner and especially Offereins. There are a few important places where I've diverged from the previous work. Here's a summary.
- I've moved the CRUD actions to methods on
BP_XProfile_Field
, which seems logical to me. - I removed the ability to set this data for XProfile Groups. Offereins, I gather from your code that you were kinda thinking of making it more broadly available, but I think that's a mistake for the first pass - we're then faced with all sorts of problems with cascading, as we've previously discussed. This is another thing we can address with a v2, if there's demand (though tbh I don't think it'll affect the data model - field_group settings will be a UI feature only, which is stored in the DB at the field level).
- I'm using kind of a wacky system for storing data. My primary goal was to build an interface and workflows that make sense. So let me explain those first, and then talk about how it works.
- By default, all fields (new and existing) are available to all member types, including the "null" type - users with no type. This means that the checkboxes are filled in for all new and existing fields.
- You can uncheck boxes, as you'd expect, to disable a field for given types.
- If you activate a new member type (via plugin), here's what happens. If an existing field is available for all types (all checkboxes are checked), then it continues to be available for all types, including the new type. If an existing field has limited availability (at least one checkbox is unchecked), then the new type will be *unchecked*. It seems to me that this is the behavior that most people would expect, since the default state of a field is to be available to everyone.
I've spent some time playing with various scenarios: new fields, existing fields, activating member types, disabling member types, etc, and the workflows described above all feel pretty natural to me. I would really love to get feedback on this.
The data model that makes it possible is, as noted, pretty weird. If all checkboxes are filled, then *nothing* is stored in the database. (This saves space: most fields will be available to everyone.) If *some* checkboxes are filled, then an xprofile fieldmeta entry will be saved for each. If *no* checkboxes are filled, then a *single* fieldmeta entry will be saved, with value '_none'
. Yes, this is weird. But the $field->get_member_types()
and $field->set_member_types()
should make it completely unnecessary for anyone to access these values directly, and these methods behave in exactly the way you would expect.
UI and code feedback welcome!
#31
@
9 years ago
Oh, one thing I forgot to mention in the above: Offereins had originally used the 'hidden fields' hook to hide fields from users. This is a good technique for a plugin, but I wanted something more direct for BP core. So I'm filtering out the off-limits fields just after they're fetched in BP_XProfile_Group::get()
. As it stands, this technique (and Offereins's technique, for that matter) will result in some database overhead during bp_has_profile()
queries. If the basic technique looks good to everyone, I can add some cache priming that will minimize the overhead.
This ticket was mentioned in Slack in #buddypress by boone. View the logs.
9 years ago
#33
@
9 years ago
Super, thanks @boonebgorges! I probably won't have time until Monday to check this out fully, but I'll get you some feedback as soon as I can.
#34
@
9 years ago
hi @boonebgorges, just tested it. Great work.
I guess my previous suggestion is not a good idea, so i won't insist anymore, although i really think a user_id shouldn't be required to fetch fields for a specific member type.
So to be consistent, as it's not possible to fetch fields for a given member type, i think we have 2 choices :
1- do not display fields restricted to one or more member types on the registration page (i don't think it's great).
2- or give a possibility to filter the fields by member types (see 5192.suggestions.patch)
About the UI: @offereins was adding a really interesting piece of information on the list of fields, i think we should have this. (see 5192.suggestions.patch)
It's not in my suggestions patch, and it's a detail but i think the meta_key should be 'member_type' instead of 'member-type', in other worlds the underscore instead of the dash just like the other meta_key we are using.
#36
@
9 years ago
Great job, Boone! For this moment I have the following feedback:
- The field groups were already talked about, so I'm really okay to leave them out for this one.
- I find it to be a clever move, using the
array( '_none' )
versus the emptyarray()
approach, considering using the default get/set methods should be sufficient for using the feature. This removes the need for an upgrade procedure. Yet, I have two things to note here:- Related to @imath's profile queries, I'm not too sure how this impacts using
BP_XProfile_Query
when using it with a/multiple user id(s) or a member_type query arg. - One thing that may confuse devs at first, is that the 'bp_xprofile_field_member_types' filter may contain
'_none'
and'none'
, both not real member types.
- Related to @imath's profile queries, I'm not too sure how this impacts using
- UI-wise, I think this is a very natural approach. Feels real nice!
- I'm really pleased with copying the i18n text-domain from my plugin :)
Questions:
- Should we disallow registration of member types with the
'_none'
or'none'
key?
#37
follow-ups:
↓ 38
↓ 39
@
9 years ago
- Keywords needs-patch added; has-patch removed
Thanks, all!
I guess my previous suggestion is not a good idea, so i won't insist anymore, although i really think a user_id shouldn't be required to fetch fields for a specific member type.
imath, my apologies. I didn't understand your previous suggestion until now. I think you're right - we should be fetching fields that are applicable to a specific set of member types. Let me work on this in the next patch.
As for how this affects registration: For v1, I think you're right that we should exclude restricted fields from registration. Devs can add custom UI to pull in these fields dynamically if they want. Maybe down the road we can add something like this to core.
One thing that may confuse devs at first, is that the 'bp_xprofile_field_member_types' filter may contain '_none' and 'none', both not real member types.
Ah, right. I think we need to keep 'none', because it's meaningful for devs ("users with no type"), but we should remove '_none'
before it hits that filter - devs should never see it.
Should we disallow registration of member types with the '_none' or 'none' key?
Yes, I meant to do this, but I forgot :) Will be in the next patch.
I'm really pleased with copying the i18n text-domain from my plugin :)
Oops :)
I'll work on a second patch.
#38
in reply to:
↑ 37
@
9 years ago
Replying to boonebgorges:
One thing that may confuse devs at first, is that the 'bp_xprofile_field_member_types' filter may contain '_none' and 'none', both not real member types.
Ah, right. I think we need to keep 'none', because it's meaningful for devs ("users with no type"), but we should remove
'_none'
before it hits that filter - devs should never see it.
Err, sorry for the confusion. I took another look at your patch and there's no situation where '_none'
hits the filter. So you can ignore it. Thanks.
#39
in reply to:
↑ 37
@
9 years ago
Replying to boonebgorges:
imath, my apologies. I didn't understand your previous suggestion until now. I think you're right - we should be fetching fields that are applicable to a specific set of member types. Let me work on this in the next patch.
No problem :) sorry for the confusion i've introduced. Actually 5192.diff is partly replying to my first concern: get the fields for the member type(s) before applying visibility levels to the remaining fields.
And yes my second concern was the ability to get fields for one or more member types without requiring the user_id. I'm happy we're ok with this possibility as it will be very easy then for a plugin to have registration forms by member types :)
Thanks a lot for your great work on this really interesting feature.
#40
@
9 years ago
- Keywords has-patch added; needs-patch removed
[5192.2.diff] is an update with the following changes:
- Fixed textdomains.
- Switched to 'member_type' instead of 'member-type' for meta_key.
- To decrease confusion, use 'null' to signify members with no member types. Continue to use '_none' internally for fields that should be unavailable to all types.
- In
bp_register_member_type()
, enforce an array of illegal names. - Added a "Member Type: Foo, Bar" label to the Profile Fields admin page.
- imath's suggestion: In
BP_XProfile_Group::get()
, instead of doing a post-query filter of fields to remove those that are not applicable to the current user's member type(s), do a pre-query check to get a list of the user's member_types, and the fields available for those types. The heavy lifting is done byBP_Xprofile_Field::get_fields_for_member_type()
, which returns a multidimensional array that looks like this:[3] => ['student', 'teacher'], [5] => ['student', 'null']
etc, where 3 and 5 are field IDs and the array values are the member types for each field. - Add 'member_type' parameter support to the
bp_has_profile()
function stack. The default value inbp_has_profile()
is 'any', which is a special value that returns only those fields that are available to *all* types. 'member_type' is ignored if 'user_id' is non-zero. This setup means that restricted fields are unavailable during registration by default.
The new queries in get_fields_for_member_type()
are a bit ugly. I will add some caching support - maybe even caching in a transient, so that all sites will get it, not just those running a persistent cache - if we are all agreed that the current strategy is a good one.
Thanks for playing along at home :)
This ticket was mentioned in Slack in #buddypress by boone. View the logs.
9 years ago
#42
@
9 years ago
Looks good Boone!
Here are a few comments:
- I found the conditional statement on line 750 of class-bp-xprofile-field.php to be pretty confusing. I'm pretty sure all we need is:
if ( ! array_intersect( $field_types, $member_types ) ) { ... }
- I like the UI on the profile field page, I think it is simple and clear.
- I think we should remove/change the Member Types label for the Name required profile field since it is not something that can be updated.
- I actually wonder if we should not have Member Type restrictions for the Primary field groups since those fields will be completed on registration before a Member Type is even assigned.
#43
@
9 years ago
@boonebgorges here's my feedbacks :
1/ thanks a lot for adding the possibility to get fields for a given member type even if the user_id is not set.
2/ thanks a lot for using member_type
instead of member-type
for the meta_key
3/ As discussed on slack, the _none
value (meaning no checkboxes are checked) should generate a message to inform the field won't be displayed to any member.
4/ The base profile field Name (primary)(required)
shouldn't have the metabox to set the member type, as it needs to always be output.
5/ Thanks a lot for BP_XProfile_Field->get_member_type_label()
, i think this piece of info in the list of xProfile fields is really interesting, but :
- when a field concerns all member types, as it's the default value somehow, the list of member types labels doesn't need to be output imho.
- in other words, maybe we should only output the labels if not all the checkboxes are checked.
6/ I strongly disagree with @tanner about disabling the possibility to affect a member type to any field belonging to the first group of fields (the "registration" group of fields). As i suggest in point 4, we should only disable this possibility for the Name (primary)(required)
field. I think it's important we leave the possibility to plugin authors to create multiple registration forms (eg: one by member type), which is a need specified as the 3rd point of this ticket's description. BTW it's pretty easy to achieve, now that it's possible to get fields for a given member type :)
7/ When no member types are registered :
- i think no labels should be output in the list of xProfile fields (
users.php?page=bp-profile-setup
) to be consistent with the fact that no metaboxes are displayed. - any field should be output no matter the value of the
member_type
into the database, because the member types feature is kind of inactive in this case.
Thanks a lot for your great work so far :)
#44
@
9 years ago
I really like where this is going! Huge thanks for your work, Boone :)
In reply to @boonebgorges:
- Some tests seem to use
$fg
in the assertions where I'd expect$the_group
to be used (inclass-bp-xprofile-group
).
In reply to @imath:
- I've read the discussion on Slack and the idea of the message accompanying the all-non-checked state gets a +1 from me.
- That restriction was already in my plugin and is adopted in the diff (bail on
1 === $field->id
). No harm there. - +1, only labels for fields that are need an explanation.
- I agree with imath.
- Good catch! +1 as well, for both points. This thing we build can become complex, but we shouldn't forget there may be installations without any member type at all.
#45
@
9 years ago
Again, thanks for the great feedback, everyone - very, very helpful.
Changes in 5192.3.diff:
- Updated the appearance of labels on the Profile Fields page. When a field is available for all types (unrestricted), I've removed the label. See the screenshot above. I have mixed feelings about this: it does make for less clutter, but I wonder if it will make people wonder whether the field is invisible to all types. I also changed the format of the label shown when a field is available for *no* types - see the red message in the screenshot.
- Added a dynamic notice to the metabox when all boxes are unchecked. See screenshot above. The message appears/disappears as you'd expect when checking/unchecking boxes. For all of these labels, I'd welcome suggestions for improved wording.
- I added a thin layer of caching for
get_fields_for_member_type()
. It's not straightforward to cache entirely, due to complications in the way that member types are registered with BP at runtime (making cache invalidation complicated), and newly registered fields should be available for all member types at the time tha they're registered. - Don't show a Member Type message for field 1.
- In
BP_XProfile_Group::get()
, skip processing 'member_type' param if no member types are registered.
I agree that we should allow admins to restrict fields in the "Base" group. I see tanner's logic here, but I agree with the others that it's an undue restriction, and that there are good use cases for having restricted fields in group 1.
Any thoughts are welcome!
#46
follow-up:
↓ 47
@
9 years ago
Just wondering whether we should account for member types that have been unregistered. Picture a scenario were a member type was selected, but now it only spooks around in the meta table without a tie to the real world. It is only relevant when we're fetching labels, but we may need do an exists()
check. Or not?
#47
in reply to:
↑ 46
@
9 years ago
Replying to Offereins:
Just wondering whether we should account for member types that have been unregistered. Picture a scenario were a member type was selected, but now it only spooks around in the meta table without a tie to the real world. It is only relevant when we're fetching labels, but we may need do an
exists()
check. Or not?
Oh yeah, good call. I think we shouldn't bother with the meta table itself. Plugins should be responsible for cleaning up their own messes, and in any case we wouldn't want to delete content, since admins may toggle a plugin on and off for testing, etc. However, we should definitely have a check before outputting the label. And in fact, the code as it stands will throw an error in this case. See lines 621-622 of get_member_type_label()
in https://buddypress.trac.wordpress.org/attachment/ticket/5192/5192.3.diff. This should get a sanity check for the next patch.
#48
@
9 years ago
Thanks a lot @boonebgorges just tested .3.patch and imho we're good to go for a first pass :)
- About the labels, as a red message is informing when a field is unavailable, i don't think the user will have any doubt. If nothing is shown, then any member can see the field.
In the list of xProfile fields : i think we could simplify the message "Unavailable to all member types" in favor of "Unavailable to all members". The first one is a bit "not completely exact" as some could understand that a member with no member type should see the field. I can think of a situation when the admin has not used the "Users with no member type" choice yet, or when he's a bit tired :)
Same for "Fields with no members type..." It's more "Fields with none of the above choices..." Or we could simplify with "This field is unavailable to all members"
But i'm not sure to be the best one to suggest english messages :)
#49
@
9 years ago
This is looking great! Big thumbs up from me. :)
Also, I understand not limiting restrictions the whole primary field group. I did not really like that restriction, just wanted to make sure we were going to have a consistent registration flow.
This ticket was mentioned in Slack in #buddypress by boone. View the logs.
9 years ago
#51
@
9 years ago
- Some fixes to unit tests
- Simplified the red "no member type" messages - thanks for the good advice, imath.
- Cleaned up docblocks
- For
get_member_type_label()
, make sure that (a) the types are listed in alphabetical order, and (b) the "null" option always appears last. - Regarding Offereins's note about inactive member types: this was actually already implemented, because
$field->get_member_types()
doesn't return invalid types :)
I showed the feature off during this week's dev chat, and people seemed to like it, so I guess this is a last call for comments before initial commit.
#52
follow-up:
↓ 53
@
9 years ago
Does this need more eyes or should this just get into the open for testing in real admin work flows?
#53
in reply to:
↑ 52
@
9 years ago
Replying to Offereins:
Does this need more eyes or should this just get into the open for testing in real admin work flows?
I'll commit it later today or tomorrow. I think we're ready for wider testing in trunk.
See also #5121, which discusses the related project of breaking out capabilities (in part so that they can be linked to user types).