NJ 230 - refactor return_header to remove state_code #5297
Conversation
|
Heroku app: https://gyr-review-app-5297-8311d7a58f0a.herokuapp.com/ |
| @@ -3,6 +3,10 @@ class ReturnHeader < SubmissionBuilder::Document | |||
| include SubmissionBuilder::FormattingMethods | |||
| include SubmissionBuilder::BusinessLogicMethods | |||
|
|
|||
| def state_submission_builder | |||
There was a problem hiding this comment.
[dust] It would make more sense to me to add ptin and preparer_person_name as methods on the default and NJ intakes rather than introduce this pattern of pulling from state submission builder, OR to add an NJ return header similar to the NJ W2. I do see that this PR follows @mpidcock's suggestion though, so I'm commenting mostly to try to understand why this implementation
There was a problem hiding this comment.
I think the original plan was to try and remove all state_code references entirely, including referencing the StateInformationService. However, that can't be done within the existing structure, since this ReturnHeader is shared between all states. It has no knowledge of what state it's representing without using state_code. The solution I chose uses the existing state_submission_builder attribute in the StateInformationService to figure out what state it's representing.
The other ideas you suggest both seem like viable alternatives!
- Add to state intake, and then pull from intake here, rather than referencing the
StateInformationService.
I prefer to avoid using intake, since conceptually, this doesn't match with how we utilize intake. intake is used for user input, while this is something static that appears on every NJ return, regardless of what the user selects.
- Adding a separate NJ Return Header
This was the approach I originally had in mind. I'm not quite sure what it would take to implement though. I believe it would involve modifying the NjReturnXml child class to override the return_header method in the parent StateReturn class. We'd also need to create a new NjReturnHeader class that is essentially identical to return_header.rb, but adds the PaidPreparerFields. This approach feels most correct to me, but I recognize it would involve a lot of duplicated XML between the ReturnHeader and the NjReturnHeader classes.
Happy to discuss further with CfA about which option feels right here.
| @@ -1,18 +1,20 @@ | |||
| module SubmissionBuilder | |||
| class StateReturn < SubmissionBuilder::Document | |||
| def document | |||
| @document = state_schema_version.present? ? | |||
| build_xml_doc(build_xml_doc_tag, stateSchemaVersion: state_schema_version) : | |||
| @document = if state_schema_version.present? | |||
There was a problem hiding this comment.
Yeah, can manually disable the linter and fix if folks prefer the old way.
Link to pivotal/JIRA issue
Is PM acceptance required? (delete one)
Reminder: merge main into this branch and get green tests before merging to main
What was done?
if state_code == 'nj'statement to conditionally place PaidPreparerFields. This refactor makes the logic consistent across all states - ifptinandpreparer_person_nameare overwritten in the state's submission builder, it places the relevant fields in the Header.How to test?
jones_mfjpersona, validated that NJ XML had PaidPreparerField block