Add hotspot vouchers #554

ufozone · 2023-12-16T20:43:23Z

No description provided.

Kane610

Thanks for the PR

Please add tests, please add to the gateway class, recommended is to also setup the pre-commit hook with setup.sh and you will catch a lot of small things

mypy and ruff fixes

ufozone · 2024-02-24T09:36:51Z

@Kane610, I am unfortunately very new to Phyton / Home Assistant development and have no idea how to do add tests, add to the gateway class and so on.

@bmos has made some changes with regard to code hygiene. Thanks for that!

Kane610 · 2024-02-25T10:11:15Z

@Kane610, I am unfortunately very new to Phyton / Home Assistant development and have no idea how to do add tests, add to the gateway class and so on.

@bmos has made some changes with regard to code hygiene. Thanks for that!

Here is a PR adding a new ApiHandler based class to the controller, adds api requests and tests, that should help you to get going

68ee89d

Don't hesitate to ask questions if you get stuck

ufozone · 2024-03-11T21:21:54Z

I have now built something for the PR from the linked PR. I didn't understand anything about the tests. I don't know if that's enough or how to test it.

Kane610 · 2024-03-15T16:05:23Z

Skimmed it through, will review during the weekend

Kane610

Really nice! Just one small thing to improve from review and you need to fix the failing validation

aiounifi/models/voucher.py

Co-authored-by: Robert Svensson <Kane610@users.noreply.github.com>

ufozone · 2024-04-03T22:09:57Z

@Kane610 , thank you very much. The suggested change has been implemented.

Kane610

Nearly there! Good job!

Some small areas of test coverage should be improved, I think there is no coverage of deletion path for example

aiounifi/models/voucher.py

Kane610

With these changes the tests should pass and coverage be 100%

You know if you setup the environment you only have to write pytest to run the tests locally ;)

diff --git a/tests/test_vouchers.py b/tests/test_vouchers.py
index 644ead6..ee81ba9 100644
--- a/tests/test_vouchers.py
+++ b/tests/test_vouchers.py
@@ -41,9 +41,9 @@ async def test_voucher_create(mock_aioresponse, unifi_controller, unifi_called_w
             "quota": 0,
             "expire_number": 3600,
             "expire_unit": 1,
-            "usage_quota": 1000,
-            "rate_max_up": 5000,
-            "rate_max_down": 2000,
+            "bytes": 1000,
+            "up": 5000,
+            "down": 2000,
             "note": "Unit Testing",
         },
     )
@@ -108,7 +108,7 @@ async def test_vouchers(mock_aioresponse, unifi_controller, unifi_called_with):
     voucher = vouchers["61facea3873fdb075ce28d71"]
     assert voucher.id == "61facea3873fdb075ce28d71"
     assert voucher.site_id == "5a32aa4ee4b0412345678910"
-    assert voucher.note is None
+    assert voucher.note == ""
     assert voucher.code == "44703"
     assert voucher.quota == 1
     assert voucher.duration == timedelta(minutes=480)
@@ -138,9 +138,12 @@ async def test_vouchers(mock_aioresponse, unifi_controller, unifi_called_with):
         json={
             "cmd": "create-voucher",
             "n": 1,
-            "quota": 0,
-            "expire_number": 3600,
+            "quota": 1,
+            "expire_number": 480,
             "expire_unit": 1,
+            "bytes": 1000,
+            "up": 2000,
+            "down": 5000,
         },
     )
 
@@ -150,6 +153,6 @@ async def test_vouchers(mock_aioresponse, unifi_controller, unifi_called_with):
         "/api/s/default/cmd/hotspot",
         json={
             "cmd": "delete-voucher",
-            "_id": "657e370a4543a555901865c7",
+            "_id": "61facea3873fdb075ce28d71",
         },
     )

ufozone · 2024-04-10T19:41:45Z

D'oh, now I see the mistakes too. 😪 I have eliminated them.

I prefer not to write how I develop... Spoiler: Not with a local runtime environment. 🤐

Kane610 · 2024-04-10T20:02:43Z

You should change to do it locally

ufozone · 2024-04-10T20:06:14Z

woohoo 100% coverage.

You should change to do it locally

Oh yeah, I didn't realize how deep I was falling down the rabbit hole last year. I just wanted to write a little integration. Now there are three and we're in this PR here.

Kane610 · 2024-04-10T20:31:57Z

I want ro go through it one last time. Jut I don't have time today and busy a few days so it would have to be on Monday

aiounifi/models/voucher.py

ufozone · 2024-04-19T17:50:08Z

So last commit

aiounifi/models/voucher.py

ufozone · 2024-04-19T20:07:23Z

Last attempt. Merge it or close this PR. It is difficult for me to document self-explanatory code in more detail.

Kane610 · 2024-04-20T20:52:52Z

Last attempt. Merge it or close this PR. It is difficult for me to document self-explanatory code in more detail.

