XProfile fields used for signup should be configurable

[johnjamesjacoby]

BuddyPress has always used fields in the "Base" field-group as sign-up fields. In our templates, register.php just calls bp_has_profile() with custom arguments.

While it is possible to conditionally filter bp_before_has_profile_parse_args when bp_is_register_page() returns true, I believe BuddyPress should offer the ability to cherry-pick which fields across all field-groups should appear on the signup page.

[johnjamesjacoby]

First pass at simple custom template loop function

[boonebgorges]

A huge +1. This is always baffling to clients.

#5192 probably hinges on this ticket.

Please be mindful of backward compatibility when working on patch - we want to be sure to fall back gracefully on the Base group, and we want to be cognizant of the various ways that people customize the registration process. I've done a number of such customizations, so once I have a sense of how you plan to approach this, I'm happy to provide feedback on this point.

[johnjamesjacoby]

​6347.01.patch simply moves the bp_has_profile() and its arguments out of register.php and into a custom function.

The additional bp_parse_args() call allows explicit filtering of bp_before_signup_fields_parse_args to more easily target the arguments.

How I plan to approach this:

• New "Signup" metabox for all fields (including base?) with a checkbox to opt-into including
• Checkbox is linked to profile field meta similar to field visibility
• Custom query cached to bp_xprofile group to determine if any custom sign-up fields have been set
• If so, use them and *not* the old base-group approach
• If not, use the old base-group approach

UX Goals:

• If a user sets any profile fields as sign-up fields, they have opted into customizing the experience, and will naturally want to continue to pick others
• If a user has not set any profile fields yet, the status quo continues as is for backwards compatibility
• Consider improving visual distinction of which fields are: required, primary, sign-up

Stretch goals:

• Maybe begin to think about removing BuddyPress's dependency on the "Base" profile group and field. They aren't really hurting anything, but they start to make less sense when fields and groups become increasingly more flexible.
• We can create another new metabox for telling BuddyPress which profile field to use for the "Base" field, which BuddyPress traditionally uses as another "Display Name" field, which I think is pretty confusing.

[johnjamesjacoby]

One part I haven't worked out is if sign-up fields need custom positioning. If so:

• Where does that specific ordering live?
• How in the UI do we make this clear without making it too complicated?
• Do we allow completely avoiding the profile field requirement? With the approach above, having no fields selected will revert to existing behavior, and not show 0 fields.

[boonebgorges]

Something like [6347.01.patch] seems like a good start, and definitely doable for 2.3. I find the syntax a little odd - wrapping a template function rather than, say, filtering the bp_has_profile() arguments - but whatevs.

[johnjamesjacoby]

Includes metabox UI

[johnjamesjacoby]

​6347.02.patch does the following:

• Introduces metabox UI for saving field-meta keyed signup_position and sanitized with an (int) cast
• Adds $signup_position variable to BP_XProfile_Field class
• Currently just a checkbox for on/off, which saves the meta as 1 for all fields (for now) while position is experimented with
• No query manipulation yet

[johnjamesjacoby]

In 9685:

XProfile: Mild clean-up to bp_has_profile():

• Add phpdoc block
• Remove extract() usage
• Small code formatting tweaks

See #6347.

[johnjamesjacoby]

In 9686:

XProfile: Refactor bp_has_profile() stack to support passing arguments as associative arrays.

• Maintains backward compatibility by parsing old-style parameters using bp_core_parse_args_array(), converting to new-style array, and throwing _deprecated_argument() error when old-style is used
• Changes calls to BP_XProfile_Data_Template::__construct() to use the new format
• Hat-tip boonebgorges for similar work to other components previously.

See #6347.

[johnjamesjacoby]

I completely forgot that BuddyPress's profile field querying is married to querying for field-groups, and fetching individual fields could be considered a secondary notion.

r9685 and r9686 tidied a few existing functions and methods, but it looks like BP_XProfile_Field could use a static getter method just for retrieving & caching fields themselves, unrelated to groups and field data.

Will work on a patch with tests soon.

[johnjamesjacoby]

In 9687:

XProfile: General code formatting cleanup to bp-xprofile-template.php. See #6347.

[johnjamesjacoby]

In 9688:

XProfile: Missed a ! in r9687. See #6347.

[johnjamesjacoby]

In 9689:

XProfile: Remove phpdoc block copy/pasta from r9686. See #6347.

[johnjamesjacoby]

In 9690:

XProfile: First pass at saving and looking for signup_position metadata when viewing the main admin editing page. See #6347.

[johnjamesjacoby]

In 9692:

XProfile: Add xprofile_field_save group to BP_Tests_BP_XProfile_Field_TestCases:test_can_delete_save() test for easier testing of this unit. See #6347.

[johnjamesjacoby]

In 9693:

