From fae4fc0362d28761aad6a0e55d99e6f8827410c7 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 25 Nov 2024 21:42:50 +0100 Subject: [PATCH 1/6] Add project: use an instance variable to avoid 500 Closes /~https://github.com/readthedocs/ext-theme/issues/477 --- readthedocs/projects/views/private.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 9efa0db13af..ab80933a98d 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -324,13 +324,19 @@ class ImportWizardView(ProjectImportMixin, PrivateViewMixin, SessionWizardView): per session (since it's per class). """ - form_list = [ - ("basics", ProjectBasicsForm), - ("config", ProjectConfigForm), - ] - initial_dict_key = "initial-data" + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + # README: declare ``form_list`` as an instance variable because we are + # removing the "config" form/step if the config file already exists in + # the repository. + # /~https://github.com/readthedocs/ext-theme/issues/477 + self.form_list = [ + ("basics", ProjectBasicsForm), + ("config", ProjectConfigForm), + ] + def get(self, *args, **kwargs): # The method from the parent should run first, # as the storage is initialized there. From ef0c084c921bbec7647dd7ca6751829c5f4f41f8 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 27 Nov 2024 13:27:50 +0100 Subject: [PATCH 2/6] Declare `form_list` at class level because it's mandatory --- readthedocs/projects/views/private.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index ab80933a98d..12b12a6b6a7 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -2,6 +2,7 @@ import structlog from allauth.socialaccount.models import SocialAccount +from django import forms from django.conf import settings from django.contrib import messages from django.contrib.messages.views import SuccessMessageMixin @@ -326,6 +327,15 @@ class ImportWizardView(ProjectImportMixin, PrivateViewMixin, SessionWizardView): initial_dict_key = "initial-data" + class DummyForm(forms.Form): + pass + + # We have to declare it at class level, so we are defining it with just a dummy form. + # Then we are overriding it on ``__init__`` with the real forms. + form_list = [ + ("dummy", DummyForm), + ] + def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) # README: declare ``form_list`` as an instance variable because we are From 000e36a23aef1cdc0be5901aa57baa9092601ae2 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 27 Nov 2024 13:42:10 +0100 Subject: [PATCH 3/6] It's required to use `OrderedDict` --- readthedocs/projects/views/private.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 9bdcadbf88b..53887e2064c 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -1,5 +1,7 @@ """Project views for authenticated users.""" +from collections import OrderedDict + import structlog from allauth.socialaccount.models import SocialAccount from django import forms @@ -341,10 +343,12 @@ def __init__(self, *args, **kwargs): # removing the "config" form/step if the config file already exists in # the repository. # /~https://github.com/readthedocs/ext-theme/issues/477 - self.form_list = [ - ("basics", ProjectBasicsForm), - ("config", ProjectConfigForm), - ] + self.form_list = OrderedDict( + [ + ("basics", ProjectBasicsForm), + ("config", ProjectConfigForm), + ] + ) def get(self, *args, **kwargs): # The method from the parent should run first, From 2e4895777d0ae649b59b2261c1af4bd2b7bcadad Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 28 Nov 2024 17:01:02 +0100 Subject: [PATCH 4/6] Use `condition_dict` to decide whether or not show the config step --- readthedocs/projects/urls/private.py | 5 +- readthedocs/projects/views/private.py | 143 ++++++++++++-------------- 2 files changed, 72 insertions(+), 76 deletions(-) diff --git a/readthedocs/projects/urls/private.py b/readthedocs/projects/urls/private.py index 73947d2b11c..d5f286f9ad5 100644 --- a/readthedocs/projects/urls/private.py +++ b/readthedocs/projects/urls/private.py @@ -70,7 +70,10 @@ ), path( "import/manual/", - ImportWizardView.as_view(), + ImportWizardView.as_view( + ImportWizardView.form_list, + condition_dict={"config": ImportWizardView.show_config_step}, + ), name="projects_import_manual", ), re_path( diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 53887e2064c..1bf7903028d 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -1,10 +1,8 @@ """Project views for authenticated users.""" -from collections import OrderedDict import structlog from allauth.socialaccount.models import SocialAccount -from django import forms from django.conf import settings from django.contrib import messages from django.contrib.messages.views import SuccessMessageMixin @@ -327,28 +325,77 @@ class ImportWizardView(ProjectImportMixin, PrivateViewMixin, SessionWizardView): """ initial_dict_key = "initial-data" - - class DummyForm(forms.Form): - pass - - # We have to declare it at class level, so we are defining it with just a dummy form. - # Then we are overriding it on ``__init__`` with the real forms. form_list = [ - ("dummy", DummyForm), + ("basics", ProjectBasicsForm), + ("config", ProjectConfigForm), ] - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - # README: declare ``form_list`` as an instance variable because we are - # removing the "config" form/step if the config file already exists in - # the repository. - # /~https://github.com/readthedocs/ext-theme/issues/477 - self.form_list = OrderedDict( - [ - ("basics", ProjectBasicsForm), - ("config", ProjectConfigForm), - ] - ) + @staticmethod + def show_config_step(wizard): + # try to get the cleaned data of step 1 + cleaned_data = wizard.get_cleaned_data_for_step("basics") or {} + repo = cleaned_data.get("repo") + remote_repository = cleaned_data.get("remote_repository") + default_branch = cleaned_data.get("default_branch") + + if ( + repo + and default_branch + and remote_repository + and remote_repository.vcs_provider == GITHUB + ): + # I don't know why `show_config_step` is called multiple times (at least 4). + # This is a problem for us because we perform external calls here and add messages to the request. + # Due to that, we are adding this instance variable to prevent this function to run multiple times. + if hasattr(wizard, "_show_config_step_executed"): + return False + + remote_repository_relations = ( + remote_repository.remote_repository_relations.filter( + user=wizard.request.user, + account__isnull=False, + ) + .select_related("account", "user") + .only("user", "account") + ) + for relation in remote_repository_relations: + service = GitHubService(relation.user, relation.account) + session = service.get_session() + + for yaml in [ + ".readthedocs.yaml", + ".readthedocs.yml", + "readthedocs.yaml", + "readthedocs.yml", + ]: + try: + querystrings = ( + f"?ref={default_branch}" if default_branch else "" + ) + response = session.head( + f"https://api.github.com/repos/{remote_repository.full_name}/contents/{yaml}{querystrings}", + timeout=1, + ) + if response.ok: + log.info( + "Read the Docs YAML file found for this repository.", + filename=yaml, + ) + messages.success( + wizard.request, + _( + "We detected a configuration file in your repository and started your project's first build." + ), + ) + wizard._show_config_step_executed = True + return False + except Exception: + log.warning( + "Failed when hitting GitHub API to check for .readthedocs.yaml file.", + filename=yaml, + ) + continue + return True def get(self, *args, **kwargs): # The method from the parent should run first, @@ -386,60 +433,6 @@ def get_template_names(self): """Return template names based on step name.""" return f"projects/import_{self.steps.current}.html" - def process_step(self, form): - # pylint: disable=too-many-nested-blocks - if isinstance(form, ProjectBasicsForm): - remote_repository = form.cleaned_data.get("remote_repository") - default_branch = form.cleaned_data.get("default_branch") - if remote_repository and remote_repository.vcs_provider == GITHUB: - remote_repository_relations = ( - remote_repository.remote_repository_relations.filter( - user=self.request.user, - account__isnull=False, - ) - .select_related("account", "user") - .only("user", "account") - ) - for relation in remote_repository_relations: - service = GitHubService(relation.user, relation.account) - session = service.get_session() - - for yaml in [ - ".readthedocs.yaml", - ".readthedocs.yml", - "readthedocs.yaml", - "readthedocs.yml", - ]: - try: - querystrings = ( - f"?ref={default_branch}" if default_branch else "" - ) - response = session.head( - f"https://api.github.com/repos/{remote_repository.full_name}/contents/{yaml}{querystrings}", - timeout=1, - ) - if response.ok: - log.info( - "Read the Docs YAML file found for this repository.", - filename=yaml, - ) - messages.success( - self.request, - _( - "We detected a configuration file in your repository and started your project's first build." - ), - ) - self.form_list.pop("config") - break - except Exception: - log.warning( - "Failed when hitting GitHub API to check for .readthedocs.yaml file.", - filename=yaml, - ) - continue - - return super().process_step(form) - def done(self, form_list, **kwargs): """ Save form data as object instance. From 19bf1f756b592c74145cd86498689d344a865c73 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 4 Dec 2024 15:52:06 +0100 Subject: [PATCH 5/6] Add comment for a potential bug --- readthedocs/projects/views/private.py | 1 + 1 file changed, 1 insertion(+) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 1bf7903028d..67584c42383 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -347,6 +347,7 @@ def show_config_step(wizard): # I don't know why `show_config_step` is called multiple times (at least 4). # This is a problem for us because we perform external calls here and add messages to the request. # Due to that, we are adding this instance variable to prevent this function to run multiple times. + # Maybe related to /~https://github.com/jazzband/django-formtools/issues/134 if hasattr(wizard, "_show_config_step_executed"): return False From bb3cd96ef6ea4a445482aa0e27fa1118e49ec980 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 4 Dec 2024 16:19:13 +0100 Subject: [PATCH 6/6] Move `condition_dict` into the class definition --- readthedocs/projects/urls/private.py | 5 +- readthedocs/projects/views/private.py | 142 ++++++++++++++------------ 2 files changed, 75 insertions(+), 72 deletions(-) diff --git a/readthedocs/projects/urls/private.py b/readthedocs/projects/urls/private.py index d5f286f9ad5..73947d2b11c 100644 --- a/readthedocs/projects/urls/private.py +++ b/readthedocs/projects/urls/private.py @@ -70,10 +70,7 @@ ), path( "import/manual/", - ImportWizardView.as_view( - ImportWizardView.form_list, - condition_dict={"config": ImportWizardView.show_config_step}, - ), + ImportWizardView.as_view(), name="projects_import_manual", ), re_path( diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 67584c42383..470378d0191 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -315,6 +315,79 @@ def post(self, request, *args, **kwargs): return HttpResponseRedirect(self.get_success_url()) +def show_config_step(wizard): + """ + Decide whether or not show the config step on "Add project" wizard. + + If the `.readthedocs.yaml` file already exist in the default branch, we + don't show this step. + """ + + # try to get the cleaned data of step 1 + cleaned_data = wizard.get_cleaned_data_for_step("basics") or {} + repo = cleaned_data.get("repo") + remote_repository = cleaned_data.get("remote_repository") + default_branch = cleaned_data.get("default_branch") + + if ( + repo + and default_branch + and remote_repository + and remote_repository.vcs_provider == GITHUB + ): + # I don't know why `show_config_step` is called multiple times (at least 4). + # This is a problem for us because we perform external calls here and add messages to the request. + # Due to that, we are adding this instance variable to prevent this function to run multiple times. + # Maybe related to /~https://github.com/jazzband/django-formtools/issues/134 + if hasattr(wizard, "_show_config_step_executed"): + return False + + remote_repository_relations = ( + remote_repository.remote_repository_relations.filter( + user=wizard.request.user, + account__isnull=False, + ) + .select_related("account", "user") + .only("user", "account") + ) + for relation in remote_repository_relations: + service = GitHubService(relation.user, relation.account) + session = service.get_session() + + for yaml in [ + ".readthedocs.yaml", + ".readthedocs.yml", + "readthedocs.yaml", + "readthedocs.yml", + ]: + try: + querystrings = f"?ref={default_branch}" if default_branch else "" + response = session.head( + f"https://api.github.com/repos/{remote_repository.full_name}/contents/{yaml}{querystrings}", + timeout=1, + ) + if response.ok: + log.info( + "Read the Docs YAML file found for this repository.", + filename=yaml, + ) + messages.success( + wizard.request, + _( + "We detected a configuration file in your repository and started your project's first build." + ), + ) + wizard._show_config_step_executed = True + return False + except Exception: + log.warning( + "Failed when hitting GitHub API to check for .readthedocs.yaml file.", + filename=yaml, + ) + continue + return True + + class ImportWizardView(ProjectImportMixin, PrivateViewMixin, SessionWizardView): """ @@ -325,79 +398,12 @@ class ImportWizardView(ProjectImportMixin, PrivateViewMixin, SessionWizardView): """ initial_dict_key = "initial-data" + condition_dict = {"config": show_config_step} form_list = [ ("basics", ProjectBasicsForm), ("config", ProjectConfigForm), ] - @staticmethod - def show_config_step(wizard): - # try to get the cleaned data of step 1 - cleaned_data = wizard.get_cleaned_data_for_step("basics") or {} - repo = cleaned_data.get("repo") - remote_repository = cleaned_data.get("remote_repository") - default_branch = cleaned_data.get("default_branch") - - if ( - repo - and default_branch - and remote_repository - and remote_repository.vcs_provider == GITHUB - ): - # I don't know why `show_config_step` is called multiple times (at least 4). - # This is a problem for us because we perform external calls here and add messages to the request. - # Due to that, we are adding this instance variable to prevent this function to run multiple times. - # Maybe related to /~https://github.com/jazzband/django-formtools/issues/134 - if hasattr(wizard, "_show_config_step_executed"): - return False - - remote_repository_relations = ( - remote_repository.remote_repository_relations.filter( - user=wizard.request.user, - account__isnull=False, - ) - .select_related("account", "user") - .only("user", "account") - ) - for relation in remote_repository_relations: - service = GitHubService(relation.user, relation.account) - session = service.get_session() - - for yaml in [ - ".readthedocs.yaml", - ".readthedocs.yml", - "readthedocs.yaml", - "readthedocs.yml", - ]: - try: - querystrings = ( - f"?ref={default_branch}" if default_branch else "" - ) - response = session.head( - f"https://api.github.com/repos/{remote_repository.full_name}/contents/{yaml}{querystrings}", - timeout=1, - ) - if response.ok: - log.info( - "Read the Docs YAML file found for this repository.", - filename=yaml, - ) - messages.success( - wizard.request, - _( - "We detected a configuration file in your repository and started your project's first build." - ), - ) - wizard._show_config_step_executed = True - return False - except Exception: - log.warning( - "Failed when hitting GitHub API to check for .readthedocs.yaml file.", - filename=yaml, - ) - continue - return True - def get(self, *args, **kwargs): # The method from the parent should run first, # as the storage is initialized there.