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

FYST 754 implement form 502 xml pdf two income subtraction #5012

Merged

Conversation

mpidcock
Copy link
Member

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

  • Yes - don't merge until JIRA issue is accepted!

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

What was done?

How to test?

Screenshots (for visual changes)

  • Before
  • After

Copy link

Heroku app: https://gyr-review-app-5012-75395960f220.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5012 (optionally add --tail)

Comment on lines 51 to 53
# MD502SU Subtractions
@md502_su.calculate
set_line(:MD502_LINE_13, :calculate_line_13)
Copy link
Member Author

Choose a reason for hiding this comment

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

These lines were moved lower, not deleted

Comment on lines 90 to 96
if primary_or_spouse == :primary
@lines[:MD_TWO_INCOME_WK_LINE_1_A].value +
@lines[:MD_TWO_INCOME_WK_LINE_2_A].value
else
@lines[:MD_TWO_INCOME_WK_LINE_1_B].value +
@lines[:MD_TWO_INCOME_WK_LINE_2_B].value
end
Copy link
Contributor

@anisharamnani anisharamnani Nov 21, 2024

Choose a reason for hiding this comment

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

        filer = primary_or_spouse == :primary ? "A" : "B"
         line_or_zero("MD_TWO_INCOME_WK_LINE_1_#{filer}") +
            line_or_zero("MD_TWO_INCOME_WK_LINE_2_{filer}")

we spoke during our pairing/review session about changing this method to look something like this in order to make it more clear that the calculations are very similar.

i think it is more clear this way, but i will also leave it up to you since it feels more stylistic & either implementation is fine.

end
end
end

describe "#calculate_line_15" do
before do
intake.direct_file_data.total_qualifying_dependent_care_expenses = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

[dust] since we're now relying on numbers to calculate these, it could make sense to also stub the lines here as opposed to setting the values for intake.direct_file_data.total_qualifying_dependent_care_expenses and intake.direct_file_data.fed_taxable_ssb

Copy link
Member Author

Choose a reason for hiding this comment

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

the update was to set the line in the calculator using those fields, but not to add the calculate_line_x methods, so the way this is set up is still correct

end
instance.calculate

expect(instance.lines[:MD_TWO_INCOME_WK_LINE_1_A].value).to eq(90)
Copy link
Contributor

Choose a reason for hiding this comment

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

[dust] since we have the pattern elsewhere of adding and subtracting the values, we could do the same here as well. for example:

 expect(instance.lines[:MD_TWO_INCOME_WK_LINE_1_A].value).to eq(100-10)

Copy link
Contributor

@anisharamnani anisharamnani left a comment

Choose a reason for hiding this comment

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

@mpidcock and i paired on reviewing this together. i have very minor comments, but overall i think this is a very clean and easy to understand approach. thanks @mpidcock !

}

# TODO: Some MFJ tests are missing spouse JSON - should not happen in prod
return 0 unless filer_json
Copy link
Contributor

@anisharamnani anisharamnani Nov 22, 2024

Choose a reason for hiding this comment

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

more of a question than a request for change, why is it no longer an issue if there isn't a filer_json?

@@ -119,6 +115,13 @@ def calculate_line_6
def calculate_line_7
@lines[:MD_TWO_INCOME_WK_LINE_6].value.clamp(0, 1_200)
end

def find_filer_json_for(primary_or_spouse)
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 this extraction!

@anisharamnani
Copy link
Contributor

@mpidcock asked for a re-review since the total unemployment per filer will be provided in the json. everything looks good! i appreciate the refactor of pulling out the method for find_filer_json_for.

…df-two-income-subtraction

# Conflicts:
#	app/lib/efile/md/md502_calculator.rb
#	app/models/direct_file_json_data.rb
#	spec/fixtures/state_file/fed_return_jsons/md/minimal_with_spouse.json
#	spec/fixtures/state_file/fed_return_xmls/md/minimal_with_spouse.xml
#	spec/lib/submission_builder/ty2024/states/md/md_return_xml_spec.rb
@mpidcock mpidcock force-pushed the FYST-754-implement-form-502-xml-pdf-two-income-subtraction branch 3 times, most recently from d6915ac to 0e8e83e Compare November 27, 2024 05:08
@mpidcock mpidcock force-pushed the FYST-754-implement-form-502-xml-pdf-two-income-subtraction branch from 0e8e83e to 0fa7f68 Compare November 27, 2024 17:29
…df-two-income-subtraction

# Conflicts:
#	app/lib/submission_builder/ty2024/states/md/documents/md502.rb
@mpidcock mpidcock merged commit d5ca3cf into main Nov 27, 2024
7 checks passed
@mpidcock mpidcock deleted the FYST-754-implement-form-502-xml-pdf-two-income-subtraction branch November 27, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants