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

feat: have the /admin/identities list call return all fields #3343

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

borisroman
Copy link
Contributor

Related issue(s)

Fixes #3342

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@CLAassistant
Copy link

CLAassistant commented Jun 23, 2023

CLA assistant check
All committers have signed the CLA.

@borisroman borisroman changed the title Have the /admin/identities list call return all fields feat: Have the /admin/identities list call return all fields Jun 23, 2023
@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (72bdeda) 78.37% compared to head (0a23fcc) 78.34%.

Files Patch % Lines
identity/handler.go 81.25% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3343      +/-   ##
==========================================
- Coverage   78.37%   78.34%   -0.03%     
==========================================
  Files         348      347       -1     
  Lines       23985    23925      -60     
==========================================
- Hits        18798    18744      -54     
+ Misses       3768     3767       -1     
+ Partials     1419     1414       -5     

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

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

identity/handler.go Outdated Show resolved Hide resolved
@borisroman borisroman force-pushed the fix/expand-credentials branch 6 times, most recently from ce055cb to 1e1f74e Compare June 28, 2023 11:06
@borisroman
Copy link
Contributor Author

@aeneasr Can you please review the new changes in this PR?

jonas-jonas
jonas-jonas previously approved these changes Aug 9, 2023
@aeneasr aeneasr changed the title feat: Have the /admin/identities list call return all fields feat: have the /admin/identities list call return all fields Sep 4, 2023
@jonas-jonas jonas-jonas requested a review from aeneasr September 8, 2023 08:17
@jonas-jonas
Copy link
Member

@borisroman could you resolve the merge conflicts? After that, this PR should be good to go.

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you! This is a great idea! It would be good if the list and get API of identities had the same parameters, which makes it easier to use the API - thanks! :)

identity/handler.go Outdated Show resolved Hide resolved
@borisroman
Copy link
Contributor Author

@aeneasr @jonas-jonas I've rebased the PR against main and implemented the requested changes. Requesting your review once more.

@lus
Copy link

lus commented Jan 11, 2024

Any update on this? This currently impacts my integration massively, I need to provide some GraphQL queries for fetching identities in my API and it would be nice if I could return the full data on paginated responses without having to fetch them for each single identity.

jonas-jonas
jonas-jonas previously approved these changes Jan 15, 2024
@borisroman borisroman force-pushed the fix/expand-credentials branch from 95674f0 to a5cdda6 Compare February 16, 2024 13:49
@borisroman
Copy link
Contributor Author

I've rebased against master again.

Any chance this can get merged @aeneasr? Or give feedback if something needs to change...

@aeneasr
Copy link
Member

aeneasr commented Feb 16, 2024

Yes! Unfortunately there is a test still failing to pass

@borisroman borisroman force-pushed the fix/expand-credentials branch from a5cdda6 to 3bd41c5 Compare February 19, 2024 22:03
@borisroman borisroman force-pushed the fix/expand-credentials branch 2 times, most recently from 40e2d37 to cc8275f Compare February 19, 2024 23:21
@borisroman borisroman force-pushed the fix/expand-credentials branch from cc8275f to 0a23fcc Compare February 19, 2024 23:30
@borisroman
Copy link
Contributor Author

@aeneasr Check, I added a test for improved code coverage; all green now!

@aeneasr aeneasr merged commit d94530a into ory:master Feb 20, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: list call GET /admin/idenitities should return all fields
5 participants