From d946b31cf425d2e661c672a185149b6ffca78ee9 Mon Sep 17 00:00:00 2001
From: objecttothis <17935339+objecttothis@users.noreply.github.com>
Date: Wed, 6 Nov 2024 01:37:47 +0400
Subject: [PATCH] Bugfix Attributes not saving (#4080)

Fixed issue with Attribute Values not saving correctly

This issue was caused by the Attribute->attributeValueExists function receiving a date which was already in Y-m-d format, so the conversion was returning false. Added logic to pass the date through if it was already in Y-m-d format.

Signed-off-by: objecttothis <objecttothis@gmail.com>
---
 app/Controllers/Attributes.php                |   8 +-
 app/Controllers/Items.php                     |  68 ++++----
 .../20210422000000_database_optimizations.php |  10 +-
 .../20210422000001_remove_duplicate_links.php |   2 +-
 ...0240826000000_fix_duplicate_attributes.php | 106 +++++++++++++
 ....4.0_attribute_links_unique_constraint.sql |  16 ++
 .../3.4.0_database_optimizations.sql          |  10 +-
 app/Helpers/locale_helper.php                 |   2 +-
 app/Models/Attribute.php                      | 147 ++++++++----------
 9 files changed, 245 insertions(+), 124 deletions(-)
 create mode 100644 app/Database/Migrations/20240826000000_fix_duplicate_attributes.php
 create mode 100644 app/Database/Migrations/sqlscripts/3.4.0_attribute_links_unique_constraint.sql

diff --git a/app/Controllers/Attributes.php b/app/Controllers/Attributes.php
index 29c955fbb..f9533bf20 100644
--- a/app/Controllers/Attributes.php
+++ b/app/Controllers/Attributes.php
@@ -64,7 +64,7 @@ class Attributes extends Secure_Controller
 	 */
 	public function postSaveAttributeValue(): void
 	{
-		$success = $this->attribute->save_value(
+		$success = $this->attribute->saveAttributeValue(
 			html_entity_decode($this->request->getPost('attribute_value')),
 			$this->request->getPost('definition_id', FILTER_SANITIZE_NUMBER_INT),
 			$this->request->getPost('item_id', FILTER_SANITIZE_NUMBER_INT),
@@ -131,7 +131,7 @@ class Attributes extends Secure_Controller
 
 				foreach($definition_values as $definition_value)
 				{
-					$this->attribute->save_value($definition_value, $definition_data['definition_id']);
+					$this->attribute->saveAttributeValue($definition_value, $definition_data['definition_id']);
 				}
 
 				echo json_encode([
@@ -180,7 +180,7 @@ class Attributes extends Secure_Controller
 	 */
 	public function getRow(int $row_id): void
 	{
-		$attribute_definition_info = $this->attribute->get_info($row_id);
+		$attribute_definition_info = $this->attribute->getAttributeInfo($row_id);
 		$attribute_definition_info->definition_flags = $this->get_attributes($attribute_definition_info->definition_flags);
 		$data_row = get_attribute_definition_data_row($attribute_definition_info);
 
@@ -210,7 +210,7 @@ class Attributes extends Secure_Controller
 	 */
 	public function getView(int $definition_id = NO_DEFINITION_ID): void
 	{
-		$info = $this->attribute->get_info($definition_id);
+		$info = $this->attribute->getAttributeInfo($definition_id);
 		foreach(get_object_vars($info) as $property => $value)
 		{
 			$info->$property = $value;
diff --git a/app/Controllers/Items.php b/app/Controllers/Items.php
index 60c252124..1cb5509d1 100644
--- a/app/Controllers/Items.php
+++ b/app/Controllers/Items.php
@@ -326,6 +326,7 @@ class Items extends Secure_Controller
 				$item_info->tax_category_id = $this->config['default_tax_category'];
 			}
 		}
+
 		$data['standard_item_locked'] = (
 			$data['item_kit_disabled']
 			&& $item_info->item_type == ITEM_KIT
@@ -512,6 +513,8 @@ class Items extends Secure_Controller
 	}
 
 	/**
+	 * Gathers attribute value information for an item and returns it in a view.
+	 *
 	 * @param int $item_id
 	 * @return void
 	 */
@@ -762,28 +765,7 @@ class Items extends Secure_Controller
 					$success &= $this->inventory->insert($inv_data, false);
 				}
 			}
-
-			// Save item attributes
-			$attribute_links = $this->request->getPost('attribute_links') ?? [];
-			$attribute_ids = $this->request->getPost('attribute_ids');
-
-			$this->attribute->delete_link($item_id);
-
-			foreach($attribute_links as $definition_id => $attribute_value)
-			{
-				$definition_type = $this->attribute->get_info($definition_id)->definition_type;
-
-				if($definition_type == DECIMAL)
-				{
-					$attribute_value = prepare_decimal($attribute_value);
-				}
-
-				$attribute_id = $definition_type === DROPDOWN
-					? $attribute_value
-					: $this->attribute->save_value($attribute_value, $definition_id, $item_id, $attribute_ids[$definition_id], $definition_type);
-
-				$this->attribute->save_link($item_id, $definition_id, $attribute_id);
-			}
+			$this->saveItemAttributes($item_id);
 
 			if($success && $upload_success)
 			{
@@ -1325,15 +1307,15 @@ class Items extends Secure_Controller
 	 */
 	private function store_attribute_value(string $value, array $attribute_data, int $item_id)
 	{
-		$attribute_id = $this->attribute->value_exists($value, $attribute_data['definition_type']);
+		$attribute_id = $this->attribute->attributeValueExists($value, $attribute_data['definition_type']);
 
-		$this->attribute->delete_link($item_id, $attribute_data['definition_id']);
+		$this->attribute->deleteAttributeLinks($item_id, $attribute_data['definition_id']);
 
 		if(!$attribute_id)
 		{
-			$attribute_id = $this->attribute->save_value($value, $attribute_data['definition_id'], $item_id, false, $attribute_data['definition_type']);
+			$attribute_id = $this->attribute->saveAttributeValue($value, $attribute_data['definition_id'], $item_id, false, $attribute_data['definition_type']);
 		}
-		else if(!$this->attribute->save_link($item_id, $attribute_data['definition_id'], $attribute_id))
+		else if(!$this->attribute->saveAttributeLink($item_id, $attribute_data['definition_id'], $attribute_id))
 		{
 			return false;
 		}
@@ -1441,4 +1423,38 @@ class Items extends Secure_Controller
 			}
 		}
 	}
+
+	/**
+	 * Saves item attributes for a given item.
+	 *
+	 * @param int $itemId The item for which attributes need to be saved to.
+	 * @return void
+	 */
+	public function saveItemAttributes(int $itemId): void
+	{
+		$attributeLinks = $this->request->getPost('attribute_links') ?? [];
+		$attributeIds = $this->request->getPost('attribute_ids');
+
+		$this->attribute->deleteAttributeLinks($itemId);
+
+		foreach($attributeLinks as $definitionId => $attributeValue)
+		{
+			$definitionType = $this->attribute->getAttributeInfo($definitionId)->definition_type;
+
+			switch($definitionType)
+			{
+				case DROPDOWN:
+					$attributeId = $attributeValue;
+					break;
+				case DECIMAL:
+					$attributeValue = prepare_decimal($attributeValue);
+				//Fall through to save the attribute value
+				default:
+					$attributeId = $this->attribute->saveAttributeValue($attributeValue, $definitionId, $itemId, $attributeIds[$definitionId], $definitionType);
+					break;
+			}
+
+			$this->attribute->saveAttributeLink($itemId, $definitionId, $attributeId);
+		}
+	}
 }
diff --git a/app/Database/Migrations/20210422000000_database_optimizations.php b/app/Database/Migrations/20210422000000_database_optimizations.php
index 33da992d9..52587e3f6 100644
--- a/app/Database/Migrations/20210422000000_database_optimizations.php
+++ b/app/Database/Migrations/20210422000000_database_optimizations.php
@@ -29,12 +29,12 @@ class Migration_database_optimizations extends Migration
 		$builder = $this->db->table('attribute_values');
 		$builder->select('attribute_id, attribute_value, attribute_decimal, attribute_date');
 		$builder->groupStart();
-			$builder->where('attribute_value IS NOT NULL');
-			$builder->where('attribute_date IS NOT NULL');
+		$builder->where('attribute_value IS NOT NULL');
+		$builder->where('attribute_date IS NOT NULL');
 		$builder->groupEnd();
 		$builder->orGroupStart();
-			$builder->where('attribute_value IS NOT NULL');
-			$builder->where('attribute_decimal IS NOT NULL');
+		$builder->where('attribute_value IS NOT NULL');
+		$builder->where('attribute_decimal IS NOT NULL');
 		$builder->groupEnd();
 		$attribute_values = $builder->get();
 
@@ -78,7 +78,7 @@ class Migration_database_optimizations extends Migration
 							break;
 					}
 
-					$attribute->save_value($value, $attribute_link['definition_id'], $attribute_link['item_id'], false, $attribute_link['definition_type']);
+					$attribute->saveAttributeValue($value, $attribute_link['definition_id'], $attribute_link['item_id'], false, $attribute_link['definition_type']);
 				}
 			}
 		}
