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

NJ 230 - refactor return_header to remove state_code #5297

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

NJ 230 - refactor return_header to remove state_code #5297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0abd092
NJ 230 - refactor return_header to remove state_Code
jachan Dec 26, 2024
558be79
PR comments
jachan Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions app/lib/submission_builder/return_header.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ class ReturnHeader < SubmissionBuilder::Document
include SubmissionBuilder::FormattingMethods
include SubmissionBuilder::BusinessLogicMethods

def state_submission_builder
Copy link
Contributor

Choose a reason for hiding this comment

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

[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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

  1. 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.

  1. 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.

StateFile::StateInformationService.submission_builder_class(@submission.data_source.state_code)
end

def document
build_xml_doc("ReturnHeaderState") do |xml|
xml.Jurisdiction "#{@submission.data_source.state_code.upcase}ST"
Expand All @@ -12,10 +16,10 @@ def document
xml.TaxPeriodEndDt date_type(Date.new(@submission.data_source.tax_return_year, 12, 31))
end
xml.TaxYr @submission.data_source.tax_return_year
if @submission.data_source.state_code.upcase == "NJ"
if state_submission_builder.ptin.present? && state_submission_builder.preparer_person_name.present?
xml.PaidPreparerInformationGrp do
xml.PTIN "P99999999"
xml.PreparerPersonNm "Self Prepared"
xml.PTIN state_submission_builder.ptin
xml.PreparerPersonNm state_submission_builder.preparer_person_name
end
end
xml.DisasterReliefTxt @intake.disaster_relief_county if @intake.respond_to?(:disaster_relief_county)
Expand Down
16 changes: 11 additions & 5 deletions app/lib/submission_builder/state_return.rb
View file
Open in desktop
jachan marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

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

Linter changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, can manually disable the linter and fix if folks prefer the old way.

build_xml_doc(build_xml_doc_tag, stateSchemaVersion: state_schema_version)
else
build_xml_doc(build_xml_doc_tag)
end
build_headers
build_main_document
build_documents
build_state_specific_tags(@document)
@document
end

def pdf_documents
included_documents.map { |item| item if item.pdf }.compact
included_documents.select { |item| item.pdf }
end

def build_headers
Expand Down Expand Up @@ -50,7 +52,7 @@ def attached_documents
end

def xml_documents
included_documents.map { |item| item if item.xml }.compact
included_documents.select { |item| item.xml }
end

def included_documents
Expand Down Expand Up @@ -128,5 +130,9 @@ def state_schema_version; end
def build_state_specific_tags(_); end

def documents_wrapper; end

def self.preparer_person_name; end

def self.ptin; end
end
end
2 changes: 1 addition & 1 deletion app/lib/submission_builder/ty2024/states/nj/documents/nj1040.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def schema_file
SchemaFileLoader.load_file("us_states", "unpacked", "NJIndividual2024V0.1", "NJIndividual", "NJForms", "FormNJ1040.xsd")
end

def document
def document
build_xml_doc("FormNJ1040") do |xml|
xml.Header do
xml.FilingStatus do
Expand Down
8 changes: 8 additions & 0 deletions app/lib/submission_builder/ty2024/states/nj/nj_return_xml.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@ def w2_class

private

def self.ptin
"P99999999"
end

def self.preparer_person_name
"Self Prepared"
end

def attached_documents_parent_tag
# Line 29 in ReturnDataNj1040.xsd
'ReturnDataState'
Expand Down
44 changes: 25 additions & 19 deletions spec/lib/submission_builder/ty2022/states/return_header_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,25 +54,6 @@
end
end

context "paid preparer information group" do
if state_code == "nj"
context "for NJ returns" do
it "adds XML elements for PaidPreparerInformationGrp" do
expect(doc.at("PaidPreparerInformationGrp PTIN").text).to eq "P99999999"
expect(doc.at("PaidPreparerInformationGrp PreparerPersonNm").text).to eq "Self Prepared"
end
end
else
context "for non NJ returns" do
it "does not add XML elements for PaidPreparerInformationGrp" do
expect(doc.at("PaidPreparerInformationGrp")).not_to be_present
expect(doc.at("PaidPreparerInformationGrp PTIN")).not_to be_present
expect(doc.at("PaidPreparerInformationGrp PreparerPersonNm")).not_to be_present
end
end
end
end

context "filer personal info" do
let(:primary_birth_date) { 40.years.ago }
let(:primary_ssn) { "100000030" }
Expand Down Expand Up @@ -365,6 +346,31 @@
end
end

context "paid preparer information group" do
context "for NJ returns", required_schema: "nj" do
let(:intake) { create(:state_file_nj_intake) }
let(:submission) { create(:efile_submission, data_source: intake) }
let(:doc) { SubmissionBuilder::ReturnHeader.new(submission).document }
it "adds XML elements for PaidPreparerInformationGrp" do
expect(doc.at("PaidPreparerInformationGrp PTIN").text).to eq "P99999999"
expect(doc.at("PaidPreparerInformationGrp PreparerPersonNm").text).to eq "Self Prepared"
end
end

StateFile::StateInformationService.active_state_codes.excluding("nj").each do |state_code|
let(:intake) { create "state_file_#{state_code}_intake".to_sym }
let(:submission) { create(:efile_submission, data_source: intake) }
let(:doc) { SubmissionBuilder::ReturnHeader.new(submission).document }
context "for #{state_code} returns" do
it "does not add XML elements for PaidPreparerInformationGrp" do
expect(doc.at("PaidPreparerInformationGrp")).not_to be_present
expect(doc.at("PaidPreparerInformationGrp PTIN")).not_to be_present
expect(doc.at("PaidPreparerInformationGrp PreparerPersonNm")).not_to be_present
end
end
end
end

context "Disaster relief" do
let(:submission) { create(:efile_submission, data_source: intake) }
let(:doc) { SubmissionBuilder::ReturnHeader.new(submission).document }
Expand Down
Loading