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

add support for downloading avatars from gravatar #1906

Merged
merged 4 commits into from
Nov 5, 2020

Conversation

eleith
Copy link
Member

@eleith eleith commented Nov 1, 2020

adding support for gravatar to the current social providers/services.
because gravatar relies on email instead of the x-socialprofile carddav
field, this requires re-working the current abstraction for how social
providers work.

also in this change is improved support for when a contact card has
multiple fields of the same type (as might be common with email) thus
they all can be checked for profile photos if one of them doesn't result
in a valid photo

fixes #1905

@eleith eleith marked this pull request as draft November 1, 2020 03:43
@eleith
Copy link
Member Author

eleith commented Nov 1, 2020

this is a draft (no linting, no tests), based on a WIP branch i'm using locally to improve photo coverage in my address book.

because gravatar requires the email field, i found the current abstraction layer for the social providers needed some iteration to be more flexible about what parts of a contact card are needed to do photo lookups.

also, because gravatar relies on the email field, i found it quite common that a contact card has multiple email fields, but only one of those would have a gravatar profile photo associated with it. thus, supporting multiple lookups (as opposed to just using the first one, irrespective of if it returns a photo or not) was important.

i've modified the social provider abstraction to both support being more general about what fields are needed for the profile photo lookup and to also support looking up an array of the same fields, if the first one doesn't return a valid profile photo.

is their appetite to see this feature land? if so, i'ld be happy to invest time in improving code quality if i can get some guidance on some of the decisions made currently and high level ideas about what to test.

@SuperSandro2000
Copy link

Can you also add support for libravatar? I think it uses the same URL scheme but a different domain and is free-er.

Copy link
Member

@call-me-matt call-me-matt left a comment

Choose a reason for hiding this comment

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

This looks great! I am very happy to see this feature evolving.
Thank you also for finding and fixing some errors in this PR.
I only found a typo and did not yet have the chance to try it out, but the code looks good.

lib/Service/Social/GravatarProvider.php Outdated Show resolved Hide resolved
lib/Service/Social/MastodonProvider.php Outdated Show resolved Hide resolved
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Please check the indentation too :)

adding support for gravatar to the current social providers/services.
because gravatar relies on email instead of the x-socialprofile carddav
field, this requires re-working the current abstraction for how social
providers work.

also in this change is improved support for when a contact card has
multiple fields of the same type (as might be common with email) thus
they all can be checked for profile photos if one of them doesn't result
in a valid photo

Signed-off-by: leith abdulla <online-nextcloud@eleith.com>
@eleith
Copy link
Member Author

eleith commented Nov 1, 2020

adopted linting/style, addressed some minor feedback and will begin working on unit testing

@call-me-matt
Copy link
Member

Just in case someone else also does not have any contacts using gravatar, you can use this email address for testing: wzueyuqgwqsgeiiqsr@niwghx.online

@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #1906 into master will increase coverage by 33.15%.
The diff coverage is 89.36%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #1906       +/-   ##
=============================================
+ Coverage     33.69%   66.85%   +33.15%     
- Complexity      178      249       +71     
=============================================
  Files            21       22        +1     
  Lines           546      691      +145     
=============================================
+ Hits            184      462      +278     
+ Misses          362      229      -133     
Impacted Files Coverage Δ Complexity Δ
lib/Service/Social/CompositeSocialProvider.php 0.00% <0.00%> (ø) 6.00 <4.00> (-4.00)
lib/Service/Social/DiasporaProvider.php 88.88% <90.00%> (+88.88%) 21.00 <20.00> (+11.00)
lib/Service/Social/MastodonProvider.php 87.80% <90.00%> (+87.80%) 19.00 <14.00> (+10.00)
lib/Service/Social/XingProvider.php 87.17% <90.00%> (+87.17%) 19.00 <18.00> (+11.00)
lib/Service/Social/TwitterProvider.php 92.68% <95.45%> (+92.68%) 17.00 <10.00> (+6.00)
lib/Service/SocialApiService.php 88.35% <95.45%> (+1.13%) 55.00 <17.00> (+8.00)
lib/Service/Social/InstagramProvider.php 92.85% <96.42%> (+92.85%) 17.00 <11.00> (+8.00)
lib/Service/Social/FacebookProvider.php 90.69% <100.00%> (+90.69%) 17.00 <12.00> (+9.00)
lib/Service/Social/GravatarProvider.php 100.00% <100.00%> (ø) 6.00 <6.00> (?)
lib/Service/Social/TumblrProvider.php 96.15% <100.00%> (+96.15%) 12.00 <11.00> (+6.00)
... and 9 more

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 247b727...ffddb70. Read the comment docs.

