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.

Sign up for GitHub

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

Jump to bottom

Fixed Error handling of REST API calls. #490

Merged
merged 2 commits into from
Mar 6, 2017
Merged

Fixed Error handling of REST API calls. #490

merged 2 commits into from
Mar 6, 2017

Conversation

h-kataria
Copy link
Contributor

@h-kataria h-kataria commented Feb 27, 2017 •
edited
Loading

@mkanoor @gmcculloug please test.

rest_api_error_handling

@h-kataria h-kataria force-pushed the rest_api_error_handling_fix branch 2 times, most recently from 1d631db to 04a2c53 Compare February 27, 2017 14:08
@gmcculloug
Copy link
Member

@mkanoor Are you aware of this issue or how to test/validate this fix?

AparnaKarve
AparnaKarve reviewed Feb 28, 2017
View reviewed changes
app/assets/javascripts/services/post_service.js Outdated
} else {
handleSuccess;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

@h-kataria Can you leverage handleFailure function in miqService here?
Check how it's being used in the other controllers.

@h-kataria
Copy link
Contributor Author

@AparnaKarve please review.

@mkanoor
Copy link
Contributor

mkanoor commented Feb 28, 2017

@gmcculloug
The way I was testing I would set the Authentication for the provider to be invalid and it would return a nil when locating the newly created service template.
@bdunne fixed his code ManageIQ/manageiq#14049 which was using a find_by instead of find_by! and returning a nil.

To test just the API error we might have to run without @bdunne 's changes

@h-kataria
Copy link
Contributor Author

@mkanoor i used ManageIQ/manageiq#14051 to mock data and help me recreate the issue.

@mkanoor
Copy link
Contributor

mkanoor commented Feb 28, 2017

@h-kataria @gmcculloug
Tested for the error condition and it works as expected.
👍

@h-kataria
Copy link
Contributor Author

@AparnaKarve please re-review.

@AparnaKarve
Copy link
Contributor

@h-kataria As we discussed, can you change postService, specifically the handleSuccess function as shown below. Also, all miqService changes can be completely undone.

function handleSuccess(response) {
  $timeout(function () {
     if (response.error) {
       var msg = __(response.error.klass + ': ' + response.error.message);
       miqService.miqFlash('error', msg);
       miqService.sparkleOff();
     } else {
       $window.location.href = redirectURL + '&flash_msg=' + successMsg;
     }
  });
}

I also recommend using .catch(miqService.handleFailure); for all API.post calls to catch exceptions. Thanks.

@h-kataria h-kataria force-pushed the rest_api_error_handling_fix branch from 21b5e96 to 65f56b9 Compare March 3, 2017 22:55
@h-kataria
Copy link
Contributor Author

@AparnaKarve made suggested changes, please re-review.

AparnaKarve
AparnaKarve reviewed Mar 3, 2017
View reviewed changes
app/assets/javascripts/services/miq_service.js Outdated
@@ -124,7 +124,7 @@ ManageIQ.angular.app.service('miqService', ['$timeout', '$document', '$q', '$log

return serializedObj;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this blank line? Since there are no real changes here, let's not include this JS in this PR at all.

AparnaKarve
AparnaKarve reviewed Mar 3, 2017
View reviewed changes
app/assets/javascripts/services/post_service.js Outdated
@@ -6,18 +6,18 @@ ManageIQ.angular.app.service('postService', ["miqService", "$timeout", "$window"
angular.toJson({
action: "edit",
resource: updateObject
})).then(handleSuccess, handleFailure);
})).then(handleSuccess, miqService.handleFailure)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to simply --

.then(handleSuccess)

No need for miqService.handleFailure in .then
(Sorry, I should have specified that before)

And the same change below.

@h-kataria h-kataria force-pushed the rest_api_error_handling_fix branch 2 times, most recently from e56c059 to b36de98 Compare March 5, 2017 00:37
@h-kataria h-kataria force-pushed the rest_api_error_handling_fix branch from b36de98 to 66581fc Compare March 5, 2017 00:38
@miq-bot
Copy link
Member

miq-bot commented Mar 5, 2017

Checked commits h-kataria/manageiq-ui-classic@6f29c20~...66581fc with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks good. 👍

@h-kataria
Copy link
Contributor Author

@AparnaKarve made suggested changes

@AparnaKarve
Copy link
Contributor

Thanks @h-kataria.
I've already tested this in the UI and should be good to go!

@h-kataria
Copy link
Contributor Author

@dclarizio can you please merge.

@dclarizio dclarizio self-assigned this Mar 6, 2017
@dclarizio dclarizio merged commit c14ea0f into ManageIQ:master Mar 6, 2017
@dclarizio dclarizio added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 6, 2017
@h-kataria h-kataria deleted the rest_api_error_handling_fix branch March 8, 2017 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AparnaKarve AparnaKarve AparnaKarve left review comments

Assignees

@dclarizio dclarizio

Labels
bug services
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@h-kataria @gmcculloug @mkanoor @AparnaKarve @miq-bot @dclarizio