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>
This commit is contained in:
objecttothis
2024-11-06 01:37:47 +04:00
committed by GitHub
parent f66ffc81b7
commit d946b31cf4
9 changed files with 245 additions and 124 deletions

View File

@@ -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;

View File

@@ -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);
}
}
}

View File

@@ -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']);
}
}
}

View File

@@ -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();

View File

@@ -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
{
}
}

View File

@@ -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`);

View File

@@ -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`;

View File

@@ -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

View File

@@ -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();