Leonlu2/error on geo values #1095

LeonLu2 · 2023-02-23T19:42:11Z

process about #641

Prerequisites:

Unless it is a documentation hotfix it should be merged against the dev branch
Branch is up-to-date with the branch to be merged with, i.e. dev
Build is successful
Code is cleaned up and formatted

Summary

add valid geo_value validator in _params
modified test_covidcast and test_delphi_epidata for server geo_value validation passing
help wanted and added TODO in test files for patch and mock the return values of geo validator in server

2. modified test_covidcast and test_delphi_epidata for server geo_value validation passing 3. add TODO for patch and mock part

melange396

good job! i found a few things that might help reduce repetition and clean it up a bit

melange396 · 2023-02-23T22:00:23Z

src/server/_params.py

+        for geo_value in geo_values:
+            if geo_value not in delphi_utils.geomap.GeoMapper().get_geo_values(geo_type):
+                raise ValidationFailedException("invalid geo_value for the requested geo_type")


This is cool! A few notes though:

You should put it in a constructor for GeoSet instead (here), and then youl'l get this check for free in the call to parse_geo_arg() below (among other places).

It would be good for the Exception to include which of the specified geo_values is the offender.

nit: you can call/create GeoMapper() outside of the loops so its only done once

integrations/server/test_covidcast.py

integrations/client/test_delphi_epidata.py

melange396 · 2023-02-23T22:08:23Z

integrations/client/test_delphi_epidata.py

+fips = ['04019', '19143', '29063'] # Example list of valid FIPS codes as strings
+msa = ['40660', '44180', '48620'] # Example list of valid MSAs as strings
+


you might wanna put these into src/acquisition/covidcast/test_utils.py so they can be shared with other tests and dont have to be repeatedly defined in each test file. you could maybe even make them defaults in CovidcastTestRow in test_utils.py, and you wont have to specify them in every call to make_default_row()!

@melange396 thanks for jumping in on this review! i should provide some context for this particular change though: initially the plan was to mock out the GeoMapper calls in tests so that we wouldn't need to use actual FIPS and MSA codes. Leon hasn't had much mocking experience though, and we tried multiple ways to get it to work with no luck. so having these constants is a sortof stopgap to get this PR out, but i don't think they're a good solution long term.

what do you think, keep the constants, or find a mock wizard (a wizard at mock, not a sham magician) to take a look?

i dont think using those constants is all that bad, though not as nice as a mock. one argument against using mocks is that theyll hide (from the tests, at least) any changes to the arguments or output of the GeoMapper. i think the constants can stick around for now (especially if we put them in one place), but definitely keep that TODO comment along with them.

integrations/client/test_delphi_epidata.py

integrations/server/test_covidcast.py

integrations/client/test_delphi_epidata.py

integrations/server/test_covidcast.py

Co-authored-by: melange396 <[email protected]>

… geo_value is invalid in the message 2. move the valid FIPS and MSA codes to test_utils.py, and import them when needed. 3. fix other failed places related to this change

…delphi/delphi-epidata into leonlu2/error_on_geo_values

krivard

👍 nice work!

tests/acquisition/covidcast/test_covidcast_row.py

krivard

Oh except the delete batch test is still failing; we should hold for that

melange396

fantastic job! just found a few little things...

src/server/_params.py

src/acquisition/covidcast/test_utils.py

integrations/client/test_delphi_epidata.py

integrations/server/test_covidcast.py

tests/server/test_params.py

src/server/_params.py

LeonLu2 · 2023-03-03T19:55:13Z

Oh except the delete batch test is still failing; we should hold for that

Yea I haven't finished working on this😂, I accidentally pushed after the commit

Co-authored-by: melange396 <[email protected]>

melange396

this is lookin really good, and i think its almost ready. the only things left are 3 unresolved comments in integrations/server/test_covidcast.py and some format-tidying suggestions i made in tests/server/test_params.py.

tests/server/test_params.py

Co-authored-by: melange396 <[email protected]>

melange396

looks great! very nice job!

LeonLu2 · 2023-03-22T00:33:33Z

looks great! very nice job!

thanks :)

krivard

small confusion about placeholder set three

integrations/server/test_covidcast.py

Co-authored-by: Katie Mazaitis <[email protected]>

krivard

👍 nice work!

sonarqubecloud · 2023-03-28T20:42:44Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

LeonLu2 added 3 commits February 19, 2023 22:03

1. add geo_value validator in _params

dd8c5c8

2. modified test_covidcast and test_delphi_epidata for server geo_value validation passing 3. add TODO for patch and mock part

1. minor comment change

d024995

1. Merge branch 'dev' into leonlu2/error_on_geo_values.

a6cc058

LeonLu2 requested a review from krivard February 23, 2023 19:42

melange396 requested changes Feb 23, 2023

View reviewed changes

LeonLu2 and others added 5 commits February 23, 2023 19:18

Update integrations/server/test_covidcast.py

6ed27d9

Co-authored-by: melange396 <[email protected]>

Update integrations/server/test_covidcast.py

e89692e

Co-authored-by: melange396 <[email protected]>

Update integrations/client/test_delphi_epidata.py

14463c8

Co-authored-by: melange396 <[email protected]>

1. put the geo validation in GeoSet class constructor, and show which…

3648a74

… geo_value is invalid in the message 2. move the valid FIPS and MSA codes to test_utils.py, and import them when needed. 3. fix other failed places related to this change

Merge branch 'leonlu2/error_on_geo_values' of https://github.com/cmu-…

