upcoming: [M3-7131] - Add AGLB Configuration Edit Tests & General Bug…

… Fixes (#9941) * add edit flow test * clean up and add reset button * Added changeset: Add AGLB Configuration e2e tests, improve error handling, and improve UX * add some bug fixes * fix table filter * remove a `useEffect` * require service targets on rules and improve config validation * make reset clear API errors --------- Co-authored-by: Banks Nussman <[email protected]>
linode · Dec 1, 2023 · 1141d10 · 1141d10
1 parent 2f467dc
commit 1141d10
Show file tree

Hide file tree

Showing 13 changed files with 288 additions and 36 deletions.
diff --git a/packages/api-v4/src/aglb/types.ts b/packages/api-v4/src/aglb/types.ts
@@ -179,7 +179,7 @@ type CertificateType = 'ca' | 'downstream';
 export interface Certificate {
   id: number;
   label: string;
-  certificate: string;
+  certificate?: string; // Not returned for Alpha
   type: CertificateType;
 }
 

diff --git a/packages/manager/.changeset/pr-9941-upcoming-features-1701203325929.md b/packages/manager/.changeset/pr-9941-upcoming-features-1701203325929.md
@@ -0,0 +1,5 @@
+---
+"@linode/manager": Upcoming Features
+---
+
+Add AGLB Configuration e2e tests, improve error handling, and improve UX ([#9941](https://github.com/linode/manager/pull/9941))
diff --git a/packages/manager/cypress/e2e/core/loadBalancers/load-balancer-configurations.spec.ts b/packages/manager/cypress/e2e/core/loadBalancers/load-balancer-configurations.spec.ts
@@ -18,6 +18,8 @@ import {
   mockGetLoadBalancerCertificates,
   mockGetLoadBalancerConfigurations,
   mockGetLoadBalancerRoutes,
+  mockUpdateLoadBalancerConfiguration,
+  mockUpdateLoadBalancerConfigurationError,
 } from 'support/intercepts/load-balancers';
 import { ui } from 'support/ui';
 
@@ -353,6 +355,151 @@ describe('Akamai Global Load Balancer configurations page', () => {
       }
     });
   });
