PR previews: Warn users when enabling the feature on incompatible projects #9291
Conversation
| 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.
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.
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.
stsewd
force-pushed
the
alert-users-invalid-setup-pr-builds
branch
from
47546f5 to
0083640
Compare
There was a problem hiding this comment.
Looks good. I left some comments in case we can simplify the logic a little bit because the double negated if was hard to read and follow to me.
Besides, I think we should make the "Manual webhook created" case not allow pull request builds because we are not 100% sure that it will work.
readthedocs/projects/forms.py
Outdated
| if msg: | ||
| field = self.fields["external_builds_enabled"] | ||
| field.disabled = True | ||
| field.help_text = msg + " " + field.help_text |
There was a problem hiding this comment.
| field.help_text = msg + " " + field.help_text | |
| field.help_text = f"{msg} {field.help_text}" |
readthedocs/projects/forms.py
Outdated
| if not self.has_supported_integration(integrations): | ||
| msg = _( | ||
| "To build from pull requests you need a " | ||
| 'GitHub or GitLab <a href="{url}">integration</a>.' |
There was a problem hiding this comment.
Where are we populating this {url} in the string? I didn't find it.
Maybe it's missing an f at the beginning?
| 'GitHub or GitLab <a href="{url}">integration</a>.' | |
| f'GitHub or GitLab <a href="{url}">integration</a>.' |
readthedocs/projects/forms.py
Outdated
| """Disable the external builds option if the project doesn't meet the requirements.""" | ||
| msg = None | ||
| url = reverse("projects_integrations", args=[self.instance.slug]) | ||
| integrations = list(self.instance.integrations.all()) |
There was a problem hiding this comment.
Why are you converting this to a list here?
There was a problem hiding this comment.
this saves one query later where we use len(integations) instead of .count()
| ) | ||
| elif not self.can_build_external_versions(integrations): | ||
| # If there is only one integration, link directly to it. | ||
| if len(integrations) == 1: |
There was a problem hiding this comment.
| if len(integrations) == 1: | |
| if integrations.count() == 1: |
| 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. |
There was a problem hiding this comment.
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.
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.
They won't receive the commit status, but at least the PR will build.
There was a problem hiding this comment.
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", []) |
There was a problem hiding this comment.
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.
| from readthedocs.projects.models import Project | ||
|
|
||
|
|
||
| class TestExternalBuildOption(TestCase): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I like testing most things at the view level, it covers more code instead of just the form.
Closes #9176