Ssafty-data api involved #2760
Ssafty-data api involved #2760
Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #2760 +/- ##
==========================================
- Coverage 53.22% 52.89% -0.34%
==========================================
Files 119 124 +5
Lines 9924 10440 +516
==========================================
+ Hits 5282 5522 +240
- Misses 4642 4918 +276
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| ) | ||
| return Response(json.dumps(res, default=str), mimetype="application/json") | ||
|
|
||
| def sd_load_data(): |
There was a problem hiding this comment.
Since SDAccident and SDInvolved are related in foreign key, and we can't have an involved in DB w/o a related accident, then the deletion/insertion order should be changed to the following:
- delete involved
- delete accidents
- insert accidents
- insert involved
Otherwise I believe we can encounter foreign key errors in DB
There was a problem hiding this comment.
In addition, I will add temporary table, fill them, and then copy to the actual, in order to avoid downtime.
There was a problem hiding this comment.
I'm not sure temporary table is necessary.
Can we possible perform it all in the same transaction (like we discussed here on CBS hebrew views)?
If we have no choice, we can add temporary tables.
| @@ -3081,3 +3081,44 @@ class TelegramForwardedMessages(Base): | |||
| message_id = Column(String(), primary_key=True) | |||
| newsflash_id = Column(BigInteger(), nullable=False) | |||
| group_sent = Column(String(), nullable=False) | |||
|
|
|||
|
|
|||
| class SDInvolved(Base): | |||
There was a problem hiding this comment.
ForeignKeyConstraint should be added to table model class, just like in Involved table
There was a problem hiding this comment.
And perhaps also ForeignKey like in LocationVefiricationHistory
| population: integer | ||
| ``` | ||
| ## `/involved` Query | ||
| The query will join `data_safety_involved` and `data_safety_accident` using `accident_id`. In addition, the query will join |
There was a problem hiding this comment.
using accident_id, provider_code and accident_year as foreign keys
There was a problem hiding this comment.
@ziv17 all in all looks good. Well done!
I added a few comments.
In addition:
-
I suggest using AccidentMarker instead of AccidentMarkerView. I assume that we won't have road_segment number for this table, but I suggest we'll handle this in a later PR, and for now I suggest keeping a placeholder for road_segment.
Let me know if there are other challenges from using AccidentMarker instead of AccidentMarkerView. -
Do we keep data consistency for DB for these tables?
(We had a similar discussion here)
I think that in the case abovewith db.get_engine().begin() as conn:kept data consistency.
I believe it can also be done with using db.session.commit() only after finishing deletions insertions. -
For better practice, I suggest you consider adding relationships for dimension tables. However I suggest adding it in a future pr, when we decide to handle the issue across our db, and not in this issue. I opened an issue for it here
|
@ziv17 all in all looks good. Added a few comments, we can also discuss in tomorrow's meeting |
WIP, /involved api implementing Atalya's comment from 9-Jan on issue 2734.
Added load of safety_data involved and accident tables to the
main.py process cbscommand line.No fine detail testing yet. Just to verify the query parameter and returned data.
You can view the API in swagger hub