-
Notifications
You must be signed in to change notification settings - Fork 40
WIP: Feature course registration #364
base: main
Are you sure you want to change the base?
WIP: Feature course registration #364
Conversation
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.
Please check my review and fix those type of mistakes in rest of your PR
. Thanks
src/Services/CourseRegistration/CourseRegistration.UnitTests/UnitTest1.cs
Outdated
Show resolved
Hide resolved
src/Services/CourseRegistration/CourseRegistration.API/Application/Behaviors/LoggingBehavior.cs
Show resolved
Hide resolved
.../CourseRegistration/CourseRegistration.API/Application/Commands/CourseRegistrationCommand.cs
Show resolved
Hide resolved
@mahedee: Please change the PR state to ready for review if you think your PR is ready for review |
|
||
public async Task<bool> Handle(CourseRegistrationCommand command, CancellationToken cancellationToken) | ||
{ | ||
var courseRegistration = new CourseRegistration.Domain.AggregatesModel.CourseRegistrationAggregate.CourseRegistration(command.CourseCode, command.CourseName, command.Description); |
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.
Please do one of choosing different aggregate root name or changing the service name.
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.
What is your suggestion for the name? We should follow a common convention.
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.
@mahedee: We have decided use suffix Service
for namespace. So the namespace of this class will be CourseRegistrationService.API.Application.Commands
. This way we can create the Aggregate with the name CourseRegistration
without any issue.
@mahedee As the PR is very big, I will try to complete the review in multiple small reviews. Alternatively, you can also break the PR into multiple small PRs too. After resolving current issues, we will move forward with pending files. |
|
||
public async Task<bool> Handle(CourseRegistrationCommand command, CancellationToken cancellationToken) | ||
{ | ||
var courseRegistration = new CourseRegistration.Domain.AggregatesModel.CourseRegistrationAggregate.CourseRegistration(command.CourseCode, command.CourseName, command.Description); |
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.
@mahedee: We have decided use suffix Service
for namespace. So the namespace of this class will be CourseRegistrationService.API.Application.Commands
. This way we can create the Aggregate with the name CourseRegistration
without any issue.
No description provided.