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 MattJet-EWU & twofingerrightclick #81

Conversation

@twofingerrightclick
Copy link

twofingerrightclick commented Feb 4, 2020

@MattJet-EWU

Keboo and others added 30 commits Jan 6, 2020
This reverts commit 4d72d20c9f44806e860d74ea3df43100e71b0214.
cherry picked (not rebase) readme from master to assignment 3 for status badge
…eated a ClientGiftService. Not dure where to create a mapper.
…rationProfile-A generic AutoMapper that ignores Id on TEntity
twofingerrightclick and others added 22 commits Jan 28, 2020
…ervice. Also, need to finish GroupTests.cs, Group.cs constructor is stubbed, need to look into private construstor and public version of it, like in User and Gift.
…Service Insert methods do this.
…n other classes. Added IUserService interface. Edited service interfaces to use their abstracted form instea dof inherited form. Altered the Startup class to enable mapping of all assembies and implemented Swagger. Updated API project with 2 NEW NuGet packages: Swagger and Automapper.Extensions.Microsoft.DependencyInjection. Updated two OLD NuGet packages: Microsoft.Extensions.Logging.Debug and Microsoft.VisualStudio.Web.CodeGeneration.Design to both 3.1.1.
…ded web api analyzers.
Copy link

MattJett-EWU 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 appropriately
  • Mocking framework used to create the test doubles ✔

// This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
public static void Configure(IApplicationBuilder app, IWebHostEnvironment env)
{
if (env.IsDevelopment())
{
app.UseDeveloperExceptionPage();
_ = app.UseDeveloperExceptionPage();

This comment has been minimized.

Copy link
@MattJett-EWU

MattJett-EWU Feb 6, 2020

odd, why is app.UseDeveloperExceptionPage(); getting replaced by this throw away variable _ = app.UseDeveloperExceptionPage();, I think I did this, but why did I? Lol

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Likely a warning. When a method returns a value, the analyzers warn you that you are not using the return value. A throw-away variable like what you did is correct since it will hint to the compiler to not allocate space for the variable, and imply discard it.




}

This comment has been minimized.

Copy link
@MattJett-EWU

MattJett-EWU Feb 6, 2020

whats going on here??? Regarding the closing braces and white space.

This comment has been minimized.

Copy link
@twofingerrightclick

twofingerrightclick Feb 6, 2020

Author

bad formatting I suppose.

Copy link
Author

twofingerrightclick left a comment

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

@@ -24,6 +24,7 @@ EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "test", "test", "{96D1C573-350A-4E65-BBD1-415BA8482E43}"
ProjectSection(SolutionItems) = preProject
test\Directory.Build.props = test\Directory.Build.props
test\SecretSanta.Data.Tests\TestBase.cs = test\SecretSanta.Data.Tests\TestBase.cs

This comment has been minimized.

Copy link
@twofingerrightclick

twofingerrightclick Feb 6, 2020

Author

maybe don't need this here. only test projects need it.

}

// GET: api/Entity
[HttpGet]

This comment has been minimized.

Copy link
@twofingerrightclick

twofingerrightclick Feb 6, 2020

Author

no http code for this method

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Technically there is an implicit 200 response code by returning.




}

This comment has been minimized.

Copy link
@twofingerrightclick

twofingerrightclick Feb 6, 2020

Author

bad formatting I suppose.

[HttpPost]
public async Task<TEntity> Post(TEntity value)
{
return await EntityService.InsertAsync(value);

This comment has been minimized.

Copy link
@twofingerrightclick

twofingerrightclick Feb 6, 2020

Author

maybe check value here?

MattJett-EWU and others added 2 commits Feb 7, 2020
…tests. Removed unused libraries in UserController and its unit tests.
Copy link
Collaborator

Keboo left a comment

(twofingerrightclick) (MattJet-EWU)

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 ✔ - technically yes though there is a disjoint with the Put methods.
  • 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 ✔
[ProducesResponseType(StatusCodes.Status404NotFound)]
public async Task<ActionResult<TEntity>> Put(int id, TEntity value)
{
if (await EntityService.UpdateAsync(id, value) is { } entity)

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Local variable 'entity' should be used. More specifically, the method is defined to return a TEntity on a 200 (Ok) response, so it should be passed there.

var sampleGift = CreateGift();
int originalId = sampleGift.Id;

var insertResult = await giftService.InsertAsync(sampleGift);

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Local variable 'insertResult' is unused

var sampleGroup = CreateGroup1();
int originalId = sampleGroup.Id;

var insertResult = await groupService.InsertAsync(sampleGroup);

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Local variable 'insertResult' is unused

//assert
using var dbContextAssert = new ApplicationDbContext(Options);
fetchedUser1 = await dbContextAssert.Users.SingleAsync(item => item.Id == fetchedUser1.Id);
var fetchedUser2 = await dbContextAssert.Users.SingleAsync(item => item.Id == 2);

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Local variable 'fetchedUser2' should be used

}

// GET: api/Entity
[HttpGet]

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Technically there is an implicit 200 response code by returning.


// This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
public static void Configure(IApplicationBuilder app, IWebHostEnvironment env)
{
if (env.IsDevelopment())
{
app.UseDeveloperExceptionPage();
_ = app.UseDeveloperExceptionPage();

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Likely a warning. When a method returns a value, the analyzers warn you that you are not using the return value. A throw-away variable like what you did is correct since it will hint to the compiler to not allocate space for the variable, and imply discard it.

{
//whats the difference between why have to use IGiftService? versus GiftService
// Arrange
List<Gift> gifts = new List<Gift>();

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

You can use a collection initializer to make this simpler.

Suggested change
List<Gift> gifts = new List<Gift>();
List<Gift> gifts = new List<Gift>
{
SampleData.CreateGift(),
SampleData.CreateGift()
};
// Assert
Assert.IsTrue(getResult is IEnumerable<Group>);

List<Group> getResultList = (List<Group>)getResult;

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Though this works a cleaner approach for checking the type would be.

if (!(getResult is List<Group> getResultList))
{
    Assert.Fail("Useful message here");
}

Another alternative would be the as cast and check for null.

fetchedUser1 = await dbContextAssert.Users.SingleAsync(item => item.Id == fetchedUser1.Id);
var fetchedUser2 = await dbContextAssert.Users.SingleAsync(item => item.Id == 2);
//Assert.AreEqual((sampleUser1.FirstName, newLastname), (fetchedUser1.FirstName, fetchedUser2.LastName));
//Assert.AreEqual((sampleUser1.FirstName, sampleUser1.LastName), (fetchedUser1.FirstName, fetchedUser1.LastName));

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Though Single will throw if there is not exactly one item that matches, it would have been better to leave these asserts in to verify that the data was actually changed.


namespace SecretSanta.Business
{
public static class NoMapAttribute

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Is this used?

@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

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