Repository: getsentry/sentry
Pull request: feat(onboarding): Bulletproof critical back actions with new tests


- closes https://github.com/getsentry/sentry/issues/88302

Problem statement:
Retro: Add acceptance tests for flow that broke in onboarding
We have acceptance tests for the simple flows in onboarding, but when more complex flows break, like re-creating a project after clicking the back button. Let's add an acceptance test for that flow

Hints from issue discussion:

Commit message: Merge branch 'master' into priscila/feat/onboarding/add-tests-to-bulletproof-critical-actions

Patch to review:
diff --git a/src/sentry/testutils/asserts.py b/src/sentry/testutils/asserts.py
index d48cd8f0b5be33..ed45188002a8c8 100644
--- a/src/sentry/testutils/asserts.py
+++ b/src/sentry/testutils/asserts.py
@@ -1,10 +1,15 @@
+import time
 from functools import reduce
 
+from django.core.exceptions import ObjectDoesNotExist
 from django.http import StreamingHttpResponse
 
+from sentry.constants import ObjectStatus
 from sentry.integrations.types import EventLifecycleOutcome
 from sentry.models.auditlogentry import AuditLogEntry
 from sentry.models.commitfilechange import CommitFileChange
+from sentry.models.organization import Organization
+from sentry.models.project import Project
 from sentry.silo.base import SiloMode
 from sentry.testutils.silo import assume_test_silo_mode
 
@@ -45,6 +50,22 @@ def assert_status_code(response, minimum: int, maximum: int | None = None):
     )
 
 
+def verify_project_deletion(org: Organization, platform: str):
+    # Poll the database to check if the project was deleted
+    # the timeout is set to 10 seconds
+    start_time = time.time()
+    while time.time() - start_time < 10:
+        try:
+            # we need to check for the status here because we do soft deletions
+            Project.objects.get(organization=org, slug=platform, status=ObjectStatus.ACTIVE)
+        except ObjectDoesNotExist:
+            # If the project doesn't exist anymore, it's deleted
+            return  # The project was successfully deleted
+
+    # If timeout reached and the project still exists, fail the test
+    assert False, f"Project with slug '{platform}' was not deleted within 10 seconds"
+
+
 @assume_test_silo_mode(SiloMode.CONTROL)
 def org_audit_log_exists(**kwargs):
     assert kwargs
diff --git a/tests/acceptance/test_create_project.py b/tests/acceptance/test_create_project.py
index 08f97ed00c28d6..9f05bc5fb42451 100644
--- a/tests/acceptance/test_create_project.py
+++ b/tests/acceptance/test_create_project.py
@@ -1,5 +1,4 @@
-from selenium.webdriver.common.by import By
-
+from sentry.testutils.asserts import verify_project_deletion
 from sentry.testutils.cases import AcceptanceTestCase
 from sentry.testutils.silo import no_silo_test
 
@@ -14,9 +13,19 @@ def setUp(self):
 
         self.path = f"/organizations/{self.org.slug}/projects/new/"
 
-    def test_no_teams(self):
+    def select_platform_and_create_project(self, platform, heading):
+        self.browser.click(f'[data-test-id="platform-{platform}"]')
+        self.browser.click('[data-test-id="create-project"]')
+
+        # Wait for the project setup page to load
+        self.browser.wait_until(xpath=f'//h2[text()="Configure {heading} SDK"]')
+
+    def load_project_creation_page(self):
         self.browser.get(self.path)
-        self.browser.wait_until_not(".loading")
+        self.browser.wait_until('[aria-label="Create Project"]')
+
+    def test_no_teams(self):
+        self.load_project_creation_page()
 
         self.browser.click(None, "//*[text()='Select a Team']")
         self.browser.click('[data-test-id="create-team-option"]')
@@ -32,16 +41,36 @@ def test_no_teams(self):
     def test_select_correct_platform(self):
         self.create_team(organization=self.org, name="team three")
 
-        self.browser.get(self.path)
-        self.browser.wait_until_not(".loading")
+        self.load_project_creation_page()
 
-        self.browser.click('[data-test-id="platform-javascript-react"]')
-        self.browser.wait_until_not(".loading")
-        self.browser.click('[data-test-id="create-project"]')
+        # Select React platform
+        self.select_platform_and_create_project("javascript-react", "React")
+
+    def test_project_deletion_on_going_back(self):
+        self.create_team(organization=self.org, name="team three")
+
+        self.load_project_creation_page()
+
+        # Select PHP Laravel platform
+        self.select_platform_and_create_project("php-laravel", "Laravel")
+
+        # Click on custom back button
+        self.browser.click('[aria-label="Back to Platform Selection"]')
+
+        # Verify project was deleted
+        verify_project_deletion(self.org, "platform-php-laravel")
+
+        # Select Next.js platform
+        self.select_platform_and_create_project("javascript-nextjs", "Next.js")
+
+        # Click on the browser native back button
+        self.browser.back()
 
-        self.browser.wait_until_not(".loading")
-        self.browser.wait_until("h2")
+        # Verify project was deleted
+        verify_project_deletion(self.org, "javascript-nextjs")
 
