-
Notifications
You must be signed in to change notification settings - Fork 0
Rate and block workers (Patricio) #4
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| public partial class AddShiftRatings : Migration | ||
| { | ||
| protected override void Up(MigrationBuilder migrationBuilder) | ||
| { | ||
| migrationBuilder.CreateTable( | ||
| name: "ShiftRatings", | ||
| columns: table => new | ||
| { | ||
| ShiftId = table.Column<int>(nullable: false), | ||
| FacilityId = table.Column<int>(nullable: false), | ||
| WorkerId = table.Column<int>(nullable: false), | ||
| Score = table.Column<int>(nullable: false) | ||
| }, | ||
| constraints: table => | ||
| { | ||
| table.PrimaryKey("PK_ShiftRatings", x => new { x.ShiftId, x.WorkerId }); | ||
| table.ForeignKey("FK_ShiftRatings_Shifts_ShiftId", x => x.ShiftId, "Shifts", "Id", onDelete: ReferentialAction.Cascade); | ||
| table.ForeignKey("FK_ShiftRatings_Facilities_FacilityId", x => x.FacilityId, "Facilities", "Id", onDelete: ReferentialAction.Cascade); | ||
| table.ForeignKey("FK_ShiftRatings_Workers_WorkerId", x => x.WorkerId, "Workers", "Id", onDelete: ReferentialAction.Cascade); | ||
| }); | ||
|
|
||
| migrationBuilder.CreateIndex("IX_ShiftRatings_FacilityId", "ShiftRatings", "FacilityId"); | ||
| migrationBuilder.CreateIndex("IX_ShiftRatings_WorkerId", "ShiftRatings", "WorkerId"); | ||
| } | ||
|
|
||
| protected override void Down(MigrationBuilder migrationBuilder) | ||
| { | ||
| migrationBuilder.DropTable("ShiftRatings"); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,8 +3,31 @@ | |
| public class FacilitiesController : ControllerBase | ||
| { | ||
| private readonly AppDbContext _dbContext; | ||
| public FacilitiesController(AppDbContext dbContext) => _dbContext = dbContext; | ||
| private readonly RateWorkerHandler _rateWorkerHandler; | ||
| private readonly BlockWorkerHandler _blockWorkerHandler; | ||
|
|
||
| [HttpGet] | ||
| public IActionResult GetFacilities() => Ok(_dbContext.Facilities.ToList()); | ||
| public FacilitiesController(AppDbContext dbContext, RateWorkerHandler rateWorkerHandler, BlockWorkerHandler blockWorkerHandler) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should not expose a dbContext here. It should live within a Repository class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this class is not following dependency inversion principle. We should use abstractions (interfaces) instead of implementations. Delegate instance creation to the framework by registering dependencies in the DI container to the proper lifetime. |
||
| { | ||
| _dbContext = dbContext; | ||
| _rateWorkerHandler = rateWorkerHandler; | ||
| _blockWorkerHandler = blockWorkerHandler; | ||
| } | ||
|
|
||
| [HttpGet("facilities/list")] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should use relative paths, otherwise we could end up having duplicate segments. |
||
| public async Task<IActionResult> GetFacilities([FromBody] RateWorkerRequest request) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GET methods should not use Body according to conventions. |
||
| { | ||
| return await _dbContext.Facilities.ToListAsync(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dbContext should not be exposed here. There should be a handler and a repository containing the dbContext and queries. We should not return full domain entities, use DTOs instead and return a proper status code (Ok, BadRequest, NotFound, etc). |
||
| } | ||
|
|
||
| [HttpPost("facilities/rate-worker")] | ||
| public async Task<IActionResult> RateWorker([FromBody] RateWorkerRequest request) | ||
| { | ||
| return await _rateWorkerHandler.HandleAsync(request); | ||
| } | ||
|
|
||
| [HttpPost("facilities/block-worker")] | ||
| public async Task<IActionResult> BlockWorker([FromBody] BlockWorkerRequest request) | ||
| { | ||
| return await _blockWorkerHandler.HandleAsync(request); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| public class BlockWorkerHandler | ||
| { | ||
| private readonly WorkerRepository _workerRepository; | ||
|
|
||
| public BlockWorkerHandler(WorkerRepository workerRepository) | ||
| { | ||
| _workerRepository = workerRepository; | ||
| } | ||
|
|
||
| public async Task<IActionResult> HandleAsync(BlockWorkerRequest request) | ||
| { | ||
| try | ||
| { | ||
| _workerRepository.BlockWorker(request.WorkerId, request.FacilityId); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. calling a blocking operation like requests to DB, files, other API synchronously will block the current thread and can lead to thread pool starvation. Add await for this kind of operations as a good practice to release the thread so it can. |
||
| } | ||
| catch (Exception e) | ||
| { | ||
| return new JsonResult(new { error = e.Message }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there should be a log for this exception and we could return a general message to the client. |
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| public class RateWorkerHandler | ||
| { | ||
| private readonly AppDbContext _dbContext; | ||
|
|
||
| public RateWorkerHandler(AppDbContext dbContext) | ||
| { | ||
| _dbContext = dbContext; | ||
| } | ||
|
|
||
| public async Task<IActionResult> HandleAsync(RateWorkerRequest request) | ||
| { | ||
| try | ||
| { | ||
| var shiftRating = new ShiftRating | ||
| { | ||
| ShiftId = request.ShiftId, | ||
| FacilityId = request.FacilityId, | ||
| WorkerId = request.WorkerId, | ||
| Score = request.Score | ||
| }; | ||
|
|
||
| _dbContext.ShiftRatings.Add(shiftRating); | ||
| await _dbContext.SaveChangesAsync(); | ||
|
|
||
| return new JsonResult(shiftRating); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| return new JsonResult(new { error = e.Message }); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| public class ShiftRating | ||
| { | ||
| public int ShiftId { get; set; } | ||
| public Shift Shift { get; set; } | ||
|
|
||
| public int FacilityId { get; set; } | ||
| public Facility Facility { get; set; } | ||
|
|
||
| public int WorkerId { get; set; } | ||
| public Worker Worker { get; set; } | ||
|
|
||
| public int Score { get; set; } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a missing property to add an optional comment (nullable).