From 1745d50c2d6762da3fc3f306b3bb939e6ceaafe0 Mon Sep 17 00:00:00 2001 From: Angel Garbarino Date: Fri, 15 Mar 2024 09:59:08 -0600 Subject: [PATCH] Test coverage and disable button fix on Secrets Sync opt-in modal (#25907) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .../templates/components/message-error.hbs | 9 ++++- .../components/secrets/page/overview.hbs | 25 +++++++------ .../addon/components/secrets/page/overview.ts | 18 ++++----- ui/lib/sync/addon/routes/secrets.ts | 35 ++++++++++-------- ui/lib/sync/addon/routes/secrets/overview.ts | 6 ++- .../sync/addon/templates/secrets/overview.hbs | 2 +- .../sync/secrets/destinations-test.js | 21 ----------- .../acceptance/sync/secrets/overview-test.js | 37 ++++++++++++++++++- ui/tests/helpers/sync/sync-selectors.js | 3 ++ .../sync/secrets/page/overview-test.js | 26 ++++++++++++- 10 files changed, 120 insertions(+), 62 deletions(-) diff --git a/ui/lib/core/addon/templates/components/message-error.hbs b/ui/lib/core/addon/templates/components/message-error.hbs index 9678ab5119..06c849a793 100644 --- a/ui/lib/core/addon/templates/components/message-error.hbs +++ b/ui/lib/core/addon/templates/components/message-error.hbs @@ -5,7 +5,14 @@ {{#if this.displayErrors}} {{#each this.displayErrors as |error|}} - + Error {{error}} diff --git a/ui/lib/sync/addon/components/secrets/page/overview.hbs b/ui/lib/sync/addon/components/secrets/page/overview.hbs index 16e5578584..0ee246450d 100644 --- a/ui/lib/sync/addon/components/secrets/page/overview.hbs +++ b/ui/lib/sync/addon/components/secrets/page/overview.hbs @@ -16,9 +16,13 @@ /> {{/unless}} -{{! show error if call to activated endpoint fails }} -{{#if @isAdapterError}} - +{{! error message if get activated-features fails, called in secret route}} +{{#if @adapterError}} + +{{/if}} +{{! error message if post to activated endpoint fails }} +{{#if this.error}} + {{/if}} {{#if @destinations}} @@ -191,7 +195,11 @@ >documentation to learn more.

- + I've read the above linked document @@ -200,15 +208,10 @@ - + diff --git a/ui/lib/sync/addon/components/secrets/page/overview.ts b/ui/lib/sync/addon/components/secrets/page/overview.ts index 353831a61f..ce7d62cd34 100644 --- a/ui/lib/sync/addon/components/secrets/page/overview.ts +++ b/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; totalVaultSecrets: number; activatedFeatures: Array; - isAdapterError: boolean; + adapterError: AdapterError | null; } export default class SyncSecretsDestinationsPageComponent extends Component { @@ -36,7 +36,8 @@ export default class SyncSecretsDestinationsPageComponent extends Component) { - this.confirmDisabled = !event.target.checked; + resetOptInModal() { + this.showActivateSecretsSyncModal = false; + this.hasConfirmedDocs = false; } @task @@ -81,9 +80,10 @@ export default class SyncSecretsDestinationsPageComponent extends Component { - 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 | AdapterError }) { + async model() { + const activatedFeatures = await this.fetchActivatedFeatures(); + const { isAdapterError } = activatedFeatures; + return { + activatedFeatures: isAdapterError ? [] : activatedFeatures, + adapterError: isAdapterError ? activatedFeatures : null, + }; + } + + afterModel(model: { activatedFeatures: Array }) { if (!model.activatedFeatures) { this.router.transitionTo('vault.cluster.sync.secrets.overview'); } diff --git a/ui/lib/sync/addon/routes/secrets/overview.ts b/ui/lib/sync/addon/routes/secrets/overview.ts index 4c088a7b53..4ac4d2fef5 100644 --- a/ui/lib/sync/addon/routes/secrets/overview.ts +++ b/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 | AdapterError; + const { activatedFeatures, adapterError } = this.modelFor('secrets') as { + activatedFeatures: Array; + 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, }); } } diff --git a/ui/lib/sync/addon/templates/secrets/overview.hbs b/ui/lib/sync/addon/templates/secrets/overview.hbs index 21e8ea4e81..c858dc27f3 100644 --- a/ui/lib/sync/addon/templates/secrets/overview.hbs +++ b/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}} /> \ No newline at end of file diff --git a/ui/tests/acceptance/sync/secrets/destinations-test.js b/ui/tests/acceptance/sync/secrets/destinations-test.js index 064008d3d4..6885acd5fe 100644 --- a/ui/tests/acceptance/sync/secrets/destinations-test.js +++ b/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(); diff --git a/ui/tests/acceptance/sync/secrets/overview-test.js b/ui/tests/acceptance/sync/secrets/overview-test.js index ea24d262f7..5dea041287 100644 --- a/ui/tests/acceptance/sync/secrets/overview-test.js +++ b/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'); + }); }); diff --git a/ui/tests/helpers/sync/sync-selectors.js b/ui/tests/helpers/sync/sync-selectors.js index ac8428cd71..e527932d2d 100644 --- a/ui/tests/helpers/sync/sync-selectors.js +++ b/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]', diff --git a/ui/tests/integration/components/sync/secrets/page/overview-test.js b/ui/tests/integration/components/sync/secrets/page/overview-test.js index 7a86cb3242..640be0af2f 100644 --- a/ui/tests/integration/components/sync/secrets/page/overview-test.js +++ b/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``, + hbs``, { 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``, + { + 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();