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

Get Client History #132

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

sschwartz96
Copy link

Hello!

I wanted an easy way to pull the client history which included offline devices.
The latest version of the UniFi Controller uses the following URL to pull device history, /proxy/network/v2/api/site/%s/clients/history?%s where the first string substitution is the site name and the second substitution are the following query parameters:

type ClientHistoryOpts struct {
	OnlyNonBlocked      bool
	IncludeUnifiDevices bool
	WithinHours         int // 0 for all time
}

I have added the required URL and function stub in the types.go and the implementation in the clients.go source files. I started to write the test, but do need some advice on how I should proceed with the test.

I am also a little confused if the client mock is being used anywhere, but added the necessary implementation in the mock files as well.

Thanks!

Copy link
Member

@davidnewhall davidnewhall left a comment

Choose a reason for hiding this comment

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

Hello, and thank you for your contribution! I'm just leaving a few notes before @platinummonkey gets in here.

clients.go Outdated Show resolved Hide resolved
params.Add("onlyNonBlocked", fmt.Sprint(opts.OnlyNonBlocked))
params.Add("includeUnifiDevices", fmt.Sprint(opts.IncludeUnifiDevices))
params.Add("withinHours", fmt.Sprint(opts.WithinHours))
paramStr := params.Encode()
Copy link
Member

Choose a reason for hiding this comment

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

This string is only used in one place; recommend relocating params.Encode() to that place.

Copy link
Author

Choose a reason for hiding this comment

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

The single place that params.Encode() is used is within a for loop ranging sites.

Would it be beneficial to update this?

clients.go Outdated Show resolved Hide resolved
clients.go Outdated Show resolved Hide resolved
clients_test.go Show resolved Hide resolved
@@ -134,6 +134,26 @@ func (m *MockUnifi) GetClientsDPI(_ []*unifi.Site) ([]*unifi.DPITable, error) {
return results, nil
}

// GetClientHistory returns a response full of client history from the UniFi Controller
func (m *MockUnifi) GetClientHistory(_ []*unifi.Site, _ *unifi.ClientHistoryOpts) ([]unifi.ClientHistory, error) {
// TODO : add logic to generate data based on ClientHistoryOpts
Copy link
Member

Choose a reason for hiding this comment

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

Can we do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

we do this in unpoller with gofakeit so it's possible.

Copy link
Author

Choose a reason for hiding this comment

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

My initial thought was just manually changing certain fields based on the ClientHistoryOpts.
Please let me know if there is an easier way with gofakeit and if adding this type of functionality would be useful for the mock client.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either works for me may be more predictable for tests with a static response

mocks/mock_client.go Outdated Show resolved Hide resolved
clients.go Outdated Show resolved Hide resolved
clients.go Outdated Show resolved Hide resolved
@platinummonkey
Copy link
Contributor

The client mocks are mainly for other people trying to integrate and want to write tests. They aren't actually exercised in this repo

@sschwartz96
Copy link
Author

@davidnewhall & @platinummonkey

Thank you for taking the time to review my PR!
I do appreciate the feedback and have made the necessary updates and resolved the pertinent threads. I do have a few feedback requests on the remaining 3

Copy link
Contributor

@platinummonkey platinummonkey left a comment

Choose a reason for hiding this comment

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

The linter seems out of date so I’ll need to fix that in a separate PR, thanks for the contribution!

@platinummonkey
Copy link
Contributor

go ahead and merge from master and the linting errors will be fixed 👍

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.

3 participants