feat: [M3-7107] - Add AGLB Certificate Delete Dialog

[bnussman-akamai]

Description 📝

• Adds a basic Delete Confirmation Dialog to the Certificate page
  • The decision to use a basic confirmation dialog rather than a type-to-confirm has been confirmed by UX ✅

Preview 📷

How to test 🧪

• Turn on the MSW
• Go to http://localhost:3000/loadbalancers/0/certificates
• Try to delete a certificate
  • Test the dialog for functionality and style
  • Note The certificate won't actually disappear upon deletion because of how the MSW and React Query work

[abailly-akamai]

Looks good!

[abailly-akamai]

Do we really need an extra selectedCertificateId piece of state here? This could just be a function with the Id as a param

[bnussman-akamai]

I am deriving the selected Certificate by doing

  const [selectedCertificateId, setSelectedCertificateId] = useState<number>();

  const selectedCertificate = data?.data.find(
    (cert) => cert.id === selectedCertificateId
  );

Would we get any benefit from using a function as compared to directly deriving it? I think either way, selectedCertificate will have to be evaluated.

Alternatively I could do

  const [selectedCertificate, setSelectedCertificate] = useState<Certificate>();

Thoughts on the best way to go about this?

[abailly-akamai]

It's not a big deal, I was merely thinking that since you are already passing the certificate to onDeleteCertificate, you could just pass the selectedCertificate to the drawer that way along with the open state and save yourself a filter map. Not sure what you mean by:

I think either way, selectedCertificate will have to be evaluated.

In any case, low on this, what you did works fine!

[cpathipa]

Overall Delete Dialog functionality LGTM! left a nitpicky comment.

[cpathipa]

Can we make it as optional instead of defining undefined ? IMO, its more cleaner.

Suggested change

	certificate: Certificate | undefined;
	certificate?: Certificate ;

[bnussman-akamai]

I think the meaning of these two things are different.

certificate?: Certificate means that certificate is an optional prop.

certificate: Certificate | undefined means that certificate is a required prop that can be undefined. This is what we want because this component expects a Certificate to be passed.

I think, in this situation, certificate: Certificate | undefined is the best way to type this prop.

[abailly-akamai]

This is a great conversation to have about our patterns btw. Certificate | undefined is a bit ambiguous. If it is required it should have data, and it it's undefined it should be checked on the parent IMO. Required props are often a good way to not defer the responsibility of rendering to the children but have that logic on the parent, which is cleaner esp for anything API related. Argument could be made here that we could even remove undefined and not render the Dialog (and show an error) if we don't have the certificate.
Optional prop is the worst option for this tho. Opinions!

[bnussman-akamai]

For sure @abailly-akamai. I would make the certificate required (certificate: Certificate) but it use to be true that conditionally rendering a MUI dialog would result in a broken open/close animation. I'm not sure if this is still true.

{selectedCertificate && (
  <DeleteCertificateDialog
    certificate={selectedCertificate}
    loadbalancerId={Number(loadbalancerId)}
    onClose={() => setIsDeleteDrawerOpen(false)}
    open={isDeleteDrawerOpen}
  />
)}

[abailly-akamai]

Oh yeah, that's not what I am suggesting. The error handling for data being undefined would need to be an early return (error) on LoadBalancerCertificates - after that any chidren can have required props and rely on the data being available

[cpathipa]

Yea, I agree @abailly-akamai Certificate should be a required prop for delete operation modal.
@bnussman-akamai If Certificate is undefined do we even render that record in the table? is this something we should handle at table level.