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.

Sign up for GitHub

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

Jump to bottom

NJ 281 - Create new table to store NJ analytics #5537

Merged
merged 4 commits into from
Feb 10, 2025
Merged

NJ 281 - Create new table to store NJ analytics #5537

merged 4 commits into from
Feb 10, 2025

Conversation

jachan
Copy link
Contributor

@jachan jachan commented Feb 6, 2025 •
edited
Loading

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

  • No - merge after code review approval

Reminder: merge main into this branch and get green tests before merging to main

What was done?

  • Update calculator fields to make Blind/Disabled exemptions visible to analytics

  • Add new record for NJ analytics (so PMs can view the metrics for different lines on NJ1040)

  • Populate analytics records using async job after submission

  • TODO:

    • Backfill script to create analytics records for all existing NJ submissions, discuss approach for backfill with CFA

How to test?

  • Metabase integration seems difficult to test without deploying to Demo, happy to discuss with CFA on best solution

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 6, 2025

Heroku app: https://gyr-review-app-5537-2f85a9723a71.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5537 (optionally add --tail)

@jachan jachan marked this pull request as ready for review February 6, 2025 18:31
jnf
jnf reviewed Feb 6, 2025
View reviewed changes
app/models/state_file_nj_analytics.rb Outdated
}
required_fields.each do |metric|
if nj1040_fields[metric].nil?
0
Copy link
Contributor

Choose a reason for hiding this comment

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

should this 0 get assigned to the metabase_metrics like what happens in the else block? i.e.,

if nj1040_fields[metric].nil?
  metabase_metrics[metric] = 0
else # ...

if so, could the whole thing be a single assignment like metabase_metrics[metric] = nj1040_fields[metric] || 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Thank you for noticing this and sharing the one-liner!

jnf
jnf reviewed Feb 6, 2025
View reviewed changes
db/schema.rb Outdated
Comment on lines 2278 to 2297
t.integer "NJ1040_LINE_12_COUNT", default: 0, null: false
t.integer "NJ1040_LINE_15", default: 0, null: false
t.integer "NJ1040_LINE_16A", default: 0, null: false
t.integer "NJ1040_LINE_16B", default: 0, null: false
t.integer "NJ1040_LINE_29", default: 0, null: false
t.integer "NJ1040_LINE_31", default: 0, null: false
t.integer "NJ1040_LINE_41", default: 0, null: false
t.integer "NJ1040_LINE_42", default: 0, null: false
t.integer "NJ1040_LINE_43", default: 0, null: false
t.integer "NJ1040_LINE_51", default: 0, null: false
t.integer "NJ1040_LINE_56", default: 0, null: false
t.integer "NJ1040_LINE_58", default: 0, null: false
t.integer "NJ1040_LINE_58_IRS", default: 0, null: false
t.integer "NJ1040_LINE_59", default: 0, null: false
t.integer "NJ1040_LINE_61", default: 0, null: false
t.integer "NJ1040_LINE_64", default: 0, null: false
t.integer "NJ1040_LINE_65", default: 0, null: false
t.integer "NJ1040_LINE_65_DEPENDENTS", default: 0, null: false
t.integer "NJ1040_LINE_7_SELF", default: 0, null: false
t.integer "NJ1040_LINE_7_SPOUSE", default: 0, null: false
Copy link
Contributor

Choose a reason for hiding this comment

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

are these left over from an earlier form of the migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Thank you for catching this! Will remove.

jnf
jnf reviewed Feb 6, 2025
View reviewed changes
spec/state_machines/efile_submission_state_machine_spec.rb
end
end

context "for not NJ" do
Copy link
Contributor

Choose a reason for hiding this comment

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

love a good negative test. thank you for adding this.

mluedke2
mluedke2 reviewed Feb 6, 2025
View reviewed changes
app/models/intake/gyr_intake.rb Outdated
@@ -397,8 +397,6 @@
#
# fk_rails_... (client_id => clients.id)
# fk_rails_... (matching_previous_year_intake_id => intakes.id)
# fk_rails_... (primary_drivers_license_id => drivers_licenses.id)
# fk_rails_... (spouse_drivers_license_id => drivers_licenses.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

[pebble] - I get these changes to intake model files sometimes locally too but I believe they can be ignored? Or can be a separate PR? But also I don't know their effects and if they are harmless then I'm also fine with merging... just want to flag to be sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appreciate the flag. Was seeing build failures previously if they weren't included, since the DB didn't match the schema. Let me investigate to see if they pop up again if these files are reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was actually a really useful comment! I had been recreating the db with bin/rails db:drop and bin/rails db:prepare, but didn't realize that it wasn't actually recreating the DB from scratch! This led to these fields being removed erroneously.

This time, I recreated the DB with the following 3 commands and things worked correctly:
bin/rails db:drop
bin/rails db:create
bin/rails db:migrate

Have updated our internal notes on how this works.

mluedke2
mluedke2 reviewed Feb 6, 2025
View reviewed changes
app/models/state_file_nj_intake.rb Outdated
# NJ1040_LINE_65 :integer default(0), not null
# NJ1040_LINE_65_DEPENDENTS :integer default(0), not null
# NJ1040_LINE_7_SELF :integer default(0), not null
# NJ1040_LINE_7_SPOUSE :integer default(0), not null
Copy link
Contributor

Choose a reason for hiding this comment

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

[potential boulder] - I'm having trouble following the addition of all these columns in both the intake and the analytics. Is there a limitation we have that we need to add both and can't make do with only the columns on the new analytics table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They shouldn't be in the intake! This is an artifact from the previous migration, which Jey called out and I am removing.

mluedke2
mluedke2 reviewed Feb 6, 2025
View reviewed changes
Copy link
Contributor

@mluedke2 mluedke2 left a comment

Choose a reason for hiding this comment

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

I left a question that's keeping me from following how this works

@jachan jachan force-pushed the nj-281-add-nj-analytics branch from 0f0ec99 to c40395c Compare February 10, 2025 19:48
mluedke2
mluedke2 approved these changes Feb 10, 2025
View reviewed changes
Copy link
Contributor

@mluedke2 mluedke2 left a comment

Choose a reason for hiding this comment

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

looks good!

jnf
jnf approved these changes Feb 10, 2025
View reviewed changes
Copy link
Contributor

@jnf jnf left a comment

Choose a reason for hiding this comment

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

stellar.

@jachan jachan merged commit 0b43dda into main Feb 10, 2025
7 checks passed
@jachan jachan deleted the nj-281-add-nj-analytics branch February 10, 2025 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnf jnf jnf approved these changes

@mluedke2 mluedke2 mluedke2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jachan @jnf @mluedke2