e884b7c

…delphi/delphi-epidata into leonlu2/error_on_geo_values

krivard approved these changes Mar 3, 2023

View reviewed changes

tests/acquisition/covidcast/test_covidcast_row.py Outdated Show resolved Hide resolved

krivard suggested changes Mar 3, 2023

View reviewed changes

melange396 requested changes Mar 3, 2023

View reviewed changes

LeonLu2 and others added 10 commits March 19, 2023 22:31

Update src/server/_params.py

9fe6196

Co-authored-by: melange396 <[email protected]>

Update src/server/_params.py

6dac204

Co-authored-by: melange396 <[email protected]>

Update src/acquisition/covidcast/test_utils.py

7b7b5e7

Co-authored-by: melange396 <[email protected]>

Update integrations/client/test_delphi_epidata.py

0a2bdf9

Co-authored-by: melange396 <[email protected]>

Update integrations/server/test_covidcast.py

ef362d8

Co-authored-by: melange396 <[email protected]>

Update integrations/server/test_covidcast.py

6e29946

Co-authored-by: melange396 <[email protected]>

Update tests/server/test_params.py

e3fe68b

Co-authored-by: melange396 <[email protected]>

1. minor changes

43a232e

Merge branch 'dev' into leonlu2/error_on_geo_values

e3054cd

1. edit geo_type to match with what we have in make_default_row

1e75209

LeonLu2 requested review from krivard and melange396 March 20, 2023 18:44

Merge branch 'dev' into leonlu2/error_on_geo_values

5fed93c

melange396 requested changes Mar 21, 2023

View reviewed changes

LeonLu2 and others added 2 commits March 21, 2023 13:24

Update integrations/server/test_covidcast.py

79cdc26

Co-authored-by: melange396 <[email protected]>

Update integrations/server/test_covidcast.py

bec5621

Co-authored-by: melange396 <[email protected]>

LeonLu2 and others added 8 commits March 21, 2023 13:31

Update tests/server/test_params.py

4578420

Co-authored-by: melange396 <[email protected]>

Update tests/server/test_params.py

4a59ead

Co-authored-by: melange396 <[email protected]>

Update tests/server/test_params.py

00b4ff2

Co-authored-by: melange396 <[email protected]>

Update tests/server/test_params.py

58fcc7f

Co-authored-by: melange396 <[email protected]>

Update tests/server/test_params.py

94e8b5f

Co-authored-by: melange396 <[email protected]>

Update tests/server/test_params.py

59ce3c9

Co-authored-by: melange396 <[email protected]>

Update tests/server/test_params.py

d41eeaa

Co-authored-by: melange396 <[email protected]>

Update test_covidcast.py

b01bb82

melange396 previously approved these changes Mar 21, 2023

View reviewed changes

krivard reviewed Mar 22, 2023

View reviewed changes

integrations/server/test_covidcast.py Outdated Show resolved Hide resolved

Update integrations/server/test_covidcast.py

e2d12ee

Co-authored-by: Katie Mazaitis <[email protected]>

LeonLu2 dismissed melange396’s stale review via e2d12ee March 22, 2023 20:33

krivard approved these changes Mar 28, 2023

View reviewed changes

krivard merged commit 8603447 into dev Mar 29, 2023

krivard deleted the leonlu2/error_on_geo_values branch March 29, 2023 20:07

krivard mentioned this pull request Apr 4, 2023

Release Delphi Epidata 0.4.8 #1119

Merged

melange396 mentioned this pull request Feb 18, 2025

Error when API request is made for an invalid region? #1365

Open

melange396 mentioned this pull request Apr 18, 2025

API request geo-validation excludes some valid regions #1641

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leonlu2/error on geo values #1095

Leonlu2/error on geo values #1095

LeonLu2 commented Feb 23, 2023 •

edited

Loading

melange396 left a comment

melange396 Feb 23, 2023

melange396 Feb 23, 2023

krivard Feb 24, 2023

melange396 Feb 24, 2023

krivard left a comment

krivard left a comment

melange396 left a comment

LeonLu2 commented Mar 3, 2023

melange396 left a comment

melange396 left a comment

LeonLu2 commented Mar 22, 2023

krivard left a comment

krivard left a comment

sonarqubecloud bot commented Mar 28, 2023

		fips = ['04019', '19143', '29063'] # Example list of valid FIPS codes as strings
		msa = ['40660', '44180', '48620'] # Example list of valid MSAs as strings

Leonlu2/error on geo values #1095

Leonlu2/error on geo values #1095

Conversation

LeonLu2 commented Feb 23, 2023 • edited Loading

Summary

melange396 left a comment

Choose a reason for hiding this comment

melange396 Feb 23, 2023

Choose a reason for hiding this comment

melange396 Feb 23, 2023

Choose a reason for hiding this comment

krivard Feb 24, 2023

Choose a reason for hiding this comment

melange396 Feb 24, 2023

Choose a reason for hiding this comment

krivard left a comment

Choose a reason for hiding this comment

krivard left a comment

Choose a reason for hiding this comment

melange396 left a comment

Choose a reason for hiding this comment

LeonLu2 commented Mar 3, 2023

melange396 left a comment

Choose a reason for hiding this comment

melange396 left a comment

Choose a reason for hiding this comment

LeonLu2 commented Mar 22, 2023

krivard left a comment

Choose a reason for hiding this comment

krivard left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Mar 28, 2023

LeonLu2 commented Feb 23, 2023 •

edited

Loading