Fix bug that causes DS requests to fulfill immediately while other requests are still open#3727
Conversation
|
Can one of the admins verify this patch? |
mitchell852
added
Traffic Portal v1
| function(dsRequests) { | ||
| // search all requests for those that are not completed and share the same deliveryservice id | ||
| angular.forEach(dsRequests, function(value) { | ||
| if (value.status != 'complete' && value.deliveryService.id == deliveryService.id) { |
There was a problem hiding this comment.
you should short circuit this condition by putting this as the first check as it will fail in most instances and then the 2nd check is never evaluated (might get you a little performance for free)
if (value.deliveryService.id == deliveryService.id && value.status != 'complete') {
There was a problem hiding this comment.
also, if you rename the function something like hasOpenRequest and check for the statuses we know to be open (draft, submitted or pending), i think your other condition will read a little nicer
if (options.status.id == $scope.COMPLETE && !hasOpenRequest(deliveryService))
the reason i mention this is because this ds request code is a bit of a mess already as i'm sure you noticed, so easy to understand naming will make everyone's life easier.
not sure if that's a standard way in the industry to name boolean methods or if it's a java thing but i always thought it rolled off the tongue nicely - https://stackoverflow.com/questions/3874350/naming-conventions-for-java-methods-that-return-booleanno-question-mark
There was a problem hiding this comment.
so i've been thinking about this a bit more. the way you are performing this check is rather expensive. you are fetching all the DS requests (open or closed) and then looping thru them to determine if there is an open one for this ds when instead this problem could easily be solved by fixing the order of operations. check out the current code:
if (options.status.id == $scope.COMPLETE) {
deliveryServiceService.updateDeliveryService(deliveryService).
then(
function() {
$state.reload(); // reloads all the resolves for the view
messageModel.setMessages([ { level: 'success', text: 'Delivery Service [ ' + deliveryService.xmlId + ' ] updated' } ], false);
createDeliveryServiceUpdateRequest(dsRequest, options.comment, true);
},
function(fault) {
$anchorScroll(); // scrolls window to top
messageModel.setMessages(fault.data.alerts, false);
}
);
} else {
createDeliveryServiceUpdateRequest(dsRequest, options.comment, false);
}
The order is simply backwards. Currently, it tries to update the ds (which succeeds) and THEN it tries to create a ds request (which fails because there is already one open).
So if you simply flip the order around and only make it update the ds IF the creation of the ds request succeeds, then problem solved.
There was a problem hiding this comment.
Thanks, that makes things a lot easier
| @@ -201,18 +197,16 @@ var FormEditDeliveryServiceController = function(deliveryService, origin, type, | |||
| // if the user chooses to complete/fulfill the update request immediately, the ds will be updated and behind the | |||
There was a problem hiding this comment.
can you rewrite this comment? it's not true anymore.
| @@ -61,44 +61,40 @@ var FormEditDeliveryServiceController = function(deliveryService, origin, type, | |||
|
|
|||
| // if the user chooses to complete/fulfill the delete request immediately, the ds will be deleted and behind the | |||
There was a problem hiding this comment.
can you update this comment as well. it's backwards.
|
@alexluckerman - is this the command you are running to run the single ds request UI test?
because i'm getting
|
|
@mitchell852 I've been using |
mitchell852
left a comment
mitchell852
left a comment
There was a problem hiding this comment.
- UI tests pass
- Manually tested
|
This change also fixed #3512 |
4 participants
I also made a small tweak to the DS requests TP test as it seemed to be failing locally for me because it wasn't waiting long enough for TP to change its URL
What does this PR (Pull Request) do?
Which Traffic Control components are affected by this PR?
This is a small bug that should not require documentation or a changelog entry
What is the best way to verify this PR?
Enable DS requests in TP and run the DS requests TP test, then manually verify that updating or deleting a DS while at least one DS request for that same DS is open fails
The following criteria are ALL met by this PR