Add migration page for GitHub App #12112
Conversation
Extracted from #11942
|
This still depends on things from readthedocs/ext-theme#570, but hopefully most of the code would stay the same, so marking it ready for review. We should be able to safely deploy this, as we still require for only admin users to connect their accounts to the new GH app. |
|
|
||
| If we didn't find any webhook to delete, we return True. | ||
| """ | ||
| owner, repo = build_utils.get_github_username_repo(url=project.repo) |
There was a problem hiding this comment.
Most of our services work over the repo of the project instead of the remote repository attached to it... so I'm doing that for this, but all things from the GH app always depend on the remote repo.
stsewd
changed the title
| Notification.objects.add( | ||
| message_id=MESSAGE_OAUTH_WEBHOOK_NOT_REMOVED, | ||
| attached_to=request.user, | ||
| dismissable=True, | ||
| format_values={ | ||
| "repo_full_name": project.remote_repository.full_name, | ||
| "project_slug": project.slug, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
This may flood the user with a lot of notifications if we are not able to remove the webhook from their projects.
There was a problem hiding this comment.
The same happens with the following notification.
Let's say the user has 10 projects connected and we are not able to remove ssh/webhook. In that case, the user will receive 20 notifications.
There was a problem hiding this comment.
Yeah, my other idea was to show these in the migration page like a list, but them we will need to maintain another UI and db models
There was a problem hiding this comment.
but hopefully users won't be experiencing this problem as the other checks should prevent this to happen (like checking if the user is admin of the repo, so the user should be able to delete webhooks and keys)
readthedocs/oauth/services/github.py
Outdated
| f"{settings.PUBLIC_API_URL}/api/v2/webhook/{project.slug}/", | ||
| f"{settings.PUBLIC_API_URL}/api/v2/webhook/github/{project.slug}/", | ||
| ] | ||
| if "app." in settings.PUBLIC_API_URL: | ||
| hook_targets.append(hook_targets[0].replace("app.", "", 1)) | ||
| hook_targets.append(hook_targets[1].replace("app.", "", 1)) |
There was a problem hiding this comment.
Why we don't hardcode the URLs here instead of doing this magic 😄
| f"{settings.PUBLIC_API_URL}/api/v2/webhook/{project.slug}/", | |
| f"{settings.PUBLIC_API_URL}/api/v2/webhook/github/{project.slug}/", | |
| ] | |
| if "app." in settings.PUBLIC_API_URL: | |
| hook_targets.append(hook_targets[0].replace("app.", "", 1)) | |
| hook_targets.append(hook_targets[1].replace("app.", "", 1)) | |
| hook_targets = [ | |
| f"https://readthedocs.org/api/v2/webhook/{project.slug}/", | |
| f"https://readthedocs.org//api/v2/webhook/github/{project.slug}/", | |
| f"https://app.readthedocs.org/api/v2/webhook/{project.slug}/", | |
| f"https://app.readthedocs.org/api/v2/webhook/github/{project.slug}/", | |
| ] |
There was a problem hiding this comment.
The URL is different for .com
There was a problem hiding this comment.
Also, I guess this can skip checking for "app." and always generate the 4 type of URLs since all of our processes are running with the new dashboard now
While this still requires readthedocs/readthedocs.org#12112, I think it should be safe to review, as the UI shouldn't change much. ~~I still need to finishe a couple of things: make all strings translable, and check for users with multiple GH accounts connected~~. - Step by step guide to connect your account to the new GH app, and also to migrate projects. - Partial support for users with more than one GH account linked (they just need to repeat the migration twice), we don't have a lot of users with more than one account connected, so this should be fine. - Automatically refresh the page once the GH tab has been closed, this should work for most cases, but it won't work if the user doesn't close the page, or if the page refreshes before our application was able to sync the repos. We still show a message about refreshing the page, so. https://github.com/user-attachments/assets/a836dfaf-4f30-41ca-a84b-8a669c838aef --------- Co-authored-by: Anthony <[email protected]>
Can be tested together with readthedocs/ext-theme#570, there is a video there showing the workflow.
What this does is groups all repositories the user has linked to a project, and works over that. The actions the user takes are as follows:
Extracted from #11942