Redirect tasks subcollection to request_tasks #15357
Conversation
remove verbs from redirect
| run_get("#{automation_requests_url(automation_request.id)}/tasks") | ||
|
|
||
| expect(response).to have_http_status(:moved_permanently) | ||
| expect(response.redirect?).to be_truthy |
There was a problem hiding this comment.
I think this expectation is redundant - the one above checks that it is a 301, this one checks that it is in the set of many redirect codes
| expect(response.redirect?).to be_truthy | ||
| expect(response.redirect_url).to include("#{automation_requests_url(automation_request.id)}/request_tasks") | ||
|
|
||
| run_get response.redirect_url, :expand => :resources |
There was a problem hiding this comment.
Although I like the idea of testing the whole flow of being redirected in this test, on the flip side this could also be a test duplication. I think we could assume that this subsequent request will do the right thing, especially if it is tested elsewhere (assuming it is). So I'm not sure this extra check is necessary. WDYT?
|
Checked commits jntullo/manageiq@2226ad2~...bfe378b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
Nice PR# @jntullo (and @imtayadeway for the suggestion/help. customers will get the 301 and their calls will continue to work. we'd still document the deprecation/redirect in 3.0.0. Thanks!! |
|
LGTM!! 😍 |
This is a continuation of #15265 which resolves BZ https://bugzilla.redhat.com/show_bug.cgi?id=1442087
Taking Tim's suggestion, I thought it was best to implement a redirect which returns a 301 "Moved Permanently" and provides all the necessary information. A "Bad Request" response does not make as much sense in this case, and this will be a solid step before removing it.
thoughts @abellotti @imtayadeway ?
@miq-bot add_label api, bug
@miq-bot assign @abellotti