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

Added option for showing origin in machinetracker #1541

Merged
merged 3 commits into from
Aug 8, 2017

Conversation

bflugon
Copy link
Contributor

@bflugon bflugon commented Jul 27, 2017

Added a checkbox for displaying which router the data originated from.

Fixes #1506

Added a checkbox for displaying which router the data originated from.
Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

Thanks Bård, looks good (but I haven't tested it yet). I just have one small suggestion for improvement.

@@ -64,6 +67,9 @@
{{ row.end_time|date:"Y-m-d H:i:s" }}
{% endif %}
</td>
{% if form_data.origin %}
<td>{{row.netbox}}</td>
Copy link
Member

Choose a reason for hiding this comment

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

Actually, it would be better to utilize row.sysname here, which is a copy of the router's sysname at the time the record was created. This way, if the netbox is deleted, old log rows will still make sense to the end user.

Changed from sisplaying row.netbox to row.sysname, because the latter will still be available even if the netbox is deleted.
@lunkwill42 lunkwill42 requested a review from jmbredal August 7, 2017 07:12
Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

This looks good, but I'd like to nitpick on the checkbox vs. the column header. The checkbox says Origin, while the column header says Router.

I would suggest maybe use Source for both of them, but I'll let @jmbredal weigh in on the matter as well...
screenshot 2017-08-07 09 15 18

@lunkwill42 lunkwill42 self-assigned this Aug 7, 2017
@lunkwill42 lunkwill42 added this to the 4.8.0 milestone Aug 8, 2017
@lunkwill42 lunkwill42 merged commit d503043 into Uninett:master Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants