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
Catch 400 error and display it in flash message in GOD #4951
Merged
himdel
merged 4 commits into
ManageIQ:master
from
ZitaNemeckova:fix_error_message_god_button
Dec 11, 2018
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
55d7178
Catch 400 error and display it in flash message instead of modal with…
ZitaNemeckova 4e4ab87
Add specs and fix Promise handling
ZitaNemeckova 74e4143
Add missing logic for new button in a group, delete saveWithAPI that …
ZitaNemeckova 2035006
Clear previous messages, add miqSparkleOff to prevent infinite sparkle
ZitaNemeckova File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -125,13 +125,34 @@ function mainCustomButtonFormController(API, miqService, $q, $http) { | |
}; | ||
|
||
vm.saveClicked = function() { | ||
miqService.sparkleOn(); | ||
miqService.miqFlashClear(); // remove previous messages | ||
var saveMsg = sprintf(__('%s "%s" has been successfully saved.'), vm.entity, vm.customButtonModel.name); | ||
vm.saveWithAPI('put', '/api/custom_buttons/' + vm.customButtonRecordId, vm.prepSaveObject(), saveMsg); | ||
himdel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return API.put('/api/custom_buttons/' + vm.customButtonRecordId, vm.prepSaveObject(), {skipErrors: [400]}) | ||
.then(function() { | ||
miqService.redirectBack(saveMsg, 'success', vm.redirectUrl); | ||
}) | ||
.catch(handleErrorMessages); | ||
}; | ||
|
||
vm.addClicked = function() { | ||
miqService.sparkleOn(); | ||
miqService.miqFlashClear(); // remove previous messages | ||
var saveMsg = sprintf(__('%s "%s" has been successfully added.'), vm.entity, vm.customButtonModel.name); | ||
vm.saveWithAPI('post', '/api/custom_buttons/', vm.prepSaveObject(), saveMsg); | ||
return API.post('/api/custom_buttons/', vm.prepSaveObject(), {skipErrors: [400]}) | ||
.then(function(response) { | ||
if (vm.customButtonGroupRecordId) { | ||
var saveMsgBtnInGrp = sprintf(__('%s "%s" has been successfully added under the selected button group.'), vm.entity, vm.customButtonModel.name); | ||
return $http.post('/generic_object_definition/add_button_in_group/' + vm.customButtonGroupRecordId + '?button_id=' + response.results[0].id) | ||
.then(function() { | ||
miqService.redirectBack(saveMsgBtnInGrp, 'success', vm.redirectUrl); | ||
}) | ||
.catch(miqService.handleFailure); | ||
} else { | ||
miqService.redirectBack(saveMsg, 'success', vm.redirectUrl); | ||
} | ||
}) | ||
.catch(handleErrorMessages); | ||
}; | ||
|
||
vm.prepSaveObject = function() { | ||
|
@@ -184,26 +205,27 @@ function mainCustomButtonFormController(API, miqService, $q, $http) { | |
}; | ||
}; | ||
|
||
vm.saveWithAPI = function(method, url, saveObject, saveMsg) { | ||
miqService.sparkleOn(); | ||
|
||
if (vm.customButtonGroupRecordId) { | ||
var saveCustomButtonPromise = API[method](url, saveObject); | ||
var saveMsgBtnInGrp = sprintf(__('%s "%s" has been successfully added under the selected button group.'), vm.entity, vm.customButtonModel.name); | ||
|
||
saveCustomButtonPromise.then(function(response) { | ||
$http.post('/generic_object_definition/add_button_in_group/' + vm.customButtonGroupRecordId + '?button_id=' + response.results[0].id) | ||
.then(miqService.redirectBack.bind(vm, saveMsgBtnInGrp, 'success', vm.redirectUrl)) | ||
.catch(miqService.handleFailure); | ||
// private functions | ||
function handleErrorMessages(error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing it never stops sparkling on error |
||
miqSparkleOff(); | ||
if (error.status === 400) { | ||
var errorMessages = error.data.error.message.split(','); | ||
errorMessages.forEach(function(message) { | ||
if (message.includes("Name has already been taken")) { | ||
add_flash(__("Name has already been taken"), "error"); | ||
return Promise.reject(); | ||
} else if (message.includes("Description has already been taken")) { | ||
add_flash(__("Description has already been taken"), "error"); | ||
return Promise.reject(); | ||
} else { | ||
return miqService.handleFailure(); | ||
} | ||
}); | ||
} else { | ||
API[method](url, saveObject) | ||
.then(miqService.redirectBack.bind(vm, saveMsg, 'success', vm.redirectUrl)) | ||
.catch(miqService.handleFailure); | ||
return miqService.handleFailure(); | ||
} | ||
}; | ||
} | ||
|
||
// private functions | ||
function getCustomButtonFormData(response) { | ||
Object.assign(vm.customButtonModel, response); | ||
|
||
|
70 changes: 70 additions & 0 deletions
70
spec/javascripts/components/generic_object_definition/main-custom-button-form_spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
describe('main-custom-button-form-component', function() { | ||
var $componentController, vm, miqService, API, $scope; | ||
beforeEach(module('ManageIQ')); | ||
beforeEach(inject(function (_$componentController_, _API_, _$httpBackend_, _miqService_, $rootScope, $q) { | ||
$componentController = _$componentController_; | ||
API = _API_; | ||
miqService = _miqService_; | ||
$scope = $rootScope.$new(); | ||
$httpBackend = _$httpBackend_; | ||
|
||
var bindings = {genericObjectDefinitionRecordId: '1', customButtonGroupRecordId: '2', customButtonRecordId: '3', redirectUrl: '/redirect/url'}; | ||
vm = $componentController("mainCustomButtonFormComponent", null, bindings); | ||
var deferred = $q.defer(); | ||
spyOn(API, 'get').and.callFake(function() {return deferred.promise;}); | ||
var domainsResponse = { | ||
data:{ | ||
distinct_instances_across_domains: ["Automation", "Event", "GenericObject", "MiqEvent", "Request", "parse_automation_request", "parse_event_stream", "parse_provider_category"] | ||
} | ||
}; | ||
$httpBackend.whenGET('/generic_object_definition/retrieve_distinct_instances_across_domains').respond(domainsResponse); | ||
vm.$onInit(); | ||
$httpBackend.flush(); | ||
})); | ||
|
||
describe('#saveClick', function() { | ||
beforeEach(function(){ | ||
spyOn(window, 'add_flash'); | ||
}); | ||
|
||
it('catch 400 and call add_flash if error is name/description taken', function(done) { | ||
var rejectionData = {status: 400, data: {error: {message: "Name has already been taken, Description has already been taken"}}}; | ||
spyOn(API, 'put').and.returnValue(Promise.reject(rejectionData)); | ||
vm.saveClicked() | ||
.then( function() { | ||
expect(add_flash).toHaveBeenCalledWith("Name has already been taken", "error"); | ||
expect(add_flash).toHaveBeenCalledWith("Description has already been taken", "error"); | ||
done(); | ||
}) | ||
.catch(function() { | ||
expect(add_flash).toHaveBeenCalledWith("Name has already been taken", "error"); | ||
expect(add_flash).toHaveBeenCalledWith("Description has already been taken", "error"); | ||
|
||
done(); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('#addClick', function() { | ||
beforeEach(function(){ | ||
spyOn(window, 'add_flash'); | ||
spyOn(miqService, 'handleFailure').and.callFake(function() {debugger;return Promise.reject()}); | ||
}); | ||
|
||
it('catch 400 and call add_flash', function(done) { | ||
var rejectionData = {status: 400, data: {error: {message: "Name has already been taken, Description has already been taken"}}}; | ||
spyOn(API, 'post').and.returnValue(Promise.reject(rejectionData)); | ||
vm.addClicked() | ||
.then( function() { | ||
expect(add_flash).toHaveBeenCalledWith("Name has already been taken", "error"); | ||
expect(add_flash).toHaveBeenCalledWith("Description has already been taken", "error"); | ||
done(); | ||
}) | ||
.catch(function () { | ||
expect(add_flash).toHaveBeenCalledWith("Name has already been taken", "error"); | ||
expect(add_flash).toHaveBeenCalledWith("Description has already been taken", "error"); | ||
done(); | ||
}); | ||
}); | ||
}); | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
..and we probably need
miqService.miqFlashClear();
in both saveClicked and addClicked.Because otherwise:
fail the validation with duplicate name
=> Name has already been taken appears
fix the name (and also edit the description)
fail the validation with duplicate desciption
=> Desciption has already been taken appears
you're seeing both flash messages, but one is obsolete now
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.
(or you could just replace
add_flash
withmiqService.miqFlash
, but that would not work when both are supposed to be shown, so you'd have to merge the messages into one)