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

feat: add option to use "Pay without pay" and show error in cases of undefined fee #2284

Merged
merged 5 commits into from
Oct 9, 2023

Conversation

jessicamcinchak
Copy link
Member

@jessicamcinchak jessicamcinchak commented Oct 5, 2023

Changes:

  • Adds Editor option to "Hide the pay buttons and show fee for information only" (off by default)
  • Updates how NaN / undefined fees are handled
    • Current behavior: if the data field referenced by the Pay component (props.fn) is undefined when Pay is reached, then automatically skip the Pay component. This led to real applications being successfully submitted without paying because of a content error a few weeks ago.
    • New behavior: if props.fn is undefined when Pay is reached, display an error and do not let the applicant continue. An airbrake error including the sessionId is also thrown/should be picked up in Slack.

Testing:

Future scope / not addressed here:

  • The Pay component is still automatically skipped in cases where the fee = 0. My understanding is we don't want to "show" this case until we can properly design how to display "reasons" (eg resubmission or exemption or other calculation)
  • Show a breakdown of how the fee was calculated (currently on roadmap here 🔮)

@jessicamcinchak
Copy link
Member Author

@jessicamcinchak
Copy link
Member Author

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Removed vultr server and associated DNS entries

@jessicamcinchak jessicamcinchak marked this pull request as draft October 5, 2023 16:10
const handleSubmit = jest.fn();
const { container } = setup(<Pay handleSubmit={handleSubmit} />);
const results = await axe(container);
expect(results).toHaveNoViolations();
Copy link
Member Author

@jessicamcinchak jessicamcinchak Oct 6, 2023

Choose a reason for hiding this comment

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

This accessibility test at the top-level of Pay was actually meaningless (it was testing the "skipped scenario" 👻), so now there's 3 accessibility tests running against the Confirm child component container in its various states:
Screenshot from 2023-10-06 09-44-45

@jessicamcinchak jessicamcinchak marked this pull request as ready for review October 6, 2023 08:10
@jessicamcinchak jessicamcinchak requested a review from a team October 6, 2023 08:11
Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

Looking great! All behaviour working as expected - thanks for also adding more a11y tests which is always good to see.

A few small comments on the code, but no show stoppers.

@jessicamcinchak jessicamcinchak merged commit 380f27e into main Oct 9, 2023
12 checks passed
@jessicamcinchak jessicamcinchak deleted the jess/pay-without-pay branch October 9, 2023 06:51
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