-
Notifications
You must be signed in to change notification settings - Fork 13
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 -520 archived intake pdf download screen #5399
Conversation
3ab00a7
to
6975caa
Compare
Heroku app: https://gyr-review-app-5399-d3511315f7b8.herokuapp.com/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple small changes requested, and a question about checking mailing address verification before allowing pdf download
@@ -11,6 +11,7 @@ | |||
Flipper.disable :show_retirement_ui unless Flipper.exist?(:show_retirement_ui) | |||
Flipper.disable :sms_notifications unless Flipper.exist?(:sms_notifications) | |||
Flipper.disable :hub_dashboard unless Flipper.exist?(:hub_dashboard) | |||
Flipper.disable :get_your_pdf unless Flipper.exist?(:get_your_pdf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this
ccf9c8c
to
6b5ae81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, one typo and one small style question for your consideration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just had one suggestion!
return if session[:mailing_verified].present? | ||
|
||
redirect_to root_path | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could all this before actions be in one method if they all redirect to root path?
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
d4219e3
to
e83fc5d
Compare
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>
Link to pivotal/JIRA issue
https://codeforamerica.atlassian.net/browse/FYST-1520
Is PM acceptance required? (delete one)
Reminder: merge main into this branch and get green tests before merging to main
What was done?
TODO
This can't be properly tested until verify ssn and other login stories are merged to step through the archive intake download flow