DISCO-3033 refactor city based suggest #667
Conversation
misaniwere
force-pushed
the
DISCO-3033-city-suggest-refactor
branch
from
e5810d3
to
c9bc733
Compare
| def prioritize_custom_location(self) -> "Location": | ||
| """Update city, region, and country with custom fields if all custom fields exist.""" | ||
| if all([self.city_custom, self.regions_custom, self.country_custom]): | ||
| self.city = self.city_custom |
There was a problem hiding this comment.
Overwriting could be problematic as we will lose the original geolocation which is used by other consumers such as the logging middleware. I think we can either return a tuple of (country, regions, city) or just return a new Location with country, regions, and city are set to the custom counterparts.
There was a problem hiding this comment.
this should be fine as we're invoking this method on a copy of the geolocation and not the original object.
There was a problem hiding this comment.
another way we could try is moving this helper function out of the model class and into backend.py and having it return a new location with the overwrite so as not to mess with the middleware. what do you think @ncloudioj ?
|
|
||
| validate_suggest_custom_location_params(city, region, country) | ||
|
|
||
| geolocation = request.scope[ScopeKey.GEOLOCATION].model_copy( |
There was a problem hiding this comment.
As of now, requests w/ the custom location is much fewer than those w/o it. So let's update it conditionally.
if country and region and city:
geolocation = request.scope[ScopeKey.GEOLOCATION].model_copy(...)
else:
geolocation = request.scope[ScopeKey.GEOLOCATION]
There was a problem hiding this comment.
hmm since we're overwriting city, region & country down the line, I feel more comfortable working with the copy in this case. also we do some validation right before this so it either errors out before getting here if we have missing fields or the values are all None which is the default.
References
JIRA: DISCO-3033
Description
Locationmodel to return preferred location based on custom values. Also added some more validation to query params.PR Review Checklist
Put an
xin the boxes that apply[DISCO-####], and has the same title (if applicable)[load test: (abort|warn)]keywords are applied (if applicable)