-        title = self.browser.find_element(by=By.CSS_SELECTOR, value="h2")
+        # Go to the projects overview page
+        self.browser.get("/organizations/%s/projects/" % self.org.slug)
 
-        assert "React" in title.text
+        # Verify that all projects are gone
+        self.browser.wait_until(xpath='//h1[text()="Remain Calm"]')
diff --git a/tests/acceptance/test_onboarding.py b/tests/acceptance/test_onboarding.py
index 71d835f93c13d5..6c6ba6b19181a9 100644
--- a/tests/acceptance/test_onboarding.py
+++ b/tests/acceptance/test_onboarding.py
@@ -1,9 +1,7 @@
-from selenium.common.exceptions import TimeoutException
-
 from sentry.models.project import Project
+from sentry.testutils.asserts import verify_project_deletion
 from sentry.testutils.cases import AcceptanceTestCase
 from sentry.testutils.silo import no_silo_test
-from sentry.utils.retries import TimedRetryPolicy
 
 
 @no_silo_test
@@ -18,7 +16,7 @@ def setUp(self):
         )
         self.login_as(self.user)
 
-    def test_onboarding(self):
+    def start_onboarding(self):
         self.browser.get("/onboarding/%s/" % self.org.slug)
 
         # Welcome step
@@ -28,29 +26,68 @@ def test_onboarding(self):
         # Platform selection step
         self.browser.wait_until('[data-test-id="onboarding-step-select-platform"]')
 
-        @TimedRetryPolicy.wrap(timeout=5, exceptions=((TimeoutException,)))
-        def click_platform_select_name(browser):
-            # Select and create React project
-            browser.click('[data-test-id="platform-javascript-react"]')
-
-            # Project getting started loads
-            browser.wait_until(xpath='//h2[text()="Configure React SDK"]')
+    def click_on_platform(self, platform):
+        self.browser.click(f'[data-test-id="platform-{platform}"]')
 
-        click_platform_select_name(self.browser)
+    def verify_project_creation(self, platform, heading):
+        # Wait for the project setup page to load
+        self.browser.wait_until(xpath=f'//h2[text()="Configure {heading} SDK"]')
 
         # Verify project was created for org
-        project = Project.objects.get(organization=self.org)
-        assert project.name == "javascript-react"
-        assert project.platform == "javascript-react"
+        project = Project.objects.get(organization=self.org, slug=platform)
+        assert project.name == platform
+        assert project.platform == platform
+
+    def test_onboarding_happy_path(self):
+        # Start onboarding
+        self.start_onboarding()
+
+        # Select React platform
+        self.click_on_platform("javascript-react")
+
+        # Verify project creation and docs load
+        self.verify_project_creation("javascript-react", "React")
 
-        # Click on back button
+    def test_project_deletion_on_going_back(self):
+        # Start onboarding
+        self.start_onboarding()
+
+        # Select Next.js platform
+        self.click_on_platform("javascript-nextjs")
+
+        # Verify project creation and docs load
+        self.verify_project_creation("javascript-nextjs", "Next.js")
+
+        # Click on custom back button
         self.browser.click('[aria-label="Back"]')
 
-        # Assert no deletion confirm dialog is shown
-        assert not self.browser.element_exists("[role='dialog']")
+        # Verify project was deleted
+        verify_project_deletion(self.org, "javascript-nextjs")
 
-        # Platform selection step
-        self.browser.wait_until('[data-test-id="onboarding-step-select-platform"]')
+        # Select React platform
+        self.click_on_platform("javascript-react")
+
+        # Verify project creation and docs load
+        self.verify_project_creation("javascript-react", "React")
+
+        # Click on the browser native back button
+        self.browser.back()
+
+        # Verify project was deleted
+        verify_project_deletion(self.org, "javascript-react")
+
+        # Click on skip onboarding
+        self.browser.click(xpath='//a[text()="Skip Onboarding"]')
+
+        # Go to the projects overview page
+        self.browser.get("/organizations/%s/projects/" % self.org.slug)
+
+        # Verify that all projects are gone
+        self.browser.wait_until(xpath='//h1[text()="Remain Calm"]')
+
+    def test_framework_modal_open_by_selecting_vanilla_platform(self):
+        # Start onboarding
+        self.start_onboarding()
 
         # Select generic platform
         self.browser.click('[data-test-id="platform-javascript"]')
@@ -68,5 +105,5 @@ def click_platform_select_name(browser):
         self.browser.click('[data-test-id="platform-javascript"]')
         self.browser.click('[aria-label="Configure SDK"]')
 
-        # Project getting started loads
-        self.browser.wait_until(xpath='//h2[text()="Configure Browser JavaScript SDK"]')
+        # Verify project creation and docs load
+        self.verify_project_creation("javascript", "Browser JavaScript")


Code review comments:
diff_hunk: @@ -45,6 +50,22 @@ def assert_status_code(response, minimum: int, maximum: int | None = None):
     )
 
 
