diff --git a/readthedocs/oauth/migrate.py b/readthedocs/oauth/migrate.py index dc543311c39..6aa6ca0a8a1 100644 --- a/readthedocs/oauth/migrate.py +++ b/readthedocs/oauth/migrate.py @@ -214,7 +214,7 @@ def _get_github_account_target(remote_repository) -> GitHubAccountTarget | None: def _get_projects_missing_migration(user) -> Iterator[tuple[Project, bool, bool]]: """ - Get all projects where the user has admin permissions that are still connected to the old GitHub OAuth App. + Get all projects where the user has access that are still connected to the old GitHub OAuth App. Returns an iterator with the project, a boolean indicating if the GitHub App is installed on the repository, and a boolean indicating if the user has admin permissions on the repository. @@ -261,7 +261,7 @@ def get_migrated_projects(user): ) -def get_valid_projects_missing_migration(user): +def get_valid_projects_missing_migration(user) -> Iterator[Project]: """ Get all projects that the user can migrate to the GitHub App. @@ -296,6 +296,16 @@ def get_migration_targets(user) -> list[MigrationTarget]: return targets +def has_projects_pending_migration(user) -> bool: + """ + Check if the user has any projects pending migration to the GitHub App. + + This includes all projects that are connected to the old GitHub OAuth App, + where the user has admin permissions and the GitHub App is installed. + """ + return any(_get_projects_missing_migration(user)) + + def get_old_app_link() -> str: """ Get the link to the old GitHub OAuth App settings page. diff --git a/readthedocs/profiles/tests/test_views.py b/readthedocs/profiles/tests/test_views.py index 3cb45c496e3..b6e65635ef9 100644 --- a/readthedocs/profiles/tests/test_views.py +++ b/readthedocs/profiles/tests/test_views.py @@ -1,6 +1,5 @@ from unittest import mock -import pytest import requests_mock from allauth.socialaccount.models import SocialAccount, SocialToken from allauth.socialaccount.providers.github.provider import GitHubProvider @@ -257,6 +256,21 @@ def test_migration_page_initial_state(self, request): assert context["step"] == "overview" assert context["step_connect_completed"] is False + assert context["github_app_name"] == "readthedocs" + assert list(context["migrated_projects"]) == [] + assert ( + context["old_application_link"] + == "https://github.com/settings/connections/applications/123" + ) + assert context["step_revoke_completed"] is False + assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "install"}) + assert response.status_code == 200 + context = response.context assert context["installation_target_groups"] == [ InstallationTargetGroup( target=GitHubAccountTarget( @@ -279,7 +293,12 @@ def test_migration_page_initial_state(self, request): repository_ids={4444}, ), ] - assert context["github_app_name"] == "readthedocs" + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "migrate"}) + assert response.status_code == 200 + context = response.context assert context["migration_targets"] == [ MigrationTarget( project=self.project_with_remote_repository, @@ -306,13 +325,15 @@ def test_migration_page_initial_state(self, request): target_id=int(self.remote_organization.remote_id), ), ] - assert list(context["migrated_projects"]) == [] - assert ( - context["old_application_link"] - == "https://github.com/settings/connections/applications/123" - ) - assert context["step_revoke_completed"] is False - assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "revoke"}) + assert response.status_code == 200 + context = response.context + assert context["has_projects_pending_migration"] is True + assert "installation_target_groups" not in context + assert "migration_targets" not in context @requests_mock.Mocker(kw="request") def test_migration_page_step_connect_done(self, request): @@ -323,6 +344,21 @@ def test_migration_page_step_connect_done(self, request): assert context["step"] == "overview" assert context["step_connect_completed"] is True + assert context["github_app_name"] == "readthedocs" + assert list(context["migrated_projects"]) == [] + assert ( + context["old_application_link"] + == "https://github.com/settings/connections/applications/123" + ) + assert context["step_revoke_completed"] is False + assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "install"}) + assert response.status_code == 200 + context = response.context assert context["installation_target_groups"] == [ InstallationTargetGroup( target=GitHubAccountTarget( @@ -345,7 +381,12 @@ def test_migration_page_step_connect_done(self, request): repository_ids={4444}, ), ] - assert context["github_app_name"] == "readthedocs" + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "migrate"}) + assert response.status_code == 200 + context = response.context assert context["migration_targets"] == [ MigrationTarget( project=self.project_with_remote_repository, @@ -372,13 +413,15 @@ def test_migration_page_step_connect_done(self, request): target_id=int(self.remote_organization.remote_id), ), ] - assert list(context["migrated_projects"]) == [] - assert ( - context["old_application_link"] - == "https://github.com/settings/connections/applications/123" - ) - assert context["step_revoke_completed"] is False - assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "revoke"}) + assert response.status_code == 200 + context = response.context + assert context["has_projects_pending_migration"] is True + assert "installation_target_groups" not in context + assert "migration_targets" not in context @requests_mock.Mocker(kw="request") def test_migration_page_step_install_done(self, request): @@ -394,6 +437,21 @@ def test_migration_page_step_install_done(self, request): assert context["step"] == "overview" assert context["step_connect_completed"] is True + assert context["github_app_name"] == "readthedocs" + assert list(context["migrated_projects"]) == [] + assert ( + context["old_application_link"] + == "https://github.com/settings/connections/applications/123" + ) + assert context["step_revoke_completed"] is False + assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "install"}) + assert response.status_code == 200 + context = response.context assert context["installation_target_groups"] == [ InstallationTargetGroup( target=GitHubAccountTarget( @@ -416,7 +474,12 @@ def test_migration_page_step_install_done(self, request): repository_ids=set(), ), ] - assert context["github_app_name"] == "readthedocs" + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "migrate"}) + assert response.status_code == 200 + context = response.context assert context["migration_targets"] == [ MigrationTarget( project=self.project_with_remote_repository, @@ -443,13 +506,15 @@ def test_migration_page_step_install_done(self, request): target_id=int(self.remote_organization.remote_id), ), ] - assert list(context["migrated_projects"]) == [] - assert ( - context["old_application_link"] - == "https://github.com/settings/connections/applications/123" - ) - assert context["step_revoke_completed"] is False - assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "revoke"}) + assert response.status_code == 200 + context = response.context + assert context["has_projects_pending_migration"] is True + assert "installation_target_groups" not in context + assert "migration_targets" not in context @requests_mock.Mocker(kw="request") @mock.patch.object(GitHubService, "remove_webhook") @@ -471,10 +536,28 @@ def test_migration_page_step_migrate_one_project( ) assert response.status_code == 302 response = self.client.get(self.url) + assert response.status_code == 200 context = response.context assert context["step"] == "overview" assert context["step_connect_completed"] is True + assert context["github_app_name"] == "readthedocs" + assert list(context["migrated_projects"]) == [ + self.project_with_remote_repository, + ] + assert ( + context["old_application_link"] + == "https://github.com/settings/connections/applications/123" + ) + assert context["step_revoke_completed"] is False + assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "install"}) + assert response.status_code == 200 + context = response.context assert context["installation_target_groups"] == [ InstallationTargetGroup( target=GitHubAccountTarget( @@ -497,7 +580,12 @@ def test_migration_page_step_migrate_one_project( repository_ids=set(), ), ] - assert context["github_app_name"] == "readthedocs" + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "migrate"}) + assert response.status_code == 200 + context = response.context assert context["migration_targets"] == [ MigrationTarget( project=self.project_with_remote_repository_no_admin, @@ -518,15 +606,15 @@ def test_migration_page_step_migrate_one_project( target_id=int(self.remote_organization.remote_id), ), ] - assert list(context["migrated_projects"]) == [ - self.project_with_remote_repository, - ] - assert ( - context["old_application_link"] - == "https://github.com/settings/connections/applications/123" - ) - assert context["step_revoke_completed"] is False - assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "revoke"}) + assert response.status_code == 200 + context = response.context + assert context["has_projects_pending_migration"] is True + assert "installation_target_groups" not in context + assert "migration_targets" not in context @requests_mock.Mocker(kw="request") @mock.patch.object(GitHubService, "remove_webhook") @@ -550,6 +638,23 @@ def test_migration_page_step_migrate_all_projects( assert context["step"] == "overview" assert context["step_connect_completed"] is True + assert context["github_app_name"] == "readthedocs" + assert list(context["migrated_projects"]) == [ + self.project_with_remote_repository, + self.project_with_remote_organization, + ] + assert ( + context["old_application_link"] + == "https://github.com/settings/connections/applications/123" + ) + assert context["step_revoke_completed"] is False + assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "install"}) + context = response.context assert context["installation_target_groups"] == [ InstallationTargetGroup( target=GitHubAccountTarget( @@ -572,7 +677,12 @@ def test_migration_page_step_migrate_all_projects( repository_ids=set(), ), ] - assert context["github_app_name"] == "readthedocs" + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "migrate"}) + assert response.status_code == 200 + context = response.context assert context["migration_targets"] == [ MigrationTarget( project=self.project_with_remote_repository_no_admin, @@ -587,16 +697,15 @@ def test_migration_page_step_migrate_all_projects( target_id=int(self.social_account_github.uid), ), ] - assert list(context["migrated_projects"]) == [ - self.project_with_remote_repository, - self.project_with_remote_organization, - ] - assert ( - context["old_application_link"] - == "https://github.com/settings/connections/applications/123" - ) - assert context["step_revoke_completed"] is False - assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "revoke"}) + assert response.status_code == 200 + context = response.context + assert context["has_projects_pending_migration"] is True + assert "installation_target_groups" not in context + assert "migration_targets" not in context @requests_mock.Mocker(kw="request") @mock.patch.object(GitHubService, "remove_webhook") @@ -622,6 +731,32 @@ def test_migration_page_step_migrate_one_project_with_errors( assert context["step"] == "overview" assert context["step_connect_completed"] is True + assert context["github_app_name"] == "readthedocs" + assert list(context["migrated_projects"]) == [ + self.project_with_remote_repository, + ] + assert ( + context["old_application_link"] + == "https://github.com/settings/connections/applications/123" + ) + assert context["step_revoke_completed"] is False + assert list(context["old_github_accounts"]) == [self.social_account_github] + + notifications = Notification.objects.for_user(self.user, self.user) + assert notifications.count() == 2 + assert notifications.filter( + message_id=MESSAGE_OAUTH_WEBHOOK_NOT_REMOVED + ).exists() + assert notifications.filter( + message_id=MESSAGE_OAUTH_DEPLOY_KEY_NOT_REMOVED + ).exists() + assert "installation_target_groups" not in context + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "install"}) + assert response.status_code == 200 + context = response.context assert context["installation_target_groups"] == [ InstallationTargetGroup( target=GitHubAccountTarget( @@ -644,7 +779,12 @@ def test_migration_page_step_migrate_one_project_with_errors( repository_ids=set(), ), ] - assert context["github_app_name"] == "readthedocs" + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "migrate"}) + assert response.status_code == 200 + context = response.context assert context["migration_targets"] == [ MigrationTarget( project=self.project_with_remote_repository_no_admin, @@ -665,24 +805,15 @@ def test_migration_page_step_migrate_one_project_with_errors( target_id=int(self.remote_organization.remote_id), ), ] - assert list(context["migrated_projects"]) == [ - self.project_with_remote_repository, - ] - assert ( - context["old_application_link"] - == "https://github.com/settings/connections/applications/123" - ) - assert context["step_revoke_completed"] is False - assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "has_projects_pending_migration" not in context - notifications = Notification.objects.for_user(self.user, self.user) - assert notifications.count() == 2 - assert notifications.filter( - message_id=MESSAGE_OAUTH_WEBHOOK_NOT_REMOVED - ).exists() - assert notifications.filter( - message_id=MESSAGE_OAUTH_DEPLOY_KEY_NOT_REMOVED - ).exists() + response = self.client.get(self.url, data={"step": "revoke"}) + assert response.status_code == 200 + context = response.context + assert context["has_projects_pending_migration"] is True + assert "installation_target_groups" not in context + assert "migration_targets" not in context @requests_mock.Mocker(kw="request") def test_migration_page_step_revoke_done(self, request): @@ -693,6 +824,21 @@ def test_migration_page_step_revoke_done(self, request): assert context["step"] == "overview" assert context["step_connect_completed"] is True + assert context["github_app_name"] == "readthedocs" + assert list(context["migrated_projects"]) == [] + assert ( + context["old_application_link"] + == "https://github.com/settings/connections/applications/123" + ) + assert context["step_revoke_completed"] is True + assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "install"}) + assert response.status_code == 200 + context = response.context assert context["installation_target_groups"] == [ InstallationTargetGroup( target=GitHubAccountTarget( @@ -715,7 +861,12 @@ def test_migration_page_step_revoke_done(self, request): repository_ids={4444}, ), ] - assert context["github_app_name"] == "readthedocs" + assert "migration_targets" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "migrate"}) + assert response.status_code == 200 + context = response.context assert context["migration_targets"] == [ MigrationTarget( project=self.project_with_remote_repository, @@ -742,10 +893,12 @@ def test_migration_page_step_revoke_done(self, request): target_id=int(self.remote_organization.remote_id), ), ] - assert list(context["migrated_projects"]) == [] - assert ( - context["old_application_link"] - == "https://github.com/settings/connections/applications/123" - ) - assert context["step_revoke_completed"] is True - assert list(context["old_github_accounts"]) == [self.social_account_github] + assert "installation_target_groups" not in context + assert "has_projects_pending_migration" not in context + + response = self.client.get(self.url, data={"step": "revoke"}) + assert response.status_code == 200 + context = response.context + assert context["has_projects_pending_migration"] is True + assert "installation_target_groups" not in context + assert "migration_targets" not in context diff --git a/readthedocs/profiles/views.py b/readthedocs/profiles/views.py index 4a12718bcd5..321d5cd7383 100644 --- a/readthedocs/profiles/views.py +++ b/readthedocs/profiles/views.py @@ -42,6 +42,7 @@ from readthedocs.oauth.migrate import get_migration_targets from readthedocs.oauth.migrate import get_old_app_link from readthedocs.oauth.migrate import get_valid_projects_missing_migration +from readthedocs.oauth.migrate import has_projects_pending_migration from readthedocs.oauth.migrate import migrate_project_to_github_app from readthedocs.oauth.notifications import MESSAGE_OAUTH_DEPLOY_KEY_NOT_REMOVED from readthedocs.oauth.notifications import MESSAGE_OAUTH_WEBHOOK_NOT_REMOVED @@ -347,13 +348,20 @@ def get_context_data(self, **kwargs): user = self.request.user context["step_connect_completed"] = self._has_new_accounts_for_old_accounts() - context["installation_target_groups"] = get_installation_target_groups_for_user(user) context["github_app_name"] = settings.GITHUB_APP_NAME - context["migration_targets"] = get_migration_targets(user) context["migrated_projects"] = get_migrated_projects(user) context["old_application_link"] = get_old_app_link() context["step_revoke_completed"] = self._is_access_to_old_github_accounts_revoked() context["old_github_accounts"] = self._get_old_github_accounts() + + # These can take some time, so we only load them when needed. + if step == MigrationSteps.install: + context["installation_target_groups"] = get_installation_target_groups_for_user(user) + if step == MigrationSteps.migrate: + context["migration_targets"] = get_migration_targets(user) + if step == MigrationSteps.revoke: + context["has_projects_pending_migration"] = has_projects_pending_migration(user) + return context def _is_access_to_old_github_accounts_revoked(self):