Project

General

Profile

Feature #4797

bp-social-media-profiles should build/validate URLs dynamically

Added by Boone Gorges almost 4 years ago. Updated over 3 years ago.

Status:
Resolved
Priority name:
Normal
Assignee:
Category name:
Public Portfolio
Target version:
Start date:
2015-10-19
Due date:
% Done:

0%

Estimated time:

Description

bp-social-media-profiles only builds and validates URLs on profile save. But this causes problems when we try to fix any parse errors; see eg #4565. We should remove this saving logic, and process the fields on display. (This will have the side effect of making the plugin quite a bit simpler.)

Dan, since you've been digging in this plugin a bit :) could you have an initial look?

History

#1 Updated by Daniel Jones over 3 years ago

Hey Boone - finally getting around to this. I think I'm starting to be able to wrap my head around it, but want to check in about a couple of things:

1. Is the goal here is to run most of the code that's currently in "save_field_data" in "display_markup" instead?
2. First step in that is to modify save_field_data so that it's only saving field=>value pairs for each user, right?
3. I should continue to use this line of code:

bp_xprofile_update_fielddata_meta( $fielddata->id, 'bp_smp_data', $smp_data );

In save_field_data, but just change the contents of $smp_data to only save the value the user provides, and generate the other values on display?

Or, should I be doing something completely different, replacing the use of bp_xprofile_update_fielddata_meta() with another way of storing the information about users and their profiles?

#2 Updated by Boone Gorges over 3 years ago

1. Is the goal here is to run most of the code that's currently in "save_field_data" in "display_markup" instead?
2. First step in that is to modify save_field_data so that it's only saving field=>value pairs for each user, right?

You'll probably be able to remove `save_field_data()` altogether - let BP handle saving the raw data - and then move the format processing to `display_markup()`.

. I should continue to use this line of code:
bp_xprofile_update_fielddata_meta( $fielddata->id, 'bp_smp_data', $smp_data );
In save_field_data, but just change the contents of $smp_data to only save the value the user provides, and generate the other values on display?

It's not clear to me, at a glance, that you'll have to save anything additional to the database. The idea is that if we have a Twitter field, and I enter 'boone', then 'boone' is saved in the database. Then, when displaying, we let the Twitter parser build the proper markup:

<a href="https://twitter.com/boone">@boone</a>

This is what I meant when I said that the plugin would probably be pretty radically simplified if we went this route.

It might turn out to be much more efficient/sane to do some pre-save validation. Take Twitter as an example. A user might enter 'boone', '@boone', 'https://twitter.com/boone', 'twitter.com/boone', etc. Maybe we want to pre-parse this data into a canonical format before saving - say, to 'boone'. This will make the display logic, which transforms it into markup, much simpler.

Regarding backward compatibility: Don't worry about it for the time being. Imagine you don't have to deal with legacy data. If you come up with a way of making this work elegantly, we can either write a migration script or a compatibility shim.

I've given you push access to https://github.com/boonebgorges/bp-social-media-profiles/. It might be easier to work on this in a more "vanilla" setting. (It's possible that this repo is out of sync with the Commons. You may want to check, and backport any Commons fixes, before doing any big work.)

Feel free to take this opportunity to rethink the plugin a bit, and let me know if you have questions or want to talk through it at all.

#3 Updated by Daniel Jones over 3 years ago

Okay great thanks for clarifying - the only question I think I have is where in the database I could store that data. Could it be a meta field (which I guess would be best as an array of site names and user-provided values) tied to the BP user object?

#4 Updated by Boone Gorges over 3 years ago

I think you should let bp-xprofile store the data itself. Currently, the plugin intercepts the profile save; it then modifies the content that bp-xprofile eventually saves, and stores some additional information as xprofilemeta. My suggestion is that we simply let bp-xprofile save as usual, or perhaps intercept the save and do some preprocessing to ensure that the values are properly formatted (and properly atomic - ie, just a username instead of the entire URL) before saving.

#5 Updated by Daniel Jones over 3 years ago

I think I've done the bulk of this here: https://github.com/cuny-academic-commons/cac/commit/0cfd4458ec5f4bf3c2cbd4fc277557c09b5d9323

I mostly just ended up moving most of the functionality that had been in "save_field_data" to "setup_user_sm_fields". I wasn't able to simplify very much, because the logic around deciding on which callback to use had to stay the same, and there wasn't much to be gained as far as I could see by trying to change the "create_field_html" function. Essentially what I do here is still generate all the data that used to go into bp_smp_data meta values for individual fields, and instead generate it on display and pass it to the function for making HTML.

Also - it looks like the callbacks already do much of the validation that you were talking about, although for some reason some of them don't make use of the URL patterns, and seem to just be trusting the user to provide the right kind of value. Should I change those? Right now those are Facebook, Vimeo, and Academia.

Let me know what you think!

#6 Updated by Daniel Jones over 3 years ago

After looking again, I don't think we'll need a migration here, because even before, the value for the profile field was stored directly through bp xprofile and not just in the field meta, and that's all we use in the new version. So I think all that'll happen is we'll have some leftover field meta values that we won't be using - I'd defer to you on whether or not it's worth it to write a script to delete all of that.

Let me know if you think this is right.

#7 Updated by Boone Gorges over 3 years ago

  • Target version changed from Future release to 1.9.8

Thanks, Dan!

This is looking good in my testing. After your changes, I think that the validate_field_data() method no longer serves a purpose, so I removed it in https://github.com/cuny-academic-commons/cac/commit/5f451d862672005423009a0aee3b6cf07dfceac7

Could I ask you to port these changes to my repo containing this plugin? https://github.com/boonebgorges/bp-social-media-profiles I think it's probably just a matter of forking, copy/pasting, and then sending a pull request, though I can't remember 100% whether that repo has diverged from the Commons version, or vice versa.

#8 Updated by Boone Gorges over 3 years ago

  • Target version changed from 1.9.8 to Not tracked

I'm going to move this ticket out of the milestone. The fix will go into 1.9.8, but I would like the changes pushed back upstream so that the Github version of the plugin remains in step with what we've got on the Commons. Thanks!

#9 Updated by Daniel Jones over 3 years ago

I think we ran into an issue with this before, where the two have diverged so that just copy and pasting the CAC version would mean removing support for a number of social media profiles: Vine, Last.fm, etc. What do you think the best thing to do is?

#10 Updated by Boone Gorges over 3 years ago

Yes, it's probably true that a straight copy will remove some things. Can you work on merging them, so that these things are not lost? By copying over the repo files with what's in CAC, you should be able to use git diff to get a pretty quick sense of what needs to be done to reconcile the differences.

#11 Updated by Daniel Jones over 3 years ago

Hey Boone - So I've gone in and reconciled the differences, and pushed them to the master branch of my fork of your repo. I'm not sure how to do the pull request though - would it work to just accept my old one now? Or do I have to do something else?

Sorry!

#12 Updated by Boone Gorges over 3 years ago

  • Status changed from Assigned to Resolved

Looks like the changes were added to the existing PR. I've just looked over the diff and am giving it a thumbs up. Thanks for your work on this!!

Also available in: Atom PDF