+def verify_project_deletion(org: Organization, platform: str):
+    # Poll the database to check if the project was deleted
+    # the timeout is set to 10 seconds
+    start_time = time.time()
+    while time.time() - start_time < 10:
+        try:
+            # we need to check for the status here because we do soft deletions
+            Project.objects.get(organization=org, slug=platform, status=ObjectStatus.ACTIVE)
+        except ObjectDoesNotExist:
+            # If the project doesn't exist anymore, it's deleted
+            return  # The project was successfully deleted
path: src/sentry/testutils/asserts.py
text: @user1:
```suggestion
        assert not Project.objects.filter(organization=org, slug=platform, status=ObjectStatus.ACTIVE).exists()
```
small optimization (exceptions are more expensive than just boolean check) and it is imo easier to read

diff_hunk: @@ -45,6 +50,22 @@ def assert_status_code(response, minimum: int, maximum: int | None = None):
     )
 
 
+def verify_project_deletion(org: Organization, platform: str):
+    # Poll the database to check if the project was deleted
+    # the timeout is set to 10 seconds
+    start_time = time.time()
+    while time.time() - start_time < 10:
path: src/sentry/testutils/asserts.py
text: @user1:
Are we sure this timeout is needed? Asking because it will make our acceptance tests runs even slower.

@author:
It does take some time for the project to be removed, and we need to wait for that. Do you know of some another way to achieve that?

@user2:
Looking at other  acceptance tests, can we do similar and wait using `self.browser.wait_until_not` - the only question is what is the right thing to wait for?

Current approach will produce a lot of unnecessary db calls, even if it is only in test environment, and it will make tests slower than they should be

@author:
The issue is that we silently delete the project, and this change isn't reflected in the DOM, so I can't use `self.browser.wait_until_not`. I added the database query because I noticed it was used frequently in the acceptance tests, but if that's not the preferred approach, I'm happy to remove it. In the tests I've written, we already navigate to the project overview page and check if the project is rendered or not using `self.browser.wait_until`. The database query was just an extra measure to be completely certain.

@user2:
Unrelated to this PR, but more to the general UX - do we want to let the user know that we have deleted the project?

Or do we want to do all of this silently without user ever knowing that the project was created and then deleted?

Because this can lead to onbaording task (`FIRST_PROJECT`) being marked as completed - without user ever being aware that they have created a project

@author:
We want to do it silently without users knowing the project has been deleted. They will still see this info in the audit log, but that is ok - we have discussed this internally. 

> Because this can lead to onbaording task (FIRST_PROJECT) being marked as completed - without user ever being aware that they have created a project

That's a great point. This scenario could occur if the user:

- Creates a project
- Navigates back in the flow
- Clicks "Skip Onboarding"

While it’s a rare edge case, I’ll bring this up to the team. Thanks for flagging this!

@user3:
> but more to the general UX - do we want to let the user know that we have deleted the project?

we used to have this modal before that made it transparent a project had been created and delete and it was kind of awkward. Mostly because for the user it didn't really feel like they had created a project, but just picked a platform on the previous screen. It's really just for technical reasons that we need to already create a project at that stage in order to generate a DSN for the code snippets we show in the next screen. So we decided to remove it.

> Because this can lead to onbaording task (FIRST_PROJECT) being marked as completed - without user ever being aware that they have created a project

that is a good observation and I would definitely say it's an unwanted side-effect. My preferred option would be to also revert this if the project is deleted in the onboarding flow, so the task doesn't appear as completed.
IF that's very complicated, then I agree with Pri that the scenario is probably rare... if you do go back, it's likely because you wanted to choose a different platform, so probably you'll still continue with the flow and create another project anyway. Since it's just in this "go back and then skip" scenario, it's probably reasonably rare that we can live with it.

diff_hunk: @@ -45,6 +50,22 @@ def assert_status_code(response, minimum: int, maximum: int | None = None):
     )
 
 
+def verify_project_deletion(org: Organization, platform: str):
path: src/sentry/testutils/asserts.py
text: @author:
this test is used in 2x different places, so I aimed to make it reusable. However, I'm unsure if this is the best location for it

@user1:
small nit: but all but one function in that file is called `assert_*`, to keep consistency can we rename this to `assert_project_deletion`?

diff_hunk: @@ -32,16 +41,36 @@ def test_no_teams(self):
     def test_select_correct_platform(self):
         self.create_team(organization=self.org, name="team three")
 
-        self.browser.get(self.path)
-        self.browser.wait_until_not(".loading")
+        self.load_project_creation_page()
 
-        self.browser.click('[data-test-id="platform-javascript-react"]')
-        self.browser.wait_until_not(".loading")
-        self.browser.click('[data-test-id="create-project"]')
+        # Select React platform
+        self.select_platform_and_create_project("javascript-react", "React")
+
+    def test_project_deletion_on_going_back(self):
+        self.create_team(organization=self.org, name="team three")
+
+        self.load_project_creation_page()
+
+        # Select PHP Laravel platform
path: tests/acceptance/test_create_project.py
text: @user1:
Can we remove this and the following comments? I dont see the value of having a comment on every line. Thx