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

Assignment4 #75

Closed
Closed

Conversation

@StevenZuelke
Copy link

StevenZuelke commented Jan 31, 2020

Finished, I did the Moq test double extra credit but not the extra commented out tests.

Copy link

mmwoodfo left a comment

Looks good

@@ -3,7 +3,17 @@
<PropertyGroup>
<Nullable Condition="$(Nullable)==''">enable</Nullable>
</PropertyGroup>

<PropertyGroup>
<IncludeOpenAPIAnalyzers>true</IncludeOpenAPIAnalyzers>

This comment has been minimized.

Copy link
@mmwoodfo

mmwoodfo Feb 4, 2020

  • Enable WebAPI analyzers and handle all warnings. ✔
services.AddScoped<IGiftService, GiftService>();
services.AddScoped<IUserService, UserService>();
services.AddScoped<IGroupService, GroupService>();
services.AddDbContext<ApplicationDbContext>();

This comment has been minimized.

Copy link
@mmwoodfo

mmwoodfo Feb 4, 2020

I believe you already did this above.

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

The only reason this does not cause a problem is because the AddDbContext method first checks to see if the DbContext has already been registered and bails out early. Many DI helper methods will actually overwrite prior registrations. You will want to avoid duplicate registrations.

var sqliteConnection = new SqliteConnection("DataSource=:memory:");
sqliteConnection.Open();

