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 : mmwoodfo & veltoc #76

Closed

Conversation

@mmwoodfo
Copy link

mmwoodfo commented Jan 31, 2020

Assignment 4
In collaboration with @Veltoc

Still working on implementing the EC.

mmwoodfo and others added 18 commits Jan 30, 2020
- Adding response type attributes to put and delete
- Cleaning up Program.cs & Test classes
- Fixing naming for swliteConnection variable
- Removing using statement in startup.cs because it broke our sqlite connection
- Fixing some nullability warnings in various files
- implementing Kevin's warning fix for the TestGift,Testuser and TestGroup class because it's a bit cleaner looking imo
- Simplifying using statement in Testbase
@mmwoodfo mmwoodfo marked this pull request as ready for review Feb 3, 2020
Copy link

Veltoc left a comment

Assignment 4

  • 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 ✔ did it with generics even

Extra Credit

  • Unit tests in EntityServiceTests fixed approprietly ✔
  • Mocking framework used to create the test doubles
Copy link
Collaborator

Keboo left a comment

(mmwoodfo) and (Veltoc)

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 ✔
@@ -2,6 +2,7 @@
<PropertyGroup>
<TargetFramework>netcoreapp3.1</TargetFramework>
<Nullable>enable</Nullable>
<IncludeOpenAPIAnalyzers>true</IncludeOpenAPIAnalyzers>

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

This certainly woks, but outside of web projects it does not add any value.

@@ -2,10 +2,19 @@

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

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

This should not be explicitly necessary since you are setting it in Directory.Build.props, but this would be my preferred location to set it.

CreateMap<User, User>().ForMember(property => property.Id, option => option.Ignore());
CreateMap<Gift, Gift>()
.ForMember(property => property.Id, option => option.Ignore())
.ForMember(property => property.CreatedBy, option => option.Ignore());

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Though this solution certainly works to make the unit tests happy, it does expose some problems. For example it still allows someone outside of the data layer to directly modify the finger printing properties (which really should be completely controlled in the data layer). You can see a solution in the Assignment4Complete branch.

}


//------------------ TEST ENTITY SERVICE CLASS ----------------------//

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

It is ok to move this into its own file. I think in class my example was a nested class which would require partials to be able to move into its own file.

@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.