Conversation
jeremystretch
left a comment
There was a problem hiding this comment.
This looks like an issue with the underlying model ordering, and shouldn't require any changes to the frontend widget (unless I'm missing something). For example, we see the ordering of the REST API endpoint is incorrect:
GET /api/dcim/module-bays/?device_id=27&fields=id,name
{
"count": 6,
"next": null,
"previous": null,
"results": [
{
"id": 29,
"name": "fan1"
},
{
"id": 30,
"name": "fan2"
},
{
"id": 31,
"name": "fan3"
},
{
"id": 32,
"name": "psu1"
},
{
"id": 33,
"name": "psu2"
},
{
"id": 34,
"name": "fan4"
}
]
}
The API endpoint should be returning these in the expected order, and the form widget should just populate them in sequence. This will probably require altering order_insertion_by on the model's MPTTMeta class (which I suppose should be 'name' rather than 'module').
jnovinger
left a comment
There was a problem hiding this comment.
Any concerns about any of the other bulk action views at all?
jeremystretch
left a comment
There was a problem hiding this comment.
This seems to still be broken. Starting with the following list of module bays:
When I add a new module bay named fan5 via the UI, it appears at the end of the list, both in the UI and in the REST API. However, after I rebuild the tree manually via dcim.ModuleBay.objects.rebuild() the ordering is now correct.
Fixes: #20911
Two issues:
This fixes item 1, item 2 still appears but would need a front-end fix so keeping that for a separate ticket.
As noted in the issue discussion, the change to order_insertion_by will fix the ordering of newly inserted items, there were other ordering problems in bulk edit and bulk rename which this also fixes. I tried either looping and doing a save or doing a bulk_update and then a rebuild at the end, for small data-sets the bulk_update if much faster, for large data-sets they are almost the same perf but bulk_update still wins out.
Just changing order_insertion_by revealed an existing bug in the CSV import when using the order_insertion_by. We don't run into it in the tests for NestedGroupModel (the other model using it) because of the way the imports are ordered in the test case as they don't cause a re-arrangement in the tree. The ordering of the ModuleBay test actually does cause a re-ordering and hits what looks like a bug in MPTT code. The additional changes in bulk_views.py so the CSV processing if done on models that inherit form MPTTModel uses a delay_mptt_updates() and rebuild at the end fixes this issue.