Problem with change in Aliases list sorting (Redmine #14015)

SteveITS

@jrey I am not seeing a problem, unless I'm misunderstanding? No matter how I sort the list I edit the alias I'm trying to edit. It does seem to skip numbers if one creates aliases on other tabs but that may not be important if it's tracking them internally, correctly.

jrey

@steveits

I'm just loading the page (not applying any additional sort by clicking on a heading)

without the patch it is loading the correct record

Screen Shot 2023-03-16 at 9.28.28 PM.png

so if I click the pencil on the last one if loads ?id=8
everything about this record is correct Name, Description and Values etc
The edit link is correct

Apply the patch and reload the page, only the last 2 in the table have switch location, but all the associated id's are wrong..
Screen Shot 2023-03-16 at 9.33.39 PM.png

so now when I click on that same (uch) record, I get ?id=5
so it is loading the wrong record 5 <> 8

if modify the parameter in the browser changing the 5 to 8 and hit enter it will load the correct record that matches the name, values, description, and would be the record I expect it to load.

without the patch
If I use the first entry on the list the id=3 that Alias is called Buddy and that is id = 3

with the patch installed
Buddy (still at the top of the list) but now has a link id=0 and when I click on that it actually loads the record for pfB_PRI1_v4 (which is not even visible on the IP list)

again here if in the browser change the 0 to a 3 and reload I get the proper Buddy record.

The links generated with the patch applied do not line up to the existing records.

jimp

I suspect once you save those aliases and they're stored in the sorted order in the config it will straighten itself out. But there might be an issue as you say if they're not in the right order in the config. I have an idea how to fix it, I'll try to reproduce it in the morning.

jrey

@jimp

I suspected the issue was having the msort as a parameter to the foreach statement.

Correct assumption it seems.
I applied the patch again, confirmed it was assigning the wrong id values.
it was
then changed the code as follows to take the msort out of the foreach:

    msort($a_aliases, 'name');
    foreach ($a_aliases as $i => $alias):

Works as expected again, well at least is assigning the correct id and loading the correct record when editing

System PHP version: 8.1.11
looks like with this version having the msort as a parameter to the foreach is problematic.

Verified by changing it back, to patch original code, and reverting patch completely -

Edits correct records
without patch = yes (original asort)
with patch = no (msort as parameter to foreach)
with patch applied but modified as above = yes (msort before foreach)

Cheers
JR

Edit: just as a footnote to this - you can delete the msort line completely and the table still loads in the same order (not really sorted by name) The displayed table does match the order if the records in the config. So the msort is not actually changing the initial order of the table that is displayed. The id= are still correct after removing the msort. Then clicking on the headers "Name" for example sort Asc or Desc. The ids remain the same.

jimp

@jrey said in System Patches package version 2.2:

@jimp
I suspected the issue was having the msort as a parameter to the foreach statement.

Yes but the msort is the point of that patch so the list shows sorted. If you view the contents of the patch it's just a short diff changing the old sort method to msort. On my systems the aliases are all aligned correctly when I check them, though. They should be getting sorted any time you make a change to the aliases, it's not clear how yours are out of order. But once I figure out how to reproduce it locally I have ideas on how to fix it and maintain the sorting.

jimp

@jrey said in System Patches package version 2.2:

Edit: just as a footnote to this - you can delete the msort line completely and the table still loads in the same order (not really sorted by name) The displayed table does match the order if the records in the config. So the msort is not actually changing the initial order of the table that is displayed. The id= are still correct after removing the msort. Then clicking on the headers "Name" for example sort Asc or Desc. The ids remain the same.

msort returns a sorted array, it doesn't modify the array, hence it being used in the foreach statement and not before. But that's getting way off the topic here, I may split this part of the discussion off into a separate thread.

jimp

OK I split this off and moved it to the 23.05 category since that change was put in after 23.01.

jrey

@jimp

Sorry I added a footnote to my previous post. Apparently we were both adding comments at the same time and you moved the thread.

so that said: with or without the msort the initial display of the table always matches the order of the records in the config.
I can then click on any of the headers, and the sort changes, with the id's remaining correct as the records move change on the display

So if your expectation is that the msort is changing the initial data order of the data displayed, that is not happening here.
with the msort as part of the foreach construct the ids are wrong (and the list is not in name order originally anyway)
with the msort above the foreach the ids are correct but the list still isn't in 'name' order
remove the msort line completely and the ids are also correct.

Not the end of the world, the list (in my case is small enough) and I can sort it by clicking on the headers if I want and that works. So as long as the id's are correct in the first place it will open the correct record. It is all good.

Let me know if there is something specific you want me to try.

now I want to go back to the original asort, as I can't remember the order of the initial list on load, I believe it was order of the config as well. Thinking about it, the records in the config are in the same order as originally created. The order in the config has never changed.

Thanks

jimp

And on each of mine the IDs always match the entries no matter what I've done so far.

I just started a fresh system and made three aliases: aaaa, cccc, then bbbb.

The order in the list is aaaa, bbbb, cccc, and the IDs to edit are 0, 2, 1.

So it's still not clear to me why yours don't match when any test I'm running comes out OK.

jrey

@jimp

Both the asort and msort give no indication that the list has been sorted.

and clearly in my case msort not giving the results as expected anyway, the initial load of the list is not sorted.

Might it be better to just load the list, (and the ids are correct) then generate a event required to order the list by name (as if clicking on the header) ??

on initial load the table looks like this (in all cases, asort, msort)

Screen Shot 2023-03-17 at 8.46.37 AM.png

After clicking on the header (the appropriate header is changed as)

Screen Shot 2023-03-17 at 8.47.07 AM.png

I could see if there are people with much larger lists that sorting would be important. But then at the same time if (for example) I've sorted that long list on Description, then made a change and the screen refreshes and the default sort goes back to name, that might be annoying too.

In my case not worth the effort to care as long as the ids match what is being loaded, I don't care about the sort order, I can seem them all right there.

So I think for right now, I am going to just revert the existing patch back to the original asort, so as not mess something up if a revised patch "fixes" it at some point down the road.

Thanks

jrey

@jimp

@jrey said in Problem with change in Aliases list sorting (Redmine #14015):

now I want to go back to the original asort, as I can't remember the order of the initial list on load,

so I've gone back to the original (asort code)
and the ids still match what I had originally posted as the order of id's 3, 4, 5, 6, 7, 9, 8, 1, 2, 0 and everything work
->> and now that I'm looking at the complete list of 10, they are in alpha order by name (which is why the id's are out of order on the display).

So....in short for me, if it ain't broke don't fix it ... no patch.

Again let me know if there is something else you want me to specifically look at.

JR

jimp

If you think it's correct you must not have any mixed case alias named (some start with uppercase, some lowercase). asort is case sensitive and will put all uppercase before all lowercase instead of putting them in actual alphabetical order.

asort:

msort:

Unfortunately, asort doesn't have a flag to do a case-insensitive sort (that actually works for this -- note there is SORT_FLAG_CASE but it does work as expected on its own or when used with the natural or string sorts).

jimp

This seems to do the trick:

uasort($aliases, function ($a, $b) { return strnatcasecmp($a['name'], $b['name']); });

jimp

I pushed a fix for #14015 (again) on 23.05 that fixes it there in a better way, but I also added a new patch for the system patches package on 23.01 that is a minimalist application of the sorting change alone.

I had to do it that way because the code for aliases in 23.05 diverged too much after 23.01 to apply the 23.05 fix directly.

Updating the system patches package to 2.2.1 when it's ready will also get the fix, you can just apply the new patch on top of the old patch and it will do the right thing.

jrey

@jimp

Okay so I reapplied the first patch to setup for "P2" version and installed that.

All good,

Much better now that editing loads the record you expect, and bonus (I guess) they are in order.

Thanks for looking into this so promptly,

(the only rules with lower case letters at the start are pfB aliases, and 1 alias that I had created for testing myTest

JR

SteveITS

@steveits said in Problem with change in Aliases list sorting (Redmine #14015):

I am not seeing a problem

Just for the record I did eventually notice this on a (different) router, in our data center. Fixed by the patch part 2.

jrey

@steveits said in Problem with change in Aliases list sorting (Redmine #14015):

Just for the record I did eventually notice this on a (different) router, in our data center. Fixed by the patch part 2.

See I'm only half crazy - lol - which I think with your confirmation now makes you the other half.
Thanks for the confirmation - good to know I'm not the only one.

and I for one am really glad it is "sorted" out.