Skip to content

LG-7449, Add first error page for inherited proofing when no information is received from API call.#7161

Merged
rnagilla-gsa merged 5 commits intomainfrom
rrn/LG-7449
Oct 19, 2022
Merged

LG-7449, Add first error page for inherited proofing when no information is received from API call.#7161
rnagilla-gsa merged 5 commits intomainfrom
rrn/LG-7449

Conversation

@rnagilla-gsa
Copy link
Contributor

@rnagilla-gsa rnagilla-gsa commented Oct 18, 2022

🎫 Ticket

LG-7449
Link to the relevant ticket.

🛠 Summary of changes

  1. Add first error page for inherited proofing when no information is received from API call for any reason
  2. Removed unused phone html page and relevant translations.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Step 1
  • Step 2
  • Step 3

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Before:
After:

🚀 Notes for Deployment

Include any special instructions for deployment.

@rnagilla-gsa rnagilla-gsa added the inherited proofing Pull Requests for the Inherited Proofing feature label Oct 18, 2022
<%= t('inherited_proofing.errors.cannot_retrieve.info') %>
</p>

<%= link_to t('inherited_proofing.buttons.try_again'), root_url, class: 'usa-button usa-button--big usa-button--wide' %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the "Try again" be going to a page for the user to try again, rather than the root URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth Yes, that work(page which does asynchronous api call) is still in progress and this is a placeholder for now.

get '/:step' => 'inherited_proofing#show', as: :step
put '/:step' => 'inherited_proofing#update'
get '/return_to_sp' => 'inherited_proofing#return_to_sp'
get '/errors/no_information' => 'inherited_proofing#no_information'
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if there's no logic associated with the route, conventionally it's preferred to at least include an empty method for the action in the controller, so that it's more obvious what actions exist when looking at the controller implementation.

class InheritedProofingController
  # ...

  def no_information; 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.

I tried testing this locally by going directly to http://localhost:3000/verify/inherited_proofing/errors/no_information, only to see a 500 error undefined method uuid' for nil:NilClass`. It seems that we need to be adding some validation in the base controller to avoid 500.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth I actually first added that method to controller and then removed it thinking that preferred. Ex. no_camera action on doc_auth/errors . I will re add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a future tasks for adding before_filters but I added before_action :confirm_two_factor_authenticated to redirect away from that


<%= render(
'shared/troubleshooting_options',
heading_tag: :h3,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should leave out this option so that the default :h2 heading is used, since otherwise there's an accessibility issue, as the heading order increases from h1 to h3.

https://dequeuniversity.com/rules/axe/3.5/heading-order

Suggested change
heading_tag: :h3,

@rnagilla-gsa rnagilla-gsa requested a review from aduth October 18, 2022 16:38
text: t('inherited_proofing.troubleshooting.options.get_va_help'),
new_tab: true,
},
].select(&:present?),
Copy link
Contributor

Choose a reason for hiding this comment

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

There's only one option, and it's not conditional, so I don't think we need the filtering here.

Suggested change
].select(&:present?),
],

Comment on lines +3 to 6
before_action :confirm_two_factor_authenticated

include Flow::FlowStateMachine
include IdvSession
Copy link
Contributor

Choose a reason for hiding this comment

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

We may be able to resolve this issue by reordering the includes instead, since IdvSession is already checking that the user is authenticated, but it's not being reached since FlowStateMachine is where the error is happening.

Suggested change
before_action :confirm_two_factor_authenticated
include Flow::FlowStateMachine
include IdvSession
include IdvSession
include Flow::FlowStateMachine

get '/:step' => 'inherited_proofing#show', as: :step
put '/:step' => 'inherited_proofing#update'
get '/return_to_sp' => 'inherited_proofing#return_to_sp'
get '/errors/no_information' => 'inherited_proofing#no_information'
Copy link
Contributor

Choose a reason for hiding this comment

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

With a nested route like this, typically I'd expect this to be broken out to a separate controller, rather than funneling all the routes through a single controller (e.g. InheritedProofingErrorsController).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably will move this to the other ErrorController and Error handling work that Jeff is working on.

Copy link
Contributor

@aduth aduth Oct 19, 2022

Choose a reason for hiding this comment

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

Can you link me to the related ticket which tracks that work?

Copy link
Contributor

Choose a reason for hiding this comment

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

@aduth The error handling methodology mentioned here, in this PR conversation, is being worked in LG-7257. You and I had a discussion about the implementation, on the 7th of this month; specifically about creating InheritedProofingErrorsController. I am waiting for this PR code to be merged, so that I can integrate the error controller.

@rnagilla-gsa rnagilla-gsa merged commit ecd1d9f into main Oct 19, 2022
@rnagilla-gsa rnagilla-gsa deleted the rrn/LG-7449 branch October 19, 2022 20:54
This was referenced Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inherited proofing Pull Requests for the Inherited Proofing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants