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

Add device group filter to widgets #9692

Merged
merged 1 commit into from Aug 8, 2019

Conversation

@Jellyfrog
Copy link
Member

commented Jan 17, 2019

Adds a device group filter to most widgets

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.

@Jellyfrog Jellyfrog force-pushed the Jellyfrog:feature/dashboard-devicegroup branch from b8c0990 to 4a37974 Jan 17, 2019

@Jellyfrog Jellyfrog force-pushed the Jellyfrog:feature/dashboard-devicegroup branch from 4a37974 to 28730b8 Jan 21, 2019

@Jellyfrog
Copy link
Member Author

left a comment

@murrant Like this you had in mind?

@murrant

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

@Jellyfrog I've merged #9716
You can use that now, feel free to update the device group query if needed.

@murrant

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Hey @Jellyfrog are you planning on finishing this?

@Jellyfrog

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Hey @Jellyfrog are you planning on finishing this?

Hi, yes, forgot about it.

@Jellyfrog Jellyfrog force-pushed the Jellyfrog:feature/dashboard-devicegroup branch 2 times, most recently from 91560d9 to 8a32707 Jun 19, 2019

@CLAassistant

This comment has been minimized.

Copy link

commented Jun 19, 2019

CLA assistant check
All committers have signed the CLA.

@Jellyfrog Jellyfrog force-pushed the Jellyfrog:feature/dashboard-devicegroup branch 2 times, most recently from 97c2271 to d01d977 Jun 19, 2019

@Jellyfrog

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

@murrant
What do you think about doing like this instead: dfcc831

We can cheat a bit and skip looking up device_group table, will generate this SQL:

select * from `eventlog`
where exists (
  select * from `device_group_device`
  where `eventlog`.`device_id` = `device_group_device`.`device_id`
  and `device_group_device`.`device_group_id` = ?
)

That should be more efficient :)

@Jellyfrog Jellyfrog force-pushed the Jellyfrog:feature/dashboard-devicegroup branch from d01d977 to dfcc831 Jun 19, 2019

app/Models/Port.php Outdated Show resolved Hide resolved

@Jellyfrog Jellyfrog force-pushed the Jellyfrog:feature/dashboard-devicegroup branch from 9c26f0a to de75a4d Jun 28, 2019

app/Models/Device.php Outdated Show resolved Hide resolved

@Jellyfrog Jellyfrog changed the title WIP: Add device group filter to widgets Add device group filter to widgets Jun 28, 2019

@Jellyfrog Jellyfrog force-pushed the Jellyfrog:feature/dashboard-devicegroup branch 6 times, most recently from 8f8d2e5 to 79e043a Jun 28, 2019

@Jellyfrog Jellyfrog force-pushed the Jellyfrog:feature/dashboard-devicegroup branch 3 times, most recently from 5882618 to 1cadf64 Jul 9, 2019

@Jellyfrog Jellyfrog force-pushed the Jellyfrog:feature/dashboard-devicegroup branch 4 times, most recently from ef8d13e to 1a742d0 Jul 9, 2019

@Jellyfrog Jellyfrog force-pushed the Jellyfrog:feature/dashboard-devicegroup branch from 7024077 to b637d4b Jul 19, 2019

app/Models/Device.php Outdated Show resolved Hide resolved
@murrant

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

You should either just check the table directly in your scope:

return $query->whereIn('device_id', function ($query) use ($deviceGroup) {
    $query->select('device_id')
        ->from('device_group_device')
        ->where('device_group_id', $deviceGroup);
});

or change the basequery so instead of Device::query() (query is often ommited)

DeviceGroup::find($deviceGroup)->devices()

or a fake device group if you want to avoid the lookup

$dg = new DeviceGroup();
$dg->id = $deviceGroup;
$query = $dg->devices()->hasAccess...

@Jellyfrog Jellyfrog force-pushed the Jellyfrog:feature/dashboard-devicegroup branch from 9b4bb80 to ceaa4bc Aug 7, 2019

@Jellyfrog Jellyfrog force-pushed the Jellyfrog:feature/dashboard-devicegroup branch from ceaa4bc to 4134d54 Aug 7, 2019

@murrant
murrant approved these changes Aug 8, 2019

@murrant murrant merged commit fc281cc into librenms:master Aug 8, 2019

5 of 6 checks passed

codeclimate 10 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

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-55-release-changelog-august-2019/9428/1

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