Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRAFT: 12552 replace mptt #12718

Closed
wants to merge 19 commits into from
Closed

DRAFT: 12552 replace mptt #12718

wants to merge 19 commits into from

Conversation

arthanson
Copy link
Collaborator

Fixes: #12552

@jeremystretch jeremystretch added the beta Concerns a bug/feature in a beta release label Jun 20, 2023
@@ -1136,7 +1128,7 @@ class InventoryItem(MPTTModel, ComponentModel):
clone_fields = ('device', 'parent', 'role', 'manufacturer', 'part_id',)

class Meta:
ordering = ('device__id', 'parent__id', '_name')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't arbitrarily drop ordering by parent Device, as it has ramifications unrelated to the tree hierarchy; for example, the order of results returned by the REST API endpoint.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeremystretch can you provide the repo scenarios that this would effect? CTE can only order on a single field (technical limitation) if we need to do two I'm not sure how to work around that.

@@ -866,7 +866,7 @@ def save(self, *args, **kwargs):
self._instantiate_components(self.device_type.frontporttemplates.all())
self._instantiate_components(self.device_type.modulebaytemplates.all())
self._instantiate_components(self.device_type.devicebaytemplates.all())
# Disable bulk_create to accommodate MPTT
# Disable bulk_create to accommodate django-tree-queries
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually necessary anymore? We had to avoid bulk creations with MPTT because the tree fields in the database needed to be recomputed, but that doesn't apply to CTE.

netbox/dcim/svg/racks.py Outdated Show resolved Hide resolved
@jeremystretch
Copy link
Member

I might be missing something, but it seems like there are some pieces still missing from the implementation.

Screenshot 2023-06-22 at 15-02-04 Regions NetBox

Some observations, using the Region model as an example:

  1. Regions are not implicitly ordered deterministically by name.
  2. Attempting to explicitly order regions by name (by clicking the table column header) appears to have no effect.
  3. Top-level regions do not display a cumulative count of all descendant regions. For example, the North America region should show 24 sites assigned.

@arthanson
Copy link
Collaborator Author

Sorting tables does seem to be broken, checking...

@jeremystretch
Copy link
Member

@arthanson given the limitations of the CTE approach we discussed, can this be closed out?

@arthanson arthanson closed this Jul 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2023
@jeremystretch jeremystretch deleted the 12552-replace-mptt branch June 20, 2024 14:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beta Concerns a bug/feature in a beta release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants