Skip to content

Merge review changes from #8 to trunk #14

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abdullah-kasim wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

Merge review changes from #8 to trunk #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
81ea99b
Simplify fire_ingestion_failure
abdullah-kasim Dec 5, 2025
bcb8d0f
Merge pull request #8 from Automattic/vip-2449-ingest-api-vip_agentfo…
abdullah-kasim Dec 5, 2025
3296664
Merge remote-tracking branch 'origin/trunk' into vip-2448-ingest-api-…
abdullah-kasim Dec 29, 2025
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
69 changes: 32 additions & 37 deletions modules/ingestion/class-ingestion.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,23 +72,7 @@ public static function ingest_post( int $post_id, \WP_Post $post ): void {
]
);

self::fire_ingestion_failure(
new Ingestion_Failure(
[
'failure_code' => Ingestion_Failure::CODE_TRANSFORM_FAILED,
'post' => $post,
'error' => new \WP_Error(
'vip_agentforce_transform_failed',
'Post transformation returned null',
[
'post_id' => $post_id,
// phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_debug_backtrace -- Intentional for error tracing.
'backtrace' => debug_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS, 5 ),
]
),
]
)
);
self::fire_ingestion_failure( $post_id, Ingestion_Failure::CODE_TRANSFORM_FAILED );
return;
}

Expand All @@ -104,24 +88,7 @@ public static function ingest_post( int $post_id, \WP_Post $post ): void {
]
);

self::fire_ingestion_failure(
new Ingestion_Failure(
[
'failure_code' => Ingestion_Failure::CODE_API_ERROR,
'post' => $post,
'error' => new \WP_Error(
'vip_agentforce_api_error',
$response['error_message'] ?? 'API call failed',
[
'post_id' => $post_id,
'response' => $response,
// phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_debug_backtrace -- Intentional for error tracing.
'backtrace' => debug_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS, 5 ),
]
),
]
)
);
self::fire_ingestion_failure( $post_id, Ingestion_Failure::CODE_API_ERROR, [ 'response' => $response ] );
return;
}

Expand All @@ -140,9 +107,37 @@ public static function ingest_post( int $post_id, \WP_Post $post ): void {
/**
* Fire the ingestion failure action.
*
* @param Ingestion_Failure $failure The ingestion failure.
* @param int $post_id The post ID that failed ingestion.
* @param string $failure_code One of the Ingestion_Failure::CODE_* constants.
* @param array<string, mixed> $details Optional additional details about the failure.
*/
private static function fire_ingestion_failure( Ingestion_Failure $failure ): void {
private static function fire_ingestion_failure( int $post_id, string $failure_code, array $details = [] ): void {
$post = get_post( $post_id );
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method now re-fetches the post object using get_post( $post_id ), but in both calling contexts (lines 75 and 91), the $post object is already available in the ingest_post() method's scope. This causes an unnecessary database query.

Consider updating the method signature to accept the \WP_Post object directly instead of just the post ID, or make it optional and only call get_post() if the post object isn't provided. For example:

private static function fire_ingestion_failure( int $post_id, string $failure_code, array $details = [], ?\WP_Post $post = null ): void {
    $post = $post ?? get_post( $post_id );
    // ...
}

Copilot uses AI. Check for mistakes.

$error_codes = [
Ingestion_Failure::CODE_TRANSFORM_FAILED => 'vip_agentforce_transform_failed',
Ingestion_Failure::CODE_API_ERROR => 'vip_agentforce_api_error',
];

$error_messages = [
Ingestion_Failure::CODE_TRANSFORM_FAILED => 'Post transformation failed',
Ingestion_Failure::CODE_API_ERROR => 'API call failed',
];
Comment on lines +122 to +125
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error messages map no longer uses the error_message from the API response. Previously, when an API error occurred, the code would use $response['error_message'] ?? 'API call failed', allowing the API to provide custom error messages. Now, all API errors will have the generic message "API call failed" regardless of what specific error the API returns. This loses valuable debugging information.

Consider either:

  1. Passing the error_message from the response through the details array and using it in the error message construction
  2. Updating the signature to accept an optional error_message parameter that overrides the default

Copilot uses AI. Check for mistakes.

$error_data = array_merge( [ 'post_id' => $post_id ], $details );

$failure = new Ingestion_Failure(
[
'failure_code' => $failure_code,
'post' => $post,
'error' => new \WP_Error(
$error_codes[ $failure_code ] ?? 'vip_agentforce_ingestion_failed',
$error_messages[ $failure_code ] ?? 'Ingestion failed',
$error_data
),
]
);

/**
* Fires when a post ingestion fails.
*
Expand Down
57 changes: 1 addition & 56 deletions tests/phpunit/test-ingestion-api-failure.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,61 +78,6 @@ function ( $failure ) use ( &$action_fired, &$received_failure ) {
$this->assertSame( $post->ID, $received_failure->post->ID );
$this->assertInstanceOf( WP_Error::class, $received_failure->error );
$this->assertSame( 'vip_agentforce_api_error', $received_failure->error->get_error_code() );
$this->assertSame( 'Simulated API failure', $received_failure->error->get_error_message() );
}

public function test_api_error_contains_response_in_error_data(): void {
$post = $this->factory()->post->create_and_get( [ 'post_status' => 'publish' ] );

add_filter( 'vip_agentforce_should_ingest_post', '__return_true' );
add_filter(
'vip_agentforce_transform_post',
// phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.FoundBeforeLastUsed -- Filter callback signature.
function ( $record, $filter_post ) {
return new Ingestion_Post_Record(
[
'site_id' => '1',
'blog_id' => '1',
'post_id' => (string) $filter_post->ID,
'site_id_blog_id' => '1_1',
'site_id_blog_id_post_id' => '1_1_' . $filter_post->ID,
'published' => true,
'last_published_at' => '2025-01-01T00:00:00+00:00',
'last_modified_at' => '2025-01-01T00:00:00+00:00',
'title' => $filter_post->post_title,
'content' => $filter_post->post_content,
'excerpt' => $filter_post->post_excerpt,
'categories' => '',
'tags' => '',
'author' => '',
'url' => 'https://example.com',
'post_type' => $filter_post->post_type,
'post_status' => $filter_post->post_status,
]
);
},
10,
2
);

/** @var Ingestion_Failure|null $received_failure */
$received_failure = null;

add_action(
'vip_agentforce_post_ingestion_failed',
function ( $failure ) use ( &$received_failure ) {
$received_failure = $failure;
}
);

Ingestion_With_Failing_Api::ingest_post( $post->ID, $post );

$this->assertInstanceOf( Ingestion_Failure::class, $received_failure );
$error_data = $received_failure->error->get_error_data();
$this->assertIsArray( $error_data );
$this->assertArrayHasKey( 'response', $error_data );
$this->assertIsArray( $error_data['response'] );
$this->assertFalse( $error_data['response']['success'] );
$this->assertArrayHasKey( 'backtrace', $error_data );
$this->assertSame( 'API call failed', $received_failure->error->get_error_message() );
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is now asserting that the error message is 'API call failed', but this doesn't match the actual error message returned by the API. The test double Ingestion_With_Failing_Api returns a response with 'error_message' => 'Simulated API failure'.

Before this change, the code would extract and use that specific error message. Now with the refactored error handling, the generic message 'API call failed' is always used. While this test passes with the new expected value, it means the test no longer verifies that API-specific error messages are preserved, and the valuable context from the API response is being lost.

Suggested change
$this->assertSame( 'API call failed', $received_failure->error->get_error_message() );
$this->assertSame( 'Simulated API failure', $received_failure->error->get_error_message() );

Copilot uses AI. Check for mistakes.
}
Comment on lines 78 to 82
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was removed which verified that error data includes response information and backtrace when an API error occurs. The test ensured that:

  1. The response array is included in error data (useful for debugging API issues)
  2. A backtrace is included in error data (useful for tracing where errors originated)

With the refactored fire_ingestion_failure() method, backtraces are no longer automatically added to error data. While the response is still passed through the $details array (line 91 in class-ingestion.php), there's no longer test coverage verifying this behavior. Consider adding back a test to verify that the response is properly included in the error data.

Copilot uses AI. Check for mistakes.
}
26 changes: 0 additions & 26 deletions tests/phpunit/test-ingestion.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,32 +372,6 @@ function () use ( &$action_fired ) {
$this->assertFalse( $action_fired, 'Action should NOT fire on successful ingestion.' );
}
Comment on lines 372 to 373
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test has been removed which verified that error data includes a backtrace for debugging purposes. The refactored fire_ingestion_failure() method no longer automatically includes backtraces in the error data. While removing automatic backtraces might be intentional to reduce noise, this represents a loss of debugging information that could be valuable for troubleshooting transformation failures. If backtraces are still needed, they should be re-added to the error data construction in the fire_ingestion_failure() method.

Copilot uses AI. Check for mistakes.

public function test_failure_error_contains_backtrace(): void {
$post = $this->factory()->post->create_and_get( [ 'post_status' => 'publish' ] );

add_filter( 'vip_agentforce_should_ingest_post', '__return_true' );
add_filter( 'vip_agentforce_transform_post', '__return_null' );

/** @var Ingestion_Failure|null $received_failure */
$received_failure = null;

add_action(
'vip_agentforce_post_ingestion_failed',
function ( $failure ) use ( &$received_failure ) {
$received_failure = $failure;
}
);

Ingestion::ingest_post( $post->ID, $post );

$this->assertInstanceOf( Ingestion_Failure::class, $received_failure );
$error_data = $received_failure->error->get_error_data();
$this->assertIsArray( $error_data );
$this->assertArrayHasKey( 'backtrace', $error_data );
$this->assertIsArray( $error_data['backtrace'] );
$this->assertNotEmpty( $error_data['backtrace'] );
}

public function test_failure_error_contains_post_id(): void {
$post = $this->factory()->post->create_and_get( [ 'post_status' => 'publish' ] );

Expand Down
Loading