Web API: Added Azure Authentication and Power BI to server-side code#15
Web API: Added Azure Authentication and Power BI to server-side code#15
Conversation
… components for optional features. Needs client-side code.
mdbrooks48
left a comment
mdbrooks48
left a comment
There was a problem hiding this comment.
This PR adds a fair amount of code to FF that is not covered in unit tests. Please add unit tests.
| //_logger.LogError(ex, model.DisplayFailMessage); | ||
| } | ||
|
|
||
| return Task.FromResult(model); |
There was a problem hiding this comment.
Why use Task.FromResult over async / await?
| model.EmbedParameters = Task.FromResult(_powerBiService.GetEmbedParams(reportGuid)).Result; | ||
| } | ||
|
|
||
| return Task.FromResult(model); |
There was a problem hiding this comment.
Same issue. Why use Task.FromResult vs async / await?
| { | ||
| public class GetTrustedIdentityProvidersQueryHandler : IRequestHandler<GetTrustedIdentityProvidersQuery, List<IdentityProviderEntityModel>> | ||
| { | ||
| private readonly SystemDataService _dbService; |
There was a problem hiding this comment.
Shouldn't this dependency be ISystemDataService?
|
|
||
| public async Task<List<IdentityProviderEntityModel>> Handle(GetTrustedIdentityProvidersQuery request, CancellationToken cancellationToken) | ||
| { | ||
| return await Task.FromResult(_dbService.GetTrustedIdentityProviderClaimValues()); |
There was a problem hiding this comment.
This is confusing. Do you anticipate that the ISystemDataService methods will be async? Either define the interface with async methods or remove the async / await from this method.
| { | ||
| public class CreateUserProfileCommandHandler : IRequestHandler<CreateUserProfileCommand, int> | ||
| { | ||
| private readonly SystemDataService _dbService; |
There was a problem hiding this comment.
Should this dependency be on ISystemDataService?
| using System.ComponentModel.DataAnnotations.Schema; | ||
| using Microsoft.EntityFrameworkCore; | ||
|
|
||
| namespace NTNMath.Data.Models; |
There was a problem hiding this comment.
Need to fix the namespace to match the project
| @@ -0,0 +1,28 @@ | |||
| | |||
| namespace referenceApp.Common.Models.Entity | |||
There was a problem hiding this comment.
What are the models in the "Common" project used for? API models should go in the API project. Persistence models in the Persistence project.
|
|
||
| [Table("Report")] | ||
| [Index("ReportSectionId", Name = "IX_Report_ReportSectionId", IsUnique = true)] | ||
| public partial class Report |
There was a problem hiding this comment.
Why is this a partial class?
| // Create a request for getting Embed token | ||
| // This method works only with new Power BI V2 workspace experience | ||
| // NOTE: Must Exclude Datasets, BI Report IDs, and Roles from the EffectiveIdentity object in 'identities' token request property when BI report has a Paginated Visual. | ||
| var tokenRequest = new GenerateTokenRequestV2( |
There was a problem hiding this comment.
Why not just call the GetEmbedTokenVersion2 method?
|
|
||
| #region Implementation Methods | ||
|
|
||
| public string? GetAccessToken() |
There was a problem hiding this comment.
Why not use async / await instead of calling methods with ExecuteAsync().Result?
2 participants
Web API: Added server-side library components, configurations, and supporting code that will save time including these two optional features in any new client project.
NOTE: Ajay will be adding the corresponding client-side code.