Feat 2532 - Change from SubUrbanWidget to AllLocationsWidget #2550
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #2550 +/- ##
==========================================
- Coverage 53.16% 52.95% -0.22%
==========================================
Files 117 119 +2
Lines 9670 9818 +148
==========================================
+ Hits 5141 5199 +58
- Misses 4529 4619 +90
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@ziv17 can you review? some changes might be needed due to the infra you added (for example with road_segment_id) |
| if resolution == BE_CONST.ResolutionCategories.STREET: | ||
| filters["involve_yishuv_name"] = location_info.get("yishuv_name") | ||
| filters["street1_hebrew"] = location_info.get("street1_hebrew") | ||
| elif resolution == BE_CONST.ResolutionCategories.SUBURBAN_ROAD: | ||
| filters["road1"] = location_info.get("road1") | ||
| filters["road_segment_name"] = location_info.get("road_segment_name") |
There was a problem hiding this comment.
Should we handle additional resolutions here, or not yet?
There was a problem hiding this comment.
When I got the first task of relocating widgets to AllLocation, Atalya told me to implement it for road+street resolutions, and we will ask about it. We can go ahead with it, but what are the identifiers in the news_flash table for non-urban intersections? Is 'non_urban_intersection_hebrew' sufficient, or should it be 'non_urban_intersection_hebrew' and 'road_segment_name'?
There was a problem hiding this comment.
OK then. Just wanted to verify
There was a problem hiding this comment.
The current situation is OK, in the coming future I'll open a separate issue regarding additional resolutions
| @@ -894,25 +894,36 @@ def set_critical( | |||
| from anyway.widgets.all_locations_widgets.injured_count_by_severity_widget import ( | |||
| InjuredCountBySeverityWidget, | |||
| ) | |||
| from anyway.request_params import get_latest_accident_date | |||
| from anyway.request_params import get_latest_accident_date, LocationInfo | |||
There was a problem hiding this comment.
models.py is a very general file. it specifies the database tables structure. I think it should not depend, or hold specific logic that uses these tables. Code that is related to Widgets should not be here, probably it should be somewhere under widgets.
There was a problem hiding this comment.
Thanks!
I agree, and I believe this change will better be in a separate pull request since this logic specific was there before.
@atalyaalon
There was a problem hiding this comment.
I agree, @EliorGigi you can create a separate issue and pr for this
Move the specified widgets from RoadSegment to AllLocation as per the details in the issue: #2532 .Additionally, I've identified a bug (which i did myself) in passing arguments within the set_critical function in the models.py file. I'm unsure about the calculation when the resolution is set to Street. Please let me know if there are any required modifications.