-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fixes #8391: Install date should appear empty when exported #8441
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
Fixes #8391: Install date should appear empty when exported #8441
Conversation
netbox/circuits/tables.py
Outdated
| verbose_name='Side Z' | ||
| ) | ||
| install_date = tables.DateColumn( | ||
| default='' |
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.
Unfortunately this approach removes the default placeholder (—) in the table when rendered in the UI, which we want to keep.
It looks like DateColumn inherits from TemplateColumn: The ideal fix for this might actually be to fix it upstream in django-tables2 by adding a value() method to DateColumn to better handle null values.
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.
Yes, I agree in that it might be better to fix it upstream. This behavior will occur again in the case of exporting other null date fields, like date_added in the Aggregate model.
I can go and open an issue there.
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.
The django-tables2 library doesn't seem to very active at the moment.
What do you think of implementing a custom DateColumn and improving the handling of null values within the project?
It could be added into the utilities/tables.py module.
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'd rather avoid that, but it would be a reasonable short-term fix. Do you want me to keep #8391 assigned to you?
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 don't like it too much either, but sure it can be a short-term fix. Yes please keep it assigned to me, I can work on it this weekend.
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.
Thanks!
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.
This is the best way I found to handle null values for all fields of type Date and DateTime, feel free to give comments on the approach.
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.
Looks great to me! Thanks for your work.
Fixes: #8391
Set the default value to export installed_date values as empty when they are null