-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Closes #6930: Add 'ID' column to object tables #7673
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
Closes #6930: Add 'ID' column to object tables #7673
Conversation
jeremystretch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid your approach of explicitly declaring a separate id column of every table isn't tenable. Instead of re-declaring this column everywhere, it should be added to the parent BaseTable class so that it's inherited by each child table automatically.
Ideally, the id column should also be added to Meta.fields automatically, but I think it's fine to leave it in the individual classes as you have it for now.
|
Yeah, that's what I figured. I was hesitant to add it directly to the |
|
I'm almost certain we never use BaseTable for anything other than database objects, and all database objects have an |
…r connections tables
…and Circuit tables
| pk = ToggleColumn() | ||
| id = tables.Column( | ||
| verbose_name='ID' | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
un-linkify the id column, because device component templates don't have a detail page to link to. The device detail page errors without this because there's no get_absolute_url method on the component template model.
|
|
||
| class Meta(BaseTable.Meta): | ||
| exclude = ('id', ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exclude the id column from the device component template tables in the device type detail view. It didn't really make sense to include them (though that might be subjective).
netbox/extras/tables.py
Outdated
| class Meta(BaseTable.Meta): | ||
| model = TaggedItem | ||
| fields = ('content_type', 'content_object') | ||
| exclude = ('id', ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exclude the id column from the 'Tagged Objects' table in the Tag detail view. This contains objects from several different models, so ID isn't unique. It didn't seem to make sense to include it (subjective).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, you can create a custom field (or use a template field) and link to the appropriate objects view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I said that wrong. TaggedItem does have an ID for the TaggedItem object itself, which is not the same as the corresponding object's ID. The TaggedItem ID isn't useful information to expose, as I don't think that's accessible/usable anywhere?
I could make the ID column expose the object ID instead, but I wasn't sure how useful that is for a couple of reasons.
- this table isn't exportable, which is the primary reason this change was requested in the first place (though, the ID column is currently on plenty of other non-exportable table views, so that's kind of moot).
- object ID isn't inherently unique in this table, so needs to be used in conjunction with the type column in order to extract useful information
- If linkify-ing it, the object column is already linked to that object's detail page
Though, now that I actually look at it again, it's a lot easier to properly include the object ID and linkify it than I initially thought it would be, so this isn't really a big deal to add/get working after all.
| linkify=True, | ||
| verbose_name='ID' | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this needs to be defined differently now that it's elevated to the BaseTable class? Also, should it move up above the Meta definition / does that matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should go above the Meta class, but I can take care of that after merge. Looks good otherwise, thanks!
|
Okay, I think this is ready for another review. I elevated the Currently, there's an issue where |
Closes: #6930
Adds an 'ID' column to most object tables, which is hidden by default. This column exposes Netbox's numeric ID for that object.
The column has been added to the following tables:
Regarding the "Circuits" table, this already exposes the circuit ID under the 'ID' header. My thought would be to change the circuit ID header to 'Circuit ID', and then add the Netbox 'ID' column, but this could potentially break anything that consumes the CSV exports, and thus would need to wait for v3.1.0.