Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add appeal reason code seed and include it in default.vw_pin_appeal #619

Conversation

wrridgeway
Copy link
Member

No description provided.

@wrridgeway wrridgeway linked an issue Oct 9, 2024 that may be closed by this pull request
@wrridgeway wrridgeway self-assigned this Oct 9, 2024
@wrridgeway
Copy link
Member Author

dbt build is currently failing because there are bad values for change in iasworld.htpar. Mirella has already fixed them and dbt should build without issue tomorrow 🤞 .

@wrridgeway wrridgeway marked this pull request as ready for review October 21, 2024 14:47
@wrridgeway wrridgeway requested a review from a team as a code owner October 21, 2024 14:47
Copy link
Contributor

@jeancochrane jeancochrane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super helpful, thanks!

dbt/seeds/ccao/schema.yml Outdated Show resolved Hide resolved
Comment on lines 41 to 42
Table containing descriptions for appeal decision reason codes from
`iasworld.htpar`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question, non-blocking] Where did these descriptions come from? Maybe we can name that source as part of this description, in case it changes in the future and we need to update the seed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. They've been living as an attachment on the open data portal forever now, my guess is we got them from Mirella/Will. I'll look through my email and add a note.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't speak to the source of the tabular version, just commenting to note that they are public as a pdf doc on the CCAO website.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of linking to the PDF doc!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. There are definitely some codes in iasWorld that aren't in the PDF, but I'll include it as a resource regardless.

@wrridgeway
Copy link
Member Author

wrridgeway commented Oct 21, 2024

@jeancochrane Would it make any sense to move default_vw_pin_appeal_no_unexpected_change_values into the qc views? It seems weird to have the view fail to build if someone enters bad data in iasWorld.

Edit: I should probably just make the re-coding syntax more explicit.

@wrridgeway wrridgeway merged commit cd4bf2d into master Oct 21, 2024
8 checks passed
@wrridgeway wrridgeway deleted the 618-add-appeal-reason-code-seed-and-include-it-in-defaultvw_pin_appeal branch October 21, 2024 18:34
@jeancochrane
Copy link
Contributor

jeancochrane commented Oct 22, 2024

Would it make any sense to move default_vw_pin_appeal_no_unexpected_change_values into the qc views? It seems weird to have the view fail to build if someone enters bad data in iasWorld.

@wrridgeway I agree that we need to rethink this test, but I'm not totally sure what the best path forward is. Check out the distribution of results for this query:

with user104s as (
    select TRIM(LOWER(user104)) as user104
    from iasworld.htpar
    where cur = 'Y' and deactivat is null
)

select user104, count(*) from user104s group by user104

It seems like the underlying data in htpar no longer match the assumptions of our view. Its values appear to be only "change", "no change", and null (discounting the two problematic rows that caused the test to start erroring out). When I add taxyr to the user104s query and group by that in the top-level select statement, I also see that all years prior to 2020 appear to be null across the board, with no values at all let alone the legacy "C", "NC", and "decrease" values.

If the distribution in htpar.user104 is correct and not the result of some sort of systemic data problem, I wonder if the best path forward for us would be to adjust the definition of the vw_pin_appeal.change field so that it selects TRIM(LOWER(user104)) for all years, and move the default_vw_pin_appeal_no_unexpected_change_values test to the iasworld.htpar source definition?

@wrridgeway
Copy link
Member Author

wrridgeway commented Oct 23, 2024

@jeancochrane I think we can clean up the code that constructs change (remove the 'decrease' clause), but i think we're already anticipating user104 being empty before 2021 - we use resact for 2020 and prior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add appeal reason code seed and include it in default.vw_pin_appeal
3 participants