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

Use sensor labels for overview/inventory pages, refactor some html-page related code #10287

Merged
merged 12 commits into from Jun 27, 2019

Conversation

Projects
None yet
3 participants
@martijn-schmidt
Copy link
Contributor

commented May 30, 2019

Hey folks,

TLDR: this pull request mostly contains some clean-up work to move similar code that was replicated across several html pages into functions.inc.php, and it enables new-style labels for the device overview and device entity-physical/inventory pages.

============

  • Moved label span creation to get_sensor_label_color() since the same code was used repeatedly on multiple different pages.

  • Removed function get_unit_for_sensor_class() since it replicated function get_units_from_sensor() feature-wise. Modified a single page that was using the removed function.

  • Move state translation and corresponding database calls to the get_state_label() function:
    Several html pages used similar database calls and sometimes very different methodology to determine the state label and state text before calling get_state_label(), so I moved that part of the task into the function itself instead of replicating the same code multiple times on different pages.

  • Refactored the device overview page to use get_state_label() and get_sensor_label_color() functions, as a "side effect" it now uses the new style labels for the non-state health sensors as well. That makes it consistent with the device health page and the LibreNMS-wide sensor table.

  • Refactored some if/else statements into ternary operators for simplicity's sake.

  • Enabled state translations and new style labels for the device entity-physical/inventory page.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 10287
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

martijn-schmidt added some commits May 30, 2019

Move state translation to get_state_label()
Several html pages used similar database calls and sometimes very
different methodology to determine the state label and state text
before calling get_state_label(), so moved that part of the task
into the function itself instead of replicating the same code
multiple times on different pages.
Move label creation to get_sensor_label_color()
Also removed a duplicate sensor class to unit function.

@kkrumm1 kkrumm1 added the WebUI label May 30, 2019

@murrant

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Thanks for this @martijn-schmidt , I have some conflicting work at #10298
I'm moving the descriptions and units into the language files for translation and easy organizing :)

@martijn-schmidt

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Hi @murrant - I've made some new commits that resolve the merge conflict and fix two typos in the language files. :)

Out of curiosity, I see that the calls to function get_units_from_sensor($sensor) are replaced with __('sensors.' . $sensor['sensor_class'] . '.unit') but I couldn't find the corresponding code for __() - I'd love to take a closer look at what it does, if you don't mind pointing in the general direction of where to find it?

EDIT, sharing this for posterity, I was pointed to the Laravel translation docs concerning the __() question: https://laravel.com/docs/5.8/localization#retrieving-translation-strings

@martijn-schmidt

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

@murrant do you need anything else for this one? Because PR #10327 which was merged today has documentation which "depends" on the fact that we replaced get_unit_for_sensor_class() with the Laravel translation code for sensor class units (eg it omits a reference to the old function that was deleted here in PR #10287).

@murrant murrant added this to the 1.53 milestone Jun 22, 2019

@martijn-schmidt

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2019

I've taken care of both requested changes, thanks for the suggestions :)

@murrant

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

Now for a design question. Why did you include the raw state value in the webui? I'm not sure how that would be useful.

@martijn-schmidt

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2019

The overview was the only page which didn't already show the raw state value in the label:
includes/html/pages/device/overview/generic/sensor.inc.php

The inventory page didn't do state translation at all before and only showed a raw number, which triggered me to begin investigating this part of the code and submit this PR:
includes/html/pages/device/entphysical.inc.php

All other pages already displayed it as state translation (raw value) and continue to behave exactly the same way as before I refactored the code:
includes/html/pages/device/health/sensors.inc.php
includes/html/pages/device/port/sensors.inc.php
includes/html/table/sensors-common.php

I wanted to make things consistent and figured that more information/visibility is usually better, moreover, I don't know whether someone might be using the raw value for things which could cause issues when taken away.

Finally, the rrdtool generated graphs don't do state translations right now, so it might be helpful to know the raw value it's currently at to see whether there have been recent changes.

However, if you don't like it, feel free to let me know and I'll remove ({$sensor['sensor_current']}) from the returned value of get_state_label() in includes/html/functions.inc.php so the other pages all conform to what the overview page did previously.

@murrant

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

I don't think the raw value is that useful. However, perhaps adding a tooltip that shows the raw value would be a good compromise ;)

@martijn-schmidt

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

As I said I'm not particularly attached to the raw value, so I've removed it. ;) In effect there's a "tooltip" already in a sense that you can see small versions of the rrd graphs when you hold your mouse over the label, so that is then good enough.

Maybe we can experiment with adding the state translation info for that sensor in the graph legend, but I'd say that's something for another PR.

@murrant murrant merged commit 9d68f27 into librenms:master Jun 27, 2019

6 checks passed

Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate 3 fixed issues
Details
license/cla Contributor License Agreement is signed.
Details

@martijn-schmidt martijn-schmidt deleted the martijn-schmidt:inventory-display-state branch Jun 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.