Job#waitForCompletion returns before exports have completed

[louismrose]

Expected Behaviour

In the following:

$job = $this->client()->jobs->get(1234)->waitForCompletion();
foreach ($job->exports as $export) {
    print_r($export->hasCompleted());
}

true should always be printed for every export.

Current Behaviour

false can be printed. This can be the source of race conditions and subtle bugs (i.e., an app using zamzar-php signals to its users that their files are converted; but when the user attempts to download them from, say S3, they are not there yet).

Possible Solution

I think we should update the implementation of Job#hasCompleted as follows:

    public function hasCompleted()
    {
        return $this->hasJobCompleted() && $this->haveAllExportsCompleted();
    }

    protected function hasJobCompleted()
    {
        return $this->isStatusSuccessful() || $this->isStatusFailed() || $this->isStatusCancelled();
    }

    protected function haveAllExportsCompleted()
    {
        if (!$this->hasExports()) {
            return true;
        }

        /** @var Export $export */
        foreach ($this->exports as $export) {
            if (!$export->hasCompleted()) {
                return false;
            }
        }

        return true;
    }

Steps to Reproduce

The following proposed PHPUnit test case reproduces this issue:

// in JobsTest.php
    public function testWaitForCompletionRespectsExports() {
        $finishedJobWithExports = $this->client()->jobs
            ->get(MockResourceIds::SUCCEEDING_MULTI_OUTPUT_JOB_ID)
            ->waitForCompletion();

        // sanity check -- there should be at least 1 export
        $this->assertTrue($finishedJobWithExports->hasExports());

        foreach ($finishedJobWithExports->exports as $export) {
            $this->assertTrue(
                $export->status == Export::STATUS_SUCCESSFUL || $export->status == Export::STATUS_FAILED,
                "Export $export->id should have completed, but is: $export->status"
            );
        }
    }

// in a new MockResourceIds.php
<?php

namespace Tests;

interface MockResourceIds {
    // IDs below correspond to those defined in zamzar-mock
    // See: https://github.com/zamzar/zamzar-mock/blob/main/README.md

    const SUCCEEDING_JOB_ID = 1;
    const SUCCEEDING_MULTI_OUTPUT_JOB_ID = 2;
    const FAILING_JOB_ID = 3;
    const SUCCEEDING_IMPORT_ID = 1;
    const FAILING_IMPORT_ID = 2;
    const FILE_ID = 1;
}

Environment

• zamzar-php 2.0.3
• PHP 8.2.16

[louismrose]

This will likely be easier to implement once #15 is fixed.

[louismrose]

We discussed this as a team today, and are going to block this behind #15 (i.e., targeted for a v3 of this SDK).