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 #73

Closed
Closed

Conversation

@AaronSauther
Copy link

AaronSauther commented Jan 29, 2020

Forgot to mark pull request as ready for review, but assignment was completed before 02/03/2020 11:59PM

AaronSauther and others added 28 commits Jan 24, 2020
@AaronSauther AaronSauther marked this pull request as ready for review Feb 4, 2020
@roonle

This comment has been minimized.

Copy link

roonle commented Feb 6, 2020

i will review this!

@@ -2,8 +2,17 @@

<PropertyGroup>
<Nullable Condition="$(Nullable)==''">enable</Nullable>
<IncludeOpenAPIAnalyzers>true</IncludeOpenAPIAnalyzers>
</PropertyGroup>

This comment has been minimized.

Copy link
@roonle

roonle Feb 6, 2020

nice work on enabling the WbApi right off the bat.


app.UseSwaggerUi3();

app.UseMvc();
}
}

This comment has been minimized.

Copy link
@roonle

roonle Feb 6, 2020

solid code to configured DI properly and with correct scopes for services.

// DELETE api/<controller>/5
[HttpDelete("{id}")]
public void Delete(int id)
{
}

This comment has been minimized.

Copy link
@roonle

roonle Feb 6, 2020

Excellent refactoring code to extend EntityController reducing duplicate code.

@roonle

This comment has been minimized.

Copy link

roonle commented Feb 6, 2020

Assignment
WebAPI Analyzers enabled ✔
DI properly configured
Services properly registered with correct scopes ✔
DB context properly registered ✔
AutoMapper properly registered ✔
NSwag properly setup ✔
API controllers properly expose CRUD operations ✔
Controllers are all unit tested ✔
Extra Credit
Unit tests in EntityServiceTests fixed approprietly ✔
Mocking framework used to create the test doubles ✔

Looks clean code

Copy link
Collaborator

Keboo left a comment

(AaronSauther)

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.Analyzers, Microsoft.CodeAnalysis.CSharp.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.Analyzers, Microsoft.CodeAnalysis.CSharp.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 ✔
  • 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.AddSwaggerDocument(config =>
{
config.PostProcess = document =>

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Nice use of expanding on the OpenAPI docs.

public Task<MockEntity?> UpdateAsync(int id, MockEntity entity)
{
int index = id - 1;
if (Database.ElementAt(index) is MockEntity)

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

I don't think this is what you want. This will retrieve the item at index not necessarily the item that matches id. A better implementation would be to find the item with the matching id, and update that one. I have pushed my assignment 4 solution and you can see an example there.

namespace SecretSanta.Api.Tests
{
[TestClass]
public class GiftControllerTests

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Though this works, in general if there is a polymorphic relationship between the classes you are testing, it makes sense to have the same relationship between your test classes.

@Keboo Keboo closed this Feb 10, 2020
@Keboo Keboo reopened this Feb 10, 2020
@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.