Test coverage and disable button fix on Secrets Sync opt-in modal (#25907)

* fix issue of checkbox value not disabling after canceling the modal

* add component test coverage in overview

* add acceptance test to see flow show banner to not show banner

* comment change

* remove unecessary hash and add settled because ci is funny

* circle ci play nice

* forgot to add my changes 🙃

* blah

* that was a lot for delinating the errors properly—😵‍💫

* pr review comments, thank you for the catches team

ui/lib/core/addon/templates/components/message-error.hbs

@@ -5,7 +5,14 @@
 
 {{#if this.displayErrors}}
   {{#each this.displayErrors as |error|}}
-    <Hds::Alert data-test-message-error @type="inline" @color="critical" class="has-top-margin-s has-bottom-margin-s" as |A|>
+    <Hds::Alert
+      data-test-message-error
+      @type="inline"
+      @color="critical"
+      class="has-top-margin-s has-bottom-margin-s"
+      ...attributes
+      as |A|
+    >
       <A.Title>Error</A.Title>
       <A.Description data-test-message-error-description>{{error}}</A.Description>
     </Hds::Alert>

ui/lib/sync/addon/components/secrets/page/overview.hbs

@@ -16,9 +16,13 @@
     />
   </Hds::Alert>
 {{/unless}}
-{{! show error if call to activated endpoint fails }}
-{{#if @isAdapterError}}
-  <MessageError @errorMessage={{@activatedFeatures.message}} />
+{{! error message if get activated-features fails, called in secret route}}
+{{#if @adapterError}}
+  <MessageError @errorMessage={{@adapterError.message}} data-test-opt-in-banner-error />
+{{/if}}
+{{! error message if post to activated endpoint fails }}
+{{#if this.error}}
+  <MessageError @errorMessage={{this.error}} data-test-opt-in-error />
 {{/if}}
 
 {{#if @destinations}}
@@ -191,7 +195,11 @@
         >documentation</Hds::Link::Inline>
         to learn more.
       </p>
-      <Hds::Form::Checkbox::Field {{on "change" this.onDocsConfirmChange}} data-test-opt-in-check as |F|>
+      <Hds::Form::Checkbox::Field
+        {{on "change" (fn (mut this.hasConfirmedDocs) (not this.hasConfirmedDocs))}}
+        data-test-opt-in-check
+        as |F|
+      >
         <F.Label>I've read the above linked document</F.Label>
       </Hds::Form::Checkbox::Field>
     </M.Body>
@@ -200,15 +208,10 @@
         <Hds::Button
           data-test-opt-in-confirm
           @text="Confirm"
-          disabled={{this.confirmDisabled}}
+          disabled={{(not this.hasConfirmedDocs)}}
           {{on "click" (perform this.onFeatureConfirm)}}
         />
-        <Hds::Button
-          data-test-save-opt-in-cancel
-          @text="Cancel"
-          @color="secondary"
-          {{on "click" (fn (mut this.showActivateSecretsSyncModal) false)}}
-        />
+        <Hds::Button data-test-opt-in-cancel @text="Cancel" @color="secondary" {{on "click" this.resetOptInModal}} />
       </Hds::ButtonSet>
     </M.Footer>
   </Hds::Modal>

ui/lib/sync/addon/components/secrets/page/overview.ts

@@ -18,13 +18,13 @@ import type RouterService from '@ember/routing/router-service';
 import type VersionService from 'vault/services/version';
 import type { SyncDestinationAssociationMetrics } from 'vault/vault/adapters/sync/association';
 import type SyncDestinationModel from 'vault/vault/models/sync/destination';
-import type { HTMLElementEvent } from 'vault/forms';
+import type AdapterError from '@ember/test/adapter';
 
 interface Args {
   destinations: Array<SyncDestinationModel>;
   totalVaultSecrets: number;
   activatedFeatures: Array<string>;
-  isAdapterError: boolean;
+  adapterError: AdapterError | null;
 }
 
 export default class SyncSecretsDestinationsPageComponent extends Component<Args> {
@@ -36,7 +36,8 @@ export default class SyncSecretsDestinationsPageComponent extends Component<Args
   @tracked destinationMetrics: SyncDestinationAssociationMetrics[] = [];
   @tracked page = 1;
   @tracked showActivateSecretsSyncModal = false;
-  @tracked confirmDisabled = true;
+  @tracked hasConfirmedDocs = false;
+  @tracked error = null;
 
   pageSize = Ember.testing ? 3 : 5; // lower in tests to test pagination without seeding more data
 
@@ -48,9 +49,6 @@ export default class SyncSecretsDestinationsPageComponent extends Component<Args
   }
 
   get isActivated() {
-    if (this.args.isAdapterError) {
-      return false;
-    }
     return this.args.activatedFeatures.includes('secrets-sync');
   }
 
@@ -68,8 +66,9 @@ export default class SyncSecretsDestinationsPageComponent extends Component<Args
   });
 
   @action
-  onDocsConfirmChange(event: HTMLElementEvent<HTMLInputElement>) {
-    this.confirmDisabled = !event.target.checked;
+  resetOptInModal() {
+    this.showActivateSecretsSyncModal = false;
+    this.hasConfirmedDocs = false;
   }
 
   @task
@@ -81,9 +80,10 @@ export default class SyncSecretsDestinationsPageComponent extends Component<Args
         .ajax('/v1/sys/activation-flags/secrets-sync/activate', 'POST');
       this.router.transitionTo('vault.cluster.sync.secrets.overview');
     } catch (error) {
+      this.error = errorMessage(error);
       this.flashMessages.danger(`Error enabling feature \n ${errorMessage(error)}`);
     } finally {
-      this.showActivateSecretsSyncModal = false;
+      this.resetOptInModal();
     }
   }
 }

ui/lib/sync/addon/routes/secrets.ts

@@ -5,7 +5,6 @@
 
 import Route from '@ember/routing/route';
 import { service } from '@ember/service';
-import { hash } from 'rsvp';
 
 import type RouterService from '@ember/routing/router-service';
 import type StoreService from 'vault/services/store';
@@ -22,22 +21,28 @@ export default class SyncSecretsRoute extends Route {
   @service declare readonly router: RouterService;
   @service declare readonly store: StoreService;
 
-  model() {
-    return hash({
-      activatedFeatures: this.store
-        .adapterFor('application')
-        .ajax('/v1/sys/activation-flags', 'GET')
-        .then((resp: ActivationFlagsResponse) => {
-          return resp.data.activated;
-        })
-        .catch((error: AdapterError) => {
-          // we break out this error while passing args to the component and handle the error in the overview template
-          return error;
-        }),
-    });
+  async fetchActivatedFeatures() {
+    return await this.store
+      .adapterFor('application')
+      .ajax('/v1/sys/activation-flags', 'GET')
+      .then((resp: ActivationFlagsResponse) => {
+        return resp.data?.activated;
+      })
+      .catch((error: AdapterError) => {
+        return error;
+      });
   }
 
-  afterModel(model: { activatedFeatures: Array<string> | AdapterError }) {
+  async model() {
+    const activatedFeatures = await this.fetchActivatedFeatures();
+    const { isAdapterError } = activatedFeatures;
+    return {
+      activatedFeatures: isAdapterError ? [] : activatedFeatures,
+      adapterError: isAdapterError ? activatedFeatures : null,
+    };
+  }
+
+  afterModel(model: { activatedFeatures: Array<string> }) {
     if (!model.activatedFeatures) {
       this.router.transitionTo('vault.cluster.sync.secrets.overview');
     }

ui/lib/sync/addon/routes/secrets/overview.ts

@@ -14,8 +14,9 @@ export default class SyncSecretsOverviewRoute extends Route {
   @service declare readonly store: StoreService;
 
   async model() {
-    const { activatedFeatures } = this.modelFor('secrets') as {
-      activatedFeatures: Array<string> | AdapterError;
+    const { activatedFeatures, adapterError } = this.modelFor('secrets') as {
+      activatedFeatures: Array<string>;
+      adapterError: AdapterError;
     };
     return hash({
       destinations: this.store.query('sync/destination', {}).catch(() => []),
@@ -24,6 +25,7 @@ export default class SyncSecretsOverviewRoute extends Route {
         .queryAll()
         .catch(() => []),
       activatedFeatures,
+      adapterError,
     });
   }
 }

ui/lib/sync/addon/templates/secrets/overview.hbs

@@ -7,5 +7,5 @@
   @destinations={{this.model.destinations}}
   @totalVaultSecrets={{this.model.associations.total_secrets}}
   @activatedFeatures={{this.model.activatedFeatures}}
-  @isAdapterError={{this.model.activatedFeatures.isAdapterError}}
+  @adapterError={{this.model.adapterError}}
 />

ui/tests/acceptance/sync/secrets/destinations-test.js

@@ -26,27 +26,6 @@ module('Acceptance | sync | destinations', function (hooks) {
     return authPage.login();
   });
 
-  test('it should show opt-in banner and modal if secrets-sync is not activated', async function (assert) {
-    assert.expect(3);
-    server.get('/sys/activation-flags', () => {
-      return {
-        data: {
-          activated: [''],
-          unactivated: ['secrets-sync'],
-        },
-      };
-    });
-
-    await visit('vault/sync/secrets/overview');
-    assert.dom(ts.overview.optInBanner).exists('Opt-in banner is shown');
-    await click(ts.overview.optInBannerEnable);
-    assert.dom(ts.overview.optInModal).exists('Opt-in modal is shown');
-    assert.dom(ts.overview.optInConfirm).isDisabled('Confirm button is disabled when checkbox is unchecked');
-    await click(ts.overview.optInCheck);
-    await click(ts.overview.optInConfirm);
-    // ARG TODO improve test coverage and try and use API to check if the opt-in was successful
-  });
-
   test('it should create new destination', async function (assert) {
     // remove destinations from mirage so cta shows when 404 is returned
     this.server.db.syncDestinations.remove();

ui/tests/acceptance/sync/secrets/overview-test.js

@@ -11,9 +11,10 @@ import syncHandlers from 'vault/mirage/handlers/sync';
 import authPage from 'vault/tests/pages/auth';
 import { click, waitFor } from '@ember/test-helpers';
 import { PAGE as ts } from 'vault/tests/helpers/sync/sync-selectors';
+import AdapterError from '@ember-data/adapter/error';
 
 // sync is an enterprise feature but since mirage is used the enterprise label has been intentionally omitted from the module name
-module('Acceptance | sync | destination', function (hooks) {
+module('Acceptance | sync | overview', function (hooks) {
   setupApplicationTest(hooks);
   setupMirage(hooks);
 
@@ -39,4 +40,38 @@ module('Acceptance | sync | destination', function (hooks) {
     await click(ts.overview.table.action('details'));
     assert.dom(ts.tab('Secrets')).hasClass('active', 'Navigates to secrets view for destination');
   });
+
+  test('it should show opt-in banner and modal if secrets-sync is not activated', async function (assert) {
+    assert.expect(6);
+    this.server.get('/sys/activation-flags', () => {
+      assert.ok(true, 'Request on initial load to check if secrets-sync is activated');
+      return {
+        data: {
+          activated: [''],
+          unactivated: ['secrets-sync'],
+        },
+      };
+    });
+    this.server.post('/sys/activation-flags/secrets-sync/activate', () => {
+      assert.ok(true, 'Request made to activate secrets-sync');
+      return {};
+    });
+    await click(ts.navLink('Secrets Sync'));
+    assert.dom(ts.overview.optInBanner).exists('Opt-in banner is shown');
+    await click(ts.overview.optInBannerEnable);
+    assert.dom(ts.overview.optInModal).exists('Opt-in modal is shown');
+    assert.dom(ts.overview.optInConfirm).isDisabled('Confirm button is disabled when checkbox is unchecked');
+    await click(ts.overview.optInCheck);
+    await click(ts.overview.optInConfirm);
+  });
+
+  test('it should show adapter error if call to activated-features fails', async function (assert) {
+    assert.expect(2);
+    this.server.get('/sys/activation-flags', () => {
+      assert.ok(true, 'Request on initial load to check if secrets-sync is activated');
+      return AdapterError.create();
+    });
+    await click(ts.navLink('Secrets Sync'));
+    assert.dom(ts.overview.optInBannerEnableError).exists('Adapter error message is shown');
+  });
 });

ui/tests/helpers/sync/sync-selectors.js

@@ -53,9 +53,12 @@ export const PAGE = {
   overview: {
     optInBanner: '[data-test-secrets-sync-opt-in-banner]',
     optInBannerEnable: '[data-test-secrets-sync-opt-in-banner-enable]',
+    optInBannerEnableError: '[data-test-opt-in-banner-error]',
     optInModal: '[data-test-secrets-sync-opt-in-modal]',
     optInCheck: '[data-test-opt-in-check]',
     optInConfirm: '[data-test-opt-in-confirm]',
+    optInCancel: '[data-test-opt-in-cancel]',
+    optInError: '[data-test-opt-in-error]',
     createDestination: '[data-test-create-destination]',
     table: {
       row: '[data-test-overview-table-row]',

ui/tests/integration/components/sync/secrets/page/overview-test.js

@@ -45,7 +45,7 @@ module('Integration | Component | sync | Page::Overview', function (hooks) {
 
     this.renderComponent = () =>
       render(
-        hbs`<Secrets::Page::Overview @destinations={{this.destinations}} @totalVaultSecrets={{7}} @activatedFeatures={{this.activatedFeatures}} @isAdapterError={{false}} />`,
+        hbs`<Secrets::Page::Overview @destinations={{this.destinations}} @totalVaultSecrets={{7}} @activatedFeatures={{this.activatedFeatures}} @adapterError={{null}} />`,
         {
           owner: this.engine,
         }
@@ -72,6 +72,30 @@ module('Integration | Component | sync | Page::Overview', function (hooks) {
     assert.dom(cta.summary).exists('CTA renders');
   });
 
+  test('it should render adapter error if post to activate secret sync fails', async function (assert) {
+    assert.expect(3);
+    this.activatedFeatures = [''];
+    this.destinations = [''];
+    const error = { errors: ['Permission denied'] };
+    this.server.post('/sys/activation-flags/secrets-sync/activate', () => {
+      assert.ok(true, 'Request made to activate secrets-sync');
+      return new Response(403, {}, error);
+    });
+
+    await render(
+      hbs`<Secrets::Page::Overview @destinations={{this.destinations}} @totalVaultSecrets={{7}} @activatedFeatures={{this.activatedFeatures}} @adapterError={{null}} />`,
+      {
+        owner: this.engine,
+      }
+    );
+
+    await click(overview.optInBannerEnable);
+    await click(overview.optInCheck);
+    await click(overview.optInConfirm);
+    assert.dom(overview.optInModal).doesNotExist('Opt-in modal closed');
+    assert.dom(overview.optInError).exists('Opt-in modal error displays');
+  });
+
   test('it should render header, tabs and toolbar for overview state', async function (assert) {
     await this.renderComponent();