services.AddDbContext<ApplicationDbContext>(options =>

This comment has been minimized.

Copy link
@mmwoodfo

mmwoodfo Feb 4, 2020

  • Ensure DB context is registered✔
//NSwagger
System.Type profileType = typeof(AutomapperConfigurationProfile);
System.Reflection.Assembly assembly = profileType.Assembly;
services.AddAutoMapper(new[] { assembly });

This comment has been minimized.

Copy link
@mmwoodfo

mmwoodfo Feb 4, 2020

  • Ensure AutoMapper is registered✔
//Setup Dependency Injection and Register services
services.AddScoped<IGiftService, GiftService>();
services.AddScoped<IUserService, UserService>();
services.AddScoped<IGroupService, GroupService>();

This comment has been minimized.

Copy link
@mmwoodfo

mmwoodfo Feb 4, 2020

  • Ensure all relevant services are registered with appropriate scopes✔
{
private IGiftService GiftService { get; }

public GiftController(IGiftService giftService)

This comment has been minimized.

Copy link
@mmwoodfo

mmwoodfo Feb 4, 2020

  • Each controller should take in a service as a constructor dependency. ✔
  • Create CRUD (Create/Read/Update/Delete) endpoints on all of the controllers using appropriate HTTP verbs ✔
[HttpDelete("{id}")]
[ProducesResponseType(StatusCodes.Status200OK)]
[ProducesResponseType(StatusCodes.Status404NotFound)]
[ProducesDefaultResponseType]

This comment has been minimized.

Copy link
@mmwoodfo

mmwoodfo Feb 4, 2020

Where relevant, apply ProducesResponseTypeAttributes to the controller methods. ✔

//https://localhost/api/Gi
[Route("api/[controller]")]
[ApiController]
public class GiftController : ControllerBase

This comment has been minimized.

Copy link
@mmwoodfo

mmwoodfo Feb 4, 2020

For now the return types from the controller methods can use the data objects from SecretSanta.Data (we will be fixing this next week). ✔

//https://localhost/api/Gi
[Route("api/[controller]")]
[ApiController]
public class UserController : ControllerBase

This comment has been minimized.

Copy link
@mmwoodfo

mmwoodfo Feb 4, 2020

  • Setup the Api project with three controllers (GiftController, GroupController, UserController) ✔
namespace SecretSanta.Api.Tests.Controllers
{
[TestClass]
public class GiftControllerTests

This comment has been minimized.

Copy link
@mmwoodfo

mmwoodfo Feb 4, 2020

  • Unit test the controllers✔
  • Creating test doubles can be strealined by using a mocking framework (such as Moq). Rather than writing your own test doubles use a mocking framewokr to create them.✔
Copy link
Collaborator

Keboo left a comment

(StevenZuelke)

Assignment4

Project Analysis Summary
Nullable Enabled:
Unexpected Disabled Warnings:
Appropriately handle all warnings:
ProjectName Nullable Solutions Analyzers Unexpected Disabled Warnings Waning Level
SecretSanta.Api Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Business Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Data Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Web Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer 4
SecretSanta.Api.Tests Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Business.Tests Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Data.Tests Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Web.Tests Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer 4

Main Assignment

  • WebAPI Analyzers enabled ✔
  • DI properly configured ✔
  • Services properly registered with correct scopes ✔
  • DB context properly registered ✔ - Though the DbContext is has two registrations.
  • AutoMapper properly registered ✔
  • NSwag properly setup ✔
  • API controllers properly expose CRUD operations ✔
  • Controllers are all unit tested ✔

Extra Credit

  • Unit tests in EntityServiceTests fixed appropriately
  • Mocking framework used to create the test doubles ✔

Peer Review

  • On time ✔
  • Accurate ✔
  • Appropriate comments given ✔
services.AddScoped<IGiftService, GiftService>();
services.AddScoped<IUserService, UserService>();
services.AddScoped<IGroupService, GroupService>();
services.AddDbContext<ApplicationDbContext>();

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

The only reason this does not cause a problem is because the AddDbContext method first checks to see if the DbContext has already been registered and bails out early. Many DI helper methods will actually overwrite prior registrations. You will want to avoid duplicate registrations.


<PropertyGroup>
</PropertyGroup>

<PropertyGroup>
<IncludeOpenAPIAnalyzers>true</IncludeOpenAPIAnalyzers>

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

These analyzers wont do anything useful outide of a web project. You only need to set them there.

This comment has been minimized.

Copy link
@StevenZuelke

StevenZuelke Feb 11, 2020

Author

@Keboo I don't understand why this means my DBContext isn't registered properly.

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 12, 2020

Collaborator

@StevenZuelke I assume this was in response to the comment up on the Startup.cs. I looked back over it, and it does work (though somewhat by accident). The first call to AddDbContext does do the registration properly, and the second one is not need and will effectively do nothing. I will update my review.

@@ -2,15 +2,20 @@

<PropertyGroup>
</PropertyGroup>

<PropertyGroup>
<IncludeOpenAPIAnalyzers>true</IncludeOpenAPIAnalyzers>

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

These analyzers wont do anything useful outide of a web project. You only need to set them there.

<PackageReference Include="Microsoft.EntityFrameworkCore" Version="3.1.1" />
<PackageReference Include="NSwag.AspNetCore" Version="13.2.2" />

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

This NuGet only needs to be added to the API project not all of the projects.

<ItemGroup>
<PackageReference Include="AutoMapper" Version="9.0.0" />
<PackageReference Include="AutoMapper.Extensions.Microsoft.DependencyInjection" Version="7.0.0" />
<PackageReference Include="Microsoft.Data.Sqlite" Version="3.1.1" />
<PackageReference Include="NSwag.AspNetCore" Version="13.2.2" />

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

This NuGet only needs to be added to the API project not all of the projects.

int idnum = gift.Id;
service.Setup(m => m.FetchByIdAsync(idnum)).ReturnsAsync(FetchHelper(idnum));
var controller = new GiftController(service.Object);
await service.Object.InsertAsync(gift);

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

What is the intent with acting on the mock object? Typically this is not required in the unit test.

[ProducesDefaultResponseType]
public async Task<ActionResult<Gift>> Put(int id, [FromBody] Gift value)
{
if (await GiftService.FetchByIdAsync(id) is Gift)

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Though this works, it makes your updates twice as slow as they need to be. This sort of logic should be moved into the service (Business project).

@Keboo Keboo closed this Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.