Stop printer edit dialog from saving details when cancelled

- Add new Object property, pendingPrinter_, to cups_edit_printer_dialog.
  This object serves as a copy of activePrinter and is used as the main
  property for changes in edit printers dialog.
- Previously, activePrinter was being directly edited in the edit
  printer dialog. This resulted in activePrinter's property values
  to change even if the user clicks on the cancel button. The new
  pendingPrinter_ object fixes this by making a copy of activePrinter
  and only modifying activePrinter when the user clicks on the save
  button.
- Edit BrowserTests to reflect this change.

Bug: 893986
Test: End to End, BrowserTest/gtest_filter=CrSettingsPrinting*

Change-Id: Ia015fcda1e53dc8848556365d49c215492418d40
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1490378
Commit-Queue: Bailey Berro <baileyberro@chromium.org>
Reviewed-by: Bailey Berro <baileyberro@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#649146}

chrome/browser/resources/settings/printing_page/cups_edit_printer_dialog.html

@@ -18,7 +18,7 @@
         <div class="settings-box first two-line">
           <cr-input class="printer-name-input" autofocus
               id="printerName"
-              value="{{activePrinter.printerName}}"
+              value="{{pendingPrinter_.printerName}}"
               on-input="onPrinterInfoChange_"
               label="$i18n{printerName}"
               maxlength=64>
@@ -27,8 +27,8 @@
         <div class="settings-box two-line">
           <cr-input label="$i18n{printerAddress}"
               id="printerAddress"
-              value="{{activePrinter.printerAddress}}"
               on-input="onPrinterInfoChange_"
+              value="{{pendingPrinter_.printerAddress}}"
               disabled="[[!networkProtocolActive_]]"
               maxlength=128>
           </cr-input>
@@ -38,7 +38,7 @@
             <div id="printerProtocol" class="label">$i18n{printerProtocol}</div>
             <div class="secondary">
               <select class="md-select" aria-labelledby="printerProtocol"
-                  value="[[activePrinter.printerProtocol]]"
+                  value="[[pendingPrinter_.printerProtocol]]"
                   on-change="onProtocolChange_"
                   disabled="[[!networkProtocolActive_]]">
                 <option value="ipp" disabled="[[!networkProtocolActive_]]">
@@ -71,7 +71,7 @@
         </div>
         <div class="settings-box two-line">
           <cr-input id="printerQueue" label="$i18n{printerQueue}"
-              value="{{activePrinter.printerQueue}}"
+              value="{{pendingPrinter_.printerQueue}}"
               on-input="onPrinterInfoChange_"
               disabled="[[!networkProtocolActive_]]"
               maxlength=64>
@@ -79,21 +79,21 @@
         </div>
         <div class="settings-box two-line">
           <cr-input label="$i18n{printerURI}" readonly
-              value="[[getPrinterURI_(activePrinter)]]">
+              value="[[getPrinterURI_(pendingPrinter_)]]">
           </cr-input>
         </div>
         <div class="settings-box two-line">
           <cr-searchable-drop-down items="[[manufacturerList]]"
               id="printerPPDManufacturer"
               label="$i18n{printerManufacturer}"
-              value="{{activePrinter.ppdManufacturer}}">
+              value="{{pendingPrinter_.ppdManufacturer}}">
           </cr-searchable-drop-down>
         </div>
         <div class="settings-box two-line">
           <cr-searchable-drop-down items="[[modelList]]"
               id="printerPPDModel"
               label="$i18n{printerModel}"
-              value="{{activePrinter.ppdModel}}">
+              value="{{pendingPrinter_.ppdModel}}">
           </cr-searchable-drop-down>
         </div>
         <div class="settings-box two-line">
@@ -113,8 +113,8 @@
           $i18n{cancel}
         </paper-button>
         <paper-button class="action-button" on-click="onSaveTap_"
