Skip to content
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 function getBack in .then instead of result of that function #990

Merged
merged 1 commit into from
Apr 12, 2017

Conversation

ZitaNemeckova
Copy link
Contributor

https://bugzilla.redhat.com/show_bug.cgi?id=1437373

In Firefox without disabled notifications.
Automation -> Ansible -> Credentials -> Configuration -> Add/Edit Credentials -> Add/Save button

Before:
There is a toaster error message after adding/saving a credentials.
After:
No toaster error message.

@miq-bot add_label bug, fine/yes, automation/ansible

@AparnaKarve please review. Thanks :)

@@ -58,13 +58,13 @@ ManageIQ.angular.app.controller('ansibleCredentialsFormController', ['$window',

vm.saveClicked = function(angularForm) {
API.put('/api/authentications/' + credentialId, vm.credentialModel)
.then(getBack(sprintf(__("Modification of Credential \"%s\" has been successfully queued."), vm.credentialModel.name), false))
.then(getBack.bind(vm, sprintf(__("Modification of Credential \"%s\" has been successfully queued."), vm.credentialModel.name), false))
Copy link
Contributor

@himdel himdel Apr 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be either .then(getBack.bind(vm, sprintf(...), false, false)) (to provide all 3 params), or .then(function() { getBack(sprintf(...)); }).

Otherwise, response becomes the 3rd param, causing an error icon instead of success on the flash.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZitaNemeckova @himdel How does using .bind resolve this issue which is visible only in Firefox?
Would love to know more about this fix. Seems interesting.
Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AparnaKarve There is a difference between giving .then a function and a result of a function. If you give it a function it runs it after it gets response from API call. But if you give it a result of a function as it was before this fix it runs the function to get the result before it gets response from API. So it generates error because there is no response but at the same time it runs success function getBack to get its result into .then. So perils of asynchronous programming :) I hope I got @himdel explanation right :D

.catch(miqService.handleFailure);
};

vm.addClicked = function(angularForm) {
API.post('/api/authentications/', vm.credentialModel)
.then(getBack(sprintf(__("Add of Credential \"%s\" has been successfully queued."), vm.credentialModel.name)))
.then(getBack.bind(vm, sprintf(__("Add of Credential \"%s\" has been successfully queued."), vm.credentialModel.name)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, otherwise response becomes the 2nd param, causing a warning icon.

@ZitaNemeckova
Copy link
Contributor Author

@himdel Fixed.

@miq-bot
Copy link
Member

miq-bot commented Apr 12, 2017

Checked commit ZitaNemeckova@7ef6327 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks good. ⭐

@himdel himdel self-assigned this Apr 12, 2017
@himdel himdel added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 12, 2017
@himdel
Copy link
Contributor

himdel commented Apr 12, 2017

Tested in the UI, we now wait for server response before redirecting. And the success flash is success again :).

@himdel himdel merged commit b6a99da into ManageIQ:master Apr 12, 2017
simaishi pushed a commit that referenced this pull request Apr 13, 2017
Use function getBack in .then instead of result of that function
(cherry picked from commit b6a99da)

https://bugzilla.redhat.com/show_bug.cgi?id=1441658
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 35909aa342f4ca6d5e377bb731e2d20dd5ea3adc
Author: Martin Hradil <[email protected]>
Date:   Wed Apr 12 11:11:45 2017 +0000

    Merge pull request #990 from ZitaNemeckova/bz1437373
    
    Use function getBack in .then instead of result of that function
    (cherry picked from commit b6a99da8ab8565bfa96ccc84aa170bdc5826a012)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1441658

@ZitaNemeckova ZitaNemeckova deleted the bz1437373 branch September 12, 2017 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants