Send site notification on Build status reporting failure and follow DRY #6033
Conversation
saadmk11
removed
the
PR: work in progress
saadmk11
changed the title
|
As I was Adding the User communication part to the task I saw that we were using some of the same code more than once. So, I tried to follow DRY and moved some of the redundant code to |
readthedocs/projects/tasks.py
Outdated
| try: | ||
| if build.project.remote_repository.account.provider == 'github': | ||
| service = GitHubService( | ||
| if provider_name == 'GitHub': |
There was a problem hiding this comment.
This should be a constant. Also, why are we checking the name instead of build.project.remote_repository.account.provider like previously?
There was a problem hiding this comment.
previously we were just getting the provider name by using build.project.remote_repository.account.provider but now we have a better way project.git_provider_name as this actually checks which provider the project is from and project.git_service_class() get the actual class the projects needs (e.g: GitHubService) so we don't need that anymore.
readthedocs/projects/constants.py
Outdated
| @@ -355,3 +355,6 @@ | |||
| ) | |||
|
|
|||
| GITHUB_PR_PULL_PATTERN = 'pull/{id}/head:external-{id}' | |||
|
|
|||
| # Git provider names | |||
| GITHUB = 'GitHub' | |||
There was a problem hiding this comment.
This isn't a good name, since it has specialized caps. It should likely be GITHUB_BRAND or something,
There was a problem hiding this comment.
GITHUB_PROVIDER_NAME maybe?
|
Updated the Changes :) |
ref: #6014