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-1675 Send filer rejection notice if efile errors have handling #5445

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

Conversation

arinchoi03
Copy link
Contributor

@arinchoi03 arinchoi03 commented Jan 25, 2025

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?

handling: means an admin set auto_wait, expose, auto_cancel flags on an efile error. When the next time this error is seen (on a different submission), it will apply that same handling for the new submission (efile_submission_transition_error that points to the shared EfileError -- see Efile::SubmissionErrorParser for more information)

  • Part 1: When efile error already exists with handling, make sure that auto_wait: true efiling error triggers a notification for the user to correct their submission (AfterTransitionTasksForRejectedReturnJob)
  • Part 2: When efile error is updated with handling and the admin presses "Reprocess", make sure the rejected submission also notifies the client if all efile errors are "auto-wait". (EfileErrorsController #reprocess method)
  • Miscellaneous:
    • Noticed that the ability file was missing permissions for efile errors with service_type set to state_file_#{state_code} so added those
    • Updated efile_submissions factory for_state attribute to utilize state_file_az_intake as the data_source instead of state_file_ny_intake to utilize what's live

How to test?

  • Describe the testing approach taken to verify the changes, including:
    • Unit/integration/manual tests
      • added back hub efile errors controller specs: deleted in Delete CTC deprecated actions #4613 -- I had to refactor to make this work & add new expectations now that the code is changing as well. Feel free to just read as if I wrote the entire doc from scratch.
      • I'm not sure how we want to acceptance test this -- for the reprocess button, I can create an efile error and associate a transition on a submisson in "rejected" status with the same error code & service_type, then press the button to reprocess the error
      • for the AfterTransitionTasksForRejectedReturnJob, create an efile error for a state, create a submission and transition the submission to "failed" state and watch it get notified?
  • Specify any relevant testing environments used (e.g., development, staging, demo, Heroku).
  • Risk Assessment
    • This is a change to the existing status of things, so we want to be extra sure that the status changes that are going in are tested thoroughly.

Screenshots (for visual changes)

  • Before
  • After

@@ -9,9 +9,10 @@ def perform(submission, transition)
if transition.efile_errors.any?(&:auto_cancel)
submission.transition_to!(:notified_of_rejection)
submission.transition_to!(:cancelled)
elsif transition.efile_errors.all?(&:auto_wait)
submission.transition_to!(:notified_of_rejection)
Copy link
Contributor Author

@arinchoi03 arinchoi03 Jan 25, 2025

Choose a reason for hiding this comment

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

in the else scenario, they remain in rejected state & see pending html on return-status and do not receive any notification (unless they travel further down & have any retry-able error codes).

Technically same pathway for the else submissions, just different syntax. The main change is the change from :notified_of_rejection instead of :waiting

pending 'Not implemented'
end
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to fill this file out before I add my code :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resuscitating an old file deleted: #4613

Copy link

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

@@ -48,7 +48,7 @@

trait :for_state do
tax_return {}
data_source { create :state_file_ny_intake }
data_source { create :state_file_az_intake }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hoping this doesnt catastrophically mess things up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it didnt! had to do a small follow up commit to fix 42ced40

efile_error: @efile_error,
efile_submission_transitions: { most_recent: true, to_state: ["rejected", "failed"] }
)
.pluck(:efile_submission_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the above block is just me reformatting code to be more readable (in ruby mine)

Copy link
Contributor

@embarnard embarnard left a comment

Choose a reason for hiding this comment

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

Nice work! 🎉

let(:failed_submission) { create :efile_submission, :for_state, :failed }
let(:wait_efile_error) { create :efile_error, code: "WAIT-ME-123", auto_wait: true, service_type: :state_file_az }
let(:cancel_efile_error) { create :efile_error, code: "CANCEL-ME-123", auto_cancel: true, service_type: :state_file_az }
let(:not_handled_error) { create :efile_error, code: "NOTHING-ME-123", auto_cancel: false, auto_wait: false, service_type: :state_file_az }
Copy link
Contributor

Choose a reason for hiding this comment

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

what is a not handled error? is that just the default error?

end
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

nice spec! 🎉

context "with nj state efile error" do
let!(:state_efile_error) { create :efile_error, service_type: "state_file_nj" }
it "cannot manage the state efile error" do
expect(subject.can?(:manage, state_efile_error)).to eq false
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe out of the scope of this ticket but should there be a test for a user role "StateFileNjStaffRole::TYPE" that can manage nj state efile erorr?

private

def auto_transition_to_state(efile_error, submission)
return :cancelled if efile_error.auto_cancel
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way we can just transition all the efile submissions to cancelled if the efile_error is auto_cancel up on line 52 instead of looping through all of them each time?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it doesn't matter that much though since we probably won't have that many submissions to reprocess each time, so more of just an idea [DUST]

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