fix: allow choose another dashboard template after pressing back

[nikitaevg]

Problem

Fix #20784.

This was caused by the following:

1. isLoading is set to true on selecting a template.
2. It's not set to false on pressing "back", so onClick returns early, I confirmed it via debugger.

Changes

Listener for the "Back" button is described here, so I set isLoading to false on clearActiveDashboardTemplate.

Does this work well for both Cloud and self-hosted?

N/A

How did you test this code?

Here's before, after.

Could you please guide me if I could add tests for this anywhere? I don't think it makes sense to test it in newDashboardLogic.test.tsx, as this functionality is spread across newDashboardLogic.ts and DashboardTemplateChooser.tsx, and I couldn't find tests that cover both of them. I also haven't found tests corresponding to this module in the commit history.

[nikitaevg]

Hi @pauldambra !

I want to start contributing to PostHog, so I'm picking simple tasks to get acquainted with the project. Could you please review this PR as the latest contributor to this file, or assign somebody else? Thank you!

[pauldambra]

Could you please guide me if I could add tests for this anywhere?

You could add tests in newDashboardLogic.test.tsx, but it isn't currently testing the kea logic in there so adding a test only for isLoading being false after calling clearActiveDashboardTemplate feels unnecessary

You could add a Cypress test to validate this works. That's the best place for tests that validate the interaction between components and logics

We only have a very basic test on top of templates here

posthog/cypress/e2e/dashboard.cy.ts

Line 90 in 92e17ce

it('Create dashboard from a template', () => {

So - since you want to learn how to contribute to posthog - you'd need to add a Cypress intercept so that the template modal had a template with a variable, then test that clicking that new template moves to the variable form in the modal, and finally that click back and selecting a template again works

I've manually tested this PR so I'd be happy to accept it without a test, but if you want to learn how to contribute on more complex fixes I'm also happy spending time reviewing changes to Cypress tests :)

[nikitaevg]

Thanks for the guidance @pauldambra

PTAL again, I added a test. What's the best way to request a review once again? Just mentioning a reviewer? I couldn't find it in the tutorials.

[nikitaevg]

FYI, I see there's a failure in a not related e2e test, which seems to be a flake, because it happens with other PRs. I'll fix it in #21259.

[nikitaevg]

Hi @Twixes! Kind reminder here, could you please review this?

[posthog-bot]

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

[Twixes]

This works, thank you for the contribution! Apologies for the delay on this, I 100% dropped the ball on reviewing in a timely manner. Will do better next time!

[nikitaevg]

Thanks for the review @Twixes and no problem!

The merge looks stuck though, is there something I should do? The failing tests are probably flakes, as I don't touch the backend part here.