Conversation
Abstracts the impact provider implementations and supports individual and bulk operations to match impact data. Implementations need so indicate whether they support the bulk operation.
WIP implementation of a SwissTPH impact provider using a PostgreSQL database connection through an unmaged ORM model.
WIP for the IDM impact provider using their PostgreSQL database through an unmaged ORM model.
… account Currently supports SwissTPH and IDM providers by fixed choices in the model.
A service that operates on top of an impact provider which combines impact data with cost data for a given scenario and builds an aggregated response structure.
Views and serialisers for the impact API.
Bewi
left a comment
Bewi
left a comment
There was a problem hiding this comment.
Globally it looks good for current state.
I mainly marked naming that are confusing to someone who didn't code it :D
Also, feels like we can improve a lot the flow to make it more readable, but we can talk about that and do it in later stages.
I'll request changes only for the technical parts.
There was a problem hiding this comment.
Same global opinion as @Bewi: overall it's good, even though some parts are hard to read (impact service, aggregation and so on)
- I only read the code, I haven't made the setup and tested this yet because I don't know how to access both new databases
- I would definitely change the casing everywhere
- I would definitely add more tests (depending on the urgency, we might skip some of them, but - for instance - we want to make sure that we correctly aggregate values)
- I would get rid of the serializer that doesn't deserialize anything and just fetches a provider
- kudos on the nice job though 👍
| import django.db.models.deletion | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): |
There was a problem hiding this comment.
Another 0021 was merged on main in the meantime, you should rebase/merge main and recreate this migration, otherwise we'll have a conflict
| ) | ||
|
|
||
|
|
||
| class ImpactService: |
There was a problem hiding this comment.
We should have tests for everything in this file
| value = serializers.FloatField(allow_null=True) | ||
| lower = serializers.FloatField(allow_null=True) | ||
| upper = serializers.FloatField(allow_null=True) |
There was a problem hiding this comment.
Should we use DecimalField instead?
api/impact/serializers.py
Outdated
| return attrs | ||
|
|
||
|
|
||
| class ImpactQuerySerializer(ImpactProviderSerializer): |
There was a problem hiding this comment.
I agree with Bewi's comments on this serializer and once new checks are done, we should add tests for this serializer
3 participants
What problem is this PR solving?
Introduce backend-side support for impact data from different providers (currently SwissTPH and IDM).
This PR is still work in progress.
Related JIRA tickets
SNT-293
Changes
The changes come in different (new) layers