- Notifications
You must be signed in to change notification settings - Fork926
fix(site): only show method warning if some template is using it#14565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for looking into this!
site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.stories.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
BrunoQuaresmaSep 5, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Right, but when you implement what we discussed inhttps://github.com/coder/coder/pull/14565/files#r1745353934 then itwill 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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| // Extracted from a real API response | ||
| export const mockNotificationsDeploymentOptions: SerpentOption[] = [ | ||
| { | ||
| name: "Notifications: Dispatch Timeout", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This will go out of sync; is there a way we can lint this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Perfectly fine to do as a follow-up, yup. Your idea sounds good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| @@ -203,6 +203,8 @@ export const baseMeta = { | |||
| }, | |||
| email: { | |||
| smarthost: "smtp.example.com", | |||
| from: "localhost", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ditto
…onEvents.stories.tsxCo-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: Could use thein operator here maybe?fields.every((field) => field in obj)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We also need to check if the field is not an empty string so I thinkBoolean(obj[field]) works best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah yeah fair enough 😎
84d312c intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.