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-1809 bounce from page when there is no request #5616

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

Conversation

DrewProebstel
Copy link
Contributor

@DrewProebstel DrewProebstel commented Feb 20, 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?

How to test?

  • Specify any relevant testing environments used (e.g., development, staging, demo, Heroku).
  • try navigate to /en/state_file/archived_intakes/verification_code/edit without entering an email first.
  • Risk Assessment
    • We could be bouncing people who are encountering whatever the unsolved problem is. But we are also logging that.

Copy link

Heroku app: https://gyr-review-app-5616-51a7378a7e6d.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5616 (optionally add --tail)

Copy link
Contributor

Choose a reason for hiding this comment

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

[pebble] i think we typically use things like .not_to be_redirect instead of expecting it to receive redirect_to? seems a little more precise (because it checks the response as opposed to just checking that redirect_to was called, which it can be without actually redirecting) though i think they serve a very similar purpose. i will give a couple of examples below

Copy link
Contributor

Choose a reason for hiding this comment

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

having said that i just realized that since there's no request being made in this spec, you can't really check the response :/ nvm

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 its def a little tricky to test, lmk if you have a better idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

but just to elaborate: i think a more typical (and you could argue, more thorough) test would involve testing an action on a controller (in this case, either a child of ArchivedIntakeController or a fake child controller created in the test file) and then having something like this:

it 'redirects to verification error page' do
  get :edit
  expect(response).to redirect_to state_file_archived_intakes_verification_error_path
end

it 'does not redirect' do
  get :edit
  expect(response).not_to be_redirect
end

Copy link
Contributor

Choose a reason for hiding this comment

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

oh i totally missed your comment in the middle there. i was leaving several in a row so probably had to refresh the page. my real suggestion is in the approval comment!

Copy link
Contributor

@jenny-heath jenny-heath left a comment

Choose a reason for hiding this comment

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

left some comments about an alternative testing approach and in writing out my argument for it, i looked into the other tests and realized that the before_action is_request_locked is covered in 3 or 4 different controller specs in the exact same way. if you're interested in a small refactor project, i would consolidate the test you added here with the request-locked specs for child controllers of ArchivedIntakeController (for example, spec/controllers/state_file/archived_intake/pdfs_controller_spec.rb:21) by making it a shared example. happy to provide more pointers if the suggestion is unclear!

i'm not sure if this is a priority change so i'm approving the PR. the refactor can be a follow-up if this is urgent but if not i'd just do it on this branch

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