EES-5275 Add backend validation to request table queries #5060
Conversation
mmyoungman
force-pushed
the
EES-5275
branch
2 times, most recently
from
abe4e47 to
81bbe6e
Compare
| } | ||
| TimePeriodRange = | ||
| [ | ||
| new TimePeriodMetaViewModel(2020, AcademicYear), |
There was a problem hiding this comment.
Does this new C# "feature" mean you have to define the types explicitly again? Normally the TimePeriodMetaViewModel type would be inferred for each item in the collection.
There was a problem hiding this comment.
It doesn't but Rider was recommending showing the type name again - I assume the rationale here is that since we don't see the type in new List<TimePeriodMetaViewModel> it prefers to explicitly say the type when creating elements.
I'm not really fussed either way - I think my preference would be to have the type stated somewhere, but it's not too hard to hover over TimePeriodRange to see the type.
|
|
||
| public record DataBlockCreateRequest | ||
| { | ||
| [Required] public string Heading { get; init; } = string.Empty; |
There was a problem hiding this comment.
I've been using the required keyword for ...Request classes, as my understanding was that the [Required] attribute was more of a code-first EF Core approach (based on other classes across the project).
There was a problem hiding this comment.
I've added the required keyword to appropriate properties.
| RuleFor(context => context.TimePeriod) | ||
| .NotNull(); | ||
|
|
||
| // NOTE: No rule for filters - a data set might have no filters! |
There was a problem hiding this comment.
I don't think it's super necessary to explain the absence of something here, same for TimePeriod comment below.
There was a problem hiding this comment.
My thinking is that it's possible someone might see this code and ask themselves the question and wonder if it's been missed, and this comment prevents someone wasting any time exploring it as it answers the question immediately.
|
|
||
| public TimePeriodQuery? TimePeriod { get; set; } | ||
|
|
||
| public IEnumerable<Guid> Filters { get; set; } = new List<Guid>(); |
There was a problem hiding this comment.
Should this be nullable if it's optional, or is it important for it to always be a list, even if it's an empty one?
There was a problem hiding this comment.
This is the way it was handled before my changes. I think you've got a point here, but it might be a bit of an involved change - I would probably have to change the frontend as well, and it could be nullable in several instances - for what I consider very little benefit.
| } | ||
| }, | ||
| Filters = new List<Guid>(), | ||
| Indicators = new List<Guid> // use collection expression -> test failures |
There was a problem hiding this comment.
Not sure on the meaning/purpose of this comment - leftover note-to-self?
There was a problem hiding this comment.
It's just a note that if you switch this from new List<Guid> to a collection expression, the unit test fails. I'll change the comment to make it clearer.
mmyoungman
force-pushed
the
EES-5275
branch
2 times, most recently
from
f6a58d7 to
5b67ea3
Compare
This PR adds validation to table queries provided in request objects to the backend. This is being done in response to EES-5256 where data blocks were created with no locations, which shouldn't be possible.
The FullTable™ requests are when we expect the request query to be enough to get details for a full table.
The LocationsOrTimePeriods requests are for two particular steps of the table tool - when the user has only selected at most a subject, locations, and a time period.
These request records are used to validate all steps of the table tool after selecting a location. They are also included in requests for creating and updating datablocks, and creating permalinks. They are validated in those cases too.
I did consider splitting endpoints that include a
LocationsOrTimePeriodsQueryRequestobject into two separate endpoints, but ultimately decided it wasn't worth it. If we decided to do this in the future it wouldn't be hard - it's more grunt work than anything else.Other changes
DataBlockCreateViewModelandDataBlockUpdateViewModelare only used as requests, so have both been moved and renamed appropriately.