diff --git a/backend/analytics_server/mhq/exapi/github.py b/backend/analytics_server/mhq/exapi/github.py index dac20b009..4ec5a6c45 100644 --- a/backend/analytics_server/mhq/exapi/github.py +++ b/backend/analytics_server/mhq/exapi/github.py @@ -1,9 +1,10 @@ import contextlib from datetime import datetime from http import HTTPStatus -from typing import Optional, Dict, Tuple, List +from typing import Optional, Dict, Tuple, List, cast import requests + from github import Github, UnknownObjectException from github.GithubException import GithubException from github.Organization import Organization as GithubOrganization @@ -11,7 +12,12 @@ from github.PullRequest import PullRequest as GithubPullRequest from github.Repository import Repository as GithubRepository +from mhq.exapi.schemas.timeline import ( + GitHubPullTimelineEvent, + GitHubPrTimelineEventsDict, +) from mhq.exapi.models.github import GitHubContributor +from mhq.exapi.models.github_timeline import GithubPullRequestTimelineEvents from mhq.utils.log import LOG PAGE_SIZE = 100 @@ -271,3 +277,107 @@ def _fetch_workflow_runs(page: int = 1): page += 1 data = _fetch_workflow_runs(page=page) return repo_workflows + + def _fetch_timeline_events( + self, repo_name: str, pr_number: int, page: int = 1 + ) -> List[Dict]: + github_url = f"{self.base_url}/repos/{repo_name}/issues/{pr_number}/timeline" + query_params = {"per_page": PAGE_SIZE, "page": page} + + try: + response = requests.get( + github_url, headers=self.headers, params=query_params + ) + except requests.RequestException as e: + raise GithubException( + HTTPStatus.SERVICE_UNAVAILABLE, f"Network error: {str(e)}" + ) from e + + if response.status_code == HTTPStatus.NOT_FOUND: + raise GithubException( + HTTPStatus.NOT_FOUND, + f"PR {pr_number} not found for repo {repo_name}", + ) + + if response.status_code == HTTPStatus.FORBIDDEN: + raise GithubRateLimitExceeded("GitHub API rate limit exceeded") + + if response.status_code != HTTPStatus.OK: + raise GithubException( + response.status_code, + f"Failed to fetch timeline events: {response.text}", + ) + + try: + return response.json() + except ValueError as e: + raise GithubException( + HTTPStatus.INTERNAL_SERVER_ERROR, f"Invalid JSON response: {str(e)}" + ) from e + + def _create_timeline_event(self, event_data: Dict) -> GitHubPrTimelineEventsDict: + return GitHubPrTimelineEventsDict( + event=event_data.get("event", ""), + data=cast(GitHubPullTimelineEvent, event_data), + ) + + def get_pr_timeline_events( + self, repo_name: str, pr_number: int + ) -> List[GithubPullRequestTimelineEvents]: + + all_timeline_events: List[GitHubPrTimelineEventsDict] = [] + page = 1 + + try: + while True: + timeline_events = self._fetch_timeline_events( + repo_name, pr_number, page + ) + if not timeline_events: + break + + all_timeline_events.extend( + [ + self._create_timeline_event(event_data) + for event_data in timeline_events + ] + ) + + if len(timeline_events) < PAGE_SIZE: + break + page += 1 + + except GithubException: + raise + except Exception as e: + raise GithubException( + HTTPStatus.INTERNAL_SERVER_ERROR, f"Unexpected error: {str(e)}" + ) from e + + return self._adapt_github_timeline_events(all_timeline_events) + + @staticmethod + def _adapt_github_timeline_events( + timeline_events: List[GitHubPrTimelineEventsDict], + ) -> List[GithubPullRequestTimelineEvents]: + adapted_timeline_events: List[GithubPullRequestTimelineEvents] = [] + + for timeline_event in timeline_events: + event_data = timeline_event.get("data") + if not event_data: + continue + + event_type = timeline_event.get("event") + if not event_type: + continue + + event = GithubPullRequestTimelineEvents(event_type, event_data) + + if all([event.timestamp, event.type, event.id, event.user]): + adapted_timeline_events.append(event) + else: + LOG.warning( + f"Skipping incomplete timeline event: {event_type} with id: {event.id}" + ) + + return adapted_timeline_events diff --git a/backend/analytics_server/mhq/exapi/models/github_timeline.py b/backend/analytics_server/mhq/exapi/models/github_timeline.py new file mode 100644 index 000000000..49b8dee56 --- /dev/null +++ b/backend/analytics_server/mhq/exapi/models/github_timeline.py @@ -0,0 +1,139 @@ +from datetime import datetime +from dataclasses import dataclass +from typing import Any, Optional, Dict, cast + + +from mhq.exapi.schemas.timeline import ( + GitHubPullTimelineEvent, +) +from mhq.store.models.code.enums import PullRequestEventType +from mhq.utils.log import LOG +from mhq.utils.time import dt_from_iso_time_string + + +@dataclass +class GithubPullRequestTimelineEventConfig: + actor_path: str + timestamp_field: str + id_path: str + + +@dataclass +class GithubPullRequestTimelineEvents: + REVIEWED_CONFIG = GithubPullRequestTimelineEventConfig( + actor_path="user", timestamp_field="submitted_at", id_path="id" + ) + + READY_FOR_REVIEW_CONFIG = GithubPullRequestTimelineEventConfig( + actor_path="actor", timestamp_field="created_at", id_path="id" + ) + + COMMENTED_CONFIG = GithubPullRequestTimelineEventConfig( + actor_path="user", timestamp_field="created_at", id_path="id" + ) + + COMMITTED_CONFIG = GithubPullRequestTimelineEventConfig( + actor_path="author.name", timestamp_field="author.date", id_path="sha" + ) + + DEFAULT_CONFIG = GithubPullRequestTimelineEventConfig( + actor_path="actor", timestamp_field="created_at", id_path="id" + ) + + EVENT_CONFIG = { + "reviewed": REVIEWED_CONFIG, + "ready_for_review": READY_FOR_REVIEW_CONFIG, + "commented": COMMENTED_CONFIG, + "committed": COMMITTED_CONFIG, + "default": DEFAULT_CONFIG, + } + EVENT_TYPE_MAPPING = { + "assigned": PullRequestEventType.ASSIGNED, + "closed": PullRequestEventType.CLOSED, + "commented": PullRequestEventType.COMMENTED, + "committed": PullRequestEventType.COMMITTED, + "convert_to_draft": PullRequestEventType.CONVERT_TO_DRAFT, + "head_ref_deleted": PullRequestEventType.HEAD_REF_DELETED, + "head_ref_force_pushed": PullRequestEventType.HEAD_REF_FORCE_PUSHED, + "labeled": PullRequestEventType.LABELED, + "locked": PullRequestEventType.LOCKED, + "merged": PullRequestEventType.MERGED, + "ready_for_review": PullRequestEventType.READY_FOR_REVIEW, + "referenced": PullRequestEventType.REFERENCED, + "reopened": PullRequestEventType.REOPENED, + "review_dismissed": PullRequestEventType.REVIEW_DISMISSED, + "review_requested": PullRequestEventType.REVIEW_REQUESTED, + "review_request_removed": PullRequestEventType.REVIEW_REQUEST_REMOVED, + "reviewed": PullRequestEventType.REVIEW, + "unassigned": PullRequestEventType.UNASSIGNED, + "unlabeled": PullRequestEventType.UNLABELED, + "unlocked": PullRequestEventType.UNLOCKED, + } + + def __init__(self, event_type: str, data: GitHubPullTimelineEvent): + self.event_type = event_type + self.data = data + + def _get_nested_value(self, path: str) -> Optional[Any]: + keys = path.split(".") + current = self.data + + for key in keys: + if isinstance(current, dict) and key in current: + current = current[key] + else: + return None + return current + + @property + def user(self) -> Optional[str]: + config = self.EVENT_CONFIG.get(self.event_type, self.EVENT_CONFIG["default"]) + actor_path = config.actor_path + + if not actor_path: + return None + + if self.event_type == "committed": + return self._get_nested_value(actor_path) + + user_data = self._get_nested_value(actor_path) + if not user_data: + return None + if isinstance(user_data, dict) and "login" in user_data: + return user_data["login"] + if hasattr(user_data, "login"): + return user_data.login + + LOG.warning( + f"User data does not contain login field for event type: {self.event_type}" + ) + return None + + @property + def timestamp(self) -> Optional[datetime]: + config = self.EVENT_CONFIG.get(self.event_type, self.EVENT_CONFIG["default"]) + timestamp_field = config.timestamp_field + timestamp_value = self._get_nested_value(timestamp_field) + + if timestamp_value: + timestamp_str = str(timestamp_value) + return dt_from_iso_time_string(timestamp_str) + return None + + @property + def raw_data(self) -> Dict: + return cast(Dict[str, Any], self.data) + + @property + def id(self) -> Optional[str]: + config = self.EVENT_CONFIG.get(self.event_type, self.EVENT_CONFIG["default"]) + id_path = config.id_path + id_value = self._get_nested_value(id_path) + return str(id_value) if id_value is not None else None + + @property + def type(self) -> Optional[PullRequestEventType]: + + return self.EVENT_TYPE_MAPPING.get( + self.event_type, PullRequestEventType.UNKNOWN + ) diff --git a/backend/analytics_server/mhq/exapi/schemas/__init__.py b/backend/analytics_server/mhq/exapi/schemas/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/backend/analytics_server/mhq/exapi/schemas/timeline.py b/backend/analytics_server/mhq/exapi/schemas/timeline.py new file mode 100644 index 000000000..a833873d9 --- /dev/null +++ b/backend/analytics_server/mhq/exapi/schemas/timeline.py @@ -0,0 +1,182 @@ +from typing import TypedDict, Optional, Union, List + + +class GithubUserDict(TypedDict): + login: str + id: int + node_id: str + avatar_url: str + gravatar_id: str + url: str + html_url: str + followers_url: str + following_url: str + gists_url: str + starred_url: str + subscriptions_url: str + organizations_url: str + repos_url: str + events_url: str + received_events_url: str + type: str + user_view_type: str + site_admin: bool + + +class GitHubAuthorDict(TypedDict): + name: str + email: str + date: str + + +class GitHubTreeDict(TypedDict): + sha: str + url: str + + +class GitHubParentDict(TypedDict): + sha: str + url: str + html_url: str + + +class GitHubVerificationDict(TypedDict): + verified: bool + reason: str + signature: str + payload: str + verified_at: str + + +class GitHubCommitEvent(TypedDict): + sha: str + node_id: str + url: str + html_url: str + author: GitHubAuthorDict + committer: GitHubAuthorDict + tree: GitHubTreeDict + message: str + parents: List[GitHubParentDict] + verification: Optional[GitHubVerificationDict] + event: str + + +class GitHubCommentEvent(TypedDict): + url: str + html_url: str + issue_url: str + id: int + node_id: str + user: GithubUserDict + created_at: str + updated_at: str + author_association: str + body: str + event: str + actor: GithubUserDict + + +class GitHubIssueEvent(TypedDict): + id: int + node_id: str + url: str + actor: GithubUserDict + event: str + commit_id: Optional[str] + commit_url: Optional[str] + created_at: str + + +class GitHubReviewRequestedEvent(GitHubIssueEvent): + review_requester: GithubUserDict + requested_reviewer: GithubUserDict + + +class GitHubMergedEvent(GitHubIssueEvent): + pass + + +class GitHubClosedEvent(GitHubIssueEvent): + state_reason: Optional[str] + + +class GitHubLinksDict(TypedDict): + html: dict + pull_request: dict + + +class GitHubReviewEvent(TypedDict): + id: int + node_id: str + user: GithubUserDict + body: str + commit_id: str + submitted_at: str + state: str + html_url: str + pull_request_url: str + author_association: str + _links: GitHubLinksDict + event: str + + +class GitHubReadyForReviewEvent(GitHubIssueEvent): + pass + + +class GitHubSourceDict(TypedDict): + type: str + issue: dict + + +class GitHubCrossReferencedEvent(TypedDict): + actor: GithubUserDict + created_at: str + updated_at: str + source: GitHubSourceDict + event: str + + +class GitHubConvertToDraftEvent(GitHubIssueEvent): + pass + + +class GitHubHeadRefDeletedEvent(GitHubIssueEvent): + pass + + +class GitHubHeadRefForcePushedEvent(GitHubIssueEvent): + before: str + after: str + + +class GitHubLabeledEvent(GitHubIssueEvent): + label: dict + + +class GitHubUnlabeledEvent(GitHubIssueEvent): + label: dict + + +GitHubPullTimelineEvent = Union[ + GitHubCommitEvent, + GitHubCommentEvent, + GitHubIssueEvent, + GitHubReviewRequestedEvent, + GitHubReadyForReviewEvent, + GitHubMergedEvent, + GitHubClosedEvent, + GitHubReviewEvent, + GitHubCrossReferencedEvent, + GitHubConvertToDraftEvent, + GitHubHeadRefDeletedEvent, + GitHubHeadRefForcePushedEvent, + GitHubLabeledEvent, + GitHubUnlabeledEvent, +] + + +class GitHubPrTimelineEventsDict(TypedDict): + event: str + data: GitHubPullTimelineEvent diff --git a/backend/analytics_server/mhq/service/code/sync/etl_code_analytics.py b/backend/analytics_server/mhq/service/code/sync/etl_code_analytics.py index 45a1a0b05..882906914 100644 --- a/backend/analytics_server/mhq/service/code/sync/etl_code_analytics.py +++ b/backend/analytics_server/mhq/service/code/sync/etl_code_analytics.py @@ -1,6 +1,7 @@ from datetime import timedelta from typing import List +from mhq.store.models.code.enums import PullRequestEventType from mhq.service.code.sync.models import PRPerformance from mhq.store.models.code import ( PullRequest, @@ -59,25 +60,42 @@ def create_pr_metrics( @staticmethod def get_pr_performance(pr: PullRequest, pr_events: [PullRequestEvent]): - pr_events.sort(key=lambda x: x.created_at) - first_review = pr_events[0] if pr_events else None + + review_events = [ + event + for event in pr_events + if event.type == PullRequestEventType.REVIEW.value + ] + review_events.sort(key=lambda x: x.created_at) + first_review = review_events[0] if review_events else None + + ready_for_review_events = [ + event + for event in pr_events + if event.type == PullRequestEventType.READY_FOR_REVIEW.value + ] + ready_for_review_events.sort(key=lambda x: x.created_at) + first_ready_for_review = ( + ready_for_review_events[0] if ready_for_review_events else None + ) + approved_reviews = list( filter( - lambda x: x.data["state"] == PullRequestEventState.APPROVED.value, - pr_events, + lambda x: x.data.get("state") == PullRequestEventState.APPROVED.value, + review_events, ) ) blocking_reviews = list( filter( - lambda x: x.data["state"] != PullRequestEventState.APPROVED.value, - pr_events, + lambda x: x.data.get("state") != PullRequestEventState.APPROVED.value, + review_events, ) ) if not approved_reviews: rework_time = -1 else: - if first_review.data["state"] == PullRequestEventState.APPROVED.value: + if first_review.data.get("state") == PullRequestEventState.APPROVED.value: rework_time = 0 else: rework_time = ( @@ -93,13 +111,21 @@ def get_pr_performance(pr: PullRequest, pr_events: [PullRequestEvent]): # Prevent garbage state when PR is approved post merging merge_time = -1 if merge_time < 0 else merge_time - cycle_time = pr.state_changed_at - pr.created_at + pull_request_ready_for_review_time = ( + first_ready_for_review.created_at + if first_ready_for_review + else pr.created_at + ) + + cycle_time = pr.state_changed_at - pull_request_ready_for_review_time if isinstance(cycle_time, timedelta): cycle_time = cycle_time.total_seconds() return PRPerformance( first_review_time=( - (first_review.created_at - pr.created_at).total_seconds() + ( + first_review.created_at - pull_request_ready_for_review_time + ).total_seconds() if first_review else -1 ), @@ -117,30 +143,34 @@ def get_rework_cycles( pr_events: [PullRequestEvent], pr_commits: [PullRequestCommit], ) -> int: + review_events = [ + review_event + for review_event in pr_events + if review_event.type == PullRequestEventType.REVIEW.value + ] - if not pr_events: + if not review_events: return 0 if not pr_commits: return 0 - - pr_events.sort(key=lambda x: x.created_at) + review_events.sort(key=lambda x: x.created_at) pr_commits.sort(key=lambda x: x.created_at) first_blocking_review = None last_relevant_approval_review = None pr_reviewers = dict.fromkeys(pr.reviewers, True) - for pr_event in pr_events: + for review_event in review_events: if ( - pr_event.state != PullRequestEventState.APPROVED.value - and pr_reviewers.get(pr_event.actor_username) + review_event.data.get("state") != PullRequestEventState.APPROVED.value + and pr_reviewers.get(review_event.actor_username) and not first_blocking_review ): - first_blocking_review = pr_event + first_blocking_review = review_event - if pr_event.state == PullRequestEventState.APPROVED.value: - last_relevant_approval_review = pr_event + if review_event.data.get("state") == PullRequestEventState.APPROVED.value: + last_relevant_approval_review = review_event break if not first_blocking_review: @@ -153,7 +183,6 @@ def get_rework_cycles( first_blocking_review.created_at - timedelta(seconds=1), last_relevant_approval_review.created_at, ) - pr_commits = list( filter( lambda x: x.created_at in interval, @@ -163,11 +192,11 @@ def get_rework_cycles( pr_reviewers = dict.fromkeys(pr.reviewers, True) blocking_reviews = list( filter( - lambda x: x.state != PullRequestEventState.APPROVED.value + lambda x: x.data.get("state") != PullRequestEventState.APPROVED.value and x.actor_username != pr.author and pr_reviewers.get(x.actor_username) and x.created_at in interval, - pr_events, + review_events, ) ) all_events = sorted(pr_commits + blocking_reviews, key=lambda x: x.created_at) diff --git a/backend/analytics_server/mhq/service/code/sync/etl_github_handler.py b/backend/analytics_server/mhq/service/code/sync/etl_github_handler.py index 61a06f8d1..261ad10af 100644 --- a/backend/analytics_server/mhq/service/code/sync/etl_github_handler.py +++ b/backend/analytics_server/mhq/service/code/sync/etl_github_handler.py @@ -6,7 +6,6 @@ from mhq.utils.github import get_custom_github_domain from github.PaginatedList import PaginatedList as GithubPaginatedList from github.PullRequest import PullRequest as GithubPullRequest -from github.PullRequestReview import PullRequestReview as GithubPullRequestReview from github.Repository import Repository as GithubRepository from mhq.exapi.github import GithubApiService @@ -16,6 +15,7 @@ RevertPRsGitHubSyncHandler, get_revert_prs_github_sync_handler, ) +from mhq.exapi.models.github_timeline import GithubPullRequestTimelineEvents from mhq.store.models import UserIdentityProvider from mhq.store.models.code import ( OrgRepo, @@ -104,7 +104,6 @@ def get_repo_pull_requests_data( github_pull_requests: GithubPaginatedList = self._api.get_pull_requests( github_repo ) - prs_to_process = [] for page in range( 0, github_pull_requests.totalCount // PR_PROCESSING_CHUNK_SIZE + 1, 1 @@ -170,10 +169,17 @@ def process_pr( ) pr_commits_model_list: List = [] - reviews: List[GithubPullRequestReview] = list(self._api.get_pr_reviews(pr)) + timeline_pr_events = self._api.get_pr_timeline_events( + pr.base.repo.full_name, pr.number + ) + reviews = [ + review + for review in timeline_pr_events + if (review.type == PullRequestEventType.REVIEW) + ] pr_model: PullRequest = self._to_pr_model(pr, pr_model, repo_id, len(reviews)) pr_events_model_list: List[PullRequestEvent] = self._to_pr_events( - reviews, pr_model, pr_event_model_list + timeline_pr_events, pr_model, pr_event_model_list ) if pr.merged_at: commits: List[Dict] = list( @@ -283,28 +289,23 @@ def _get_state(pr: GithubPullRequest) -> PullRequestState: @staticmethod def _to_pr_events( - reviews: [GithubPullRequestReview], + timeline_events: List[GithubPullRequestTimelineEvents], pr_model: PullRequest, - pr_events_model: [PullRequestEvent], + pr_events_model: List[PullRequestEvent], ) -> List[PullRequestEvent]: pr_events: List[PullRequestEvent] = [] pr_event_id_map = {event.idempotency_key: event.id for event in pr_events_model} - for review in reviews: - if not review.submitted_at: - continue # Discard incomplete reviews - - actor = review.raw_data.get("user", {}) - username = actor.get("login", "") if actor else "" - + for event in timeline_events: + username = event.user pr_events.append( PullRequestEvent( - id=pr_event_id_map.get(str(review.id), uuid.uuid4()), + id=pr_event_id_map.get(str(event.id), uuid.uuid4()), pull_request_id=str(pr_model.id), - type=PullRequestEventType.REVIEW.value, - data=review.raw_data, - created_at=review.submitted_at.replace(tzinfo=pytz.UTC), - idempotency_key=str(review.id), + type=event.type.value, + data=event.raw_data, + created_at=event.timestamp, + idempotency_key=str(event.id), org_repo_id=pr_model.repo_id, actor_username=username, ) diff --git a/backend/analytics_server/mhq/store/models/code/enums.py b/backend/analytics_server/mhq/store/models/code/enums.py index 5494e536f..032854c91 100644 --- a/backend/analytics_server/mhq/store/models/code/enums.py +++ b/backend/analytics_server/mhq/store/models/code/enums.py @@ -22,13 +22,33 @@ class PullRequestState(Enum): class PullRequestEventState(Enum): - CHANGES_REQUESTED = "CHANGES_REQUESTED" - APPROVED = "APPROVED" - COMMENTED = "COMMENTED" + CHANGES_REQUESTED = "changes_requested" + APPROVED = "approved" + COMMENTED = "commented" class PullRequestEventType(Enum): + ASSIGNED = "ASSIGNED" + CLOSED = "CLOSED" + COMMENTED = "COMMENTED" + COMMITTED = "COMMITTED" + CONVERT_TO_DRAFT = "CONVERT_TO_DRAFT" + HEAD_REF_DELETED = "HEAD_REF_DELETED" + HEAD_REF_FORCE_PUSHED = "HEAD_REF_FORCE_PUSHED" + LABELED = "LABELED" + LOCKED = "LOCKED" + MERGED = "MERGED" + READY_FOR_REVIEW = "READY_FOR_REVIEW" + REFERENCED = "REFERENCED" + REOPENED = "REOPENED" + REVIEW_DISMISSED = "REVIEW_DISMISSED" + REVIEW_REQUESTED = "REVIEW_REQUESTED" + REVIEW_REQUEST_REMOVED = "REVIEW_REQUEST_REMOVED" REVIEW = "REVIEW" + UNASSIGNED = "UNASSIGNED" + UNLABELED = "UNLABELED" + UNLOCKED = "UNLOCKED" + UNKNOWN = "UNKNOWN" class PullRequestRevertPRMappingActorType(Enum): diff --git a/backend/analytics_server/mhq/utils/time.py b/backend/analytics_server/mhq/utils/time.py index a16313504..6f62f5bcd 100644 --- a/backend/analytics_server/mhq/utils/time.py +++ b/backend/analytics_server/mhq/utils/time.py @@ -273,5 +273,30 @@ def fill_missing_week_buckets( def dt_from_iso_time_string(j_str_dt) -> Optional[datetime]: if not j_str_dt: return None - dt_without_timezone = datetime.strptime(j_str_dt, "%Y-%m-%dT%H:%M:%S.%f%z") - return dt_without_timezone.astimezone(pytz.UTC) + datetime_formats = [ + "%Y-%m-%dT%H:%M:%S.%f%z", + "%Y-%m-%dT%H:%M:%S%z", + "%Y-%m-%dT%H:%M:%S.%fZ", + ] + normalized_dt_str = ( + j_str_dt.replace("Z", "+00:00") if j_str_dt.endswith("Z") else j_str_dt + ) + for fmt in datetime_formats: + try: + if "%z" in fmt: + dt_without_timezone = datetime.strptime(normalized_dt_str, fmt) + else: + dt_without_timezone = datetime.strptime(j_str_dt, fmt).replace( + tzinfo=pytz.UTC + ) + + return dt_without_timezone.astimezone(pytz.UTC) + except ValueError: + continue + try: + dt = datetime.fromisoformat(normalized_dt_str) + if dt.tzinfo is None: + dt = dt.replace(tzinfo=pytz.UTC) + return dt.astimezone(pytz.UTC) + except (ValueError, AttributeError): + return None diff --git a/backend/analytics_server/tests/factories/models/code.py b/backend/analytics_server/tests/factories/models/code.py index 793dc2624..493aaa987 100644 --- a/backend/analytics_server/tests/factories/models/code.py +++ b/backend/analytics_server/tests/factories/models/code.py @@ -13,6 +13,7 @@ PullRequestEvent, PullRequestEventType, PullRequestState, + PullRequestEventState, PullRequest, RepoWorkflowRuns, RepoWorkflowRunsStatus, @@ -94,7 +95,7 @@ def get_pull_request_event( data=( { "user": {"login": reviewer or "User"}, - "state": state or "APPROVED", + "state": state or PullRequestEventState.APPROVED.value, "author_association": "NONE", } if not data diff --git a/backend/analytics_server/tests/factories/models/exapi/github.py b/backend/analytics_server/tests/factories/models/exapi/github.py index 2f5615acc..e0f38aa80 100644 --- a/backend/analytics_server/tests/factories/models/exapi/github.py +++ b/backend/analytics_server/tests/factories/models/exapi/github.py @@ -3,6 +3,7 @@ from datetime import datetime from typing import Dict +from mhq.exapi.models.github_timeline import GithubPullRequestTimelineEvents from mhq.utils.time import time_now @@ -157,3 +158,27 @@ def get_github_workflow_run_dict( "updated_at": updated_at, "html_url": html_url, } + + +def get_github_pr_timeline_event( + event_id: str = "123456", + user_login: str = "abc", + event_type: str = "reviewed", + timestamp: datetime = None, + raw_data: Dict = None, +) -> GithubPullRequestTimelineEvents: + if timestamp is None: + timestamp = time_now() + + if raw_data is None: + raw_data = { + "id": event_id, + "user": {"login": user_login}, + "submitted_at": timestamp.isoformat(), + "state": "approved", + } + + return GithubPullRequestTimelineEvents( + event_type=event_type, + data=raw_data, + ) diff --git a/backend/analytics_server/tests/service/code/sync/test_etl_code_analytics.py b/backend/analytics_server/tests/service/code/sync/test_etl_code_analytics.py index 764bcd260..3ae87bd8d 100644 --- a/backend/analytics_server/tests/service/code/sync/test_etl_code_analytics.py +++ b/backend/analytics_server/tests/service/code/sync/test_etl_code_analytics.py @@ -2,6 +2,7 @@ from mhq.service.code.sync.etl_code_analytics import CodeETLAnalyticsService from mhq.store.models.code import PullRequestState, PullRequestEventState +from mhq.store.models.code.enums import PullRequestEventType from mhq.utils.time import time_now from tests.factories.models.code import ( get_pull_request, @@ -605,3 +606,141 @@ def test_create_pr_metrics_bot_detection_in_review_events(): for bot_reviewer in bot_reviewers: assert bot_reviewer not in pr_metrics.reviewers + + +def test_cycle_time_uses_ready_for_review_when_available(): + """Test that cycle time uses first ready_for_review event as start time when available.""" + pr_service = CodeETLAnalyticsService() + t1 = time_now() + t2 = t1 + timedelta(hours=2) + t3 = t2 + timedelta(days=1) + + pr = get_pull_request( + state=PullRequestState.MERGED, created_at=t1, state_changed_at=t3, updated_at=t3 + ) + + ready_for_review_event = get_pull_request_event( + pull_request_id=pr.id, + type=PullRequestEventType.READY_FOR_REVIEW.value, + created_at=t2, + ) + + performance = pr_service.get_pr_performance(pr, [ready_for_review_event]) + + assert performance.cycle_time == 86400 + + +def test_cycle_time_falls_back_to_created_at_when_no_ready_for_review(): + """Test that cycle time uses PR creation time when no ready_for_review events exist.""" + pr_service = CodeETLAnalyticsService() + t1 = time_now() + t2 = t1 + timedelta(days=1) + + pr = get_pull_request( + state=PullRequestState.MERGED, created_at=t1, state_changed_at=t2, updated_at=t2 + ) + + performance = pr_service.get_pr_performance(pr, []) + + assert performance.cycle_time == 86400 + + +def test_cycle_time_uses_earliest_ready_for_review_when_multiple(): + """Test that cycle time uses the earliest ready_for_review event when multiple exist.""" + pr_service = CodeETLAnalyticsService() + t1 = time_now() + t2 = t1 + timedelta(hours=1) + t3 = t1 + timedelta(hours=3) + t4 = t1 + timedelta(days=1) + + pr = get_pull_request( + state=PullRequestState.MERGED, created_at=t1, state_changed_at=t4, updated_at=t4 + ) + + ready_for_review_1 = get_pull_request_event( + pull_request_id=pr.id, + type=PullRequestEventType.READY_FOR_REVIEW.value, + created_at=t2, + ) + + ready_for_review_2 = get_pull_request_event( + pull_request_id=pr.id, + type=PullRequestEventType.READY_FOR_REVIEW.value, + created_at=t3, + ) + + performance = pr_service.get_pr_performance( + pr, [ready_for_review_2, ready_for_review_1] + ) + + assert performance.cycle_time == 82800 + + +def test_first_response_time_uses_ready_for_review_when_available(): + """Test that first response time uses ready_for_review as start time when available.""" + pr_service = CodeETLAnalyticsService() + t1 = time_now() + t2 = t1 + timedelta(hours=2) + t3 = t2 + timedelta(hours=1) + + pr = get_pull_request(created_at=t1, updated_at=t1) + + ready_for_review_event = get_pull_request_event( + pull_request_id=pr.id, + type=PullRequestEventType.READY_FOR_REVIEW.value, + created_at=t2, + ) + + review_event = get_pull_request_event( + pull_request_id=pr.id, type=PullRequestEventType.REVIEW.value, created_at=t3 + ) + + performance = pr_service.get_pr_performance( + pr, [ready_for_review_event, review_event] + ) + assert performance.first_review_time == 3600 + + +def test_first_response_time_falls_back_to_created_at_when_no_ready_for_review(): + """Test that first response time uses PR creation time when no ready_for_review events exist.""" + pr_service = CodeETLAnalyticsService() + t1 = time_now() + t2 = t1 + timedelta(hours=3) + pr = get_pull_request(created_at=t1, updated_at=t1) + + review_event = get_pull_request_event( + pull_request_id=pr.id, type=PullRequestEventType.REVIEW.value, created_at=t2 + ) + + performance = pr_service.get_pr_performance(pr, [review_event]) + assert performance.first_review_time == 10800 + + +def test_cycle_time_ready_for_review_with_draft_pr_workflow(): + """Test cycle time calculation for draft PR workflow with ready_for_review event.""" + pr_service = CodeETLAnalyticsService() + t1 = time_now() + t2 = t1 + timedelta(days=2) # ready for review 2 days after creation (draft period) + t3 = t2 + timedelta(hours=4) # first review + t4 = t2 + timedelta(days=1) # merged + + pr = get_pull_request( + state=PullRequestState.MERGED, created_at=t1, state_changed_at=t4, updated_at=t4 + ) + + ready_for_review_event = get_pull_request_event( + pull_request_id=pr.id, + type=PullRequestEventType.READY_FOR_REVIEW.value, + created_at=t2, + ) + + review_event = get_pull_request_event( + pull_request_id=pr.id, type=PullRequestEventType.REVIEW.value, created_at=t3 + ) + + performance = pr_service.get_pr_performance( + pr, [ready_for_review_event, review_event] + ) + + assert performance.cycle_time == 86400 + assert performance.first_review_time == 14400 diff --git a/backend/analytics_server/tests/service/code/sync/test_etl_github_handler.py b/backend/analytics_server/tests/service/code/sync/test_etl_github_handler.py index b2344e24c..41844b52b 100644 --- a/backend/analytics_server/tests/service/code/sync/test_etl_github_handler.py +++ b/backend/analytics_server/tests/service/code/sync/test_etl_github_handler.py @@ -12,8 +12,8 @@ ) from tests.factories.models.exapi.github import ( get_github_commit_dict, - get_github_pull_request_review, get_github_pull_request, + get_github_pr_timeline_event, ) from tests.utilities import compare_objects_as_dicts @@ -199,8 +199,16 @@ def test__to_pr_events_given_an_empty_list_of_events_returns_an_empty_list(): def test__to_pr_events_given_a_list_of_only_new_events_returns_a_list_of_pr_events(): pr_model = get_pull_request() - event1 = get_github_pull_request_review() - event2 = get_github_pull_request_review() + event1 = get_github_pr_timeline_event( + event_id="123456", + user_login="user1", + event_type="reviewed", + ) + event2 = get_github_pr_timeline_event( + event_id="789012", + user_login="user2", + event_type="reviewed", + ) events = [event1, event2] pr_events = GithubETLHandler._to_pr_events(events, pr_model, []) @@ -210,19 +218,19 @@ def test__to_pr_events_given_a_list_of_only_new_events_returns_a_list_of_pr_even pull_request_id=str(pr_model.id), org_repo_id=pr_model.repo_id, data=event1.raw_data, - created_at=event1.submitted_at, + created_at=event1.timestamp, type="REVIEW", idempotency_key=event1.id, - reviewer=event1.user_login, + reviewer=event1.user, ), get_pull_request_event( pull_request_id=str(pr_model.id), org_repo_id=pr_model.repo_id, data=event2.raw_data, - created_at=event2.submitted_at, + created_at=event2.timestamp, type="REVIEW", idempotency_key=event2.id, - reviewer=event2.user_login, + reviewer=event2.user, ), ] @@ -232,18 +240,26 @@ def test__to_pr_events_given_a_list_of_only_new_events_returns_a_list_of_pr_even def test__to_pr_events_given_a_list_of_new_events_and_old_events_returns_a_list_of_pr_events(): pr_model = get_pull_request() - event1 = get_github_pull_request_review() - event2 = get_github_pull_request_review() + event1 = get_github_pr_timeline_event( + event_id="123456", + user_login="user1", + event_type="reviewed", + ) + event2 = get_github_pr_timeline_event( + event_id="789012", + user_login="user2", + event_type="reviewed", + ) events = [event1, event2] old_event = get_pull_request_event( pull_request_id=str(pr_model.id), org_repo_id=pr_model.repo_id, data=event1.raw_data, - created_at=event1.submitted_at, + created_at=event1.timestamp, type="REVIEW", idempotency_key=event1.id, - reviewer=event1.user_login, + reviewer=event1.user, ) pr_events = GithubETLHandler._to_pr_events(events, pr_model, [old_event]) @@ -254,10 +270,10 @@ def test__to_pr_events_given_a_list_of_new_events_and_old_events_returns_a_list_ pull_request_id=str(pr_model.id), org_repo_id=pr_model.repo_id, data=event2.raw_data, - created_at=event2.submitted_at, + created_at=event2.timestamp, type="REVIEW", idempotency_key=event2.id, - reviewer=event2.user_login, + reviewer=event2.user, ), ]