feat(site): add task notifications to user settings

[ssncferreira]

Description

Add UI support for the new Task Events notification templates (Task Working and Task Idle) added in backend PR #19965. The /api/v2/notifications/templates/system endpoint now returns these templates, therefore this PR keeps the UI in sync.

Changes

• Added Task Working and Task Idle to notification template mocks so Storybook reflects backend data.
• Deployment Settings already lists all templates from the API, so no code change was needed (stories updated automatically).
• Updated the User Settings filter to include Task Events so they appear for end users.

Depends on: #19965

[buenos-nachos]

I think all the changes look pretty straightforward and make sense

I'm going to approve in a minute – I just need to test something out with the story setup. I think we can sneak in some updates to all the findByText calls to make them less fragile and also test for accessibility

[buenos-nachos]

Okay, I did some digging, and I think the current testing setup is papering over some problems with the code

Namely, the change to the runtime code was in user settings version of NotificationPage, while the test update was in the deployment settings version of the NotificationsPage. I'm guessing that was an accident, but the test still succeeded, because we're only checking for text, not for whether the UI elements are valid

I think that realistically, we can keep the test updates for the deployment settings version. And then for the user settings version, we can get away with throwing this play function on the default story:

play: async ({ canvasElement }) => {
	const canvas = within(canvasElement);

	// We only need to findBy for the first group, because all groups should
	// load in at the same time
	await canvas.findByRole("checkbox", { name: "Task Events" });
	void canvas.getByRole("checkbox", { name: "Template Events" });
	void canvas.getByRole("checkbox", { name: "User Events" });
	void canvas.getByRole("checkbox", { name: "Workspace Events" });
	void canvas.getByRole("checkbox", { name: "Custom Events" });
},

Trying to abstract that into a loop seemed like overkill

[aslilac]

@Parkreiner why would you void the gets?

I'm not a fan of these kinds of patterns where you do something once and then do basically the same thing a different way many more times. it needs a comment to explain itself and doesn't provide any real benefit imo.

[buenos-nachos]

@aslilac I dunno, it's something I've considered doing lately to make it clear that there is a usable return value, but it's deliberately not being used. The get methods already function like assertions since they throw if the element isn't available synchronously; I'd rather use them over the queryBy methods with null assertions

I had the first be a find to indicate that there shouldn't be any additional state transitions needed for the rest of the checkboxes to be available, which also gives us the assurance that all elements load in at once. But I can see that veering more into testing implementation details too much, and can see the argument that we should be papering over render behavior

I'm fine with making all of them find-based. I think the most important thing is asserting that each title has a checkbox associated with it

[aslilac]

yeah, asserting that you can click them and it affects checkbox state is a good plan

[ssncferreira]

Namely, the change to the runtime code was in user settings version of NotificationPage, while the test update was in the deployment settings version of the NotificationsPage. I'm guessing that was an accident, but the test still succeeded, because we're only checking for text, not for whether the UI elements are valid

@Parkreiner PR #19965 introduced a new Task Events notification type with two templates, Task Idle and Task Working. As a result, the /api/v2/notifications/templates/system endpoint now returns these templates. To keep the UI consistent, I added the two templates to the MockSystemNotificationTemplates so Storybook stays in sync with the backend. The Deployment Settings view already renders all templates from the API, so no code change was required there and the stories updated automatically with the mocks update. For the User Settings view, we filter to settings a user can control, so I updated the filter to ensure the new Task Events appear as expected. I should have included this context in the PR description, sorry for the confusion.

[ssncferreira]

I think that realistically, we can keep the test updates for the deployment settings version. And then for the user settings version, we can get away with throwing this play function on the default story:

play: async ({ canvasElement }) => {
	const canvas = within(canvasElement);

	// We only need to findBy for the first group, because all groups should
	// load in at the same time
	await canvas.findByRole("checkbox", { name: "Task Events" });
	void canvas.getByRole("checkbox", { name: "Template Events" });
	void canvas.getByRole("checkbox", { name: "User Events" });
	void canvas.getByRole("checkbox", { name: "Workspace Events" });
	void canvas.getByRole("checkbox", { name: "Custom Events" });
},

Trying to abstract that into a loop seemed like overkill

@Parkreiner addressed in d1d600f
Added this test to the default story on the user’s notifications page. I used findByRole for all checkboxes because it makes it explicit that a checkbox exists for every event. Additionally, in Storybook’s UI it’s visible that all checks passed.

[ssncferreira]

@aslilac @Parkreiner thank you for the review and comments. I believe I’ve addressed all of them. Would it be okay if I merge the PR as-is? I just want to make sure it goes in before the release freeze 🙂
If anything needs to change in the tests, would it be okay if I handle it in a follow-up PR?

[buenos-nachos]

Almost there. I think we accidentally added some flaky behavior for CI, but I just posted a suggestion for how to take care of it