[erp5_ui_test] Be always explicit (with sorting) and bugfix fr arguments to FooModule_createObjects

bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/FooModule_createObjects.py

@@ -2,6 +2,8 @@
 from DateTime import DateTime
 category_list = ('a', 'b', 'a/a1', 'a/a2')
 big_category_list = ('c1', 'c10', 'c2', 'c20', 'c3', 'c4')
+start = int(start)
+num = int(num)
 
 for i in range(start, start + num):
   category = category_list[i % len(category_list)]

bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/ListBox_setPropertyList.py

@@ -18,7 +18,7 @@ d = dict(
   field_all_columns = '',
   field_search_columns = '',
   field_sort_columns = '',
-  field_sort = 'id', # Very important, this allow to test tales expression on listbox_XXX fields
+  field_sort = 'id | DESC', # Very important, this allow to test tales expression on listbox_XXX fields
   field_editable_columns = 'id',
   field_stat_columns = '',
   field_url_columns = '',

[Vincent Pelletier]

Reverting this commit fixes several test regressions:

• testListBox.TestListBox.test_01_CheckListBoxLinesWithStat
• testFormPrintoutAsODT.TestFormPrintoutAsODT (all 8 failures)

It also fixes at least some of the many errors in:

• testFunctionalAnonymousSelection (25 failures - reverting fixes at least testSelectionPreviousNextButtons)
• testFunctionalCore (39 failures)

Also, default sort order is supposed to be ascending, not descending, and commit message does not explain why descending order was chosen.

/cc @romain

[Tomáš Peterka]

I tried both orders and some (about 5) RenderJS tests were passing only with descending order.

Let me do a careful revert of this commit. I will fix the failing RenderJS tests which suppose descending order because it will be less work then fixing all the ~ 70 failing other ones.

I'll MR in a while.

[Tomáš Peterka]

After discussion with @seb it seems that new UI and old UI are handling partially-specified sort differently. Partially-specified means that the developer specifies only column-name but not the actual sort (ASC or DESC). @romain might be interested too

[Tomáš Peterka]

The whole sorting requires a bit deeper inspection. I did revert that commit with expected failure of only one (mine) test.

[Vincent Pelletier]

Thanks.

For a tangential background discussion on sorting, my opinion (as a big instance maintainer) is that modules should not have sorts by default. This does not mean that sorts are always bad, but in my experience providing sorts user did not request on a list which contains many documents is a recipe for becomming unusable leaving users with no other option than poking us for a correction.

As for default sort order, it is very much a subjective topic I think. I feel that for alphabetic columns ascending sort is more natural. Dates are often best sorted in ascendingdescending order ("user wants latest document" kind of use-case, where "reverse sort" is much better than "forward sort + go to last page"). Numeric columns best sort order in absence of user choice is unclear to me. OTOH, do we really want to tell/guess the column type to decide on the sort ? ... and do we even want to sort before we know the order user wants the sort to be in ? So many questions...

[EDIT] Think one way, write the other... Sort is hard, man...

[Tomáš Peterka]

The main issue is how ListBox validates its sort field. If you type "id" it will replace it with "id | id" which itself doesn't make sense. Then you expect the frontend to know whether the default is ascending or descending. The problem is that new UI has undefined behaviour if it receives anything else than "ascending" or "descending" as a sorting order.

[Vincent Pelletier]

I agree that using the key as a value when no value is given is bad. I guess this comes from some code used for list column setting (where it makes sense to use title "id" for columns "id" if developper was too lazy to tell the title to use) which would have been absent-mindedly reused for other listbox properties.

I believe that strictly speaking catalog behaviour on unexpected sort order is also underfined, but not prevented (the latter being bad IMHO, but once it's there it's just impossible to get rid of it without getting criticised for breaking stuff...).

[Tomáš Peterka]

Implemented and fixed in !334 (merged)