fix(site): only show method warning if some template is using it #14565
Conversation
site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.stories.tsx
Show resolved
Hide resolved
| spyOn(API, "updateNotificationTemplateMethod").mockResolvedValue(); | ||
| const user = userEvent.setup(); | ||
| const canvas = within(canvasElement); | ||
| const option = await canvas.findByText("Workspace Marked as Dormant"); |
There was a problem hiding this comment.
I wonder if we should include the template ID as a data attribute and select on that? It'll be more stable than the name.
There was a problem hiding this comment.
In this case, it doesn't matter too much since we control the data. I prefer to use the name in UI tests to match the user behavior.
There was a problem hiding this comment.
We control it, yes, but we may rename this template, which will cause this test to fail mysteriously.
IDs will likely never be changed, so it's more resilient.
There was a problem hiding this comment.
This test won't fail if we rename the template because we're using mocked data. It's not an end-to-end test. I understand that using IDs makes the test more resilient, but this is a UI test in a controlled context, so I think we should test it as the user would. However, if you strongly prefer using IDs, we can switch to that approach and start a discussion with the front-end guild to establish it as a convention, since the community uses a different method. The goal is to test the UI as the user would.
There was a problem hiding this comment.
Right, but when you implement what we discussed in https://github.com/coder/coder/pull/14565/files#r1745353934 then it will go out of sync, and this is an entirely preventable problem.
I'm not part of the front-end guild so it's up to you.
site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.tsx
Outdated
Show resolved
Hide resolved
| // Extracted from a real API response | ||
| export const mockNotificationsDeploymentOptions: SerpentOption[] = [ | ||
| { | ||
| name: "Notifications: Dispatch Timeout", |
There was a problem hiding this comment.
This will go out of sync; is there a way we can lint this?
There was a problem hiding this comment.
The only check we have here is the type, as we do for the other mocks as well, but I know it is not sufficient. I've been thinking about how we could improve our mocks and keep them in sync. One option that came to mind was to use a script that communicates with the API and generates the mocks. However, this would require more investigation and is beyond the scope of this pull request. What do you think about this idea? Do you see a simpler way we could achieve better results?
There was a problem hiding this comment.
Perfectly fine to do as a follow-up, yup. Your idea sounds good 👍
There was a problem hiding this comment.
LGTM, thanks @BrunoQuaresma!
I think waiting for @bcpeinhardt is a good idea though since I'm not qualified to review frontend code.
site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.stories.tsx
Outdated
Show resolved
Hide resolved
| @@ -203,6 +203,8 @@ export const baseMeta = { | |||
| }, | |||
| email: { | |||
| smarthost: "smtp.example.com", | |||
| from: "localhost", | |||
…onEvents.stories.tsx Co-authored-by: Danny Kopping <danny@coder.com>
| obj: Record<string, string | undefined>, | ||
| fields: string[], | ||
| ): boolean { | ||
| return fields.every((field) => Boolean(obj[field])); |
There was a problem hiding this comment.
Nit: Could use the in operator here maybe? fields.every((field) => field in obj)
There was a problem hiding this comment.
We also need to check if the field is not an empty string so I think Boolean(obj[field]) works best.
Previously, we were showing the warning regardless of whether a template was using the misconfigured notification method or not. However, we realized this could be too noisy, so we decided to display the warning only when the user has a template configured to use the misconfigured method.