-
Notifications
You must be signed in to change notification settings - Fork 479
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
Remove unused TeacherApplication React component #26448
Conversation
dashboard/config/routes.rb
Outdated
@@ -459,7 +459,6 @@ | |||
get 'workshop_dashboard/*path', to: 'workshop_dashboard#index' | |||
get 'workshop_dashboard', to: 'workshop_dashboard#index' | |||
|
|||
get 'teacher_application', to: 'teacher_application#new' | |||
get 'teacher_application/international_teachers', to: 'teacher_application#international_teachers' | |||
get 'teacher_application/thanks', to: 'teacher_application#thanks' | |||
get 'teacher_application/manage', to: 'teacher_application#manage' |
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.
should these other routes also be removed?
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.
I think we want to keep get 'teacher_application', to: 'teacher_application#new'
(the one you are removing) because it redirects to the current teacher application, so I'm guessing we think people might still be using that url, but I can't speak to the rest of these routes. I don't think we need them but I'm not confident enough to be the final sign-off.
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.
May be fine to remove - do we have access to data about whether people still use this url enough to keep it around?
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.
Ahh, good call; I hadn't realized that this route is pointing to the new content and not dashboard/app/views/pd/teacher_application/new.html.haml
We probably should still remove these if we can, but that should be done in a separate PR as a separate effort.
…being used to redirect _away_ from the removed content
@@ -10,23 +10,6 @@ class Pd::TeacherApplicationController < ApplicationController | |||
id_param: 'teacher_application_id' | |||
) | |||
|
|||
# GET /pd/teacher_application |
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.
this action should also be added back in since the route still exists
As part of the React ES6 Conversion effort