EES-5437 Fix deleted location levels not being considered in mapping state #5179
Conversation
| @@ -134,11 +134,15 @@ await userService | |||
|
|
|||
| private async Task UpdateMappingsCompleteAndVersion(Guid nextDataSetVersionId, CancellationToken cancellationToken) | |||
| { | |||
| var locationMappingTypes = await GetDistinctLocationOptionMappingTypes( | |||
| var hasDeletedLocationLevels = await HasDeletedLocationLevels( | |||
There was a problem hiding this comment.
It took me a while to work out why we needed this, as I immediately figured we'd be able to rely on the source location option mappings having no mapping available, and thus being able to infer that it must be a major update. But then I saw why we're filtering out those mapping types from GetLocationOptionMappingTypes in order to correctly infer that the location mappings are complete.
I'm very happy for this to go in in this form, but I do wonder if we're maybe missing a concept which is making certain special cases a little tricky.
When we map something with "AutoNone", normally we're basically saying that the backend couldn't establish a mapping, and so it's up to the user to either find a mapping, or confirm that there is indeed no mapping. Under normal circumstances, this results in the location mappings being complete, as the user has done their due diligence.
As you've highlighted here though, there are times where a user has NO course of action, and no way to resolve the "AutoNone" cases. Here then we're having to rely on extra logic and queries to work out if we can ignore them or not.
But as you've probably seen, we also have to do this with deleted or renamed Filters, which then means we have that 'orrible query to bring back all Filter and Filter Option combos in order to work out which ones we should ignore and which ones we shouldn't when considering if filter mappings are complete.
This now kind've highlights to me that maybe a simpler solution without this extra logic would be to distinguish between mappings that the user can do something about, versus mappings that the user has no way of resolving, at the point where we first generate the mappings.
Therefore you'll have a bunch of "AutoNones" that the user has to resolve, and a bunch of "AutoNones" that the user can't do anything about. We could support this with a "Resolvable"-type flag on each Mapping, or a different mapping type like "AutoNoneConfirmationRequired" - naming is not my strong point so take these suggestions with a pinch of salt! This would then allow us to remove the additional logic that we've now got in place for ignoring deleted Levels and deleted / renamed Filters and remove additional queries to detect deleted location levels. It'll all be worked out and stored right at the start and we can use the extra information to ignore the mappings that the user can't resolve much easier.
There was a problem hiding this comment.
As discussed on Slack, just going to proceed with the current solution for the time being.
We may revisit the model some more if we decide it's worth the upfront effort to make changes to the mapping plan now instead of later down the line.
There are some additional changes we might want to consider like:
- Using level codes (e.g.
NAT) instead of the enum strings - Restructuring location mappings to be more like filter mappings
| Guid nextDataSetVersionId, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| var targetDataSetVersionIdParam = new NpgsqlParameter("targetDataSetVersionId", nextDataSetVersionId); | ||
|
|
||
| // Find the distinct mapping types across all location options across all levels. | ||
| var deletedLevelCount = await publicDataDbContext.Database |
There was a problem hiding this comment.
Impressed with your JSONB query skills!
…changes This fixes the data set version mapping plan having incorrect state for locations after the mappings have been initially created. This previously did not correctly account for changes to location levels i.e. their addition or deletion.
- Adds named parameters - Removes unnecessary client instantiations - Adds async/await for DbContext calls
…ng updates This fixes a bug in location mappings where any deleted location levels would not be considered during mapping updates for: - The `LocationMappingsComplete` flag - The calculated version number These are now correctly handled meaning that deleted location levels result in major updates, and their options are not considered when calculating the `LocationMappingsComplete` flag.
This PR fixes a bug in the calculation of mapping states not considering deleted location levels correctly. Location levels are not mappable to one another, so only add or delete operations are possible if they are changed.
This impacts the following states during mapping:
LocationMappingsCompleteflag in the mapping plan