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

Makes the GraphQL user type reusable. #15691

Merged
merged 5 commits into from
Apr 22, 2024

Conversation

gvkries
Copy link
Contributor

@gvkries gvkries commented Apr 8, 2024

Enhances how the GraphQL UserType is build to make it more reliable when reused e.g. by #15389 The UserType does not depend on the schema builder to be populated any longer.

Also fixes #15540

@hishamco
Copy link
Member

hishamco commented Apr 8, 2024

@hyzx86 your review please

@hyzx86
Copy link
Contributor

hyzx86 commented Apr 9, 2024

Hi @hishamco , When you review PR daily, do you manually check out contributors' repositories or do you have any tools to assist in this process?

@hishamco
Copy link
Member

hishamco commented Apr 9, 2024

You can review by looking to the code, but sometimes you might need to try the change that made

@hyzx86
Copy link
Contributor

hyzx86 commented Apr 9, 2024

You can review by looking to the code, but sometimes you might need to try the change that made

Yes, I need to try this PR

@hyzx86
Copy link
Contributor

hyzx86 commented Apr 9, 2024

I feel fine with the code and functionality.

but there is one small issue :
I haven't enabled UserCustomSettings yet, but it's still available in the me field
image

Copy link
Contributor

@hyzx86 hyzx86 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 for your contribution
I merged your code in #15389 and it works just fine .

@gvkries
Copy link
Contributor Author

gvkries commented Apr 9, 2024

I feel fine with the code and functionality.

but there is one small issue : I haven't enabled UserCustomSettings yet, but it's still available in the me field image

Yes, it looks at the stereotype only. Whether the feature is actually enabled is not checked (and was not before this PR). Could be improved, but I don't think this is important.

@hishamco
Copy link
Member

hishamco commented Apr 9, 2024

@MikeAlhayek anything to add here?

@Piedone
Copy link
Member

Piedone commented Apr 21, 2024

@hishamco please merge if you approve.

@Piedone
Copy link
Member

Piedone commented Apr 21, 2024

@hyzx86 for conducting reviews, see these yet unmerged tips I added.

@hishamco hishamco requested a review from hyzx86 April 21, 2024 21:40
Copy link
Contributor

@hyzx86 hyzx86 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 to me

@hyzx86
Copy link
Contributor

hyzx86 commented Apr 22, 2024

@hishamco , Merge it please 😎 , the PR #15389 need this

@hishamco
Copy link
Member

Just waiting for a last note then I will merge

@hishamco hishamco merged commit 0140e68 into OrchardCMS:main Apr 22, 2024
5 checks passed
@gvkries gvkries deleted the gvkries/user-graphql-15540 branch April 23, 2024 07:25
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.

The CustomUserSettings in the graphql me field is registered incorrectly
4 participants