Fixes for PUT on /projects/ unsetting client_id

This commit is contained in:
David Bomba
2026-05-03 20:32:16 +10:00
parent 16ad420cb2
commit ccbcaefbc3
10 changed files with 221 additions and 48 deletions

View File

@@ -57,7 +57,7 @@ use ReflectionMethod;
*/
class ImportElasticSearchableModels extends Command
{
private const LARGE_IMPORT_JOB_THRESHOLD = 10000;
private const LARGE_IMPORT_JOB_THRESHOLD = 20000;
private const OPTION_DATABASE = 'database';

View File

@@ -65,9 +65,7 @@ class UpdateProjectRequest extends Request
$this->files->set('file', [$this->file('file')]);
}
if (isset($input['client_id'])) {
unset($input['client_id']);
}
$input['client_id'] = $this->project->client_id;
if (array_key_exists('color', $input) && is_null($input['color'])) {
$input['color'] = '';

View File

@@ -801,17 +801,22 @@ class BaseTransformer
])
->first();
return $project ? $project->id : $this->createProject($name, $clientId);
if ($project) {
return $project->id;
}
if (! $clientId) {
return null;
}
return $this->createProject($name, $clientId);
}
private function createProject($name, $clientId)
private function createProject(string $name, int $clientId): int
{
$project = ProjectFactory::create($this->company->id, $this->company->owner()->id);
$project->name = $name;
if ($clientId) {
$project->client_id = $clientId;
}
$project->client_id = $clientId;
$project->saveQuietly();

View File

@@ -1878,6 +1878,12 @@ class Import implements ShouldQueue
$modified['client_id'] = $this->transformId('clients', $resource['client_id']);
}
if (! isset($modified['client_id'])) {
nlog('Skipping project import row without client_id (legacy id: ' . ($resource['id'] ?? '') . ')');
continue;
}
/** @var \App\Models\Project $project **/
$project = Project::create($modified);

View File

@@ -85,7 +85,8 @@ class InvoiceTransactionEventEntry
} elseif ($invoice->is_deleted) {
}
/** If the invoice hasn't changed its state... return early!! */ elseif (BcMath::comp($invoice->amount, $event->invoice_amount) == 0 || $event->period->format('Y-m-d') == $period) {
/** If the invoice hasn't changed its state... return early!! */
elseif (BcMath::comp($invoice->amount, $event->invoice_amount) == 0 || $event->period->format('Y-m-d') == $period) {
nlog("event period => {$period} => " . $event->period->format('Y-m-d'));
nlog("invoice amount => {$invoice->amount} => " . $event->invoice_amount);
nlog("apparently no change in amount or period");

View File

@@ -39,12 +39,6 @@ class ProcessPayment extends Component
$_context = $this->getContext($this->_key);
if (isset($_context['payment_processed'])) {
$this->payment_view = $_context['payment_processed']['payment_view'];
$this->payment_data_payload = $_context['payment_processed']['payment_data_payload'];
return;
}
$data = [
'company_gateway_id' => $_context['company_gateway_id'],
'payment_method_id' => $_context['gateway_type_id'],
@@ -100,11 +94,6 @@ class ProcessPayment extends Component
);
}
$this->setContext($this->_key, 'payment_processed', [
'payment_view' => $this->payment_view,
'payment_data_payload' => $this->payment_data_payload,
]);
}
public function placeholder(): string

View File

