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 663 implement form 502 su xml pdf part 1 #4942

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

anisharamnani
Copy link
Contributor

@anisharamnani anisharamnani commented Nov 1, 2024

Link to pivotal/JIRA issue

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

Is PM acceptance required? (delete one)

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

What was done?

This PR adds Form 502SU including personal information, lines 1, and ab. It also adds the subtractions total from line 1 in Form 502 SU to Form 502.

How to test?

Line ab includes information from DF JSON interestOnGovernmentBonds, so an interest report including the interestOnGovernmentBonds is needed.

Screenshots (for visual changes)

Screenshot 2024-11-01 at 4 33 44 PM
Screenshot 2024-11-01 at 4 34 24 PM
Screenshot 2024-11-01 at 4 34 31 PM

Copy link

github-actions bot commented Nov 1, 2024

Heroku app: https://gyr-review-app-4942-a9d00e5c3015.herokuapp.com/
View logs: heroku logs --app gyr-review-app-4942 (optionally add --tail)

@anisharamnani anisharamnani marked this pull request as ready for review November 1, 2024 21:19
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.

left 3 little pebble questions. i'm definitely interested in the answers, but i don't think they're anything that should delay merging.

def initialize(value_access_tracker:, lines:, intake:)
@value_access_tracker = value_access_tracker
@lines = lines
@intake = intake
Copy link
Contributor

Choose a reason for hiding this comment

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

[pebble] aside from the assignment on line 10, the intake isn't referenced so i don't believe we need to allocate an instance variable for it.

Copy link
Contributor Author

@anisharamnani anisharamnani Nov 4, 2024

Choose a reason for hiding this comment

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

ah, this is what happens when you're new and copy pasta what you've seen in other files, you're completely right. there's no need to allocate an instance variable.

def initialize(submission)
@submission = submission
builder = StateFile::StateInformationService.submission_builder_class(:md)
@xml_document = builder.new(submission).document
Copy link
Contributor

Choose a reason for hiding this comment

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

[pebble] similar to previous comment -- doesn't look like this instance variable is referenced anywhere in this class. could it be removed?

def document
build_xml_doc("Form502SU", documentId: "Form502SU") do |xml|
xml.Subtractions do |subtractions|
subtractions.Total calculated_fields.fetch(:MD502_SU_LINE_1)
Copy link
Contributor

Choose a reason for hiding this comment

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

[pebble] is there supposed to be an = here or is Total a method name receiving a parameter?

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 is intentional! the build_xml_doc method generates the xml for the state return. therefore the value for Total will be assigned from this method.

Anisha Ramnani and others added 5 commits November 6, 2024 10:25
…form 502SU, and add code letters from form 502SU to form 502
# Conflicts:
#	app/lib/efile/line_data.yml
#	app/lib/efile/md/md502_calculator.rb
#	spec/lib/efile/md/md502_calculator_spec.rb
#	spec/lib/pdf_filler/md502_pdf_spec.rb
@@ -85,5 +90,21 @@ def filing_status(method)
def checkbox_value(value)
value.present? ? 'Yes' : 'Off'
end

def generate_codes_for_502_su
calculated_fields_code_letters = {MD502_SU_LINE_AB: "ab", MD502_SU_LINE_U: "u", MD502_SU_LINE_V: "v"}
Copy link
Member

Choose a reason for hiding this comment

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

[specs of dust] I think this would be slightly better with newlines for each pair (esp because the list may grow in the future), but incredibly minor.

Suggested change
calculated_fields_code_letters = {MD502_SU_LINE_AB: "ab", MD502_SU_LINE_U: "u", MD502_SU_LINE_V: "v"}
calculated_fields_code_letters = {
MD502_SU_LINE_AB: "ab",
MD502_SU_LINE_U: "u",
MD502_SU_LINE_V: "v"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

happy to do that :)

Copy link
Member

@mpidcock mpidcock left a comment

Choose a reason for hiding this comment

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

Looks great!

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.

3 participants