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-555 Add MD W2 info to XML #4937

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tahsinaislam
Copy link
Contributor

Link to pivotal/JIRA issue

https://codeforamerica.atlassian.net/browse/FYST-555

Is PM acceptance required?

  • 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?

  • Checked that all local w2 values (box18-20) , when edited pass in the new info to the state xml.
  • Added in box 14 node for STPICKUP

How to test?

  • Added in unit test for this in the state return spec for the xml

Screenshots (for visual changes)

Screenshot 2024-10-31 at 12 18 24 PM

@@ -232,7 +232,7 @@
<EmployerEIN>001245768</EmployerEIN>
<EmployerNameControlTxt>BTB</EmployerNameControlTxt>
<EmployerName>
<BusinessNameLine1Txt>Brendan T. Byrne State Forest</BusinessNameLine1Txt>
<BusinessNameLine1Txt>Brendan T Byrne State Forest</BusinessNameLine1Txt>
</EmployerName>
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 erroring out in the state return. I'm assuming federal xmls will not have errors with (.)s so wanted fix this

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 double checked that the BusinessNameLine1Txt type should not include periods.

Copy link

Heroku app: https://gyr-review-app-4937-3f02de10b8f3.herokuapp.com/
View logs: heroku logs --app gyr-review-app-4937 (optionally add --tail)

@tahsinaislam tahsinaislam marked this pull request as ready for review October 31, 2024 17:32
Copy link
Contributor

@arinchoi03 arinchoi03 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 to me but would love to chat about why sometimes we don't need to add nodes and sometimes we don't?

if existing_deductions
existing_deductions.add_next_sibling(stpickup_node)
else
state_local_tax_grp_node.add_previous_sibling(stpickup_node)
Copy link
Contributor

@arinchoi03 arinchoi03 Oct 31, 2024

Choose a reason for hiding this comment

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

Can we tuple about this -- not familiar with this add_child/add_next_sibling pattern and not sure when it's necessary

[Maybe dust or maybe pebble?] Also, if state_local_tax_grp_node does not exist, what happens? It looks like it can be removed on line 13. Is it just not possible for this to happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

followup, it is definitely possible for state_local_tax_grp_node to not exist (or to have been removed) See discussion here on why we should find a different node to add previous sibling to or just add at the end of the w2 if there are no other potential nodes

@@ -232,7 +232,7 @@
<EmployerEIN>001245768</EmployerEIN>
<EmployerNameControlTxt>BTB</EmployerNameControlTxt>
<EmployerName>
<BusinessNameLine1Txt>Brendan T. Byrne State Forest</BusinessNameLine1Txt>
<BusinessNameLine1Txt>Brendan T Byrne State Forest</BusinessNameLine1Txt>
</EmployerName>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 double checked that the BusinessNameLine1Txt type should not include periods.


expect(w2_node.css("OtherDeductionsBenefitsGrp Desc")[0]&.text).to eq "414HSUB"
expect(w2_node.css("OtherDeductionsBenefitsGrp Amt")[0]&.text).to eq "250"
end
Copy link
Contributor

@arinchoi03 arinchoi03 Nov 1, 2024

Choose a reason for hiding this comment

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

[Dust] since the set up is the same for both it blocks in this context, can we just put them all in the same it block & add comments if necessary?

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.

2 participants