CouncilDataProject / cdp-backend

Data storage utilities and processing pipelines used by CDP instances.
https://councildataproject.org/cdp-backend
Mozilla Public License 2.0
22 stars 26 forks source link

bugfix/revert-validator-fixes #164

Closed evamaxfield closed 2 years ago

evamaxfield commented 2 years ago

Reverts CouncilDataProject/cdp-backend#163

This PR created pipeline problems: https://github.com/CouncilDataProject/seattle-staging/runs/5219968602?check_suite_focus=true

Relevant part of the pipeline: https://github.com/CouncilDataProject/cdp-backend/blob/main/cdp_backend/pipeline/event_gather_pipeline.py#L1373

Logged warning:

[WARNING: event_gather_pipeline:1389 2022-02-16 17:04:24,396] Provided 'decision' is not an approved constant. Provided: 'None' Should be one of: ['Failed', 'Passed'] See: cdp_backend.database.constants.EventMinutesItemDecision. Creating EventMinutesItem without decision value.

DB model: https://github.com/CouncilDataProject/cdp-backend/blob/main/cdp_backend/database/models.py#L556

The only field on that db model with a validator is decision but it's not a required field, so why didn't this go through?

Because the validator function doesn't allow None as an option. This is why I was so confused.

I am going to revert this change and change the parameter name to be more explicit about what that parameter is doing. It isn't making it required or not, it is allowing None as a valid option.

I will also update tests and add more docs to make sure we don't forget this again :joy:

codecov[bot] commented 2 years ago

Codecov Report

Merging #164 (b71a91a) into main (290d573) will not change coverage. The diff coverage is 66.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #164   +/-   ##
=======================================
  Coverage   94.56%   94.56%           
=======================================
  Files          50       50           
  Lines        2558     2558           
=======================================
  Hits         2419     2419           
  Misses        139      139           
Impacted Files Coverage Δ
cdp_backend/pipeline/event_gather_pipeline.py 85.75% <0.00%> (ø)
cdp_backend/database/validators.py 84.48% <100.00%> (ø)
cdp_backend/tests/database/test_validators.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 290d573...b71a91a. Read the comment docs.

tohuynh commented 2 years ago

I didn't think my validator function would cause such a headache!

Given Isaac's comment, I think you only need a None test case for when the data model field is optional, not for when the data model field is required since that test case is covered (hopefully) by FireO.

evamaxfield commented 2 years ago

Should be good to go now!