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

Replace legacy menu with new Blade generated one #10173

Merged
merged 22 commits into from May 10, 2019

Conversation

Projects
None yet
4 participants
@murrant
Copy link
Member

commented May 1, 2019

Supports translation
Custom menu functionality ported to Blade and documented

Lots of fun with global variables...

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 5926
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.

@murrant murrant added the WebUI label May 1, 2019

@laf

This comment has been minimized.

Copy link
Member

commented May 1, 2019

I can't test every bit of this as I don't have all menus/items but these two have small issues

Screenshot 2019-05-01 at 22 42 46

Screenshot 2019-05-01 at 22 42 18

@murrant

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

@laf I fixed the double divider and a couple other similar issues. But I could not reproduce the issue with custom port descriptions.

@CirnoT

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

@murrant The issue with custom descriptions comes from $config['custom_descr'][] = ''; in includes/defaults.inc.php. Either change it to $config['custom_descr'] = array(); or make sure that custom_descr is not "" in @foreach


There's also still this double-divider issue (to reproduce add

$config['int_peering']             = 0;
$config['int_core']                = 0;
$config['int_l2tp']                = 0;
$config['int_customers']                = 0;
$config['enable_billing'] = 1;

to config.php)

chrome_2019-05-04_15-14-49


Not sure if this is desired, but it seems that previously ignored ports were not counted for Down and Disabled, and now they are.

@@ -287,7 +287,7 @@
'device' => $device['device_id'],
'tab' => 'apps',
);
foreach ($app_list as $app) {
foreach (\LibreNMS\Util\ObjectCache::applications() as $app) {
echo $sep;
if ($vars['app'] == $app['app_type']) {

This comment has been minimized.

Copy link
@CirnoT

CirnoT May 4, 2019

Contributor

You can't access app_type on $app anymore, as it's an object of App\Models\Application and app_type is protected property. Model needs to be extended to include public function, for example appType. nicecase below will have to be replaced by call to ->displayName().

This comment has been minimized.

Copy link
@murrant

murrant May 5, 2019

Author Member

Thanks, I'll check, but fyi Eloquent models implement array access.

This comment has been minimized.

Copy link
@murrant

murrant May 6, 2019

Author Member

The problem was the collection was nested, so I had to flatten it first.

@murrant

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

Ok, got those fixed. I think this might be ready. Thanks @CirnoT

@CirnoT

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

includes/html/pages/apps.inc.php is still broken and won't display anything on Overview (details in review on that file):
chrome_2019-05-05_15-37-48

There's still issue with services too:
chrome_2019-05-05_15-36-27

In addition, ignored ports are still counted as disabled and down - I believe they shouldn't be. (looks like old code was wrong, these ports are not ignored)

Looks like issue with double divider on ports is still present too.

@@ -151,13 +151,13 @@
<a href="{{ url('services') }}" class="dropdown-toggle" data-hover="dropdown" data-toggle="dropdown"><i class="fa fa-cogs fa-fw fa-lg fa-nav-icons hidden-md" aria-hidden="true"></i> <span class="hidden-sm">Services</span></a>
<ul class="dropdown-menu">
<li><a href="{{ url('services') }}"><i class="fa fa-cogs fa-fw fa-lg" aria-hidden="true"></i> All Services </a></li>
@if($service_status)
@if($service_counts['total'])
<li role="presentation" class="divider"></li>

This comment has been minimized.

Copy link
@CirnoT

CirnoT May 5, 2019

Contributor

I think simplest fix here would be to change

<li role="presentation" class="divider"></li>

to

@if($service_counts['warning'] or $service_counts['critical'])
<li role="presentation" class="divider"></li>
@endif
@murrant

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Thanks for testing @CirnoT
I think I have all of those items fixed now.

Show resolved Hide resolved LibreNMS/Util/ObjectCache.php Outdated

@murrant murrant referenced this pull request May 6, 2019

Merged

Traditional Chinese language support #10178

1 of 1 task complete

@murrant murrant changed the title WIP Replace legacy menu with new Blade generated one Replace legacy menu with new Blade generated one May 7, 2019

murrant added some commits May 7, 2019

@CirnoT

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Looks good to me now.

Found issue when testing another PR together with this:
Undefined index: pseudowire (View: /opt/librenms/resources/views/layouts/menu.blade.php)

@CirnoT CirnoT referenced this pull request May 7, 2019

Merged

Move container to page in blade tempates #10195

1 of 1 task complete
@murrant

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

I can't reproduce that @CirnoT

@CirnoT

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Sorry somehow I forgot repro steps: Make sure you have pseudowire disabled in config and open 'Manage Users' page.

$config['enable_pseudowires']           = 0;

@murrant murrant added this to the 1.52 milestone May 10, 2019

@murrant murrant merged commit 9ede688 into librenms:master May 10, 2019

5 of 6 checks passed

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

@murrant murrant deleted the murrant:one-menu branch May 10, 2019

spencerbutler added a commit to spencerbutler/librenms that referenced this pull request May 21, 2019

Replace legacy menu with new Blade generated one (librenms#10173)
* Remove legacy index php file

* fix routing page missing data

* WIP

* fix $navbar global usage

* remove global use of $locations

* ObjectCache again...

* move vars.inc.php to init.php for legacy ajax

* navbar is more local than I thought before.  Fix it.

* Fix some sensors tables escaping

* restore custom menu functionality, but with blade
and docs

* cleanup

* tidy menu @if checks

* Fix up the rest of the global variables and remove print-menubar.php

* consolidate some counting in the menu

* filter out empty custom port descr types

* Fix up custom port groups

* Fix up apps menu

* Fix services menu when all are ok

* Limit cached data to the user it is for

* Fix style

* A few clean ups

* fix pseudowire bug

@lock lock bot locked as resolved and limited conversation to collaborators Jul 9, 2019

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