-
Notifications
You must be signed in to change notification settings - Fork 357
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
Use task queue for CRUD operations on auth key pair #136
Conversation
|
||
add_flash(_("%{model} creation failed: Task start failed: ID [%{id}]") % | ||
{:model => ui_lookup(:table => 'auth_key_pair_cloud'), | ||
:id => task_id.to_s}, :error) unless task_id.kind_of?(Integer) |
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 know this code has been there before, but instead of ui_lookup(:table => 'auth_key_pair_cloud')
, please use
just plain Key Pair
string:
_("Key Pair creation failed: Task start failed: ID [%{id}]") % {:id => task_id.to_s}}
if MiqTask.status_ok?(task.status) | ||
add_flash(_("%{model} \"%{name}\" created") % { | ||
:model => ui_lookup(:table => 'auth_key_pair_cloud'), | ||
:name => key_pair_name |
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.
Same as above, use just Key Pair
instead of ui_lookup()
add_flash(_("Unable to create %{model} \"%{name}\": %{details}") % { | ||
:model => ui_lookup(:table => 'auth_key_pair_cloud'), | ||
:name => key_pair_name, | ||
:details => task.message |
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.
Same as above: Key Pair
f8555bd
to
8290073
Compare
@mzazrivec Requested changes have been made. |
Still waiting on the associated model PR before we can merge this, but this UI has been tested with the associated model PR changes. |
task_id = kls.create_key_pair_queue(session[:userid], ext_management_system, options) | ||
|
||
add_flash(_("Key Pair creation failed: Task start failed: ID [%{id}]") % | ||
{:id => task_id.to_s}, :error) unless task_id.kind_of?(Integer) |
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.
The above doesn't quite make sense to me. If task_id
is not integer, we render a flash saying that
a task with id task_id
failed. What would task_id
contain at this point? It certainly wouldn't be integer.
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 is the pattern that's used in most existing code like this, but I see the problem. Looking at the code that generates the return value, the only way task_id won't be an integer is if something fails in task generation and it's somehow nil (rather than raising an exception). It seems that the right answer here would be to remove the ID from the flash message, since we won't have an ID in the failure case here.
if @flash_array | ||
javascript_flash(:spinner_off => true) | ||
else | ||
initiate_wait_for_task(:task_id => task_id, :action => "create_finished") |
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.
Also, can't we have just one if
instead of two? I mean:
if task_id.kind_of?(Integer) # or some other valid condition
initiate_wait_for_task()
else
add_flash()
javascript_flash(:spinner_off => true)
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.
It looks like that would be shorter in this case, although wouldn't the way it's done in the PR be slightly more robust -- if some other condition added an error to @flash_array we'd catch it here?
8290073
to
4d7bf7b
Compare
@mzazrivec I've resolved the first comment (avoiding referencing task_id that's not an integer). I'm holding off on the second one -- it seems safer to leave it as-is, but I can change that if you think the shorter version (avoiding the 'if' statement) is worth the risk. |
4d7bf7b
to
451ddc6
Compare
@mzazrivec I went ahead and made the if/else structural changes too as suggested. It's easier to follow the code that way. |
Checked commit sseago@451ddc6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/controllers/auth_key_pair_cloud_controller.rb
|
Uses task queue instead of making direct provider API calls from the UI. This PR contains the necessary UI changes.
Depends on model changes in ManageIQ/manageiq#13464