-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Make IExternalLoginEventHandler support update User‘s properties #12845
Conversation
Perhaps ClaimsToUpdate should also be added? |
@hyzx86 you could request a review from the reviewers section instead of ping in the comment. I think you already did in another PR ;) |
BTW some of the sugession might require a discussion before submitting a PR |
Yes ,here. |
Are you talking about this ? |
I like the idea of using the existing script but I am not a fan of the way it's done with merging an object. I would prefer an api like the current context.setUserProperty("FirtName", "Han");
// or
context.userProperties["FirtName"] = "Han";
context.userProperties.firstName = "Han"; |
But if the attribute path is deep the script will be like this? context.setUserProperty("UserProfileInternal.UserProfilePart.FirtName.Text", "Han"); Then build the Json object with comma separation to update the existing user properties
Will there be any problems if use merge? |
Makes sense. Could it be "rooted" of the part though, like {
firstName: {
text: "Han"
}
} |
My idea is to be compatible with CustomUserSettings In addition, I want to add UserClaims to this function |
@sebastienros this PR is ready to merge |
What is this one holding back? It would be super useful 😊 |
@hyzx86 please fix the conflict |
@hishamco Done |
We might need to triage this one |
This pull request has merge conflicts. Please resolve those before requesting a review. |
Could you please fix the conflict? |
@hishamco please bring this up on the triage meeting. The PR has been in limbo for too long. |
It's updated so fast... I've fix the conflict many times 🤣, so let's talk about whether this PR is possible first, to save everyone some time |
src/OrchardCore.Modules/OrchardCore.Users/Controllers/AccountController.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Users/Controllers/AccountController.cs
Outdated
Show resolved
Hide resolved
test/OrchardCore.Tests/OrchardCore.Users/AccountControllerTests.cs
Outdated
Show resolved
Hide resolved
test/OrchardCore.Tests/OrchardCore.Users/AccountControllerTests.cs
Outdated
Show resolved
Hide resolved
test/OrchardCore.Tests/OrchardCore.Users/AccountControllerTests.cs
Outdated
Show resolved
Hide resolved
I'm OK with it if you're OK. |
@hyzx86 please react to the latest changes then I can merge, I know if you wait a merge conflict will show up :) |
This pull request has merge conflicts. Please resolve those before requesting a review. |
…s.cs Co-authored-by: Hisham Bin Ateya <[email protected]>
…ontroller.cs Co-authored-by: Hisham Bin Ateya <[email protected]>
…ontroller.cs Co-authored-by: Hisham Bin Ateya <[email protected]>
Done , It's finally going to be merged into the main branch. I'm looking forward to it. 🎉🎉🎉🎉🎉🎉 |
test/OrchardCore.Tests/OrchardCore.Users/AccountControllerTests.cs
Outdated
Show resolved
Hide resolved
test/OrchardCore.Tests/OrchardCore.Users/AccountControllerTests.cs
Outdated
Show resolved
Hide resolved
test/OrchardCore.Tests/OrchardCore.Users/AccountControllerTests.cs
Outdated
Show resolved
Hide resolved
test/OrchardCore.Tests/OrchardCore.Users/AccountControllerTests.cs
Outdated
Show resolved
Hide resolved
…s.cs Co-authored-by: Hisham Bin Ateya <[email protected]>
…s.cs Co-authored-by: Hisham Bin Ateya <[email protected]>
…s.cs Co-authored-by: Hisham Bin Ateya <[email protected]>
…s.cs Co-authored-by: Hisham Bin Ateya <[email protected]>
test/OrchardCore.Tests/OrchardCore.Users/AccountControllerTests.cs
Outdated
Show resolved
Hide resolved
@hyzx86 finally it has been merged :) thanks for your patient and contribution |
Summary of this PR
relate #6263
Enable the login script to generate user properties other than Roles,We can use the login script update user.properties , just like all of
CustomUserSettings
UserClaims
and user'sProperties