New Option Attributes Have 0 for Priority

[bhsmither]

In product.options.inc.php, line 107, new attributes are databased without a priority.

In Catalogue->getProductOptions(), the query specifies a SORT BY priority then by value_name.

So, even if a number of attributes are meticulously added in a desired order, that order is ignored - which may confuse the new admin.

Only when the Option Attributes are saved a second time does CC apply the priority (see lines 128-148). And, once the priority has been determined, the SORT BY value_name expression in the query becomes meaningless, as it seems the priority update results in a straight, non-repeating (and non foreign-keyed) sequence.

Perhaps a better approach is to make the database table Cubecart_option_value, 'priority' column have an AUTO_INCREMENT characteristic. This will, at the start, after adding new attributes, at least keep the order of these attributes in the order entered.

[briansandall]

See #1172 and #1173 - sorting should work properly now that those have been merged, i.e. with or without priority, they will appear in the correct order.

[bhsmither]

I will assert the mentioned issues are not related to this issue.

[briansandall]

I see, you're talking about the screen where one actually adds the option values ('attributes') themselves, my mistake.

Unfortunately one cannot have multiple auto-increment columns in a single table and CubeCart_option_value 'value_id' is already auto-incrementing.

Another solution would be to allow NULL for the priority and have that as the default, then use a query such as ORDER BY COALESCE(priority, value_id). I think this would achieve the desired sorting order.

On second thought, value_name would never be sorted by with the above query, but perhaps that isn't a good choice of sort criteria anyway?

[bhsmither]

Re: only one AUTO_INCREMENT,

I find that my database designer does, in fact, only allow one column to have this attribute. Prior to actually trying this, all my research never explicitly said this restriction exists.

I'm also finding restrictions on trying to SELECT MAX(priority) on an INSERT query. But there are hints on how this can be done successfully.

[bhsmither]

This seemed to work:

INSERT INTO CubeCart_option_value
    (value_name, option_id, priority)
VALUES
  ('6-Cell', '6', (
    SELECT * FROM (
      SELECT MAX(priority) FROM CubeCart_option_value
      )
    AS _t
    )
  +1
  );

From: http://dev.mysql.com/doc/refman/5.7/en/subquery-restrictions.html

[briansandall]

Perhaps defining the desired behavior will help us reach a solution. From what I understand, the goals are as follows:

1. Option attributes should appear in the order they were added until otherwise specified
2. Option attributes added later should appear after previous entries, even or especially those with specific priority settings
3. Option attributes with the same priority should be sorted by value_name (based on existing code)

Using the following data:

select (value_id, value_name, priority) from cubecart_option_value:
1, 'One', 1
2, 'Two', 2
3, 'Three', 0
4, 'Four', 0
5, 'Five', 0

Number 1 seems to work properly: 'Three', 'Four', and 'Five' are ordered according to how they were entered, i.e. by their value_id. However, when the column being sorted on (in this case, priority) contains identical values, the resulting set is not in any guaranteed order. See this StackOverflow question.

Number 3 doesn't work at all for products.options.inc.php because of line 241 and line 254 - the sort is only done on priority with no backup column. I would argue we don't actually want them to be ordered by value name anyway but rather solely based on order of entry and specified priority, even for Catalogue::getProductOptions. Probably a separate issue.

Number 2 seems to be the tricky problem, but it can be solved by changing the sort query on line 241 of products.options.inc.php to $sort = array('IF(priority>0, priority, value_id)' => 'ASC');

That change would also guarantee the ordering expected by number 1 and allow number 3 to potentially function if we decide to both keep and fix secondary sorting by value_name.

Screenshot after the above fix:

Setting priority to a non-zero or non-null value by default, on the other hand, would preclude us from ever implementing number 3 unless we also created a method of setting priority values of equal value. There could also be other unknown ramifications affecting e.g. plugins. Furthermore, should it ever need to be known, it would then be impossible to determine which entries were explicitly assigned priorities. I therefore maintain that setting priority upon insert is not an appropriate solution.

[briansandall]

@bhsmither Does my proposed solution resolve the issue for you? If so, I'll make a PR; if not, please discuss in what ways it does not meet your needs.

Proposed solution:

// In admin/sources/products.options.inc.php, change line 241 from:
$sort = array('priority' => 'ASC');

// to:
$sort = array('IF(priority>0, priority, value_id)' => 'ASC');

[bhsmither]

I'm going to assert that Database->select(), when processing the $order parameter, will not include a column that is trying to go by the name 'IF(priority>0, priority, value_id)'. ORDER BY column names as an array must exist in the array of $allowed names (as determined by Database->getFields()).

This SQL function as a proxy for a column name would work, however, if it were passed to select() as a string literal, not as an array.

[briansandall]

You are correct - further testing revealed that the sort was being completely ignored, despite my initial (false) positive. I wasn't aware that Database->select applied a filter to some arguments when passed by array.

Removing line 241 and changing line 254 (now 253) to include it as a string literal as you suggested has given expected results in several tests.

// new line 253:
$values = $GLOBALS['db']->select('CubeCart_option_value', false, false, 'IF(priority>0, priority, value_id) ASC');

I note however that there is still an edge case of sorts where an option value with priority of 0 will sort before a value with priority less than or equal to the first value's id. E.g. value_1 {id=1, priority=0} appears before value_2 {id=2, priority=1}. I don't believe this is a realistic possibility, however, as the priority for all existing option values is updated any time any priority is set via the admin panel. Any new option values added with a priority of 0 will sort by their value_id which is guaranteed to be higher than the highest existing priority.

In other words, I think the solution above will be acceptably functional.

[abrookbanks]

Fixed with briansandall@81f4311