diff --git a/app/Database/Migrations/20210422000001_remove_duplicate_links.php b/app/Database/Migrations/20210422000001_remove_duplicate_links.php
index 10effca2b..505f2b094 100644
--- a/app/Database/Migrations/20210422000001_remove_duplicate_links.php
+++ b/app/Database/Migrations/20210422000001_remove_duplicate_links.php
@@ -51,7 +51,7 @@ class Migration_remove_duplicate_links extends Migration
 			$builder->where('definition_id', $duplicated_link['definition_id']);
 			$builder->delete();
 
-			$attribute->save_link($duplicated_link['item_id'], $duplicated_link['definition_id'], $duplicated_link['attribute_id']);
+			$attribute->saveAttributeLink($duplicated_link['item_id'], $duplicated_link['definition_id'], $duplicated_link['attribute_id']);
 		}
 
 		$this->db->transComplete();
diff --git a/app/Database/Migrations/20240826000000_fix_duplicate_attributes.php b/app/Database/Migrations/20240826000000_fix_duplicate_attributes.php
new file mode 100644
index 000000000..6c4ff8d99
--- /dev/null
+++ b/app/Database/Migrations/20240826000000_fix_duplicate_attributes.php
@@ -0,0 +1,106 @@
+<?php
+
+namespace App\Database\Migrations;
+
+use CodeIgniter\Database\Migration;
+use App\Models\Attribute;
+use CodeIgniter\Database\ResultInterface;
+
+class fix_duplicate_attributes extends Migration
+{
+	/**
+	 * Perform a migration step.
+	 */
+	public function up(): void
+	{
+		$rows_to_keep = $this->get_all_duplicate_attributes();
+		$this->remove_duplicate_attributes($rows_to_keep);
+
+		helper('migration');
+
+		$this->drop_foreign_key_constraints();
+
+		execute_script(APPPATH . 'Database/Migrations/sqlscripts/3.4.0_attribute_links_unique_constraint.sql');
+	}
+
+	/**
+	 * Retrieves from the database all rows where the item_id and definition_id are the same AND the sale_id/receiving_id is null.
+	 * It also excludes null item_id rows as those are dropdown items.
+	 *
+	 * @return ResultInterface Results containing item_id, definition_id and attribute_id in each row.
+	 */
+	private function get_all_duplicate_attributes(): ResultInterface
+	{
+		$builder = $this->db->table('attribute_links');
+		$builder->select('item_id, definition_id, MIN(attribute_id) as attribute_id');
+		$builder->where('sale_id IS NULL');
+		$builder->where('receiving_id IS NULL');
+		$builder->where('item_id IS NOT NULL');
+		$builder->groupBy('item_id, definition_id');
+		$builder->having('COUNT(attribute_id) > 1');
+		return $builder->get();
+	}
+
+	/**
+	 * Removes the duplicate attributes from the database.
+	 *
+	 * @param ResultInterface $rows_to_keep A multidimensional associative array containing item_id, definition_id and attribute_id in each row which should be kept in the database.
+	 * @return void
+	 */
+	private function remove_duplicate_attributes(ResultInterface $rows_to_keep): void
+	{
+		$attribute = model(Attribute::class);
+		foreach($rows_to_keep->getResult() as $row)
+		{
+			$attribute->deleteAttributeLinks($row->item_id, $row->definition_id);	//Deletes all attribute links for the item_id/definition_id combination
+			$attribute->saveAttributeLink($row->item_id, $row->definition_id, $row->attribute_id);
+		}
+	}
+
+	/**
+	 * Drops the foreign key constraints from the attribute_links table.
+	 * This is required to successfully create the generated unique constraint.
+	 *
+	 * @return void
+	 */
+	private function drop_foreign_key_constraints(): void
+	{
+		$foreignKeys = [
+			'ospos_attribute_links_ibfk_1',
+			'ospos_attribute_links_ibfk_2',
+			'ospos_attribute_links_ibfk_3',
+			'ospos_attribute_links_ibfk_4',
+			'ospos_attribute_links_ibfk_5'
+		];
+
+		$current_prefix = $this->db->getPrefix();
+		$this->db->setPrefix('');
+		$database_name = $this->db->database;
+
+		foreach ($foreignKeys as $fk)
+		{
+			$builder = $this->db->table('INFORMATION_SCHEMA.TABLE_CONSTRAINTS');
+			$builder->select('CONSTRAINT_NAME');
+			$builder->where('TABLE_SCHEMA', $database_name);
+			$builder->where('TABLE_NAME', 'ospos_attribute_links');
+			$builder->where('CONSTRAINT_TYPE', 'FOREIGN KEY');
+			$builder->where('CONSTRAINT_NAME', $fk);
+			$query = $builder->get();
+
+			if($query->getNumRows() > 0)
+			{
+				$this->db->query("ALTER TABLE `ospos_attribute_links` DROP FOREIGN KEY `$fk`");
+			}
+		}
+
+		$this->db->setPrefix($current_prefix);
+	}
+
+	/**
+	 * Revert a migration step.
+	 */
+	public function down(): void
+	{
+
+	}
+}
diff --git a/app/Database/Migrations/sqlscripts/3.4.0_attribute_links_unique_constraint.sql b/app/Database/Migrations/sqlscripts/3.4.0_attribute_links_unique_constraint.sql
new file mode 100644
index 000000000..6de8d6ae8
--- /dev/null
+++ b/app/Database/Migrations/sqlscripts/3.4.0_attribute_links_unique_constraint.sql
@@ -0,0 +1,16 @@
+# Prevents duplicate attribute links with the same definition_id and item_id.
+# This accounts for dropdown rows (null item_id) and rows associated with sales or receivings.
+ALTER TABLE `ospos_attribute_links`
+	ADD COLUMN `generated_unique_column` VARCHAR(255) GENERATED ALWAYS AS (
+		CASE
+			WHEN `sale_id` IS NULL AND `receiving_id` IS NULL AND `item_id` IS NOT NULL THEN CONCAT(`definition_id`, '-', `item_id`)
+			ELSE NULL
+		END
+		) STORED,
+	ADD UNIQUE INDEX `attribute_links_uq3` (`generated_unique_column`);
+
+ALTER TABLE `ospos_attribute_links`	ADD CONSTRAINT `ospos_attribute_links_ibfk_1` FOREIGN KEY (`definition_id`) REFERENCES `ospos_attribute_definitions` (`definition_id`) ON DELETE RESTRICT;
+ALTER TABLE `ospos_attribute_links`	ADD CONSTRAINT `ospos_attribute_links_ibfk_2` FOREIGN KEY (`attribute_id`) REFERENCES `ospos_attribute_values` (`attribute_id`) ON DELETE RESTRICT;
+ALTER TABLE `ospos_attribute_links`	ADD CONSTRAINT `ospos_attribute_links_ibfk_3` FOREIGN KEY (`item_id`) REFERENCES `ospos_items` (`item_id`);
+ALTER TABLE `ospos_attribute_links`	ADD CONSTRAINT `ospos_attribute_links_ibfk_4` FOREIGN KEY (`receiving_id`) REFERENCES `ospos_receivings` (`receiving_id`);
+ALTER TABLE `ospos_attribute_links`	ADD CONSTRAINT `ospos_attribute_links_ibfk_5` FOREIGN KEY (`sale_id`) REFERENCES `ospos_sales` (`sale_id`);
diff --git a/app/Database/Migrations/sqlscripts/3.4.0_database_optimizations.sql b/app/Database/Migrations/sqlscripts/3.4.0_database_optimizations.sql
index 25d06cbda..50f702b95 100644
--- a/app/Database/Migrations/sqlscripts/3.4.0_database_optimizations.sql
+++ b/app/Database/Migrations/sqlscripts/3.4.0_database_optimizations.sql
@@ -1,10 +1,10 @@
 #ospos_attribute_values table
 ALTER TABLE `ospos_attribute_values` ADD UNIQUE(`attribute_date`);