@@ -700,6 +700,10 @@ class QbInvoice implements SyncInterface
foreach ($payment_ids as $payment_id) {
if(!$payment_id) {
continue;
}
$payment = $this->service->sdk->FindById('Payment', $payment_id);
$payment_transformer = new PaymentTransformer($this->service->company);

View File

@@ -133,11 +133,17 @@ class TaxPeriodReport extends BaseExport
private function initializeData(): self
{
// First sweep: only active statuses (SENT/PARTIAL/PAID).
// CANCELLED/REVERSED/deleted invoices early-return without writing an
// event in InvoiceTransactionEventEntry::run, so including them here
// causes them to be reprocessed on every report run forever. The
// second sweep below handles their state transitions instead.
$q = Invoice::withTrashed()
->with('payments')
->where('company_id', $this->company->id)
->whereIn('status_id', [2,3,4,5,6])
->whereIn('status_id', [Invoice::STATUS_SENT, Invoice::STATUS_PARTIAL, Invoice::STATUS_PAID])
->where('is_deleted', false)
->whereBetween('date', ['1970-01-01', $this->end_date])
// ->whereDoesntHave('transaction_events'); //filter by no transaction events for THIS month.
->whereDoesntHave('transaction_events', function ($query) {
$query->where('period', $this->end_date);
});
@@ -152,6 +158,7 @@ class TaxPeriodReport extends BaseExport
//Harvest point in time records for cash payments.
\App\Models\Paymentable::where('paymentable_type', 'invoices')
->where('paymentable_id', $invoice->id)
->whereIn('payment_id', $invoice->payments->pluck('id'))
->get()
->groupBy(function ($paymentable) {
@@ -159,8 +166,9 @@ class TaxPeriodReport extends BaseExport
})
->map(function ($group) {
return $group->first();
})->each(function (\App\Models\Paymentable $pp) {
(new InvoiceTransactionEventEntryCash())->run($pp->paymentable, \Carbon\Carbon::parse($pp->created_at)->startOfMonth()->format('Y-m-d'), \Carbon\Carbon::parse($pp->created_at)->endOfMonth()->format('Y-m-d'));
})->each(function (\App\Models\Paymentable $pp) use ($invoice) {
// Pass $invoice directly to avoid morph lazy-load on $pp->paymentable.
(new InvoiceTransactionEventEntryCash())->run($invoice, \Carbon\Carbon::parse($pp->created_at)->startOfMonth()->format('Y-m-d'), \Carbon\Carbon::parse($pp->created_at)->endOfMonth()->format('Y-m-d'));
});
}
@@ -196,7 +204,7 @@ class TaxPeriodReport extends BaseExport
});
$this->backfillClassificationBreakdown();
$this->backfillClassificationBreakdown(); // TEMP disabled to test perf impact
return $this;
}
@@ -282,9 +290,25 @@ class TaxPeriodReport extends BaseExport
private function resolveQuery(): Builder
{
$start_date = $this->start_date;
$end_date = $this->end_date;
$cash_accounting = $this->cash_accounting;
$query = Invoice::query()
->withTrashed()
->with('transaction_events')
->with(['transaction_events' => function ($q) use ($start_date, $end_date, $cash_accounting) {
$q->whereBetween('period', [$start_date, $end_date])
->orderBy('timestamp', 'desc');
if ($cash_accounting) {
$q->where(function ($sub_q) {
$sub_q->where('event_id', '!=', TransactionEvent::INVOICE_UPDATED)
->orWhere('metadata->tax_report->tax_summary->status', 'reversed');
});
} else {
$q->where('event_id', TransactionEvent::INVOICE_UPDATED);
}
}])
->where('company_id', $this->company->id);
if ($this->cash_accounting) { //cash
@@ -530,28 +554,12 @@ class TaxPeriodReport extends BaseExport
$query->cursor()->each(function ($invoice) {
$invoice->transaction_events()
->when(!$this->cash_accounting, function ($query) {
$query->where('event_id', TransactionEvent::INVOICE_UPDATED);
})
->when($this->cash_accounting, function ($query) {
$query->where(function ($sub_q) {
$sub_q->where('event_id', '!=', TransactionEvent::INVOICE_UPDATED)
->orWhere('metadata->tax_report->tax_summary->status', 'reversed');
});
// $query->where('event_id', '!=', TransactionEvent::INVOICE_UPDATED);
})
->whereBetween('period', [$this->start_date, $this->end_date])
->orderBy('timestamp', 'desc')
->cursor()
->each(function ($event) use ($invoice) {
// transaction_events were eager-loaded with the same filters in resolveQuery();
// iterate the loaded collection rather than firing a fresh query per invoice.
foreach ($invoice->transaction_events as $event) {
/** @var Invoice $invoice */
$this->processTransactionEvent($event, $invoice);
});
}
});
return $this;

View File

@@ -296,6 +296,17 @@ trait GeneratesCounter
*/
public function getNextProjectNumber(Project $project): string
{
if (! $project->client_id) {
$this->resetCompanyCounters($project->company);
$counter = $project->company->settings->project_number_counter;
$project_number = $this->checkEntityNumber(Project::class, $project, $counter, $project->company->settings->counter_padding, $project->company->settings->project_number_pattern);
$this->incrementCounter($project->company, 'project_number_counter');
return $this->replaceUserVars($project, $project_number);
}
$entity_number = $this->getNextEntityNumber(Project::class, $project->client, false);
return $this->replaceUserVars($project, $entity_number);

View File

@@ -0,0 +1,151 @@
<?php
/**
* Invoice Ninja (https://invoiceninja.com).
*
* @link https://github.com/invoiceninja/invoiceninja source repository
*
* @copyright Copyright (c) 2026. Invoice Ninja LLC (https://invoiceninja.com)
*
* @license https://www.elastic.co/licensing/elastic-license
*/
namespace Tests\Feature;
use App\Models\Project;
use App\Utils\Traits\MakesHash;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Foundation\Testing\DatabaseTransactions;
use Illuminate\Support\Facades\Session;
use Tests\MockAccountData;
use Tests\TestCase;
/**
* Project client_id integrity: prior bug was PUT JSON containing "client_id": null passing through
* prepareForValidation because isset(null) is false, so fill() cleared client_id. Updates must use
* array_key_exists when stripping client_id from update payloads.
*
* All scenarios use real HTTP payloads only (no factories for Project).
*/
class ProjectClientIdApiTest extends TestCase
{
use DatabaseTransactions;
use MakesHash;
use MockAccountData;
protected function setUp(): void
{
parent::setUp();
$this->makeTestData();
Session::start();
Model::reguard();
}
/**
* @return array<string, string>
*/
private function apiHeaders(): array
{
return [
'X-API-SECRET' => config('ninja.api_secret'),
'X-API-TOKEN' => $this->token,
];
}
/**
* @param array<string, mixed> $payload
* @return array<string, mixed>
*/
private function assertCreatedProjectResponse(array $payload): array
{
$response = $this->withHeaders($this->apiHeaders())->postJson('/api/v1/projects', $payload);
$response->assertStatus(200);
return $response->json();
}
public function test_store_project_with_full_api_payload_succeeds(): void
{
$body = $this->assertCreatedProjectResponse([
'client_id' => $this->client->hashed_id,
'name' => 'Payload Created Project',
'task_rate' => 75.5,
'budgeted_hours' => 12,
'public_notes' => 'notes',
]);
$this->assertNotEmpty($body['data']['id']);
$this->assertSame('Payload Created Project', $body['data']['name']);
$this->assertSame(75.5, $body['data']['task_rate']);
$this->assertSame($this->encodePrimaryKey($this->client->id), $body['data']['client_id']);
}
public function test_store_project_without_client_id_returns_422(): void
{
$response = $this->withHeaders($this->apiHeaders())->postJson('/api/v1/projects', [
'name' => 'Missing Client',
'task_rate' => 10,
]);
$response->assertStatus(422);
}
/**
* Regression: JSON null must not clear client_id (prepareForValidation must strip by key, not isset).
*/
public function test_put_payload_with_explicit_null_client_id_does_not_clear_client(): void
{
$created = $this->assertCreatedProjectResponse([
'client_id' => $this->client->hashed_id,
'name' => 'Before Put Null Client Key',
'task_rate' => 22,
]);
$hashedProjectId = $created['data']['id'];
$expectedClientHash = $this->encodePrimaryKey($this->client->id);
$response = $this->withHeaders($this->apiHeaders())->putJson("/api/v1/projects/{$hashedProjectId}", [
'client_id' => null,
'name' => 'After Put Should Keep Client',
'task_rate' => 33,
'budgeted_hours' => 0,
]);
$response->assertStatus(200);
$this->assertSame($expectedClientHash, $response->json('data.client_id'));
$this->assertSame('After Put Should Keep Client', $response->json('data.name'));
$this->assertSame(
$this->client->id,
Project::query()->find($this->decodePrimaryKey($hashedProjectId))->client_id
);
}
public function test_put_payload_omitting_client_id_preserves_client(): void
{
$created = $this->assertCreatedProjectResponse([
'client_id' => $this->client->hashed_id,
'name' => 'Omit Client Field',
'task_rate' => 5,
]);
$hashedProjectId = $created['data']['id'];
$expectedClientHash = $this->encodePrimaryKey($this->client->id);
$response = $this->withHeaders($this->apiHeaders())->putJson("/api/v1/projects/{$hashedProjectId}", [
'name' => 'Renamed Only',
'task_rate' => 6,
]);
$response->assertStatus(200);
$this->assertSame($expectedClientHash, $response->json('data.client_id'));
$this->assertSame(
$this->client->id,
Project::query()->find($this->decodePrimaryKey($hashedProjectId))->client_id
);
}
}