Implement Common API Endpoint - #8
Conversation
SummarySummary
CoverageSchoolAccount.Collect.Api - 69.1%
SchoolAccount.Collect.Application - 68.4%
SchoolAccount.Collect.Infrastructure - 80%
SchoolAccount.Collect.SharedKernel - 65.7%
|
| namespace SchoolAccount.Collect.Application.Status.GetStatus; | ||
|
|
||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.AllConstructors)] | ||
| public sealed class GetStatusQueryHandler : IQueryHandler<GetStatusQuery, StatusResponse> |
There was a problem hiding this comment.
There's no test coverage on this class, if you look at the sample GetOrganisationByLaestabQueryHandler there's a unit test, probably worth adding one for consistency.
| { | ||
| var response = new StatusResponse | ||
| { | ||
| Details = getStatusQuery |
There was a problem hiding this comment.
This is the kind of thing that might benefit from refactoring.
You basically need a mapper from the Request to your query object.
There's a few options, with various pros and cons. My first stab would be seeing which classes properties are accessed the most in the logic, and see if there's a logical place it fits.
For example, could it be a ToQuery method on GetStatusRequest, which can then be easily unit tested.
Some people prefer to keep the requests slim, without logic, but it keeps the logic near to the only thing that uses it, and avoids have to add static extension methods or builders.
| Ukprn = x.Ukprn, | ||
| Laestab = laestab, | ||
| Interesting = interesting, | ||
| Actions = interesting ? new List<Action> { new() } : new(), |
There was a problem hiding this comment.
This reads a little odd with the new List<Action> { new() } because of the defaults in the Action and Status.
I'd probably keep the Action and Status without specific defaults and explicitly set the Name and Status explicitly on creation
|
|
||
| public sealed record OrganisationResponse | ||
| { | ||
| public string Id { get; init; } = string.Empty; |
There was a problem hiding this comment.
You might be able to get away without the default string.Empty if you've got an init
| @API_HostAddress = http://localhost:5101 | ||
|
|
||
| ### Get status for school with Local Authority | ||
| POST {{API_HostAddress}}/status |
There was a problem hiding this comment.
Maybe add one for a validation error returned too?
| } | ||
|
|
||
| [Fact] | ||
| public async Task Status_endpoint_should_return_error_messages_for_all_missing_required_parameters_with_empty_academy() |
There was a problem hiding this comment.
Tests look really good, do you think it needs separate validation tests for academy and trust rather than covering all possible fields in one?
There was a problem hiding this comment.
I can get rid of them, I'd only put them in to show the different amount of errors you can get depending on which empty fields you input, so trust and academy show different as LocalAuthority isn't required on a trust, but for academy LocalAuthority had 3 required parameters, so I could show the error messages all in one test but it might not show the reason for all those fields being required
| public async Task Status_endpoint_should_return_error_messages_for_all_missing_required_parameters_with_empty_academy() | ||
| { | ||
| // Arrange | ||
| var organisation = new Organisation |
There was a problem hiding this comment.
You could lean on anonymous objects a bit here and create something like the following to create a skeletal object with fields missing.
var request = new { Organisations = new[] { new { Category = new { } , LocalAuthority = new {} } } };
2 participants
SAB-263