-ALTER TABLE `ospos_attribute_values` ADD UNIQUE(`attribute_decimal`); 
+ALTER TABLE `ospos_attribute_values` ADD UNIQUE(`attribute_decimal`);
 
 #opsos_attribute_definitions table
 ALTER TABLE `ospos_attribute_definitions` MODIFY `definition_flags` tinyint(1) NOT NULL;
-ALTER TABLE `ospos_attribute_definitions` ADD INDEX(`definition_name`); 
+ALTER TABLE `ospos_attribute_definitions` ADD INDEX(`definition_name`);
 ALTER TABLE `ospos_attribute_definitions` ADD INDEX(`definition_type`);
 
 #ospos_cash_up table
@@ -67,6 +67,7 @@ ALTER TABLE `ospos_expense_categories` ADD INDEX(`category_description`);
 ALTER TABLE `ospos_giftcards` MODIFY `deleted` tinyint(1) DEFAULT 0 NOT NULL;
 
 #ospos_items table
+ALTER TABLE `ospos_items` DROP FOREIGN KEY `ospos_items_ibfk_1`;
 ALTER TABLE `ospos_items` MODIFY `deleted` tinyint(1) DEFAULT 0 NOT NULL;
 ALTER TABLE `ospos_items` MODIFY `stock_type` tinyint(1) DEFAULT 0 NOT NULL;
 ALTER TABLE `ospos_items` MODIFY `item_type` tinyint(1) DEFAULT 0 NOT NULL;
@@ -81,7 +82,7 @@ ALTER TABLE `ospos_item_kits` MODIFY `print_option` tinyint(1) DEFAULT 0 NOT NUL
 ALTER TABLE `ospos_item_kits` ADD INDEX(`name`,`description`);
 
 #ospos_people table
-ALTER TABLE `ospos_people` ADD INDEX(`first_name`, `last_name`, `email`, `phone_number`); 
+ALTER TABLE `ospos_people` ADD INDEX(`first_name`, `last_name`, `email`, `phone_number`);
 
 #ospos_receivings_items
 ALTER TABLE `ospos_receivings_items` MODIFY `discount_type` tinyint(1) DEFAULT 0 NOT NULL;
@@ -104,7 +105,7 @@ ALTER TABLE `ospos_sales_taxes` MODIFY `print_sequence` tinyint(1) DEFAULT 0 NOT
 ALTER TABLE `ospos_sales_taxes` MODIFY `rounding_code` tinyint(1) DEFAULT 0 NOT NULL;
 
 #ospos_sessions table
-ALTER TABLE `ospos_sessions` ADD INDEX(`id`); 
+ALTER TABLE `ospos_sessions` ADD INDEX(`id`);
 ALTER TABLE `ospos_sessions` ADD INDEX(`ip_address`);
 
 #ospos_stock_locations table
@@ -112,7 +113,6 @@ ALTER TABLE `ospos_stock_locations` MODIFY `deleted` tinyint(1) DEFAULT 0 NOT NU
 
 #ospos_suppliers table
 ALTER TABLE `ospos_expenses` DROP FOREIGN KEY `ospos_expenses_ibfk_3`;
-ALTER TABLE `ospos_items` DROP FOREIGN KEY `ospos_items_ibfk_1`;
 ALTER TABLE `ospos_receivings` DROP FOREIGN KEY `ospos_receivings_ibfk_2`;
 ALTER TABLE `ospos_suppliers` DROP FOREIGN KEY `ospos_suppliers_ibfk_1`;
 
diff --git a/app/Helpers/locale_helper.php b/app/Helpers/locale_helper.php
index 31849fcde..e79127175 100644
--- a/app/Helpers/locale_helper.php
+++ b/app/Helpers/locale_helper.php
@@ -428,7 +428,7 @@ function to_quantity_decimals(?string $number): string
 }
 
 /**
- * Converts a float to locale-specific number format for display.
+ * Converts a string to locale-specific number format for display.
  *
  * @param string|null $decimals
  * @param int $type
diff --git a/app/Models/Attribute.php b/app/Models/Attribute.php
index d79e7681a..c253ff297 100644
--- a/app/Models/Attribute.php
+++ b/app/Models/Attribute.php
@@ -65,73 +65,71 @@ class Attribute extends Model
 
 	/**
 	 * Returns whether an attribute_link row exists given an item_id and optionally a definition_id
+	 *
 	 * @param int $item_id ID of the item to check for an associated attribute.
-	 * @param bool $definition_id Attribute definition ID to check.
+	 * @param int|bool $definition_id Attribute definition ID to check.
 	 * @return bool returns true if at least one attribute_link exists or false if no attributes exist for that item and attribute.
 	 */
-	public function link_exists(int $item_id, bool $definition_id = false): bool
+	public function attributeLinkExists(int $item_id, int|bool $definition_id = false): bool
 	{
 		$builder = $this->db->table('attribute_links');
 		$builder->where('item_id', $item_id);
 		$builder->where('sale_id', null);
 		$builder->where('receiving_id', null);
 
-		if(empty($definition_id))
+		if($definition_id)
+		{
+			$builder->where('definition_id', $definition_id);
+		}
+		else
 		{
 			$builder->where('definition_id IS NOT NULL');
 			$builder->where('attribute_id', null);
 		}
-		else
-		{
-			$builder->where('definition_id', $definition_id);
-		}
-
-		return ($builder->get()->getNumRows() > 0);	//TODO: This is returning a result of 1 on dropdown
+		$results = $builder->countAllResults();
+		return $results > 0;
 	}
 
 	/**
 	 * Determines if a given attribute_value exists in the attribute_values table and returns the attribute_id if it does
 	 *
-	 * @param float|string $attribute_value The value to search for in the attribute values table.
-	 * @param string $definition_type The definition type which will dictate which column is searched.
+	 * @param float|string $attributeValue The value to search for in the attribute values table.
+	 * @param string $definitionType The definition type which will dictate which column is searched.
 	 * @return int|bool The attribute ID of the found row or false if no attribute value was found.
 	 */
-	public function value_exists(float|string $attribute_value, string $definition_type = TEXT): bool|int
+	public function attributeValueExists(float|string $attributeValue, string $definitionType = TEXT): bool|int
 	{
 		$config = config(OSPOS::class)->settings;
 
-		switch($definition_type)
+		switch($definitionType)
 		{
 			case DATE:
-				$data_type = 'date';
-				$attribute_date_value = DateTime::createFromFormat($config['dateformat'], $attribute_value);
-				$attribute_value = $attribute_date_value->format('Y-m-d');
+				$dataType = 'date';
+				$attributeDateValue = DateTime::createFromFormat($config['dateformat'], $attributeValue);
+				$attributeValue = $attributeDateValue ? $attributeDateValue->format('Y-m-d') : $attributeValue;
 				break;
 			case DECIMAL:
-				$data_type = 'decimal';
+				$dataType = 'decimal';
 				break;
 			default:
-				$data_type = 'value';
+				$dataType = 'value';
 				break;
 		}
 
 		$builder = $this->db->table('attribute_values');
 		$builder->select('attribute_id');
-		$builder->where("attribute_$data_type", $attribute_value);
+		$builder->where("attribute_$dataType", $attributeValue);
 		$query = $builder->get();
 
-		if($query->getNumRows() > 0)
-		{
-			return $query->getRow()->attribute_id;
-		}
-
-		return false;
+		return $query->getNumRows() > 0
+			? $query->getRow()->attribute_id
+			: false;
 	}
 
 	/**
 	 * Gets information about a particular attribute definition
 	 */
-	public function get_info(int $definition_id): object
+	public function getAttributeInfo(int $definition_id): object
 	{
 		$builder = $this->db->table('attribute_definitions AS definition');
 		$builder->select('parent_definition.definition_name AS definition_group, definition.*');
@@ -140,7 +138,7 @@ class Attribute extends Model
 
 		$query = $builder->get();
 
-		if($query->getNumRows() == 1)	//TODO: ===
+		if($query->getNumRows() === 1)
 		{
 			return $query->getRow();
 		}
@@ -514,18 +512,18 @@ class Attribute extends Model
 	 */
 	private function checkbox_attribute_values(int $definition_id): array
 	{
-		$zero_attribute_id = $this->value_exists('0');
-		$one_attribute_id = $this->value_exists('1');
+		$zero_attribute_id = $this->attributeValueExists('0');
+		$one_attribute_id = $this->attributeValueExists('1');
 
 		if(!$zero_attribute_id)
 		{
-			$zero_attribute_id = $this->save_value('0', $definition_id, false, false, CHECKBOX);
+			$zero_attribute_id = $this->saveAttributeValue('0', $definition_id, false, false, CHECKBOX);
 		}
 
 		if(!$one_attribute_id)
 		{
-			$one_attribute_id = $this->save_value('1', $definition_id, false, false, CHECKBOX);
-			$one_attribute_id = $this->save_value('1', $definition_id, false, false, CHECKBOX);
+			$one_attribute_id = $this->saveAttributeValue('1', $definition_id, false, false, CHECKBOX);
+			$one_attribute_id = $this->saveAttributeValue('1', $definition_id, false, false, CHECKBOX);
 		}
 
 		return [$zero_attribute_id, $one_attribute_id];
@@ -610,31 +608,32 @@ class Attribute extends Model
 	/**
 	 * Inserts or updates an attribute link
 	 *
-	 * @param int $item_id
-	 * @param int $definition_id
-	 * @param int $attribute_id
-	 * @return bool
+	 * @param int $itemId
+	 * @param int $definitionId
+	 * @param int $attributeId
+	 * @return bool True if the attribute link was saved successfully, false otherwise.
 	 */
-	public function save_link(int $item_id, int $definition_id, int $attribute_id): bool
+	public function saveAttributeLink(int $itemId, int $definitionId, int $attributeId): bool
 	{
 		$this->db->transStart();
 
 		$builder = $this->db->table('attribute_links');
 
-		if($this->link_exists($item_id, $definition_id))
+		if($this->attributeLinkExists($itemId, $definitionId))
 		{
-			$builder->set(['attribute_id' => $attribute_id]);
-			$builder->where('definition_id', $definition_id);
-			$builder->where('item_id', $item_id);
+			$builder->set(['attribute_id' => $attributeId]);
+			$builder->where('definition_id', $definitionId);
+			$builder->where('item_id', $itemId);
 			$builder->where('sale_id', null);
 			$builder->where('receiving_id', null);
 			$builder->update();
 		}
 		else
 		{
-			$data = ['attribute_id' => $attribute_id,
-				'item_id' => $item_id,
-				'definition_id' => $definition_id
+			$data = [
+				'attribute_id' => $attributeId,
+				'item_id' => $itemId,
+				'definition_id' => $definitionId
 			];
 			$builder->insert($data);
 		}
@@ -646,10 +645,10 @@ class Attribute extends Model
 
 	/**
 	 * @param int $item_id
-	 * @param bool $definition_id
+	 * @param int|bool $definition_id
 	 * @return bool
 	 */
-	public function delete_link(int $item_id, bool $definition_id = false): bool
+	public function deleteAttributeLinks(int $item_id, int|bool $definition_id = false): bool
 	{
 		$delete_data = ['item_id' => $item_id];
 
@@ -843,35 +842,34 @@ class Attribute extends Model
 	 * @param string $definition_type
 	 * @return int
 	 */
-	public function save_value(string $attribute_value, int $definition_id, $item_id = false, $attribute_id = false, string $definition_type = DROPDOWN): int
+	public function saveAttributeValue(string $attribute_value, int $definition_id, int|bool $item_id = false, int|bool $attribute_id = false, string $definition_type = DROPDOWN): int
 	{
 		$config = config(OSPOS::class)->settings;
-		$locale_date_format = $config['dateformat'];
 
 		$this->db->transStart();
 
+		switch($definition_type)
+		{
+			case DATE:
+				$data_type				= 'date';
+				$attribute_date_value	= DateTime::createFromFormat($config['dateformat'], $attribute_value);
+				$attribute_value		= $attribute_date_value->format('Y-m-d');
+				break;
+			case DECIMAL:
+				$data_type	= 'decimal';
+				break;
+			default:
+				$data_type	= 'value';
+				break;
+		}
+
 		//New Attribute
 		if(empty($attribute_id) || empty($item_id))
 		{
-		//Update attribute_value
-			$attribute_id = $this->value_exists($attribute_value, $definition_type);
+			$attribute_id = $this->attributeValueExists($attribute_value, $definition_type);
 
 			if(!$attribute_id)
 			{
-				switch($definition_type)	//TODO: Duplicated code
-				{
-					case DATE:
-						$data_type				= 'date';
-						$attribute_date_value	= DateTime::createFromFormat($locale_date_format, $attribute_value);
-						$attribute_value		= $attribute_date_value->format('Y-m-d');
-						break;
-					case DECIMAL:
-						$data_type	= 'decimal';
-						break;
-					default:
-						$data_type	= 'value';
-						break;
-				}
 
 				$builder = $this->db->table('attribute_values');
 				$builder->set(["attribute_$data_type" => $attribute_value]);
@@ -892,22 +890,7 @@ class Attribute extends Model
 		}
 		//Existing Attribute
 		else
-		{//TODO: TAKE A LOOK AT THIS DUPLICATE CODE FRAGMENT FROM ABOVE AND SEE ABOUT REFACTORING OUT A FUNCTION
-			switch($definition_type)
-			{
-				case DATE:
-					$data_type				= 'date';
-					$attribute_date_value	= DateTime::createFromFormat($locale_date_format, $attribute_value);
-					$attribute_value		= $attribute_date_value->format('Y-m-d');
-					break;
-				case DECIMAL:
-					$data_type	= 'decimal';
-					break;
-				default:
-					$data_type	= 'value';
-					break;
-			}
-
+		{
 			$builder = $this->db->table('attribute_values');
 			$builder->set(["attribute_$data_type" => $attribute_value]);
 			$builder->where('attribute_id', $attribute_id);
@@ -1037,9 +1020,9 @@ class Attribute extends Model
 
 		foreach($attributes as $attribute)
 		{
-			$new_attribute_id = $this->save_value($attribute['attribute_value'], $definition_id, false, $attribute['attribute_id'], $definition_type);
+			$new_attribute_id = $this->saveAttributeValue($attribute['attribute_value'], $definition_id, false, $attribute['attribute_id'], $definition_type);
 
-			if(!$this->save_link($attribute['item_id'], $definition_id, $new_attribute_id))
+			if(!$this->saveAttributeLink($attribute['item_id'], $definition_id, $new_attribute_id))
 			{
 				log_message('error', 'Transaction failed');
 				$this->db->transRollback();