From d06c78119b9c3b5b43bb0413d9bc6bec7ac8401b Mon Sep 17 00:00:00 2001 From: Jeppe Bundgaard Date: Thu, 11 Jun 2026 12:04:56 +0200 Subject: [PATCH] Fix customer registration duplicate recovery --- services/nginx/app/objects/users_o.php | 71 +++++---- services/nginx/app/routes/authRoute.php | 142 +++++++++++++++--- .../nginx/app/tests/auth/RegisterCvrTest.php | 84 ++++++++++- 3 files changed, 241 insertions(+), 56 deletions(-) diff --git a/services/nginx/app/objects/users_o.php b/services/nginx/app/objects/users_o.php index 902f0d28..4ffd41bd 100644 --- a/services/nginx/app/objects/users_o.php +++ b/services/nginx/app/objects/users_o.php @@ -127,40 +127,47 @@ class users_o extends db private function importCustomerFromExternalSource(int $customer_number): object|bool { - global $db; // Get the customer data from the external source $economic = new economicCustomers(); $customer_data = $economic->getCustomerId($customer_number); - // DEBUG: Return the customer data - // Check if the customer exists if ($customer_data) { - // Avoid SQL injection - $customer_number = $db->escape_string($customer_data->customerNumber); - // Double check if the customer exists - $sql = "SELECT * FROM $this->table WHERE customer_number = '$customer_number'"; - $result = $db->query($sql); - if ($result->num_rows > 0) { - $this->id = $result->fetch_assoc()['id']; - $this->getObjectProperties(); - } else { - // Import the customer - $this->add($customer_number, '', 0); - // Nullify the password - $this->password->nullify(); - // If the customer has an email address, save it - if (isset($customer_data->email)) { - $this->email->set($customer_data->email); - } - // If the customer has a name, save it as the display name - if (isset($customer_data->name)) { - $this->display_name->set($customer_data->name); - } - } + return $this->importCustomerFromEconomicCustomerData($customer_data); } - // Else return false + return false; } + public function importCustomerFromEconomicCustomerData(object $customer_data): users_o|bool + { + global $db; + + if (!isset($customer_data->customerNumber) || !is_numeric($customer_data->customerNumber)) { + return false; + } + + $customer_number = $db->escape_string((string)$customer_data->customerNumber); + $sql = "SELECT * FROM $this->table WHERE customer_number = '$customer_number'"; + $result = $db->query($sql); + if ($result->num_rows > 0) { + $this->id = (int)$result->fetch_assoc()['id']; + $this->getObjectProperties(); + return $this; + } + + $this->add($customer_number, '', 0); + $this->password->nullify(); + + if (isset($customer_data->email)) { + $this->email->set($customer_data->email); + } + + if (isset($customer_data->name)) { + $this->display_name->set($customer_data->name); + } + + return $this; + } + /** * @throws Exception */ @@ -258,7 +265,7 @@ class users_o extends db * @param int|null $user_id The user id to add the attribute to * @throws Exception If the user is not selected, and the user_id is null */ - public function addAttribute(string $attribute, int $user_id = null): void + public function addAttribute(string $attribute, ?int $user_id = null): void { global $db; if ($user_id === null) { @@ -272,7 +279,7 @@ class users_o extends db $db->query($sql); } - public function deleteAttribute(string $attribute, int $user_id = null): void + public function deleteAttribute(string $attribute, ?int $user_id = null): void { global $db; if ($user_id === null) { @@ -304,7 +311,7 @@ class users_o extends db $db->query($sql); } - public function doesUserHaveAttribute(string $attribute, int $user_id = null): bool + public function doesUserHaveAttribute(string $attribute, ?int $user_id = null): bool { global $db; if ($user_id === null) { @@ -518,7 +525,7 @@ class users_o extends db return customer_name_cache_payload_builder::build($cached_name, $fallback_name); } - public function getCustomerEcocomicData(int $customer_number = null): users_o + public function getCustomerEcocomicData(?int $customer_number = null): users_o { // Check if the customer number is set if (!isset($this->customer_number) && $customer_number === null) { @@ -717,7 +724,7 @@ class users_o extends db $this->permissions = $perms; } - public function getUserAttributes(int $user_id = null): array + public function getUserAttributes(?int $user_id = null): array { global $db; if ($user_id === null) { @@ -1106,7 +1113,7 @@ class users_o extends db * Set the password for the user * @throws Exception If the user is not selected */ - public function setPassword(string $password = null): void + public function setPassword(?string $password = null): void { self::requireSelected(); global $db; diff --git a/services/nginx/app/routes/authRoute.php b/services/nginx/app/routes/authRoute.php index 8c7652cf..b3de8ea3 100644 --- a/services/nginx/app/routes/authRoute.php +++ b/services/nginx/app/routes/authRoute.php @@ -409,15 +409,7 @@ class authRoute * Check if the cvr already exists */ $economic = new economic(); - $economic_response = ($economic->customers->customers->search([ - 'corporateIdentificationNumber' => (string)$cvr, - ], [ - 'skipPages' => 0, - 'pageSize' => 1, // Since the limit is 1000, we need to set the page size to 1000. - ])->collection); - if (!is_array($economic_response)) { - $economic_response = []; - } + $economic_response = $this->searchEconomicCustomersByCvr($economic, (string)$cvr); $localUserExists = $this->localCustomerNumberExists($companyPhone); $matchingEconomicCustomer = $this->findEconomicCustomerByNumber($economic_response, $companyPhone); @@ -455,15 +447,37 @@ class authRoute // Get the CVR company information used for the e-conomic customer payload. $companyInformation = (new virkdata())->getCompanyInformation($cvr, '', []); $name = (string)($companyInformation->name ?? ''); - $result = $economic->createCustomer( - (int)$companyPhone, - $name, - (int)$cvr, - (string)$invoiceEmail, - (int)$companyPhone, - (int)$contactPhone, - $companyInformation, - ); + try { + $result = $economic->createCustomer( + (int)$companyPhone, + $name, + (int)$cvr, + (string)$invoiceEmail, + (int)$companyPhone, + (int)$contactPhone, + $companyInformation, + ); + } catch (Exception $exception) { + $recoveredCustomer = $this->recoverRegistrationAfterCreateFailure( + $economic, + (string)$cvr, + $companyPhone, + (string)$invoiceEmail, + $exception + ); + + if ($recoveredCustomer !== null) { + $response->success($recoveredCustomer, 200); + } + + $this->logRegisterCvrIssue('AUTH_REGISTER_CVR_CREATE_FAILED', [ + 'phase' => 'create', + 'cvr' => (string)$cvr, + 'requestedCustomerNumber' => $companyPhone, + 'message' => $exception->getMessage(), + ]); + $response->error('Failed to create customer in e-conomic.', 502); + } if (!isset($result->customerNumber) || !is_numeric($result->customerNumber)) { $this->logRegisterCvrIssue('AUTH_REGISTER_CVR_INVALID_CREATE_RESPONSE', [ @@ -495,7 +509,7 @@ class authRoute ); } - $this->bootstrapLocalCustomerOrFail($companyPhone); + $this->bootstrapLocalCustomerOrFail($companyPhone, $result); $this->sendRegistrationWelcomeEmails($companyPhone, (string)$invoiceEmail); $response->success($result, 201); }); @@ -758,6 +772,61 @@ class authRoute return count($rows) > 0; } + private function searchEconomicCustomersByCvr(economic $economic, string $cvr): array + { + $economic_response = ($economic->customers->customers->search([ + 'corporateIdentificationNumber' => $cvr, + ], [ + 'skipPages' => 0, + 'pageSize' => 1, + ])->collection); + + return is_array($economic_response) ? $economic_response : []; + } + + private function recoverRegistrationAfterCreateFailure( + economic $economic, + string $cvr, + int $customerNumber, + string $invoiceEmail, + Exception $exception + ): ?object { + if (!$this->isRecoverableEconomicDuplicateError($exception)) { + return null; + } + + try { + $economic_response = $this->searchEconomicCustomersByCvr($economic, $cvr); + } catch (Exception $searchException) { + $this->logRegisterCvrIssue('AUTH_REGISTER_CVR_CREATE_RECOVERY_SEARCH_FAILED', [ + 'phase' => 'create_recovery', + 'cvr' => $cvr, + 'requestedCustomerNumber' => $customerNumber, + 'message' => $searchException->getMessage(), + ]); + return null; + } + + $matchingEconomicCustomer = $this->findEconomicCustomerByNumber($economic_response, $customerNumber); + if ($matchingEconomicCustomer === null || $this->localCustomerNumberExists($customerNumber)) { + return null; + } + + $this->bootstrapLocalCustomerOrFail($customerNumber, $matchingEconomicCustomer); + $this->sendRegistrationWelcomeEmails($customerNumber, $invoiceEmail); + + return $matchingEconomicCustomer; + } + + private function isRecoverableEconomicDuplicateError(Exception $exception): bool + { + $message = strtolower($exception->getMessage()); + + return str_contains($message, 'already exists') + || str_contains($message, 'already exist') + || str_contains($message, 'duplicate'); + } + private function findEconomicCustomerByNumber(array $customers, int $customerNumber): ?object { foreach ($customers as $customer) { @@ -785,19 +854,46 @@ class authRoute /** * @throws Exception */ - private function bootstrapLocalCustomerOrFail(int $customerNumber): users_o + private function bootstrapLocalCustomerOrFail(int $customerNumber, ?object $economicCustomer = null): users_o { global $response; - $customer = (new users_o())->getUserByCustomerNumber($customerNumber); - if (method_exists($customer, 'exists') && $customer->exists()) { - return $customer; + $customer = new users_o(); + try { + $customer = $customer->getUserByCustomerNumber($customerNumber); + if (method_exists($customer, 'exists') && $customer->exists()) { + return $customer; + } + } catch (Exception $exception) { + $this->logRegisterCvrIssue('AUTH_REGISTER_CVR_LOCAL_BOOTSTRAP_LOOKUP_FAILED', [ + 'customerNumber' => $customerNumber, + 'message' => $exception->getMessage(), + ]); + } + + if ( + $economicCustomer !== null + && $this->extractEconomicCustomerNumber($economicCustomer) === $customerNumber + && method_exists($customer, 'importCustomerFromEconomicCustomerData') + ) { + try { + $importedCustomer = $customer->importCustomerFromEconomicCustomerData($economicCustomer); + if (is_object($importedCustomer) && method_exists($importedCustomer, 'exists') && $importedCustomer->exists()) { + return $importedCustomer; + } + } catch (Exception $exception) { + $this->logRegisterCvrIssue('AUTH_REGISTER_CVR_LOCAL_SNAPSHOT_BOOTSTRAP_FAILED', [ + 'customerNumber' => $customerNumber, + 'message' => $exception->getMessage(), + ]); + } } $this->logRegisterCvrIssue('AUTH_REGISTER_CVR_LOCAL_BOOTSTRAP_FAILED', [ 'customerNumber' => $customerNumber, ]); $response->error('Customer was created in e-conomic but could not be imported locally.', 500); + throw new Exception('Customer was created in e-conomic but could not be imported locally.'); } /** diff --git a/services/nginx/app/tests/auth/RegisterCvrTest.php b/services/nginx/app/tests/auth/RegisterCvrTest.php index 6e246e11..9b7a723b 100644 --- a/services/nginx/app/tests/auth/RegisterCvrTest.php +++ b/services/nginx/app/tests/auth/RegisterCvrTest.php @@ -69,6 +69,8 @@ namespace classes { { public static array $mock_collection = []; public static ?object $mock_create_response = null; + public static ?\RuntimeException $mock_create_exception = null; + public static array $mock_collection_after_create_exception = []; public static array $search_calls = []; public static array $create_calls = []; @@ -78,6 +80,8 @@ namespace classes { { self::$mock_collection = []; self::$mock_create_response = null; + self::$mock_create_exception = null; + self::$mock_collection_after_create_exception = []; self::$search_calls = []; self::$create_calls = []; } @@ -113,6 +117,11 @@ namespace classes { 'company_information' => $companyInformation, ]; + if (self::$mock_create_exception !== null) { + self::$mock_collection = self::$mock_collection_after_create_exception; + throw self::$mock_create_exception; + } + $response = self::$mock_create_response ?? (object)[ 'customerNumber' => (int)$number, ]; @@ -214,6 +223,7 @@ namespace objects { { public static array $mock_existing_customer_numbers = []; public static array $mock_importable_customer_numbers = []; + public static bool $mock_external_lookup_enabled = true; public static array $interaction_log = []; public int $id = 0; @@ -223,6 +233,7 @@ namespace objects { { self::$mock_existing_customer_numbers = []; self::$mock_importable_customer_numbers = []; + self::$mock_external_lookup_enabled = true; self::$interaction_log = []; } @@ -242,7 +253,8 @@ namespace objects { self::$interaction_log[] = 'bootstrap:' . $customerNumber; $existsLocally = in_array($customerNumber, self::$mock_existing_customer_numbers, true); - $canImport = in_array($customerNumber, self::$mock_importable_customer_numbers, true); + $canImport = self::$mock_external_lookup_enabled + && in_array($customerNumber, self::$mock_importable_customer_numbers, true); if ($existsLocally || $canImport) { $this->id = $customerNumber; @@ -257,6 +269,22 @@ namespace objects { return $this; } + public function importCustomerFromEconomicCustomerData(object $customerData): self|bool + { + $customerNumber = (int)($customerData->customerNumber ?? 0); + if ($customerNumber <= 0) { + return false; + } + + self::$interaction_log[] = 'snapshot-import:' . $customerNumber; + $this->id = $customerNumber; + $this->exists = true; + self::$mock_existing_customer_numbers[] = $customerNumber; + self::$mock_existing_customer_numbers = array_values(array_unique(self::$mock_existing_customer_numbers)); + + return $this; + } + public function exists(): bool { return $this->exists; @@ -509,6 +537,60 @@ namespace { assert_true(\classes\slack::$customer_registration_notifications[0]['customer_number'] === 12345678, 'Fresh registration Slack notification must use the created customer number.'); }, ], + [ + 'name' => 'Successful registration falls back to the create response when immediate import lookup misses', + 'params' => array_merge($baseParams, ['contactPhone' => 87654320]), + 'setup' => static function (): void { + \classes\economic::$mock_create_response = (object)[ + 'customerNumber' => 12345678, + 'name' => 'Mock Company', + 'email' => 'test@test.com', + ]; + \objects\users_o::$mock_external_lookup_enabled = false; + }, + 'expected_success' => (object)[ + 'customerNumber' => 12345678, + 'name' => 'Mock Company', + 'email' => 'test@test.com', + ], + 'expected_status' => 201, + 'assert' => static function (): void { + assert_true(count(\classes\economic::$create_calls) === 1, 'Fresh registration must call create exactly once.'); + assert_true(\objects\users_o::$interaction_log[0] === 'bootstrap:12345678', 'Fresh registration must try the standard local bootstrap first.'); + assert_true(\objects\users_o::$interaction_log[1] === 'snapshot-import:12345678', 'Fresh registration must import from the create response when the immediate lookup misses.'); + assert_true(count(\classes\email::$sent) === 2, 'Snapshot fallback registration must send two welcome emails.'); + assert_true(count(\classes\email::$superuser_notifications) === 1, 'Snapshot fallback registration must notify opted-in superusers once.'); + assert_true(count(\classes\slack::$customer_registration_notifications) === 1, 'Snapshot fallback registration must notify Slack once.'); + }, + ], + [ + 'name' => 'Duplicate create response recovers a just-created e-conomic customer and sends notifications', + 'params' => $baseParams, + 'setup' => static function (): void { + \classes\economic::$mock_create_exception = new \RuntimeException('e-conomic request failed with HTTP 400: Customer already exists'); + \classes\economic::$mock_collection_after_create_exception = [ + (object)[ + 'customerNumber' => 12345678, + 'name' => 'Recovered After Create', + 'email' => 'test@test.com', + ], + ]; + \objects\users_o::$mock_importable_customer_numbers = [12345678]; + }, + 'expected_success' => (object)[ + 'customerNumber' => 12345678, + 'name' => 'Recovered After Create', + 'email' => 'test@test.com', + ], + 'expected_status' => 200, + 'assert' => static function (): void { + assert_true(count(\classes\economic::$create_calls) === 1, 'Recovery must still record the attempted create call.'); + assert_true(count(\classes\economic::$search_calls) === 2, 'Recovery must verify the duplicate by searching e-conomic again.'); + assert_true(count(\classes\email::$sent) === 2, 'Duplicate create recovery must send two welcome emails.'); + assert_true(count(\classes\email::$superuser_notifications) === 1, 'Duplicate create recovery must notify opted-in superusers once.'); + assert_true(count(\classes\slack::$customer_registration_notifications) === 1, 'Duplicate create recovery must notify Slack once.'); + }, + ], [ 'name' => 'Fresh create mismatch returns conflict without local bootstrap or email', 'params' => $baseParams,