Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

more robust social ids for social sync #1843

Merged
merged 1 commit into from
Oct 12, 2020
Merged

more robust social ids for social sync #1843

merged 1 commit into from
Oct 12, 2020

Conversation

call-me-matt
Copy link
Member

@call-me-matt call-me-matt commented Oct 10, 2020

@codecov
Copy link

codecov bot commented Oct 10, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@1494d93). Click here to learn what that means.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1843   +/-   ##
=========================================
  Coverage          ?   37.02%           
  Complexity        ?      160           
=========================================
  Files             ?       19           
  Lines             ?      497           
  Branches          ?        0           
=========================================
  Hits              ?      184           
  Misses            ?      313           
  Partials          ?        0           
Impacted Files Coverage Δ Complexity Δ
lib/Service/Social/FacebookProvider.php 0.00% <0.00%> (ø) 8.00 <0.00> (?)
lib/Service/Social/InstagramProvider.php 0.00% <0.00%> (ø) 9.00 <0.00> (?)
lib/Service/Social/MastodonProvider.php 0.00% <0.00%> (ø) 9.00 <0.00> (?)
lib/Service/Social/TumblrProvider.php 0.00% <0.00%> (ø) 6.00 <0.00> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1494d93...e3814b7. Read the comment docs.

@call-me-matt
Copy link
Member Author

call-me-matt commented Oct 10, 2020

iOS adds an x-apple to the value for social profiles. Is this legal?

X-SOCIALPROFILE;type=Instagram;x-user=nextclouders:x-apple:nextclouders.

Nextcloud interprets it like this:
apple-marker

If the format from apple is acceptable, this PR filters out the additional x-apple.
Alternatively, the contacts app could be adapted to strip the value (which would display things nicer in the web app)

@call-me-matt call-me-matt self-assigned this Oct 10, 2020
@call-me-matt call-me-matt requested a review from skjnldsv October 10, 2020 11:38
@call-me-matt call-me-matt added 0. Needs triage 2. developing Work in progress compatibility Compatibility with other services labels Oct 10, 2020
@call-me-matt call-me-matt changed the title robuster social ids for social sync more robust social ids for social sync Oct 10, 2020
@@ -35,6 +35,7 @@ public function __construct() {
* @return string
*/
public function cleanupId(string $candidate):?string {
$candidate = preg_replace('/^' . preg_quote('x-apple:', '/') . '/', '', $candidate);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should go into the level above so $candidate is already sanitized?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$profileId = $socialProvider->cleanupId($socialEntry['value']);

Copy link
Member Author

@call-me-matt call-me-matt Oct 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, it can be there, too. But apple only attaches this to custom fields. For certain networks (for example Facebook and Twitter) it adds the domain of the service and no x-apple. So because it is not valid for all networks, I only added it where necessary. I'll let you decide where it's best.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I guess it's easier to have it once and for all rather than on every provider?
Maybe I'm missing something 🤔 😉

Copy link
Member Author

@call-me-matt call-me-matt Oct 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to be clear: we do not need it for every provider. apple only adds this "x-apple" thingy to the networks it does not know. All pre-defined networks (in the iOS-contacts-app) don't have that tag. So I thought its more a temporary thing and it can be removed time after time from the known networks.

@call-me-matt
Copy link
Member Author

big news: I found a better way to get the profile id from facebook for any profile! It works very nicely (until fb changes their site...)

- get facebook id from usernames
- accept mastodon usernames without leading at
- ignore iOS marker as in 1107#issuecomment-706518724

Signed-off-by: call-me-matt <nextcloud@matthiasheinisch.de>
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 0. Needs triage 2. developing Work in progress labels Oct 12, 2020
@skjnldsv skjnldsv merged commit ca7a921 into master Oct 12, 2020
@skjnldsv skjnldsv deleted the fix/social-apple branch October 12, 2020 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews compatibility Compatibility with other services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants