-
Notifications
You must be signed in to change notification settings - Fork 4
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 provisional assessment stages to PIN value views #641
Add provisional assessment stages to PIN value views #641
Conversation
default.vw_pin_value
and reporting.vw_pin_value_long
-- If the PIN has no stages but its year is not the current | ||
-- assessment year, it is likely a data error from a prior | ||
-- year that we don't want to include in our results. In | ||
-- contrast, if the PIN is in the current year but has no | ||
-- stages, it is most likely a provisional value for a PIN | ||
-- that has not mailed yet | ||
CARDINALITY(stages.procnames) != 0 | ||
OR asmt.taxyr = DATE_FORMAT(NOW(), '%Y') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this logic seem correct to you? It made sense in my QC, but it's hard to know for sure given that the only provisional values that currently exist in our database are pre-certified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does make perfect sense to me. The only thing I'll mention is that because of these conditional constraints you probably don't need to have and {{ tablename }}.cur = 'Y'
in your first macro, but better safe than sorry.
WHEN stage_values.pre_certified_tot IS NOT NULL THEN 1.5 | ||
WHEN stage_values.mailed_tot IS NOT NULL THEN 1 | ||
WHEN stage_values.pre_mailed_tot IS NOT NULL THEN 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 0
makes a lot of sense for the pre-mail stage number, but 1.5
feels kind of weird for pre-certified. Is it risky to increment these so that they run 1-5
(or 0-4
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's clear from 0, 1, 1.5, 2, 3 which stages are traditional and which are not. I think it's better to have pre-certified
be a weird number than to make it more difficult to be able to quickly divine which are the main stages. pre-certified
isn't stage 2, it's a temporary stage that only exists in certain limited contexts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going by that logic, can we make pre_mailed
stage 0.5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome Jean. Thanks for whipping reporting.vw_pin_value_long
into shape.
-- If the PIN has no stages but its year is not the current | ||
-- assessment year, it is likely a data error from a prior | ||
-- year that we don't want to include in our results. In | ||
-- contrast, if the PIN is in the current year but has no | ||
-- stages, it is most likely a provisional value for a PIN | ||
-- that has not mailed yet | ||
CARDINALITY(stages.procnames) != 0 | ||
OR asmt.taxyr = DATE_FORMAT(NOW(), '%Y') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does make perfect sense to me. The only thing I'll mention is that because of these conditional constraints you probably don't need to have and {{ tablename }}.cur = 'Y'
in your first macro, but better safe than sorry.
WHEN stage_values.mailed_tot IS NOT NULL THEN 1 | ||
WHEN stage_values.pre_mailed_tot IS NOT NULL THEN 0 | ||
END AS stage_num | ||
FROM stage_values | ||
), | ||
|
||
change_reasons AS ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I should have done when I originally made this view 😅, but perhaps a quick comment as to why clean_values
and change_reason
exist? Just something like "Assign numeric values to each stage" or "Gather change reason codes for each stage"
WHEN stage_values.pre_certified_tot IS NOT NULL THEN 1.5 | ||
WHEN stage_values.mailed_tot IS NOT NULL THEN 1 | ||
WHEN stage_values.pre_mailed_tot IS NOT NULL THEN 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's clear from 0, 1, 1.5, 2, 3 which stages are traditional and which are not. I think it's better to have pre-certified
be a weird number than to make it more difficult to be able to quickly divine which are the main stages. pre-certified
isn't stage 2, it's a temporary stage that only exists in certain limited contexts.
-- the difference for legacy compatibility | ||
'BOR CERTIFIED' | ||
], | ||
ARRAY[0, 1, 1.5, 2, 3], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder if there's a way to pull these stage number values form vw_pin_value
, but I'm sure you already tried.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I concur with @wrridgeway, this is amazing work. @ccao-jardine should be pleased we can final report on pre-mailed/certified values.
WHEN stage_values.pre_certified_tot IS NOT NULL THEN 1.5 | ||
WHEN stage_values.mailed_tot IS NOT NULL THEN 1 | ||
WHEN stage_values.pre_mailed_tot IS NOT NULL THEN 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going by that logic, can we make pre_mailed
stage 0.5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (non-blocking): It looks like column names/descriptions are missing for the new pre_
columns. It's probably worth adding them for the sake of completeness and clarity.
All makes sense. I am indeed excited for this, and appreciate the attention to detail.
For what it's worth, I think we are safe here. Our public reporting assets that rely on those views are pretty robust: none of them automatically update, and we can apply filters on stage_name to future-proof them as a second layer of protection. |
Background
This PR updates the
default.vw_pin_value
andreporting.vw_pin_value_long
views using our new knowledge of null procnames to add two new assessment stages:PRE-MAILED
: Provisional values that will be mailed once first-pass desk review completesASSESSOR PRE-CERTIFIED
: Provisional values that will be set for second-pass once appeals completeCloses #640.
Open questions
reporting.vw_assessment_roll_muni
filters bystage_name
such that these new stages will not be included, butreporting.vw_assessment_roll
does not. Do we need to update the latter to filter out provisional stages?cur = 'Y'
condition, which automatically becomes false once the record gets stamped with a procname. (This doesn't stop us from determining the values for past assessment stages, however, because we can use thevalclass IS NULL
condition in those cases to find the final stamped record for each stage.) Is it acceptable that provisional values are only available for currently open towns?Testing
I added a few unit tests in this PR to cover obvious problems with the new assessment stages, but I also ran some one-off QC queries to make sure that this PR doesn't accidentally change any non-provisional values that are already in prod. Expand each of the sections below to see the checks I ran in detail.
Make sure total counts of records with non-provisional assessment stages match between prod and dev
Check
default.vw_pin_value
:Check
reporting.vw_pin_value_long
:Make sure counts of records in each assessment stage match between prod and dev
Check
default.vw_pin_value
:Check
reporting.vw_pin_value_long
:Make sure all values for records with non-provisional assessment stages match between prod and dev
Check
default.vw_pin_value
:Check
reporting.vw_pin_value_long
: