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 nfdi ops #1068

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open

Add nfdi ops #1068

wants to merge 32 commits into from

Conversation

marcvs
Copy link

@marcvs marcvs commented Mar 26, 2025

I'm adding a whole bunch of OPs for the german NFDI (national research data initiative).
Local tox did not work for me, so I try it this way.

My tests are not extensive so far. If initial feedback is positive, I'm willing to fix the tests, too.

marcvs and others added 30 commits February 27, 2025 14:09
…1.0 (python-social-auth#1049)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…1.1 (python-social-auth#1050)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…1.2 (python-social-auth#1051)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
… v0.24.1 (python-social-auth#1052)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
This is possibly a breaking change for a few backends but makes the code
follow RFC and removes need for every backend to override this.

Fixes python-social-auth#284
Using md5 is insecure, but these services mandate it, so clearly
describe that in the code.
This is mandated by third-party, but make the md5 usage explicitely
documented.
This is mandated by third-party, but make the md5 usage explicitely
documented.
This is a more secure way of keeping things up to date.
The try/except block should be used only for code expected to fail.
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
The random module is not suitable for use in security scopes.
The API is no longer available.
…cial-auth#1064)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@bgruening
Copy link

@marcvs thanks a lot. I think your local CI error is not happening here, which is good.

I'm not sure the types errors are related - to be they don't seem related. But lets wait for the maintainers to comment.

Can you maybe cleanup your commits a bit. There are many unrelated commits in this PR.

@nijel
Copy link
Member

nijel commented Mar 31, 2025

The type errors are not caused by your changes, but by pyright changes. I've pinned working version in #1072, so if you merge current master, it should pass. I will squash commits on merge, so don't worry much about messed up history here.

@bgruening
Copy link

@nijel thanks a lot!

@marcvs
Copy link
Author

marcvs commented Mar 31, 2025

Yup. merged master and pushed to my PR branch.
Many thanks!

Copy link

codecov bot commented Apr 1, 2025

Codecov Report

Attention: Patch coverage is 92.38095% with 8 lines in your changes missing coverage. Please review.

Project coverage is 77.56%. Comparing base (326d4b3) to head (4f62515).
Report is 120 commits behind head on master.

Files with missing lines Patch % Lines
social_core/backends/nfdi.py 90.36% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1068      +/-   ##
==========================================
- Coverage   77.88%   77.56%   -0.33%     
==========================================
  Files         347      348       +1     
  Lines       10669    10708      +39     
  Branches      504      460      -44     
==========================================
- Hits         8310     8306       -4     
- Misses       2200     2245      +45     
+ Partials      159      157       -2     
Flag Coverage Δ
unittests 77.56% <92.38%> (-0.33%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@nijel nijel left a comment

Choose a reason for hiding this comment

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

The code looks mostly good. I'd like to see test for user data parsing. Also it would be great to add matching documentation to https://github.com/python-social-auth/social-docs.

# users will be allowed
ALLOWED_ENTITLEMENTS = []

def get_user_details(self, response):
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 add tests for this? You can mock it using user_data_url and user_data_body attributes on the test class.

Copy link
Author

Choose a reason for hiding this comment

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

Ok; Many thanks!
Will do it, but won't manage before end of next week.

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.

4 participants