-            disabled="[[!canSavePrinter_(activePrinter.*,
-                        printerInfoChanged_)]]">
+            disabled="[[!canSavePrinter_(pendingPrinter_.*,
+                                         printerInfoChanged_)]]">
           $i18n{editPrinterButtonText}
         </paper-button>
       </div>

chrome/browser/resources/settings/printing_page/cups_edit_printer_dialog.js

@@ -15,11 +15,18 @@ Polymer({
   ],
 
   properties: {
-    /** @type {!CupsPrinterInfo} */
-    activePrinter: {
-      type: Object,
-      notify: true,
-    },
+    /**
+     * The currently saved printer.
+     * @type {CupsPrinterInfo}
+     */
+    activePrinter: Object,
+
+    /**
+     * Printer that holds the modified changes to activePrinter and only
+     * applies these changes when the save button is clicked.
+     * @type {CupsPrinterInfo}
+     */
+    pendingPrinter_: Object,
 
     /**
      * If the printer needs to be re-configured.
@@ -62,7 +69,7 @@ Polymer({
 
     networkProtocolActive_: {
       type: Boolean,
-      computed: 'isNetworkProtocol_(activePrinter.printerProtocol)',
+      computed: 'isNetworkProtocol_(pendingPrinter_.printerProtocol)',
     },
 
     /** @type {?Array<string>} */
@@ -88,15 +95,18 @@ Polymer({
   },
 
   observers: [
-    'printerPathChanged_(activePrinter.*)',
-    'selectedEditManufacturerChanged_(activePrinter.ppdManufacturer)',
-    'onModelChanged_(activePrinter.ppdModel)',
+    'printerPathChanged_(pendingPrinter_.*)',
+    'selectedEditManufacturerChanged_(pendingPrinter_.ppdManufacturer)',
+    'onModelChanged_(pendingPrinter_.ppdModel)',
   ],
 
   /** @override */
   attached: function() {
+    // Create a copy of activePrinter so that we can modify its fields.
+    this.pendingPrinter_ = /** @type{CupsPrinterInfo} */
+        (Object.assign({}, this.activePrinter));
     settings.CupsPrintersBrowserProxyImpl.getInstance()
-        .getPrinterPpdManufacturerAndModel(this.activePrinter.printerId)
+        .getPrinterPpdManufacturerAndModel(this.pendingPrinter_.printerId)
         .then(
             this.onGetPrinterPpdManufacturerAndModel_.bind(this),
             this.onGetPrinterPpdManufacturerAndModelFailed_.bind(this));
@@ -104,7 +114,7 @@ Polymer({
         .getCupsPrinterManufacturersList()
         .then(this.manufacturerListChanged_.bind(this));
     this.userPPD_ =
-        settings.printing.getBaseName(this.activePrinter.printerPPDPath);
+        settings.printing.getBaseName(this.pendingPrinter_.printerPPDPath);
   },
 
   /**
@@ -112,7 +122,7 @@ Polymer({
    * @private
    */
   printerPathChanged_: function(change) {
-    if (change.path != 'activePrinter.printerName') {
+    if (change.path != 'pendingPrinter_.printerName') {
       this.needsReconfigured_ = true;
     }
   },
@@ -122,7 +132,7 @@ Polymer({
    * @private
    */
   onProtocolChange_: function(event) {
-    this.set('activePrinter.printerProtocol', event.target.value);
+    this.set('pendingPrinter_.printerProtocol', event.target.value);
     this.onPrinterInfoChange_();
   },
 
@@ -138,6 +148,7 @@ Polymer({
 
   /** @private */
   onSaveTap_: function() {
+    this.updateActivePrinter_();
     if (this.needsReconfigured_) {
       settings.CupsPrintersBrowserProxyImpl.getInstance()
           .reconfigureCupsPrinter(this.activePrinter);
@@ -174,8 +185,8 @@ Polymer({
    * @private
    */
   onGetPrinterPpdManufacturerAndModel_: function(info) {
-    this.set('activePrinter.ppdManufacturer', info.ppdManufacturer);
-    this.set('activePrinter.ppdModel', info.ppdModel);
+    this.set('pendingPrinter_.ppdManufacturer', info.ppdManufacturer);
+    this.set('pendingPrinter_.ppdModel', info.ppdModel);
 
     // |needsReconfigured_| needs to reset to false after |ppdManufacturer| and
     // |ppdModel| are initialized to their correct values.
@@ -214,7 +225,7 @@ Polymer({
    */
   selectedEditManufacturerChanged_: function(manufacturer) {
     // Reset model if manufacturer is changed.
-    this.set('activePrinter.ppdModel', '');
+    this.set('pendingPrinter_.ppdModel', '');
     this.modelList = [];
     if (!!manufacturer && manufacturer.length != 0) {
       settings.CupsPrintersBrowserProxyImpl.getInstance()
@@ -249,9 +260,9 @@ Polymer({
       return;
     }
     this.manufacturerList = manufacturersInfo.manufacturers;
-    if (this.activePrinter.ppdManufacturer.length != 0) {
+    if (this.pendingPrinter_.ppdManufacturer.length != 0) {
       settings.CupsPrintersBrowserProxyImpl.getInstance()
-          .getCupsPrinterModelsList(this.activePrinter.ppdManufacturer)
+          .getCupsPrinterModelsList(this.pendingPrinter_.ppdManufacturer)
           .then(this.modelListChanged_.bind(this));
     }
   },
@@ -263,7 +274,7 @@ Polymer({
   modelListChanged_: function(modelsInfo) {
     if (modelsInfo.success) {
       this.modelList = modelsInfo.models;
-      // ModelListChanged_ is the final step of initializing activePrinter.
+      // ModelListChanged_ is the final step of initializing pendingPrinter.
       this.arePrinterFieldsInitialized_ = true;
     }
   },
@@ -273,7 +284,7 @@ Polymer({
    * @private
    */
   printerPPDPathChanged_: function(path) {
-    this.set('activePrinter.printerPPDPath', path);
+    this.set('pendingPrinter_.printerPPDPath', path);
     this.invalidPPD_ = !path;
     if (!this.invalidPPD_) {
       // A new valid PPD file should be treated as a saveable change.
@@ -287,9 +298,21 @@ Polymer({
    * @return {boolean}
    */
   isPrinterValid: function() {
-    return settings.printing.isNameAndAddressValid(this.activePrinter) &&
+    return settings.printing.isNameAndAddressValid(this.pendingPrinter_) &&
         settings.printing.isPPDInfoValid(
-            this.activePrinter.ppdManufacturer, this.activePrinter.ppdModel,
-            this.activePrinter.printerPPDPath);
+            this.pendingPrinter_.ppdManufacturer, this.pendingPrinter_.ppdModel,
+            this.pendingPrinter_.printerPPDPath);
+  },
+
+  /*
+   * Helper function to copy over modified fields to activePrinter.
+   * @private
+   */
+  updateActivePrinter_: function() {
+    this.activePrinter =
+        /** @type{CupsPrinterInfo} */ (Object.assign({}, this.pendingPrinter_));
+    // Set ppdModel since there is an observer that clears ppdmodel's value when
+    // ppdManufacturer changes.
+    this.activePrinter.ppdModel = this.pendingPrinter_.ppdModel;
   },
 });

chrome/test/data/webui/settings/cups_printer_page_tests.js

@@ -487,8 +487,22 @@ suite('EditPrinterDialog', function() {
   // Sets ppdManufacturer and ppdModel since ppdManufacturer has an observer
   // that erases ppdModel when ppdManufacturer changes.
   function setPpdManufacturerAndPpdModel(manufacturer, model) {
-    dialog.activePrinter.ppdManufacturer = manufacturer;
-    dialog.activePrinter.ppdModel = model;
+    dialog.pendingPrinter_.ppdManufacturer = manufacturer;
+    dialog.pendingPrinter_.ppdModel = model;
+  }
+
+  function clickSaveButton(dialog) {
+    assertTrue(!!dialog, 'Dialog is null for save');
+    const saveButton = dialog.$$('.action-button');
+    assertTrue(!!saveButton, 'Button is null');
+    saveButton.click();
+  }
+
+  function clickCancelButton(dialog) {
+    assertTrue(!!dialog, 'Dialog is null for cancel');
+    const cancelButton = dialog.$$('.cancel-button');
+    assertTrue(!!cancelButton, 'Button is null');
+    cancelButton.click();
   }
 
   /** @type {?settings.TestCupsPrintersBrowserProxy} */
@@ -503,7 +517,7 @@ suite('EditPrinterDialog', function() {
 
     dialog = document.createElement('settings-cups-edit-printer-dialog');
 
-    dialog.activePrinter = {
+    dialog.pendingPrinter_ = {
       ppdManufacturer: '',
       ppdModel: '',
       printerAddress: '',
@@ -537,7 +551,7 @@ suite('EditPrinterDialog', function() {
    * Test that USB printers can be editted.
    */
   test('USBPrinterCanBeEdited', function() {
-    dialog.activePrinter = {
+    dialog.pendingPrinter_ = {
       ppdManufacturer: '',
       ppdModel: '',
       printerAddress: '03f0/e414?serial=CD4234',
@@ -559,8 +573,8 @@ suite('EditPrinterDialog', function() {
       printerStatus: '',
     };
 
-    // Set activePrinter.ppdManufactuer and activePrinter.ppdModel to simulate
-    // a printer for which we have a PPD.
+    // Set pendingPrinter_.ppdManufactuer and pendingPrinter_.ppdModel to
+    // simulate a printer for which we have a PPD.
     setPpdManufacturerAndPpdModel('manufacturer', 'model');
 
     // Edit the printer name.
@@ -580,7 +594,7 @@ suite('EditPrinterDialog', function() {
    * invalid.
    */
   test('EditPrinter', function() {
-    dialog.activePrinter = {
+    dialog.pendingPrinter_ = {
       ppdManufacturer: '',
       ppdModel: '',
       printerAddress: '192.168.1.13',
@@ -628,8 +642,8 @@ suite('EditPrinterDialog', function() {
     assertTrue(saveButton.disabled);
   });
 
-  test('TestEditNameAndSave', function() {
-    dialog.activePrinter = {
+  test('CloseEditDialogDoesNotModifyActivePrinter', function() {
+    const expectedPrinter = {
       ppdManufacturer: '',
       ppdModel: '',
       printerAddress: 'test123',
@@ -650,6 +664,59 @@ suite('EditPrinterDialog', function() {
       printerQueue: 'moreinfohere',
       printerStatus: '',
     };
+
+    dialog.activePrinter = Object.assign({}, expectedPrinter);
+
+    const nameField = dialog.$$('.printer-name-input');
+    assertTrue(!!nameField);
+    nameField.value = 'edited printer name';
+
+    const addressField = dialog.$$('#printerAddress');
+    assertTrue(!!addressField);
+    addressField.value = '9.9.9.9';
+
+    const queueField = dialog.$$('#printerQueue');
+    assertTrue(!!queueField);
+    queueField.value = 'edited/print';
+
+    const protocolField = dialog.$$('.md-select');
+    assertTrue(!!protocolField);
+    protocolField.value = 'http';
+
+    clickCancelButton(dialog);
+
+    // Assert that activePrinter properties were not changed.
+    assertEquals(expectedPrinter.printerName, dialog.activePrinter.printerName);
+    assertEquals(
+        expectedPrinter.printerAddress, dialog.activePrinter.printerAddress);
+    assertEquals(
+        expectedPrinter.printerQueue, dialog.activePrinter.printerQueue);
+    assertEquals(
+        expectedPrinter.printerProtocol, dialog.activePrinter.printerProtocol);
+  });
+
+  test('TestEditNameAndSave', function() {
+    dialog.pendingPrinter_ = {
+      printerAutoconf: false,
+      printerDescription: '',
+      printerId: 'id_123',
+      printerManufacturer: '',
+      printerModel: '',
+      printerMakeAndModel: '',
+      printerName: 'Test Printer',
+      printerPPDPath: '',
+      printerPpdReference: {
+        userSuppliedPpdUrl: '',
+        effectiveMakeAndModel: '',
+        autoconf: false,
+      },
+      ppdManufacturer: '',
+      ppdModel: '',
+      printerAddress: '03f0/e414?serial=CD4234',
+      printerProtocol: 'usb',
+      printerQueue: 'moreinfohere',
+      printerStatus: '',
+    };
     setPpdManufacturerAndPpdModel('manufacture', 'model');
 
     // Initializing activePrinter will set |needsReconfigured_| to true. Reset
@@ -663,7 +730,6 @@ suite('EditPrinterDialog', function() {
 
     Polymer.dom.flush();
 
-    // Editing only the printer name results in a updateCupsPrinter.
     const saveButton = dialog.$$('.action-button');
     saveButton.click();
 
@@ -674,10 +740,10 @@ suite('EditPrinterDialog', function() {
   });
 
   test('TestEditFieldsAndSave', function() {
-    dialog.activePrinter = {
+    dialog.pendingPrinter_ = {
       ppdManufacturer: '',
       ppdModel: '',
-      printerAddress: 'test123',
+      printerAddress: '03f0/e414?serial=CD4234',
       printerAutoconf: false,
       printerDescription: '',
       printerId: 'id_123',
@@ -719,8 +785,7 @@ suite('EditPrinterDialog', function() {
     queueField.value = expectedQueue;
     assertTrue(dialog.needsReconfigured_);
 
-    const saveButton = dialog.$$('.action-button');
-    saveButton.click();
+    clickSaveButton(dialog);
 
     return cupsPrintersBrowserProxy.whenCalled('reconfigureCupsPrinter')
         .then(function() {
@@ -730,7 +795,7 @@ suite('EditPrinterDialog', function() {
   });
 
   test('TestChangingNameEnablesSaveButton', function() {
-    dialog.activePrinter_ = {
+    dialog.pendingPrinter_ = {
       ppdManufacturer: '',
       ppdModel: '',
       printerAddress: 'test:123',
@@ -766,7 +831,7 @@ suite('EditPrinterDialog', function() {
   });
 
   test('TestChangingAddressEnablesSaveButton', function() {
-    dialog.activePrinter = {
+    dialog.pendingPrinter_ = {
       ppdManufacturer: '',
       ppdModel: '',
       printerAddress: 'test:123',
@@ -802,7 +867,7 @@ suite('EditPrinterDialog', function() {
   });
 
   test('TestChangingQueueEnablesSaveButton', function() {
-    dialog.activePrinter = {
+    dialog.pendingPrinter_ = {
       ppdManufacturer: '',
       ppdModel: '',
       printerAddress: 'test:123',
@@ -838,7 +903,7 @@ suite('EditPrinterDialog', function() {
   });
 
   test('TestChangingProtocolEnablesSaveButton', function() {
-    dialog.activePrinter = {
+    dialog.pendingPrinter_ = {
       ppdManufacturer: '',
       ppdModel: '',
       printerAddress: 'test:123',
@@ -874,7 +939,7 @@ suite('EditPrinterDialog', function() {
   });
 
   test('TestChangingModelEnablesSaveButton', function() {
-    dialog.activePrinter = {
+    dialog.pendingPrinter_ = {
       ppdManufacturer: '',
       ppdModel: '',
       printerAddress: 'test:123',