@eleith eleith force-pushed the support-gravatar branch 2 times, most recently from c452f09 to 529bb95 Compare November 2, 2020 03:42
@eleith
Copy link
Member Author

eleith commented Nov 2, 2020

@call-me-matt code coverage dropped because off pushing more of the x-socialprofile logic directly into the individual social providers, duplicating that logic into each of the providers, and also providing handling for supporting multiple image URLs from one provider being found.

there aren't any existing unit tests for the social providers, so i haven't taken a stab at those yet. with that said, if there was an obvious place to abstract some of the x-socialprofile handling, it would help decrease the complexity and make testing easier in the future.

otherwise, linting and unit tests are now done (and test coverage of socialapiservice actually increased).

@eleith eleith requested a review from call-me-matt November 2, 2020 03:50
@eleith eleith marked this pull request as ready for review November 2, 2020 03:51
Signed-off-by: leith abdulla <online-nextcloud@eleith.com>
@skjnldsv skjnldsv added 2. developing Work in progress enhancement New feature or request labels Nov 2, 2020
Copy link
Member

@call-me-matt call-me-matt left a comment

Choose a reason for hiding this comment

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

Great job, thank you

css/icons.scss Outdated Show resolved Hide resolved
lib/Service/Social/DiasporaProvider.php Outdated Show resolved Hide resolved
lib/Service/Social/DiasporaProvider.php Show resolved Hide resolved
this commit adds unit tests for all providers while also reducing some
redundancy in looking up social fields

minor feedback was addressed as well as some minor bugs fixed

Signed-off-by: leith abdulla <online-nextcloud@eleith.com>
use a gravatar logo instead of a wordpress logo

Signed-off-by: leith abdulla <online-nextcloud@eleith.com>
@eleith eleith requested a review from call-me-matt November 4, 2020 20:01
@eleith
Copy link
Member Author

eleith commented Nov 4, 2020

@call-me-matt all feedback has been addressed. given there was a bug in one of the providers, i went ahead and added unit tests for all of the other providers and this PR should increase code coverage overall now.

@eleith eleith added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 4, 2020
Copy link
Member

@call-me-matt call-me-matt left a comment

Choose a reason for hiding this comment

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

looks good, works nicely, great job!

@eleith
Copy link
Member Author

eleith commented Nov 4, 2020

@call-me-matt can i merge this myself, or does someone else need to? (the merge button does highlight for me, but wanted to double check...)

@call-me-matt
Copy link
Member

can i merge this myself, or does someone else need to?

I guess so, but usually I wait for @skjnldsv as the maintainer of this app for feedback. Normally, he also does the merge right away.

@skjnldsv
Copy link
Member

skjnldsv commented Nov 5, 2020

can i merge this myself, or does someone else need to?

I guess so, but usually I wait for @skjnldsv as the maintainer of this app for feedback. Normally, he also does the merge right away.

If it's green, please do! 💪✨

@eleith eleith merged commit 9226cb1 into nextcloud:master Nov 5, 2020
@eleith eleith deleted the support-gravatar branch November 5, 2020 19:29
@eleith
Copy link
Member Author

eleith commented Nov 5, 2020

thanks everyone. pleasure working with you both on this one.

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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support for gravatar profile photo lookups
4 participants