Maybe I'm using bad wording to express myself here and I apologize if that's the case. This should be read as me trying to motivate my issue with the current description in pydoc, what I have an understanding of what I'm missing in it based on your explanation and why this is important for me in the long term.

Look here. The comment says "Code in known format 00000-00000". If the code where to be self explanatory it would not do anything but making sure that there is a hyphen in the middle. Now the code does something else as well, which is not obvious to me either what that means. It should be obvious why these two paths exist. You had to play around with it yourself to figure out the five character limit.

I'm more or less asking you to rephrasing this "I experimented a bit and received vouchers with 5, 8 and regular 10 characters. When entering the code on the hotspot page, the hyphen must be placed after the fifth digit.". I don't think this is an unreasonable request worth getting defensive over.

I've previously accepted PRs with code that when I needed to change things had to spend a lot of time to understand to when refactoring things. If functionality is not clear then it will come back as a personal cost on me as I'm the one sitting on long term maintenance of the code base. That's why I want to make sure to make new mistakes and learn from old.

I hope I've explained myself enough and you have an understanding behind my motivations with this request.

/R

ufozone · 2024-04-21T10:51:27Z

In the Hotspot Manager UI, the regular 10-digit code is displayed with the hyphen "in the middle". However, the unofficial API delivers it without this hyphen.

Now it would be simple to deliver the code in the model as it comes from the API.

However, the captive portal expects the hyphen to be entered. So I build in the code the hyphen "in the middle" - f"{c[:5]}-{c[5:]}".

Through some nasty manipulation, I managed to get back a five and an eight-digit code. Putting a hyphen in there seems to be wrong. But maybe not. I don't know. Maybe it's just an edge case. There was no rocket science in the position of the hyphen.

That's why I've now simplified the code. If it cracks at some point, we'll work on it then.

Kane610 · 2024-04-21T17:46:47Z

Test fails

FAILED tests/test_vouchers.py::test_vouchers[voucher_payload0] - AssertionError: assert '44703-' == '44703'

44703

44703-

Will the output from unifi be with or without hyphen?

Kane610

Ok, to merge once test pass, two questions for completeness

Kane610 · 2024-04-21T17:48:57Z

aiounifi/interfaces/vouchers.py

+                expire_number=int(
+                    voucher.duration.total_seconds() / 60  # Get minutes.
+                ),
+                expire_unit=1,


As the default value is 1 for expire unit would this be ok, or is there a specific reason this is 1

Suggested change

expire_unit=1,

expire_unit=voucher.expire_unit,

exire_unit is not an attribute of Voucher. 1 = minute, because expire_number came from voucher.duration in Seconds / 60 = Minutes

The attributes for vouchers of the return values of the API are called differently than those for creating.
For example, when creating, you pass the number of vouchers to be created. But these are then created as separate vouchers.
And when creating, you specify the expiry time and in which unit you specify the expiry time. In the return, you always get the duration time in seconds. However, you cannot create a voucher in seconds ....

Alright, yes indeed, thanks, as it has the default value assigned this can just be removed.

Suggested change

expire_unit=1,

Kane610 · 2024-04-21T17:49:42Z

aiounifi/interfaces/vouchers.py

