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 KS-D & 7hawks #78

Closed
wants to merge 8 commits into from
Closed

Conversation

@KS-D
Copy link

KS-D commented Feb 4, 2020

collaborated with @7hawks

@KS-D

This comment has been minimized.

Copy link
Author

KS-D commented Feb 4, 2020

We will review this.

services.AddDbContext<ApplicationDbContext>(options =>
options.EnableSensitiveDataLogging()
.UseSqlite(sqliteConnection));

services.AddScoped<IGiftService, GiftService>();
services.AddScoped<IUserService, UserService>();
services.AddScoped<IGroupService, GroupService>();
Comment on lines +27 to +33

This comment has been minimized.

Copy link
@KS-D

KS-D Feb 6, 2020

Author

Services are added with the proper scoping and the database is properly registered.

System.Type profileType = typeof(AutomapperConfigurationProfile);
System.Reflection.Assembly assembly = profileType.Assembly;
services.AddAutoMapper(new[] { assembly });
Comment on lines +35 to +37

This comment has been minimized.

Copy link
@KS-D

KS-D Feb 6, 2020

Author

Automapper properly registered.

});
app.UseOpenApi();

app.UseSwaggerUi3();

This comment has been minimized.

Copy link
@KS-D

KS-D Feb 6, 2020

Author

NSwag is set up in the services and called during the configure.

Copy link
Author

KS-D 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 ✔

Extra Credit

  • Unit tests in EntityServiceTests fixed approprietly
  • Mocking framework used to create the test doubles ✔
@7hawks

This comment has been minimized.

Copy link

7hawks commented Feb 6, 2020

I will review this too.

Copy link

7hawks left a comment

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 ❌ Not quite sure how to go about this.
Mocking framework used to create the test doubles ✔

Everything looks good to me.

Copy link
Collaborator

Keboo left a comment

(KS-D) (7hawks )

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 ✔
{
var controller = new GiftController(new Mock<IGiftService>().Object);

Assert.IsNotNull(controller);

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

In general you don't need to test the result of the constructor to make sure it is non-null. For this to be null would require a bug in the compiler. Simply omitting the assertion with a comment that you are merely checking to make sure the constructor did not through an exception is sufficient.

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