Skip to content

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

Merged
merged 5 commits into from
Jun 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/user/integrations.rst
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ GitLab
* Go to the :guilabel:`Settings` > :guilabel:`Webhooks` page for your project
* For **URL**, use the URL of the integration on Read the Docs,
found on the :guilabel:`Admin` > :guilabel:`Integrations` page
* Leave the default **Push events** selected and mark **Tag push events** also
* Leave the default **Push events** selected,
additionally mark **Tag push events** and **Merge request events**.
* Finish by clicking **Add Webhook**

Gitea
Expand Down
72 changes: 72 additions & 0 deletions readthedocs/projects/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if len(integrations) == 1:
if integrations.count() == 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
Copy link
Member

Choose a reason for hiding this comment

The 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".

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

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", [])
Copy link
Member Author

@stsewd stsewd Jun 2, 2022

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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:
Expand Down
31 changes: 31 additions & 0 deletions readthedocs/projects/migrations/0089_update_help_text.py
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",
),
),
]
4 changes: 3 additions & 1 deletion readthedocs/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,9 @@ class Project(models.Model):
external_builds_enabled = models.BooleanField(
_('Build pull requests for this project'),
default=False,
help_text=_('More information in <a href="https://docs.readthedocs.io/page/guides/autobuild-docs-for-pull-requests.html">our docs</a>') # noqa
help_text=_(
'More information in <a href="https://docs.readthedocs.io/page/guides/autobuild-docs-for-pull-requests.html">our docs</a>.' # noqa
),
)
external_builds_privacy_level = models.CharField(
_('Privacy level of Pull Requests'),
Expand Down
100 changes: 100 additions & 0 deletions readthedocs/projects/tests/test_views.py
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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we testing this under test_views.py? Shouldn't we test this under test_forms.py since the logic change was on the form itself and does not depends on a view?

Copy link
Member Author

Choose a reason for hiding this comment

The 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."
)
)