Fix bug in voice command update operation

* VCUO now properly returns an error
* VCUO now properly removes commands that failed
* Added unit tests for VCUO

3 files changed, 137 insertions, 19 deletions

diff --git a/SmartDeviceLink/SDLVoiceCommandUpdateOperation.m b/SmartDeviceLink/SDLVoiceCommandUpdateOperation.m
index 00af39062..fcab811b8 100644
--- a/SmartDeviceLink/SDLVoiceCommandUpdateOperation.m
+++ b/SmartDeviceLink/SDLVoiceCommandUpdateOperation.m
@@ -195,7 +195,14 @@ NS_ASSUME_NONNULL_BEGIN
 
 + (NSArray<SDLDeleteCommand *> *)sdl_deleteCommands:(NSArray<SDLDeleteCommand *> *)deleteCommands subtractDeleteCommands:(NSArray<SDLDeleteCommand *> *)subtractCommands {
     NSMutableArray<SDLDeleteCommand *> *mutableDeleteCommands = deleteCommands.mutableCopy;
-    [mutableDeleteCommands removeObjectsInArray:subtractCommands];
+    for (SDLDeleteCommand *subtractCommand in subtractCommands) {
+        for (SDLDeleteCommand *deleteCommand in mutableDeleteCommands) {
+            if ([subtractCommand.cmdID isEqualToNumber:deleteCommand.cmdID]) {
+                [mutableDeleteCommands removeObject:deleteCommand];
+                break;
+            }
+        }
+    }
 
     return [mutableDeleteCommands copy];
 }
@@ -226,7 +233,7 @@ NS_ASSUME_NONNULL_BEGIN
         self.internalError = [NSError sdl_voiceCommandManager_pendingUpdateSuperseded];
         self.completionHandler(self.currentVoiceCommands, self.error);
     } else {
-        self.completionHandler(self.currentVoiceCommands, nil);
+        self.completionHandler(self.currentVoiceCommands, self.internalError);
     }
 
     [super finishOperation];
diff --git a/SmartDeviceLinkTests/DevAPISpecs/SDLVoiceCommandOperationSpec.m b/SmartDeviceLinkTests/DevAPISpecs/SDLVoiceCommandOperationSpec.m
index 6def3969e..a5186992d 100644
--- a/SmartDeviceLinkTests/DevAPISpecs/SDLVoiceCommandOperationSpec.m
+++ b/SmartDeviceLinkTests/DevAPISpecs/SDLVoiceCommandOperationSpec.m
@@ -6,6 +6,7 @@
 #import "SDLAddCommandResponse.h"
 #import "SDLDeleteCommand.h"
 #import "SDLDeleteCommandResponse.h"
+#import "SDLError.h"
 #import "SDLFileManager.h"
 #import "SDLHMILevel.h"
 #import "SDLVoiceCommand.h"
@@ -21,19 +22,41 @@
 
 QuickSpecBegin(SDLVoiceCommandOperationSpec)
 
