-
-
Notifications
You must be signed in to change notification settings - Fork 80
Add author name in blog post #428
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 author name in blog post #428
Conversation
@linkdotnet please review this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will checkout your branch and play with it later to get some better grasp on your ideas.
src/LinkDotNet.Blog.Infrastructure/Persistence/Sql/Mapping/BlogPostConfiguration.cs
Show resolved
Hide resolved
src/LinkDotNet.Blog.Web/Features/Admin/BlogPostEditor/Components/CreateNewBlogPost.razor
Outdated
Show resolved
Hide resolved
src/LinkDotNet.Blog.Web/Features/Home/Components/AccessControl.razor
Outdated
Show resolved
Hide resolved
src/LinkDotNet.Blog.Web/Features/Home/Components/AccessControl.razor
Outdated
Show resolved
Hide resolved
...s/LinkDotNet.Blog.IntegrationTests/Infrastructure/Persistence/Sql/BlogPostRepositoryTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still confident that we don't need any custom dialog or HttpContext
in the first place to make that feature work.
Here I set the name on my IDP (Auth0):

Which then I got via the claim and displayed in the blog:

For that we don't need much things:
- We have to tell OIDC that we want profile information:
options.Scope.Add("profile");
- I wrote a small service that gets the user's name:
public async ValueTask<string?> GetDisplayNameAsync()
{
var user = (await authenticationStateProvider.GetAuthenticationStateAsync()).User;
if (user?.Identity is not { IsAuthenticated: true })
{
return null;
}
// Check a few (not sure what order is the best and if covers all but worked when I spiked something together)
var name = user.FindFirst("name")?.Value
?? user.FindFirst("preferred_username")?.Value
?? user.FindFirst("nickname")?.Value;
return string.IsNullOrWhiteSpace(name) ? null : name;
}
The advantage here is that we don't need HttpContext
at all and we solely rely on how Blazor handles things aka AuthenticationStateProvider
. This service than can be used in all sorts of places - that needs a bit design.
In the screenshot you are seeing, I really just did this:
@inject ICurrentUserService CurrentUser
...
@code {
protected override async Task OnInitializedAsync()
{
AuthStateProvider.AuthenticationStateChanged += OnAuthStateChanged;
displayName = await CurrentUser.GetDisplayNameAsync();
}
Of course we need to check for authentication changes and so on, but that is "fine-tuning".
Let me know your thoughts about this one. I am absolutely open to discuss any direction, I would just like that it is "the standard of doing things" so also in a year folks understand the code and why it is there.
src/LinkDotNet.Blog.Infrastructure/Migrations/BlogDbContextModelSnapshot.cs
Show resolved
Hide resolved
src/LinkDotNet.Blog.Web/Authentication/Dummy/DummyLoginManager.cs
Outdated
Show resolved
Hide resolved
public async Task ShouldAuthorNameNullWhenNotGiven() | ||
{ | ||
var blogPost = BlogPost.Create("Title", "Subtitle", "Content", "url", true, tags: new[] { "Tag 1", "Tag 2" }); | ||
await DbContext.BlogPosts.AddAsync(blogPost, TestContext.Current.CancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just from a style point of view: The other tests are using directly the Repository
object rather than the DbContext
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test to validate if the AuthorName
is not saved in the db, then at the time of loading with GetByIdAsync()
of the Repository
it should come as null
.
First it adds a BlogPost
entity in the db which don't have AuthorName
in it with the DbContext
. Then it pulls the data with GetByIdAsync()
of the Repository
and validate the AuthorName
is null
or not.
This same pattern has been followed in the ShouldLoadBlogPost
test as well.
...DotNet.Blog.IntegrationTests/Web/Features/Admin/BlogPostEditor/CreateNewBlogPostPageTests.cs
Outdated
Show resolved
Hide resolved
@linkdotnet thanks for the detail explanation. I have understood your approach now. I have made the code changes as per your suggestion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good work - we are almost there!
src/LinkDotNet.Blog.Infrastructure/Migrations/BlogDbContextModelSnapshot.cs
Show resolved
Hide resolved
src/LinkDotNet.Blog.Web/Features/Home/Components/AccessControl.razor
Outdated
Show resolved
Hide resolved
src/LinkDotNet.Blog.Web/Features/Services/IUserRecordService.cs
Outdated
Show resolved
Hide resolved
...DotNet.Blog.UnitTests/Web/Features/Admin/BlogPostEditor/Components/CreateNewBlogPostTests.cs
Outdated
Show resolved
Hide resolved
|
||
var cut = Render<AccessControl>(); | ||
|
||
cut.FindAll("label:contains('Test Author')").ShouldHaveSingleItem(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the Test Author
coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done the setup in the constructor.
Good job on this one - looking good |
If you update the "Authentication": {
"Provider": "Auth0",
"Domain": "",
"ClientId": "",
"ClientSecret": ""
}, Also you need to update the - if (builder.Environment.IsDevelopment())
- {
- builder.Services.UseDummyAuthentication();
- }
- else
- {
- builder.Services.UseAuthentication();
- }
+ builder.Services.UseAuthentication(); |
@linkdotnet it is good news that the solution is working with identity provider's claim information. Let me know when you will find a good place to show the |
Sorry swamped with work - if you have other ideas rather than next to "Logout"/"Login" - I am open. On top of my head, we can keep the Lock icon and instead of "Logout" we put the Name in. |
When multi user mode is enable, then we can show the author name at the place of logout. If someone clicks on the author name then a drop down will open and logout link will be inside that. Do you like this idea? |
Yeah - that sounds good. That in combination with the two places where the author should be visible and the feature is done |
Do you also want to show the author name in the blog preview page? |
If the flag is true |
Ok, I will update the PR accordingly. |
I have made the code changes. Please review. |
Icon added. Please review when you feel better. |
LGTM - there are some super minor things with the alignment, but I will fix them on the master branch. |
6c1d848
to
dfa563b
Compare
Great job guys!!! |
I have added author name in the blog post.
Related to #253