Skip to content

Trac PRs: Keep stored PR data when GitHub requests fail #1078

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
obenland wants to merge 5 commits into from
Closed

Trac PRs: Keep stored PR data when GitHub requests fail #1078

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f77c370
Trac PRs: Keep stored PR data when GitHub requests fail
obenland Oct 20, 2026
d860c48
Trac PRs: Link new pull requests when only part of their details load
obenland Oct 20, 2026
fbdec4c
Trac PRs: Fetch partial pull request details through their own function
obenland Oct 20, 2026
2bb9da2
Trac PRs: Shorten the filter that drops never fetched pull requests
obenland Oct 20, 2026
3e4dcf0
Trac PRs: Declare types in the new tests and unstack comments
obenland Oct 20, 2026
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
156 changes: 114 additions & 42 deletions api.wordpress.org/public_html/dotorg/trac/pr/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,32 @@

/**
* Fetches and reformats the Github PR API response to the details we need.
*
* @param string $repo The repository, as `owner/name`.
* @param int $pr The PR number.
* @return object|false The PR's details, or false if any of them could not be fetched.
*/
function fetch_pr_data( $repo, $pr ) {
$details = fetch_partial_pr_data( $repo, $pr );
if ( ! $details || $details->partial ) {
return false;
}

unset( $details->partial );

return $details;
}

/**
* Fetches a PR's details, even if only the PR itself could be fetched.
*
* The lists that failed are then empty and `partial` is set, so the details must not replace stored ones.
*
* @param string $repo The repository, as `owner/name`.
* @param int $pr The PR number.
* @return object|false The PR's details, or false if the PR could not be fetched.
*/
function fetch_partial_pr_data( $repo, $pr ) {
$url = '/repos/' . $repo . '/pulls/' . intval( $pr );
$data = api_request( $url );

Expand All @@ -13,65 +37,63 @@ function fetch_pr_data( $repo, $pr ) {
return false;
}

// Get Travis CI State.
$check_runs = [];
$raw_check_runs = api_request(
'/repos/' . $repo . '/commits/' . $data->head->sha . '/check-runs',
null,
[ 'Accept: application/vnd.github.antiope-preview+json' ]
);
if ( ! empty( $raw_check_runs->check_runs ) ) {
foreach ( $raw_check_runs->check_runs as $check ) {
switch ( $check->status ) {
case 'queued':
case 'in_progress':
$check_runs[ $check->app->name ] = 'in_progress';
break;
case 'completed':
switch( $check->conclusion ) {
case 'success':
$check_runs[ $check->app->name ] = 'success';
break;
case 'failure':
$check_runs[ $check->app->name ] = 'failed';
break;
case 'action_required':
$check_runs[ $check->app->name ] = $check->output->title;
break;
}
}
}
}
$_check_runs = $raw_check_runs->check_runs ?? null;

$_reviews = api_request(
'/repos/' . $repo . '/pulls/' . intval( $pr ) . '/reviews',
null,
[ 'Accept: application/vnd.github.antiope-preview+json' ]
);
$reviews = [];
if ( $_reviews ) {
foreach ( $_reviews as $r ) {
if (
in_array( $r->state, [ 'CHANGES_REQUESTED', 'APPROVED' ] ) &&
! in_array( $r->user->login, $reviews[ $r->state ] ?? [], true )
) {
$reviews[ $r->state ][] = $r->user->login;
}
}
}

$touches_tests = false;
$_files = api_request(
'/repos/' . $repo . '/pulls/' . intval( $pr ) . '/files?per_page=999',
'/repos/' . $repo . '/pulls/' . intval( $pr ) . '/files?per_page=100',
null,
[ 'Accept: application/vnd.github.antiope-preview+json' ]
);
if ( $_files ) {
foreach ( $_files as $f ) {
if ( preg_match( '!(^tests/|/tests/)!', $f->filename ) ) {
$touches_tests = true;

// Get Travis CI State.
$check_runs = [];
foreach ( is_array( $_check_runs ) ? $_check_runs : [] as $check ) {
switch ( $check->status ) {
case 'queued':
case 'in_progress':
$check_runs[ $check->app->name ] = 'in_progress';
break;
}
case 'completed':
switch ( $check->conclusion ) {
case 'success':
$check_runs[ $check->app->name ] = 'success';
break;
case 'failure':
$check_runs[ $check->app->name ] = 'failed';
break;
case 'action_required':
$check_runs[ $check->app->name ] = $check->output->title;
break;
}
}
}

$reviews = [];
foreach ( is_array( $_reviews ) ? $_reviews : [] as $r ) {
if (
in_array( $r->state, [ 'CHANGES_REQUESTED', 'APPROVED' ] ) &&
! in_array( $r->user->login, $reviews[ $r->state ] ?? [], true )
) {
$reviews[ $r->state ][] = $r->user->login;
}
}

$touches_tests = false;
foreach ( is_array( $_files ) ? $_files : [] as $f ) {
if ( preg_match( '!(^tests/|/tests/)!', $f->filename ) ) {
$touches_tests = true;
break;
}
}

Expand Down Expand Up @@ -100,9 +122,59 @@ function fetch_pr_data( $repo, $pr ) {
'html_url' => $data->html_url,
],
'trac_ticket' => determine_trac_ticket( $data ),
// A failed request returns GitHub's error object instead of a list.
'partial' => ! is_array( $_check_runs ) || ! is_array( $_reviews ) || ! is_array( $_files ),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
];
}

/**
* Picks the linked PRs whose data is due a refresh from GitHub.
*
* Each refresh is four GitHub API requests, so a ticket with many PRs refreshes only the stalest few per request.
*
* @param array $prs Rows of `trac_github_prs`, with `data` decoded.
* @param bool $authenticated Whether the request is from a logged in user.
* @param int $limit The most PRs to pick.
* @return array The PRs to refresh, never fetched ones first, then the stalest.
*/
function select_prs_to_refresh( $prs, $authenticated, $limit = 10 ) {
$stale = array_filter(
$prs,
function ( $data ) use ( $authenticated ) {
if ( ! $data->data ) {
return true;
}

$checked_ago = time() - strtotime( $data->last_checked );

// Once a day, the webhook refreshes them when they're reopened.
if ( 'closed' === $data->data->state ) {
return $checked_ago >= 24 * 60 * 60;
}

// 5 minutes for logged in requests, 60 mins for unauthenticated.
if ( $checked_ago >= ( $authenticated ? 5 * 60 : 60 * 60 ) ) {
return true;
}

// Every two minutes while the PR was created/updated in the last half hour, or its unit tests are running.
$recently_updated = strtotime( $data->data->updated_at ) > time() - 30 * 60;
$tests_running = in_array( 'in_progress', (array) $data->data->check_runs, true );

return $checked_ago >= 2 * 60 && ( $recently_updated || $tests_running );
}
);

usort(
$stale,
function ( $a, $b ) {
return ( $a->data ? strtotime( $a->last_checked ) : 0 ) <=> ( $b->data ? strtotime( $b->last_checked ) : 0 );
}
);

return array_slice( $stale, 0, $limit );
}

/**
* Find a WordPress.org user by a Github login.
*/
Expand Down
88 changes: 34 additions & 54 deletions api.wordpress.org/public_html/dotorg/trac/pr/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,63 +54,43 @@
return $data;
} );

// Refresh any data that's needed
// Rules:
// - 5 minutes for logged in requests, 60 mins for unauthenticated.
// - If PR created/updated in last half hour, every two minutes
array_walk( $prs, function( $data ) use ( $authenticated ) {
global $wpdb;

if (
// If no data..
! $data->data ||
// or it's out of date..
strtotime( $data->last_checked ) <= time() - ($authenticated ? 5*60 : 60*60) ||
// or the PR was created/updated within the last 30 minutes AND is more than 2 minutes out of date
(
strtotime( $data->data->updated_at ) > time() - 30*60
&&
strtotime( $data->last_checked ) <= time() - 2*60
) ||
// or unit tests are running, then 2min.
(
$data->data->check_runs
&&
in_array( 'in_progress', (array) $data->data->check_runs )
&&
strtotime( $data->last_checked ) <= time() - 2*60
)
) {
$pr_data = fetch_pr_data( $data->repo, $data->pr );

if ( $pr_data ) {
$data->data = $pr_data;

// TODO: catch the trac ticket changing and update the database.
unset( $data->data->trac_ticket );

// Check if we now have an author for this PR, the author may link their account after creating the PR.
if ( ! $data->author ) {
$data->author = (int) find_wporg_user_by_github( $pr_data->user->name, 'ID' );
}

$wpdb->update(
'trac_github_prs',
[
'data' => json_encode( $pr_data ),
'last_checked' => gmdate( 'Y-m-d H:i:s' ),
'author' => $data->author,
],
[
'repo' => $data->repo,
'pr' => $data->pr
]
);
// Refresh any data that's needed.
foreach ( select_prs_to_refresh( $prs, $authenticated ) as $data ) {
$pr_data = fetch_pr_data( $data->repo, $data->pr );

if ( $pr_data ) {
Comment thread
obenland marked this conversation as resolved.
$data->data = $pr_data;

// TODO: catch the trac ticket changing and update the database.
unset( $data->data->trac_ticket );

// Check if we now have an author for this PR, the author may link their account after creating the PR.
if ( ! $data->author ) {
$data->author = (int) find_wporg_user_by_github( $pr_data->user->name, 'ID' );
}

$wpdb->update(
'trac_github_prs',
[
'data' => json_encode( $pr_data ),
'last_checked' => gmdate( 'Y-m-d H:i:s' ),
'author' => $data->author,
],
[
'repo' => $data->repo,
'pr' => $data->pr,
]
);
}
}

return $data;
} );
// PRs that have never been fetched have nothing to show yet.
$prs = array_filter(
$prs,
function ( $data ) {
return $data->data;
}
);

// Expiry is an hour for everyone..
// ..unless authenticated and a linked PR has changed within the last week, then 5 min.
Expand Down
Loading
Loading