+
+  describe('update', () => {
+    it('edits a HTTPS configuration', () => {
+      const configuration = configurationFactory.build({ protocol: 'https' });
+      const loadbalancer = loadbalancerFactory.build({
+        configurations: [{ id: configuration.id, label: configuration.label }],
+      });
+      const certificates = certificateFactory.buildList(3);
+      const routes = routeFactory.buildList(3);
+
+      mockGetLoadBalancer(loadbalancer).as('getLoadBalancer');
+      mockGetLoadBalancerConfigurations(loadbalancer.id, [configuration]).as(
+        'getConfigurations'
+      );
+      mockGetLoadBalancerCertificates(loadbalancer.id, certificates).as(
+        'getCertificates'
+      );
+      mockUpdateLoadBalancerConfiguration(loadbalancer.id, configuration).as(
+        'updateConfiguration'
+      );
+      mockGetLoadBalancerRoutes(loadbalancer.id, routes).as('getRoutes');
+
+      cy.visitWithLogin(
+        `/loadbalancers/${loadbalancer.id}/configurations/${configuration.id}`
+      );
+
+      cy.wait([
+        '@getFeatureFlags',
+        '@getClientStream',
+        '@getLoadBalancer',
+        '@getConfigurations',
+        '@getRoutes',
+        '@getCertificates',
+      ]);
+
+      // In edit mode, we will disable the "save" button if the user hasn't made any changes
+      ui.button
+        .findByTitle('Save Configuration')
+        .should('be.visible')
+        .should('be.disabled');
+
+      cy.findByLabelText('Configuration Label')
+        .should('be.visible')
+        .clear()
+        .type('new-label');
+
+      cy.findByLabelText('Port').should('be.visible').clear().type('444');
+
+      ui.button
+        .findByTitle('Apply More Certificates')
+        .should('be.visible')
+        .should('be.enabled')
+        .click();
+
+      ui.drawer.findByTitle('Apply Certificates').within(() => {
+        cy.findByLabelText('Host Header').type('example-1.com');
+
+        cy.findByLabelText('Certificate').click();
+
+        ui.autocompletePopper.findByTitle(certificates[1].label).click();
+
+        ui.button
+          .findByTitle('Save')
+          .should('be.visible')
+          .should('be.enabled')
+          .click();
+      });
+
+      ui.button
+        .findByTitle('Save Configuration')
+        .should('be.visible')
+        .should('be.enabled')
+        .click();
+
+      cy.wait('@updateConfiguration');
+    });
+
+    it('shows API errors when editing', () => {
+      const configuration = configurationFactory.build({ protocol: 'https' });
+      const loadbalancer = loadbalancerFactory.build({
+        configurations: [{ id: configuration.id, label: configuration.label }],
+      });
+
+      mockGetLoadBalancer(loadbalancer).as('getLoadBalancer');
+      mockGetLoadBalancerConfigurations(loadbalancer.id, [configuration]).as(
+        'getConfigurations'
+      );
+
+      const errors = [
+        { field: 'label', reason: 'Bad Label.' },
+        { field: 'port', reason: 'Port number is too high.' },
+        { field: 'protocol', reason: 'This protocol is not supported.' },
+        {
+          field: 'certificates[0].id',
+          reason: 'Certificate 0 does not exist.',
+        },
+        {
+          field: 'certificates[0].hostname',
+          reason: 'That hostname is too long.',
+        },
+        { field: 'route_ids', reason: 'Some of these routes do not exist.' },
+      ];
+
+      mockUpdateLoadBalancerConfigurationError(
+        loadbalancer.id,
+        configuration.id,
+        errors
+      ).as('updateConfigurationError');
+
+      cy.visitWithLogin(
+        `/loadbalancers/${loadbalancer.id}/configurations/${configuration.id}`
+      );
+
+      cy.wait([
+        '@getFeatureFlags',
+        '@getClientStream',
+        '@getLoadBalancer',
+        '@getConfigurations',
+      ]);
+
+      // In edit mode, we will disable the "save" button if the user hasn't made any changes
+      ui.button
+        .findByTitle('Save Configuration')
+        .should('be.visible')
+        .should('be.disabled');
+
+      cy.findByLabelText('Configuration Label')
+        .should('be.visible')
+        .clear()
+        .type('new-label');
+
+      ui.button
+        .findByTitle('Save Configuration')
+        .should('be.visible')
+        .should('be.enabled')
+        .click();
+
+      cy.wait('@updateConfigurationError');
+
+      for (const error of errors) {
+        cy.findByText(error.reason).should('be.visible');
+      }
+    });
+  });
+
   describe('delete', () => {
     it('deletes a configuration', () => {
       const loadbalancer = loadbalancerFactory.build();

diff --git a/packages/manager/cypress/support/intercepts/load-balancers.ts b/packages/manager/cypress/support/intercepts/load-balancers.ts
@@ -119,6 +119,45 @@ export const mockCreateLoadBalancerConfiguration = (
   );
 };
 
+/**
+ * Intercepts PUT request to update an AGLB configuration.
+ *
+ * @param loadBalancerId - ID of load balancer for which to update the configuration.
+ * @param configuration - The AGLB configuration being updated.
+ *
+ * @returns Cypress chainable.
+ */
+export const mockUpdateLoadBalancerConfiguration = (
+  loadBalancerId: number,
+  configuration: Configuration
+) => {
+  return cy.intercept(
+    'PUT',
+    apiMatcher(`/aglb/${loadBalancerId}/configurations/${configuration.id}`),
+    makeResponse(configuration)
+  );
+};
+
+/**
+ * Intercepts PUT request to update an AGLB configuration.
+ *
+ * @param loadBalancerId - ID of load balancer for which to update the configuration.
+ * @param configuration - The AGLB configuration being updated.
+ *
+ * @returns Cypress chainable.
+ */
+export const mockUpdateLoadBalancerConfigurationError = (
+  loadBalancerId: number,
+  configurationId: number,
+  errors: APIError[]
+) => {
+  return cy.intercept(
+    'PUT',
+    apiMatcher(`/aglb/${loadBalancerId}/configurations/${configurationId}`),
+    makeResponse({ errors }, 400)
+  );
+};
+
 /**
  * Intercepts POST request to create an AGLB configuration and returns an error.
  *

diff --git a/...src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.test.tsx b/...src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.test.tsx
@@ -1,4 +1,3 @@
-import { Certificate } from '@linode/api-v4';
 import { act, waitFor } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import React from 'react';
@@ -8,18 +7,19 @@ import { renderWithTheme } from 'src/utilities/testHelpers';
 
 import { EditCertificateDrawer } from './EditCertificateDrawer';
 
-const mockTLSCertificate: Certificate = {
+const mockTLSCertificate = {
   certificate: mockCertificate,
   id: 0,
   label: 'test-tls-cert',
   type: 'downstream',
-};
-const mockCACertificate: Certificate = {
+} as const;
+
+const mockCACertificate = {
   certificate: mockCertificate,
   id: 0,
   label: 'test-ca-cert',
   type: 'ca',
-};
+} as const;
 
 describe('EditCertificateDrawer', () => {
   it('should contain the name of the cert in the drawer title and label field', () => {

diff --git a/...ager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx b/...ager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx
@@ -35,15 +35,15 @@ export const EditCertificateDrawer = (props: Props) => {
   const formik = useFormik<UpdateCertificatePayload>({
     enableReinitialize: true,
     initialValues: {
-      certificate: certificate?.certificate.trim(),
+      certificate: certificate?.certificate?.trim(),
       key: '',
       label: certificate?.label ?? '',
       type: certificate?.type,
     },
     async onSubmit(values) {
       // The user has not edited their cert or the private key, so we exclude both cert and key from the request.
       const shouldIgnoreField =
-        certificate?.certificate.trim() === values.certificate &&
+        certificate?.certificate?.trim() === values.certificate &&
         values.key === '';
 
       try {

diff --git a/.../src/features/LoadBalancers/LoadBalancerDetail/Configurations/ApplyCertificatesDrawer.tsx b/.../src/features/LoadBalancers/LoadBalancerDetail/Configurations/ApplyCertificatesDrawer.tsx
@@ -85,6 +85,7 @@ export const ApplyCertificatesDrawer = (props: Props) => {
                 )
               }
               errorText={formik.errors.certificates?.[index]?.['id']}
+              filter={{ type: 'downstream' }}
               loadbalancerId={loadbalancerId}
               value={id}
             />

diff --git a/...manager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/CertificateTable.tsx b/...manager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/CertificateTable.tsx
@@ -8,18 +8,21 @@ import { TableCell } from 'src/components/TableCell';
 import { TableHead } from 'src/components/TableHead';
 import { TableRow } from 'src/components/TableRow';
 import { TableRowEmpty } from 'src/components/TableRowEmpty/TableRowEmpty';
+import { Typography } from 'src/components/Typography';
 import { useLoadBalancerCertificatesQuery } from 'src/queries/aglb/certificates';
 
-import type { Configuration } from '@linode/api-v4';
+import type { CertificateConfig, Configuration } from '@linode/api-v4';
+import type { FormikErrors } from 'formik';
 
 interface Props {
   certificates: Configuration['certificates'];
+  errors: FormikErrors<CertificateConfig>[] | string[];
   loadbalancerId: number;
   onRemove: (index: number) => void;
 }
 
 export const CertificateTable = (props: Props) => {
-  const { certificates, loadbalancerId, onRemove } = props;
+  const { certificates, errors, loadbalancerId, onRemove } = props;
 
   const { data } = useLoadBalancerCertificatesQuery(
     loadbalancerId,
@@ -40,10 +43,27 @@ export const CertificateTable = (props: Props) => {
         {certificates.length === 0 && <TableRowEmpty colSpan={3} />}
         {certificates.map((cert, idx) => {
           const certificate = data?.data.find((c) => c.id === cert.id);
+          const error = errors[idx];
+          const hostnameError =
+            typeof error !== 'string' ? error?.hostname : undefined;
+          const idError = typeof error !== 'string' ? error?.id : undefined;
+
+          const generalRowError = typeof error === 'string' ? error : idError;
+
           return (
             <TableRow key={idx}>
-              <TableCell>{certificate?.label ?? cert.id}</TableCell>
-              <TableCell>{cert.hostname}</TableCell>
+              <TableCell>
+                {certificate?.label ?? cert.id}
+                {generalRowError && (
+                  <Typography color="error">{generalRowError}</Typography>
+                )}
+              </TableCell>
+              <TableCell>
+                {cert.hostname}
+                {hostnameError && (
+                  <Typography color="error">{hostnameError}</Typography>
+                )}
+              </TableCell>
               <TableCell actionCell>
                 <IconButton
                   aria-label={`Remove Certificate ${