-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
PR previews: Warn users when enabling the feature on incompatible projects #9291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0083640
2bb3b0c
1f6f65e
0deec34
8012352
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
from django.conf import settings | ||
from django.contrib.auth.models import User | ||
from django.template.loader import render_to_string | ||
from django.urls import reverse | ||
from django.utils.translation import gettext_lazy as _ | ||
|
||
from readthedocs.builds.constants import INTERNAL | ||
|
@@ -272,6 +273,77 @@ def __init__(self, *args, **kwargs): | |
else: | ||
self.fields['default_version'].widget.attrs['readonly'] = True | ||
|
||
self.setup_external_builds_option() | ||
|
||
def setup_external_builds_option(self): | ||
"""Disable the external builds option if the project doesn't meet the requirements.""" | ||
integrations = list(self.instance.integrations.all()) | ||
has_supported_integration = self.has_supported_integration(integrations) | ||
can_build_external_versions = self.can_build_external_versions(integrations) | ||
|
||
# External builds are supported for this project, | ||
# don't disable the option. | ||
if has_supported_integration and can_build_external_versions: | ||
return | ||
|
||
msg = None | ||
url = reverse("projects_integrations", args=[self.instance.slug]) | ||
if not has_supported_integration: | ||
msg = _( | ||
"To build from pull requests you need a " | ||
f'GitHub or GitLab <a href="{url}">integration</a>.' | ||
) | ||
if has_supported_integration and not can_build_external_versions: | ||
# If there is only one integration, link directly to it. | ||
if len(integrations) == 1: | ||
url = reverse( | ||
"projects_integrations_detail", | ||
args=[self.instance.slug, integrations[0].pk], | ||
) | ||
msg = _( | ||
"To build from pull requests your repository's webhook " | ||
"needs to send pull request events. " | ||
f'Try to <a href="{url}">resync your integration</a>.' | ||
) | ||
|
||
if msg: | ||
field = self.fields["external_builds_enabled"] | ||
field.disabled = True | ||
field.help_text = f"{msg} {field.help_text}" | ||
|
||
def has_supported_integration(self, integrations): | ||
supported_types = {Integration.GITHUB_WEBHOOK, Integration.GITLAB_WEBHOOK} | ||
for integration in integrations: | ||
if integration.integration_type in supported_types: | ||
return True | ||
return False | ||
|
||
def can_build_external_versions(self, integrations): | ||
""" | ||
Check if external versions can be enabled for this project. | ||
|
||
A project can build external versions if: | ||
|
||
- They are using GitHub or GitLab. | ||
- The repository's webhook is setup to send pull request events. | ||
|
||
If the integration's provider data isn't set, | ||
it could mean that the user created the integration manually, | ||
and doesn't have an account connected. | ||
So we don't know for sure if the webhook is sending pull request events. | ||
Comment on lines
+330
to
+333
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should mark this case as "can't build external versions". Otherwise, some people will enable it and it won't work. It's better to let people enable the feature if we are 100% sure it will work. Besides, these cases will be hard to track and debug when receiving a support request like "My PRs are not building". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should still support those cases, these are users using an enterprise instance of GitHub or users that don't want to give full permissions to RTD by connecting their accounts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They won't receive the commit status, but at least the PR will build. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Seems we should at least mention this in the UI as help text, then. |
||
""" | ||
for integration in integrations: | ||
provider_data = integration.provider_data | ||
if integration.integration_type == Integration.GITHUB_WEBHOOK and ( | ||
not provider_data or "pull_request" in provider_data.get("events", []) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could resync the webhook for the user if we don't see they have the pull request event, this is probably the case of old webhooks only, new webhooks are created with the full events. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's safe to re-sync the webhook on behalf of the user, I'd say we should do it automatically. Otherwise, we could send the user a notification that we recommend them to re-sync the webhook to enable the feature. |
||
): | ||
return True | ||
if integration.integration_type == Integration.GITLAB_WEBHOOK and ( | ||
not provider_data or provider_data.get("merge_requests_events") | ||
): | ||
return True | ||
return False | ||
|
||
def clean_conf_py_file(self): | ||
filename = self.cleaned_data.get('conf_py_file', '').strip() | ||
if filename and 'conf.py' not in filename: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# Generated by Django 3.2.13 on 2022-06-02 17:49 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
("projects", "0088_domain_field_edits"), | ||
] | ||
|
||
operations = [ | ||
migrations.AlterField( | ||
model_name="historicalproject", | ||
name="external_builds_enabled", | ||
field=models.BooleanField( | ||
default=False, | ||
help_text='More information in <a href="https://docs.readthedocs.io/page/guides/autobuild-docs-for-pull-requests.html">our docs</a>.', | ||
verbose_name="Build pull requests for this project", | ||
), | ||
), | ||
migrations.AlterField( | ||
model_name="project", | ||
name="external_builds_enabled", | ||
field=models.BooleanField( | ||
default=False, | ||
help_text='More information in <a href="https://docs.readthedocs.io/page/guides/autobuild-docs-for-pull-requests.html">our docs</a>.', | ||
verbose_name="Build pull requests for this project", | ||
), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
from django.contrib.auth.models import User | ||
from django.test import TestCase | ||
from django.urls import reverse | ||
from django_dynamic_fixture import get | ||
|
||
from readthedocs.integrations.models import Integration | ||
from readthedocs.projects.models import Project | ||
|
||
|
||
class TestExternalBuildOption(TestCase): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we testing this under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like testing most things at the view level, it covers more code instead of just the form. |
||
def setUp(self): | ||
self.user = get(User) | ||
self.project = get(Project, users=[self.user]) | ||
self.integration = get( | ||
Integration, | ||
integration_type=Integration.GITHUB_WEBHOOK, | ||
project=self.project, | ||
) | ||
self.url = reverse("projects_advanced", args=[self.project.slug]) | ||
self.client.force_login(self.user) | ||
|
||
def test_unsuported_integration(self): | ||
self.integration.delete() | ||
resp = self.client.get(self.url) | ||
field = resp.context["form"].fields["external_builds_enabled"] | ||
self.assertTrue(field.disabled) | ||
self.assertTrue( | ||
field.help_text.startswith( | ||
"To build from pull requests you need a GitHub or GitLab" | ||
) | ||
) | ||
|
||
get( | ||
Integration, | ||
project=self.project, | ||
integration_type=Integration.BITBUCKET_WEBHOOK, | ||
) | ||
resp = self.client.get(self.url) | ||
field = resp.context["form"].fields["external_builds_enabled"] | ||
self.assertTrue(field.disabled) | ||
self.assertTrue( | ||
field.help_text.startswith( | ||
"To build from pull requests you need a GitHub or GitLab" | ||
) | ||
) | ||
|
||
def test_github_integration(self): | ||
self.integration.provider_data = {} | ||
self.integration.save() | ||
|
||
resp = self.client.get(self.url) | ||
field = resp.context["form"].fields["external_builds_enabled"] | ||
self.assertFalse(field.disabled) | ||
self.assertTrue(field.help_text.startswith("More information in")) | ||
|
||
self.integration.provider_data = {"events": ["pull_request"]} | ||
self.integration.save() | ||
resp = self.client.get(self.url) | ||
field = resp.context["form"].fields["external_builds_enabled"] | ||
self.assertFalse(field.disabled) | ||
self.assertTrue(field.help_text.startswith("More information in")) | ||
|
||
self.integration.provider_data = {"events": []} | ||
self.integration.save() | ||
resp = self.client.get(self.url) | ||
field = resp.context["form"].fields["external_builds_enabled"] | ||
self.assertTrue(field.disabled) | ||
self.assertTrue( | ||
field.help_text.startswith( | ||
"To build from pull requests your repository's webhook needs to send pull request events." | ||
) | ||
) | ||
|
||
def test_gitlab_integration(self): | ||
self.integration.integration_type = Integration.GITLAB_WEBHOOK | ||
self.integration.provider_data = {} | ||
self.integration.save() | ||
|
||
resp = self.client.get(self.url) | ||
field = resp.context["form"].fields["external_builds_enabled"] | ||
self.assertFalse(field.disabled) | ||
self.assertTrue(field.help_text.startswith("More information in")) | ||
|
||
self.integration.provider_data = {"merge_requests_events": True} | ||
self.integration.save() | ||
resp = self.client.get(self.url) | ||
field = resp.context["form"].fields["external_builds_enabled"] | ||
self.assertFalse(field.disabled) | ||
self.assertTrue(field.help_text.startswith("More information in")) | ||
|
||
self.integration.provider_data = {"merge_requests_events": False} | ||
self.integration.save() | ||
resp = self.client.get(self.url) | ||
field = resp.context["form"].fields["external_builds_enabled"] | ||
self.assertTrue(field.disabled) | ||
self.assertTrue( | ||
field.help_text.startswith( | ||
"To build from pull requests your repository's webhook needs to send pull request events." | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.