+        """Create voucher on controller."""
+        return await self.controller.request(
+            VoucherCreateRequest.create(
+                number=1,


Number is a required item to create an instance of Voucher, is there a reason 1 is hardcoded?

Number is not an attribute of Voucher. Number is the number of vouchers to be create in one request

Alright, I would just add the default value to the create request and remove this instead.

Suggested change

number=1,

Kane610 · 2024-04-21T18:17:05Z

Right now the test expects "44703-"

Kane610 · 2024-04-21T18:32:08Z

aiounifi/interfaces/vouchers.py

+                expire_number=int(
+                    voucher.duration.total_seconds() / 60  # Get minutes.
+                ),
+                expire_unit=1,


Alright, yes indeed, thanks, as it has the default value assigned this can just be removed.

Suggested change

expire_unit=1,

Kane610 · 2024-04-21T18:34:05Z

aiounifi/interfaces/vouchers.py

+        """Create voucher on controller."""
+        return await self.controller.request(
+            VoucherCreateRequest.create(
+                number=1,


Alright, I would just add the default value to the create request and remove this instead.

Suggested change

number=1,

aiounifi/models/voucher.py

Kane610 · 2024-04-21T18:59:36Z

Suggested changes in #554

ufozone added 5 commits December 16, 2023 21:37

Add hotspot vouchers

5b1131f

Remove unused imports

3086a3e

Remove unused imports

b88738a

Fix wrong return type for status_expires model

54d1fe4

Fix expiration input from given Voucher model

45e328f

Kane610 requested changes Dec 18, 2023

View reviewed changes

bmos and others added 5 commits February 15, 2024 19:15

mypi and ruff fixes

89065c7

more type fixes

5e17d97

use f-string in code method

759190f

Merge pull request #1 from bmos/master

2ac284f

mypy and ruff fixes

Merge branch 'Kane610:master' into master

8ed62ca

Fix voucher format

d79d090

ufozone and others added 4 commits March 11, 2024 20:50

Merge branch 'Kane610:master' into master

d08890b

Add voucher interface to gateway class

15ec646

Add voucher to test controller

06d9956

Add tests for vouchers

04db869

Kane610 requested changes Apr 3, 2024

View reviewed changes

aiounifi/models/voucher.py Outdated Show resolved Hide resolved

ufozone and others added 2 commits April 3, 2024 23:18

Update aiounifi/models/voucher.py

5e2d896

Co-authored-by: Robert Svensson <Kane610@users.noreply.github.com>

Merge branch 'Kane610:master' into master

b66e569

ufozone mentioned this pull request Apr 3, 2024

Use own implementation for vouchers in aiounifi ufozone/ha-unifi-voucher#39

Closed

Fix RUFF warnings

31acc29

ufozone mentioned this pull request Apr 3, 2024

Use own implementation for vouchers in aiounifi ufozone/ha-unifi-voucher#40

Merged

Kane610 requested changes Apr 7, 2024

View reviewed changes

ufozone added 2 commits April 7, 2024 09:48

Mark some attributes as not required

81e897c

Update DocString and Typing

4fc8417

Kane610 requested changes Apr 10, 2024

View reviewed changes

ufozone added 2 commits April 10, 2024 21:36

Fix voucher tests

43e7f78

Fix voucher tests

632b764

Kane610 reviewed Apr 18, 2024

View reviewed changes

aiounifi/models/voucher.py Outdated Show resolved Hide resolved

Update docstring for voucher model

e960312

Kane610 reviewed Apr 19, 2024

View reviewed changes

aiounifi/models/voucher.py Outdated Show resolved Hide resolved

ufozone added 2 commits April 19, 2024 21:17

Update docstring of voucher model

c5d3e1a

Update docstring of voucher model

137e644

ufozone added 2 commits April 20, 2024 13:37

Remove message keys of voucher commands

29d99e2

Fix RUFF warning

688eb20

This was referenced Apr 20, 2024

Use voucher implementation from aiounifi ufozone/ha-unifi-voucher#53

Closed

Use voucher implementation from aiounifi ufozone/ha-unifi-voucher#56

Merged

Simplify voucher code generation

b5de641

Kane610 reviewed Apr 21, 2024

View reviewed changes

Update tests for vouchers

eefe2c1

Fix tests for vouchers

88105a0

Kane610 approved these changes Apr 21, 2024

View reviewed changes

Kane610 merged commit 075ac4a into Kane610:master Apr 21, 2024
1 check passed

Kane610 mentioned this pull request Apr 21, 2024

Tweak vouchers #645

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hotspot vouchers #554

Add hotspot vouchers #554

ufozone commented Dec 16, 2023

Kane610 left a comment

ufozone commented Feb 24, 2024

Kane610 commented Feb 25, 2024

ufozone commented Mar 11, 2024

Kane610 commented Mar 15, 2024

Kane610 left a comment

ufozone commented Apr 3, 2024

Kane610 left a comment

Kane610 left a comment •

edited

Loading

ufozone commented Apr 10, 2024

Kane610 commented Apr 10, 2024

ufozone commented Apr 10, 2024

Kane610 commented Apr 10, 2024

ufozone commented Apr 19, 2024

ufozone commented Apr 19, 2024

Kane610 commented Apr 20, 2024

ufozone commented Apr 21, 2024 •

edited

Loading

Kane610 commented Apr 21, 2024

Kane610 left a comment

Kane610 Apr 21, 2024

ufozone Apr 21, 2024

Kane610 Apr 21, 2024

Kane610 Apr 21, 2024

ufozone Apr 21, 2024

Kane610 Apr 21, 2024

Kane610 commented Apr 21, 2024

Kane610 Apr 21, 2024

Kane610 Apr 21, 2024

Kane610 commented Apr 21, 2024

Add hotspot vouchers #554

Add hotspot vouchers #554

Conversation

ufozone commented Dec 16, 2023

Kane610 left a comment

Choose a reason for hiding this comment

ufozone commented Feb 24, 2024

Kane610 commented Feb 25, 2024

ufozone commented Mar 11, 2024

Kane610 commented Mar 15, 2024

Kane610 left a comment

Choose a reason for hiding this comment

ufozone commented Apr 3, 2024

Kane610 left a comment

Choose a reason for hiding this comment

Kane610 left a comment • edited Loading

Choose a reason for hiding this comment

ufozone commented Apr 10, 2024

Kane610 commented Apr 10, 2024

ufozone commented Apr 10, 2024

Kane610 commented Apr 10, 2024

ufozone commented Apr 19, 2024

ufozone commented Apr 19, 2024

Kane610 commented Apr 20, 2024

ufozone commented Apr 21, 2024 • edited Loading

Kane610 commented Apr 21, 2024

Kane610 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kane610 commented Apr 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kane610 commented Apr 21, 2024

Kane610 left a comment •

edited

Loading

ufozone commented Apr 21, 2024 •

edited

Loading