XProfile: Improvements to bp_xprofile_update_meta_cache():

• Cleaner logic for deciding whether a query is necessary
• Bail if: no query necessary, no where conditions, no uncached data, no data retrieved
• Avoid possible debug notice if $where_sql was never defined
• Fewer indentations and less needless execution via improved sanity checks

See #6347.

[sooskriszta]

By default, there should be no xprofile fields required at signup #5583

[johnjamesjacoby]

First pass at refactoring BP_XProfile_Field

[johnjamesjacoby]

​6347.03.patch is a big patch that does the following:

• Introduces smarter get() method for free-form querying of field data
• Uses this new static method where appropriate
• Modifies BP_XProfile_Field methods as appropriate
• Introduces field caching, including for field options
• Passes existing unit tests; needs tests to confirm cache invalidation is working correctly
• Works with existing xprofile_ functions
• Works with field groups & options
• Treats fields and options as one and the same, allowing them to use the same CRUD methods

I would appreciate feedback and help with the following:

• Are the query params okay? What can we add/remove/rename to make this better?
• Should we introduce a dedicated parse_args() method that can be used for DELETE queries also?
• Anyone want to wire-up the meta_query part?
• Our existing meta-cache implementation is a bit of a bolt-on, making it difficult to unwind smoothly with multiple layers of caching happening. It would be nice to simplify this eventually.
• Field groups getting Fields getting Field Data is quite the stack. It's not ideal, but I think it will need to be improved not as part of this initiative.

TL;DR - this is a big patch that touches lots of existing code. Similar to attachments, I'd like to commit this into trunk ASAP to continue iterating on the approach before this patch goes too stale.

[boonebgorges]

Thanks for your work on this so far.

I am a big fan of converting the disparate query methods to get() wrappers, especially since it builds in cache support for all these methods. I see that you've built in centralized invalidation for these caches - which I like - but let's make sure to check that this covers all the ways in which the field table can be updated. I haven't looked at this specifically, but my knowledge of the bp-xprofile component is such that I wouldn't be surprised if we were doing some direct UPDATE/INSERT queries elsewhere.

It looks to me like the get()/cache support refactoring comprises a standalone improvement. It would be nice to see it broken out at the time of commit. The signup IDs stuff is pretty straightforward and independent once the get() refactor is done.

I'm not sure I understand the parse_args() suggestion. Is it just so that we don't have to repeat the $args signature? A class property $default_args or something like that would also work.

The existing meta-cache implementation - by which I assume you mean bp_xprofile_update_meta_cache() - is indeed pretty clunky, and is built specifically with the needs of bp_has_profile() and its funky nesting in mind. I agree that it's probably a good idea to make it more flexible. Something like your update_field_caches() method, except for meta caches, might be good, though we'd want to make sure that in cases where we *do* cascade the hierarchy, we do a single query for each level of the hierarchy to populate the cache. (In other words: if querying for four profile groups, each with two fields, we do a single query to populate the four groupmeta caches, and a single query to populate the eight fieldmeta caches - *not* four separate fieldmeta cache queries, as the normal tree-walking technique would suggest.)

[johnjamesjacoby]

More incomplete iteration; uploading here incase I die before tomorrow

[johnjamesjacoby]

More incomplete iteration, again...

[Offereins]

The call for getting "this into trunk ASAP to continue iterating on the approach before this patch goes too stale" - does it still count? I was wondering what is needed to get this ticket going on.

The patch contains the signup-position metabox, but there's no screen where the user could get an overview of all selected fields and manually rearrange them. I think that is a required addition. Can I help with that or should we wait untill all the query/caching-stuff is in?

[DJPaul]

I'm not sure either what's up -- it sounds like there are some parts that could be pulled into a separate patch, but I know Boone's done some xprofile cache tweaks recently, I don't know if there's any overlap/redundancies.

Reluctantly moving to 2.5 because this hasn't received any love in the last 6 months.

[DJPaul]

We're closing this ticket because it has not received any contribution or comments for at least two years. We have decided that it is better to close tickets that are good ideas, which have not gotten (or are unlikely to get) contributions, rather than keep things open indefinitely. This will help us share a more realistic roadmap for BuddyPress with you.

Everyone very much appreciates the time and effort that you spent sharing your idea with us. On behalf of the entire BuddyPress team, thank you.

If you feel strongly that this enhancement should still be added to BuddyPress, and you are able to contribute effort towards it, we encourage you to re-open the ticket, or start a discussion about it in our Slack channel. Please consider that time has proven that good ideas without contributions do not get built.

For more information, see ​https://bpdevel.wordpress.com/2018/01/21/our-awaiting-contributions-milestone-contains/
or find us on Slack, in the #buddypress channel: ​https://make.wordpress.org/chat/

[imath]

I believe we should try to carry on the work John started here during the 8.0.0 release.