UI: Unsaved changes modal refinements (#10148) (#10248)

* Initial unsaved changes bug fixes

* Fix some transition bugs

* VAULT-39913 mount params

* Fix failing tests

* spitballin (#10179)

* spitballin

* fix second save bug

* Fix failing tests

* set initital state

* Put initial state back

---------



* Address feedback!

* Fix failing tests

* Add changelog

---------

Co-authored-by: Kianna <30884335+kiannaquach@users.noreply.github.com>
Co-authored-by: claire bontempo <68122737+hellobontempo@users.noreply.github.com>

changelog/_10148.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+ui: After clicking Save or Discard in the unsaved changes modal, the user will now navigate to the intended destination link.
+```

ui/app/components/secret-engine/page/general-settings.hbs

@@ -45,7 +45,7 @@
     aria-label="general settings form"
     id="general-settings-form"
   >
-    <Hds::Text::Body class="has-top-bottom-margin-xxs">
+    <Hds::Text::Body @tag="p" class="has-top-margin-xs">
       Mount parameters that you can tune to fit required engine behavior.
     </Hds::Text::Body>
 
@@ -69,7 +69,12 @@
     <div class="field is-grouped has-top-bottom-margin">
       <Hds::ButtonSet>
         <Hds::Button @text="Save changes" type="submit" disabled={{this.saveGeneralSettings.isRunning}} data-test-submit />
-        <Hds::Button @text="Discard" @color="secondary" {{on "click" this.discardChanges}} data-test-cancel />
+        <Hds::Button
+          @text="Discard"
+          @color="secondary"
+          {{on "click" (fn this.unsavedChanges.transition "vault.cluster.secrets.backend.configuration.general-settings")}}
+          data-test-cancel
+        />
       </Hds::ButtonSet>
     </div>
   </form>
@@ -77,6 +82,6 @@
 
 <UnsavedChangesModal
   @onSave={{this.saveGeneralSettings}}
-  @onDiscard={{this.discardChanges}}
+  @onDiscard={{(fn this.unsavedChanges.transition "vault.cluster.secrets.backend.configuration.general-settings")}}
   @changedFields={{this.modalChangedFields}}
 />

ui/app/components/secret-engine/page/general-settings.ts

@@ -8,9 +8,8 @@ import { task } from 'ember-concurrency';
 import { service } from '@ember/service';
 import { tracked } from '@glimmer/tracking';
 import { convertToSeconds } from 'core/utils/duration-utils';
-import { action } from '@ember/object';
 
-import type Router from '@ember/routing/router';
+import type RouterService from '@ember/routing/router-service';
 import type FlashMessageService from 'vault/services/flash-messages';
 import type ApiService from 'vault/services/api';
 import type SecretsEngineResource from 'vault/resources/secrets/engine';
@@ -40,7 +39,7 @@ interface Args {
 }
 
 export default class GeneralSettingsComponent extends Component<Args> {
-  @service declare readonly router: Router;
+  @service declare readonly router: RouterService;
   @service declare readonly api: ApiService;
   @service declare readonly flashMessages: FlashMessageService;
   @service declare readonly unsavedChanges: UnsavedChangesService;
@@ -172,17 +171,10 @@ export default class GeneralSettingsComponent extends Component<Args> {
 
       this.flashMessages.success('Engine settings successfully updated.', { title: 'Configuration saved' });
 
-      this.unsavedChanges.showModal = false;
-      this.router.transitionTo(this.router.currentRouteName);
+      this.unsavedChanges.transition('vault.cluster.secrets.backend.configuration.general-settings');
     } catch (e) {
       const { message } = await this.api.parseError(e);
       this.errorMessage = message;
     }
   });
-
-  @action
-  discardChanges() {
-    const currentRouteName = this.router.currentRouteName;
-    this.router.transitionTo(currentRouteName);
-  }
 }

ui/app/routes/vault/cluster/secrets/backend/configuration/general-settings.ts

@@ -33,8 +33,6 @@ export default class SecretsBackendConfigurationGeneralSettingsRoute extends Rou
   @service declare readonly pluginCatalog: PluginCatalogService;
   @service declare readonly unsavedChanges: UnsavedChangesService;
 
-  oldModel: Record<string, unknown> | undefined;
-
   async model() {
     const secretsEngine = this.modelFor('vault.cluster.secrets.backend') as SecretsEngineResource;
     const { data } = await this.pluginCatalog.getRawPluginCatalogData();
@@ -67,22 +65,16 @@ export default class SecretsBackendConfigurationGeneralSettingsRoute extends Rou
   willTransition(transition: Transition) {
     // eslint-disable-next-line ember/no-controller-access-in-routes
     const controller = this.controllerFor(this.routeName) as RouteController;
-
     const { model } = controller;
 
+    const state = model ? (model['secretsEngine'] as Record<string, unknown> | undefined) : {};
+    this.unsavedChanges.setup(state);
     // Only intercept transition if leaving THIS route and there are changes
     const targetRoute = transition?.to?.name ?? '';
-    // Saving transitions to the index route, so we do not one to intercept the transition then
-    if (this.routeName !== targetRoute && this.oldModel && model) {
-      const oldModel = this.oldModel['secretsEngine'] as Record<string, unknown> | undefined;
-      const currentModel = model['secretsEngine'] as Record<string, unknown> | undefined;
-      this.unsavedChanges.setupProperties(oldModel, currentModel);
-      this.unsavedChanges.getDiff();
 
-      if (this.unsavedChanges.hasChanges) {
-        transition.abort();
-        this.unsavedChanges.showModal = true;
-      }
+    if (this.routeName !== targetRoute && this.unsavedChanges.hasChanges) {
+      transition.abort();
+      this.unsavedChanges.show(transition);
     }
     return true;
   }

ui/app/services/unsaved-changes.ts

@@ -6,35 +6,67 @@
 import Service from '@ember/service';
 import { tracked } from '@glimmer/tracking';
 import { create } from 'jsondiffpatch';
+import { action } from '@ember/object';
+import { service } from '@ember/service';
+
+import type Transition from '@ember/routing/transition';
+import type RouterService from '@ember/routing/router-service';
 
 // this service tracks the unsaved changes modal state.
 export default class UnsavedChangesService extends Service {
-  @tracked changedFields: Array<string> = [];
+  @service declare readonly router: RouterService;
+
   @tracked showModal = false;
 
   @tracked initialState: Record<string, unknown> | undefined;
   @tracked currentState: Record<string, unknown> | undefined;
+  @tracked intendedTransition: Transition | undefined; // saved transition from willTransition hook before exiting with unsaved changes
 
-  setupProperties(
-    initialState: Record<string, unknown> | undefined,
-    currentState: Record<string, unknown> | undefined
-  ) {
-    this.initialState = initialState;
-    this.currentState = currentState;
+  setup(state: Record<string, unknown> | undefined) {
+    // ensure unsaved-changes intendedTransition is intiially set to undefined each time the user transition
+    this.intendedTransition = undefined;
+    // set up unsaved-changes service state
+    this.currentState = state;
   }
 
-  getDiff() {
+  get changedFields() {
     const diffpatcher = create({});
     const delta = diffpatcher.diff(this.initialState, this.currentState);
 
-    const changedFields = delta ? Object.keys(delta) : [];
-
-    this.changedFields = changedFields;
-
-    return changedFields;
+    return delta ? Object.keys(delta) : [];
   }
 
   get hasChanges() {
     return this.changedFields.length > 0;
   }
+
+  get transitionInfo() {
+    return {
+      routeName: this.intendedTransition?.to?.name,
+      params: this.intendedTransition?.to?.params,
+    };
+  }
+
+  show(transition: Transition) {
+    this.intendedTransition = transition;
+    this.showModal = true;
+  }
+
+  // This method is to update the initial state so it can be called after a successful
+  // save or if the user has decided to discard changes
+  @action
+  resetUnsavedState() {
+    this.initialState = this.currentState;
+  }
+
+  @action
+  transition(route: string) {
+    const { routeName: intendedRoute } = this.transitionInfo || {};
+    if (intendedRoute) {
+      this.resetUnsavedState();
+      this.router.transitionTo(intendedRoute);
+    } else {
+      this.router.transitionTo(route);
+    }
+  }
 }

ui/lib/core/addon/components/unsaved-changes-modal.ts

@@ -48,7 +48,10 @@ export default class UnsavedChangesModal extends Component<Args> {
     }
 
     if (action === 'discard') {
-      this.unsavedChanges.changedFields = [];
+      // If a user has clicked "Discard" the models have already been compared
+      // and they do not want to save the changes.
+      // Update initialState so the transition does not re-abort.
+      this.unsavedChanges.resetUnsavedState();
       this.args.onDiscard();
     }
   }

ui/tests/acceptance/secrets/backend/keymgmt/workflow-test.js

@@ -77,10 +77,12 @@ module('Acceptance | Enterprise | keymgmt-configuration-workflow', function (hoo
     await fillIn(GENERAL.inputByAttr('default_lease_ttl'), 11);
     await fillIn(GENERAL.selectByAttr('default_lease_ttl'), 'm');
     await fillIn(GENERAL.textareaByAttr('description'), 'Updated awesome description.');
-    await click(GENERAL.breadcrumbAtIdx(0));
+    await click(GENERAL.breadcrumbAtIdx(1));
     assert.dom(GENERAL.modal.container('unsaved-changes')).exists('Unsaved changes exists');
     await click(GENERAL.button('save'));
-    assert.strictEqual(currentRouteName(), 'vault.cluster.secrets.backend.configuration.general-settings');
+    assert.strictEqual(currentRouteName(), 'vault.cluster.secrets.backend.list-root');
+    await visit(`/vault/secrets-engines/${keymgmtType}/configuration`);
+
     assert
       .dom(GENERAL.textareaByAttr('description'))
       .hasValue('Updated awesome description.', 'description was tuned from unsaved changes modal');
@@ -93,10 +95,12 @@ module('Acceptance | Enterprise | keymgmt-configuration-workflow', function (hoo
     await fillIn(GENERAL.inputByAttr('default_lease_ttl'), 12);
     await fillIn(GENERAL.selectByAttr('default_lease_ttl'), 'm');
     await fillIn(GENERAL.textareaByAttr('description'), 'Some awesome description.');
-    await click(GENERAL.breadcrumbAtIdx(0));
+    await click(GENERAL.breadcrumbAtIdx(1));
     assert.dom(GENERAL.modal.container('unsaved-changes')).exists('Unsaved changes exists');
     await click(GENERAL.button('discard'));
-    assert.strictEqual(currentRouteName(), 'vault.cluster.secrets.backend.configuration.general-settings');
+    assert.strictEqual(currentRouteName(), 'vault.cluster.secrets.backend.list-root');
+    await visit(`/vault/secrets-engines/${keymgmtType}/configuration`);
+
     assert
       .dom(GENERAL.textareaByAttr('description'))
       .hasValue(

ui/tests/integration/components/unsaved-changes-modal-test.js

@@ -15,7 +15,10 @@ module('Integration | Component | UnsavedChangesModal', function (hooks) {
   hooks.beforeEach(function () {
     this.unsavedChanges = this.owner.lookup('service:unsavedChanges');
     this.unsavedChanges.showModal = true;
-    this.unsavedChanges.changedFields = ['Description', 'Secrets duration'];
+    const initialState = { title: 'Title 1', description: 'Old description' };
+    const currentState = { title: 'Title 1', description: 'New description' };
+    this.unsavedChanges.initialState = initialState;
+    this.unsavedChanges.currentState = currentState;
     this.save = () => 'saved!';
     this.discard = () => 'discarded!';
   });
@@ -25,19 +28,16 @@ module('Integration | Component | UnsavedChangesModal', function (hooks) {
     assert.dom(GENERAL.modal.header('unsaved-changes')).hasText('Unsaved changes');
     assert
       .dom(GENERAL.modal.body('unsaved-changes'))
-      .hasText(
-        `You've made changes to the following: Description Secrets duration Would you like to apply them?`
-      );
+      .hasText(`You've made changes to the following: Description Would you like to apply them?`);
   });
 
   test('it shows unsaved changes modal with custom changedFields', async function (assert) {
-    this.changedFields = ['Field 1', 'Field 2'];
     await render(
       hbs`<UnsavedChangesModal @onSave={{this.save}} @onDiscard={{this.discard}} @changedFields={{this.changedFields}}/>`
     );
     assert.dom(GENERAL.modal.header('unsaved-changes')).hasText('Unsaved changes');
     assert
       .dom(GENERAL.modal.body('unsaved-changes'))
-      .hasText(`You've made changes to the following: Field 1 Field 2 Would you like to apply them?`);
+      .hasText(`You've made changes to the following: Description Would you like to apply them?`);
   });
 });

ui/tests/unit/services/unsaved-changes-test.js

@@ -13,20 +13,24 @@ module('Unit | Service | unsaved-changes', function (hooks) {
     this.unsavedChanges = this.owner.lookup('service:unsaved-changes');
   });
 
-  test('it should set properties', async function (assert) {
-    const initialState = { title: 'Title 1', description: 'Old description' };
-    const currentState = { title: 'New Title 1', description: 'New description' };
-    this.unsavedChanges.setupProperties(initialState, currentState);
-    assert.deepEqual(this.unsavedChanges.initialState, initialState);
-    assert.deepEqual(this.unsavedChanges.currentState, currentState);
-  });
-
-  test('it should update changedFields when getDiff is called', async function (assert) {
+  test('it should get changedFields', async function (assert) {
     const initialState = { title: 'Title 1', description: 'Old description' };
     const currentState = { title: 'Title 1', description: 'New description' };
-    this.unsavedChanges.setupProperties(initialState, currentState);
-    assert.deepEqual(this.unsavedChanges.changedFields, []);
-    this.unsavedChanges.getDiff();
+    this.unsavedChanges.initialState = initialState;
+    this.unsavedChanges.currentState = currentState;
+
     assert.deepEqual(this.unsavedChanges.changedFields, ['description']);
   });
+  test('it should get hasChanges', async function (assert) {
+    const initialState = { title: 'Title 1', description: 'Old description' };
+    const currentState = { title: 'Title 1', description: 'New description' };
+    this.unsavedChanges.initialState = initialState;
+    this.unsavedChanges.currentState = currentState;
+
+    assert.true(this.unsavedChanges.hasChanges, 'shows that there are unsaved changes');
+    currentState.description = 'Old description';
+    this.unsavedChanges.initialState = initialState;
+    this.unsavedChanges.currentState = currentState;
+    assert.false(this.unsavedChanges.hasChanges, 'shows that there are no unsaved changes');
+  });
 });