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

Fix setting fullname for ORCID #666

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hmpf
Copy link
Contributor

@hmpf hmpf commented Feb 15, 2022

Originally, fullname was set to response['person']['name'], which in
ORCID is a large json object. Everywhere else, fullname is assumed to be
a string. It cannot be used directly as a fullname but needs converting.

Proposed changes

The patch ensures that fullname is a string, in a similar way to how other backends do it.

Types of changes

Please check the type of change your PR introduces:

  • Release (new release request)
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (PEP8, lint, formatting, renaming, etc)
  • Refactoring (no functional changes, no api changes)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build related changes (build process, tests runner, etc)
  • Other (please describe):

Checklist

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

(I had to install libxmlsec1-dev manually on debian in order to get tox to run.)

Originally, fullname was set to `response['person']['name']`, which in
ORCID is a large json object. Everywhere else, fullname is assumed to be
a string. It cannot be used directly as a fullname but needs converting.
@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #666 (2846d5a) into master (17476e3) will increase coverage by 0.26%.
The diff coverage is 75.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
+ Coverage   76.70%   76.96%   +0.26%     
==========================================
  Files         317      317              
  Lines        9598     9646      +48     
  Branches      465     1033     +568     
==========================================
+ Hits         7362     7424      +62     
+ Misses       2084     2071      -13     
+ Partials      152      151       -1     
Flag Coverage Δ
unittests 76.96% <75.79%> (+0.26%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
social_core/backends/apple.py 69.13% <0.00%> (ø)
social_core/backends/asana.py 86.95% <ø> (ø)
social_core/backends/atlassian.py 100.00% <ø> (ø)
social_core/backends/azuread_tenant.py 0.00% <0.00%> (ø)
social_core/backends/battlenet.py 0.00% <ø> (ø)
social_core/backends/beats.py 0.00% <ø> (ø)
social_core/backends/cilogon.py 90.47% <ø> (ø)
social_core/backends/coding.py 0.00% <ø> (ø)
social_core/backends/discord.py 0.00% <0.00%> (ø)
social_core/backends/douban.py 0.00% <0.00%> (ø)
... and 146 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 98c81ad...2846d5a. Read the comment docs.

if name:
first_name = name.get('given-names', {}).get('value', '')
last_name = name.get('family-name', {}).get('value', '')
fullname = first_name + ' ' + last_name
fullname = fullname.strip()
Copy link
Member

Choose a reason for hiding this comment

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

Can you please look why this is not covered by tests? The user data payload seems to be present in the test case, but it doesn't seem to be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants