LG-10530: Officially turn on new GPO routes#9184
Conversation
matthinz
force-pushed
the
matthinz/10530-rename-gpo-routes-part-2
branch
from
9d5a948 to
1cbed0e
Compare
soniaconnolly
left a comment
soniaconnolly
left a comment
There was a problem hiding this comment.
LGTM, one spec suggestion.
I tried out verify by mail on the branch. All urls and redirects work as expected.
There was a problem hiding this comment.
Should this use a route helper?
| expect(subject.fallback_back_path).to eq('/verify/by_mail/enter_code') | |
| expect(subject.fallback_back_path).to eq(idv_verify_by_mail_enter_code_path) |
There was a problem hiding this comment.
Yeah I think it doesn't because the helpers aren't available in the presenter spec, but I think I'll just include them and make the change.
Keep redirects in place for old routes to support 50/50 state [skip changelog]
matthinz
force-pushed
the
matthinz/10530-rename-gpo-routes-part-2
branch
from
1cbed0e to
7b71c24
Compare
| get '/by_mail/request_letter' => 'by_mail/request_letter#index', as: :request_letter | ||
| put '/by_mail/request_letter' => 'by_mail/request_letter#create' | ||
|
|
||
| # Temporary routes + redirects supporting GPO route renaming |
There was a problem hiding this comment.
Is there a follow-up ticket to remove these temporary routes @matthinz ?
There was a problem hiding this comment.
@aduth we had a separate route cleanup ticket that did not cover these. I just checked and it looks like we're still getting traffic to these paths. I'll file a ticket to follow up again in a bit.
There was a problem hiding this comment.
👋 @matthinz Checking in again, I see these routes are still here. Did you create a follow-up ticket?
4 participants
Follow-on to #9136. Should not be merged until that is deployed.