+__block SDLDeleteCommandResponse *successDelete = nil;
+__block SDLDeleteCommandResponse *failDelete = nil;
+__block SDLAddCommandResponse *successAdd = nil;
+__block SDLAddCommandResponse *failAdd = nil;
+
+__block SDLVoiceCommand *newVoiceCommand1 = [[SDLVoiceCommand alloc] initWithVoiceCommands:@[@"NewVC1"] handler:^{}];
+__block SDLVoiceCommand *newVoiceCommand2 = [[SDLVoiceCommand alloc] initWithVoiceCommands:@[@"NewVC2"] handler:^{}];
+__block SDLVoiceCommand *oldVoiceCommand1 = [[SDLVoiceCommand alloc] initWithVoiceCommands:@[@"OldVC1"] handler:^{}];
+__block SDLVoiceCommand *oldVoiceCommand2 = [[SDLVoiceCommand alloc] initWithVoiceCommands:@[@"OldVC2"] handler:^{}];
+
+
 describe(@"a voice command operation", ^{
     __block TestConnectionManager *testConnectionManager = nil;
     __block SDLVoiceCommandUpdateOperation *testOp = nil;
 
-    __block SDLVoiceCommand *newVoiceCommand1 = [[SDLVoiceCommand alloc] initWithVoiceCommands:@[@"NewVC1"] handler:^{}];
-    __block SDLVoiceCommand *newVoiceCommand2 = [[SDLVoiceCommand alloc] initWithVoiceCommands:@[@"NewVC2"] handler:^{}];
-    __block SDLVoiceCommand *oldVoiceCommand1 = [[SDLVoiceCommand alloc] initWithVoiceCommands:@[@"OldVC1"] handler:^{}];
-    __block SDLVoiceCommand *oldVoiceCommand2 = [[SDLVoiceCommand alloc] initWithVoiceCommands:@[@"OldVC2"] handler:^{}];
-
     __block NSError *callbackError = nil;
     __block NSArray<SDLVoiceCommand *> *callbackCurrentVoiceCommands = nil;
 
     beforeEach(^{
+        successDelete = [[SDLDeleteCommandResponse alloc] init];
+        successDelete.success = @YES;
+        successDelete.resultCode = SDLResultSuccess;
+
+        failDelete = [[SDLDeleteCommandResponse alloc] init];
+        failDelete.success = @NO;
+        failDelete.resultCode = SDLResultDisallowed;
+
+        successAdd = [[SDLAddCommandResponse alloc] init];
+        successAdd.success = @YES;
+        successAdd.resultCode = SDLResultSuccess;
+
+        failAdd = [[SDLAddCommandResponse alloc] init];
+        failAdd.success = @NO;
+        failAdd.resultCode = SDLResultDisallowed;
+
         newVoiceCommand1.commandId = 1;
         newVoiceCommand2.commandId = 2;
         oldVoiceCommand1.commandId = 3;
@@ -80,15 +103,17 @@ describe(@"a voice command operation", ^{
             });
         });
 
+        // if it has voice commands to delete
         context(@"if it has voice commands to delete", ^{
             beforeEach(^{
-                testOp = [[SDLVoiceCommandUpdateOperation alloc] initWithConnectionManager:testConnectionManager newVoiceCommands:@[newVoiceCommand1, newVoiceCommand2] oldVoiceCommands:@[oldVoiceCommand1, oldVoiceCommand2] updateCompletionHandler:^(NSArray<SDLVoiceCommand *> * _Nonnull newCurrentVoiceCommands, NSError * _Nullable error) {
+                testOp = [[SDLVoiceCommandUpdateOperation alloc] initWithConnectionManager:testConnectionManager newVoiceCommands:@[] oldVoiceCommands:@[oldVoiceCommand1, oldVoiceCommand2] updateCompletionHandler:^(NSArray<SDLVoiceCommand *> * _Nonnull newCurrentVoiceCommands, NSError * _Nullable error) {
                     callbackCurrentVoiceCommands = newCurrentVoiceCommands;
                     callbackError = error;
                 }];
                 [testOp start];
             });
 
+            // and the delete succeeds
             context(@"and the delete succeeds", ^{
                 beforeEach(^{
                     SDLDeleteCommandResponse *deleteOld1 = [[SDLDeleteCommandResponse alloc] init];
@@ -101,35 +126,121 @@ describe(@"a voice command operation", ^{
 
                     [testConnectionManager respondToRequestWithResponse:deleteOld1 requestNumber:0 error:nil];
                     [testConnectionManager respondToRequestWithResponse:deleteOld2 requestNumber:1 error:nil];
+                    [testConnectionManager respondToLastMultipleRequestsWithSuccess:YES];
                 });
 
-                fit(@"should update the current voice commands and attempt to send the adds", ^{
-                    expect(testOp.currentVoiceCommands).to(haveCount(0));
-                    expect(testConnectionManager.receivedRequests).to(haveCount(4));
+                it(@"should update the current voice commands", ^{
+                    expect(callbackCurrentVoiceCommands).to(haveCount(0));
+                    expect(callbackError).to(beNil());
+                    expect(testConnectionManager.receivedRequests).to(haveCount(2));
                 });
             });
 
+            // and the delete fails
             context(@"and the delete fails", ^{
+                beforeEach(^{
+                    [testConnectionManager respondToRequestWithResponse:failDelete requestNumber:0 error:[NSError sdl_lifecycle_failedWithBadResult:SDLResultDisallowed info:nil]];
+                    [testConnectionManager respondToRequestWithResponse:failDelete requestNumber:1 error:[NSError sdl_lifecycle_failedWithBadResult:SDLResultDisallowed info:nil]];
+                    [testConnectionManager respondToLastMultipleRequestsWithSuccess:NO];
+                });
 
+                it(@"should update the current voice commands and attempt to send the adds", ^{
+                    expect(callbackCurrentVoiceCommands).to(haveCount(2));
+                    expect(callbackError).toNot(beNil());
+                    expect(testConnectionManager.receivedRequests).to(haveCount(2));
+                });
             });
 
+            // and the delete partially fails
             context(@"and the delete partially fails", ^{
+                beforeEach(^{
+                    [testConnectionManager respondToRequestWithResponse:failDelete requestNumber:0 error:[NSError sdl_lifecycle_failedWithBadResult:SDLResultDisallowed info:nil]];
+                    [testConnectionManager respondToRequestWithResponse:successDelete requestNumber:1 error:nil];
+                    [testConnectionManager respondToLastMultipleRequestsWithSuccess:NO];
+                });
 
+                it(@"should update the current voice commands and attempt to send the adds", ^{
+                    expect(callbackCurrentVoiceCommands).to(haveCount(1));
+                    expect(callbackError).toNot(beNil());
+                    expect(testConnectionManager.receivedRequests).to(haveCount(2));
+                });
             });
         });
 
         context(@"if it doesn't have any voice commands to delete", ^{
+            beforeEach(^{
+                testOp = [[SDLVoiceCommandUpdateOperation alloc] initWithConnectionManager:testConnectionManager newVoiceCommands:@[newVoiceCommand1, newVoiceCommand2] oldVoiceCommands:@[] updateCompletionHandler:^(NSArray<SDLVoiceCommand *> * _Nonnull newCurrentVoiceCommands, NSError * _Nullable error) {
+                    callbackCurrentVoiceCommands = newCurrentVoiceCommands;
+                    callbackError = error;
+                }];
+                [testOp start];
+            });
+
             context(@"adding voice commands", ^{
                 context(@"and the add succeeds", ^{
-
+                    beforeEach(^{
+                        SDLAddCommandResponse *addNew1 = [[SDLAddCommandResponse alloc] init];
+                        addNew1.success = @YES;
+                        addNew1.resultCode = SDLResultSuccess;
+
+                        SDLAddCommandResponse *addNew2 = [[SDLAddCommandResponse alloc] init];
+                        addNew2.success = @YES;
+                        addNew2.resultCode = SDLResultSuccess;
+
+                        [testConnectionManager respondToRequestWithResponse:addNew1 requestNumber:0 error:nil];
+                        [testConnectionManager respondToRequestWithResponse:addNew2 requestNumber:1 error:nil];
+                        [testConnectionManager respondToLastMultipleRequestsWithSuccess:YES];
+                    });
+
+                    it(@"should update the current voice commands", ^{
+                        expect(callbackCurrentVoiceCommands).to(haveCount(2));
+                        expect(callbackError).to(beNil());
+                        expect(testConnectionManager.receivedRequests).to(haveCount(2));
+                    });
                 });
 
                 context(@"and the add fails", ^{
-
+                    beforeEach(^{
+                        SDLAddCommandResponse *addNew1 = [[SDLAddCommandResponse alloc] init];
+                        addNew1.success = @NO;
+                        addNew1.resultCode = SDLResultDisallowed;
+
+                        SDLAddCommandResponse *addNew2 = [[SDLAddCommandResponse alloc] init];
+                        addNew2.success = @NO;
+                        addNew2.resultCode = SDLResultDisallowed;
+
+                        [testConnectionManager respondToRequestWithResponse:addNew1 requestNumber:0 error:[NSError sdl_lifecycle_failedWithBadResult:SDLResultDisallowed info:nil]];
+                        [testConnectionManager respondToRequestWithResponse:addNew2 requestNumber:1 error:[NSError sdl_lifecycle_failedWithBadResult:SDLResultDisallowed info:nil]];
+                        [testConnectionManager respondToLastMultipleRequestsWithSuccess:NO];
+                    });
+
+                    it(@"should update the current voice commands", ^{
+                        expect(callbackCurrentVoiceCommands).to(haveCount(0));
+                        expect(callbackError).toNot(beNil());
+                        expect(testConnectionManager.receivedRequests).to(haveCount(2));
+                    });
                 });
 
                 context(@"and the add partially fails", ^{
-
+                    beforeEach(^{
+                        SDLAddCommandResponse *addNew1 = [[SDLAddCommandResponse alloc] init];
+                        addNew1.success = @NO;
+                        addNew1.resultCode = SDLResultDisallowed;
+
+                        SDLAddCommandResponse *addNew2 = [[SDLAddCommandResponse alloc] init];
+                        addNew2.success = @YES;
+                        addNew2.resultCode = SDLResultSuccess;
+
+                        [testConnectionManager respondToRequestWithResponse:addNew1 requestNumber:0 error:[NSError sdl_lifecycle_failedWithBadResult:SDLResultDisallowed info:nil]];
+                        [testConnectionManager respondToRequestWithResponse:addNew2 requestNumber:1 error:nil];
+                        [testConnectionManager respondToLastMultipleRequestsWithSuccess:NO];
+                    });
+
+                    it(@"should update the current voice commands", ^{
+                        expect(callbackCurrentVoiceCommands).to(haveCount(1));
+                        expect(callbackError).toNot(beNil());
+                        expect(testConnectionManager.receivedRequests).to(haveCount(2));
+                    });
                 });
             });
         });
diff --git a/SmartDeviceLinkTests/TestUtilities/TestConnectionManager.m b/SmartDeviceLinkTests/TestUtilities/TestConnectionManager.m
index ded122d0a..03b7398c2 100644
--- a/SmartDeviceLinkTests/TestUtilities/TestConnectionManager.m
+++ b/SmartDeviceLinkTests/TestUtilities/TestConnectionManager.m
@@ -48,11 +48,11 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (void)sendRequests:(nonnull NSArray<SDLRPCRequest *> *)requests progressHandler:(nullable SDLMultipleAsyncRequestProgressHandler)progressHandler completionHandler:(nullable SDLMultipleRequestCompletionHandler)completionHandler {
     [requests enumerateObjectsUsingBlock:^(SDLRPCRequest * _Nonnull request, NSUInteger idx, BOOL * _Nonnull stop) {
-        [self sendConnectionRequest:request withResponseHandler:nil];
-
-        if (progressHandler != nil) {
-            progressHandler(request, nil, nil, (double)idx / (double)requests.count);
-        }
+        [self sendConnectionRequest:request withResponseHandler:^(__kindof SDLRPCRequest * _Nullable request, __kindof SDLRPCResponse * _Nullable response, NSError * _Nullable error) {
+            if (progressHandler != nil) {
+                progressHandler(request, response, error, (double)idx / (double)requests.count);
+            }
+        }];
     }];
 
     [_multipleCompletionBlocks addObject:completionHandler];