[Payment Request] Incomplete item shouldn't appear as selected when tapped.

ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/chrome/browser/ui/payments/payment_request_selector_view_controller.h"
 
 #include "base/mac/foundation_util.h"
 #include "components/strings/grit/components_strings.h"
 #import "ios/chrome/browser/ui/autofill/cells/status_item.h"
 #import "ios/chrome/browser/ui/collection_view/cells/MDCCollectionViewCell+Chrome.h"
@@ skipping 243 matching lines @@
             (paymentRequestSelectorViewController:didSelectItemAtIndexForEditing
                                                     :)]) {
       [self.delegate paymentRequestSelectorViewController:self
                            didSelectItemAtIndexForEditing:index];
     }
     return;
   }
 
   switch (item.type) {
     case ItemTypeSelectableItem: {
-      // Update the currently selected cell, if any.
-      if (self.dataSource.selectedItemIndex != NSUIntegerMax) {
-        DCHECK(self.dataSource.selectedItemIndex <
-               [[self.dataSource selectableItems] count]);
-        CollectionViewItem<PaymentsIsSelectable>* oldSelectedItem =
-            [[self.dataSource selectableItems]
-                objectAtIndex:self.dataSource.selectedItemIndex];
-        oldSelectedItem.accessoryType = MDCCollectionViewCellAccessoryNone;
-        [self reconfigureCellsForItems:@[ oldSelectedItem ]];
-      }
-
-      // Update the newly selected cell.
       CollectionViewItem<PaymentsIsSelectable>* newSelectedItem =
           reinterpret_cast<CollectionViewItem<PaymentsIsSelectable>*>(item);
-      newSelectedItem.accessoryType = MDCCollectionViewCellAccessoryCheckmark;
-      [self reconfigureCellsForItems:@[ newSelectedItem ]];
-
+      // Update the currently selected and the newly selected cells only if the
+      // newly selected cell corresponds to a complete item and can be selected.
+      if (newSelectedItem.isComplete) {
+        // Update the currently selected cell, if any.
+        if (self.dataSource.selectedItemIndex != NSUIntegerMax) {
+          DCHECK(self.dataSource.selectedItemIndex <
+                 [[self.dataSource selectableItems] count]);
+          CollectionViewItem<PaymentsIsSelectable>* oldSelectedItem =
+              [[self.dataSource selectableItems]
+                  objectAtIndex:self.dataSource.selectedItemIndex];
+          oldSelectedItem.accessoryType = MDCCollectionViewCellAccessoryNone;
+          [self reconfigureCellsForItems:@[ oldSelectedItem ]];
+        }
+        // Update the newly selected cell.
+        newSelectedItem.accessoryType = MDCCollectionViewCellAccessoryCheckmark;
+        [self reconfigureCellsForItems:@[ newSelectedItem ]];
+      }
       // Notify the delegate of the selection.
       NSUInteger index =
           [self.collectionViewModel indexInItemTypeForIndexPath:indexPath];
       DCHECK(index < [[self.dataSource selectableItems] count]);
       [self.delegate paymentRequestSelectorViewController:self
                                      didSelectItemAtIndex:index];

[macourteau, 2017/06/27 15:27:12]

Do we still want to notify the delegate of selection even if the item wasn't
selected? Would it be worth adding another message, like
"didSelectIncompleteItemAtIndex:"?

[Moe, 2017/06/28 01:57:44]

I see your point. I think this call is fine though. The user is really selecting
the item. It's the coordinator that then decides if the item is good enough for
selection or whether it should be edited. But of course the line is blurry
because that information is also needed for display purposes.

[macourteau, 2017/06/28 13:59:14]

Ack. I still feel that the state is confusing: delegate is notified of
selection, but the controller does not consider the item selected. It might make
sense now that the code is "fresh", but that might be confusing if we end up
debugging this code again in e.g. 6 months.

Maybe add a parameter to the
paymentRequestSelectorViewController:didSelectItemAtIndex: selector, e.g.
"selectionIsValid:"? Even if that value was unused at the moment, I think it
would make sense, and would make it easy for the delegate to decide to e.g.
prompt the user for the missing data, to allow selection of this item.

My $0.02 though, you are more familiar with this code, so feel free to ignore my
suggestions if they don't make sense.

       break;
     }
     case ItemTypeAddItem: {
       if ([self.delegate
               respondsToSelector:@selector
               (paymentRequestSelectorViewControllerDidSelectAddItem:)]) {
         [self.delegate
             paymentRequestSelectorViewControllerDidSelectAddItem:self];
       }
       break;
@@ skipping 24 matching lines @@
     hidesInkViewAtIndexPath:(NSIndexPath*)indexPath {
   NSInteger type = [self.collectionViewModel itemTypeForIndexPath:indexPath];
   if (type == ItemTypeHeader) {
     return YES;
   } else {
     return NO;
   }
 }
 
 @end