[feature] Emit signal in freeradius accounting view #341 #350

devkapilbansal · 2021-11-15T19:27:20Z

Closes #341

coveralls · 2021-11-15T19:42:25Z

Coverage increased (+0.002%) to 99.141% when pulling 716a4a4 on issues/341-emit-signal into db2fbbe on master.

nemesifier

It looks good, please add the docs.

nemesifier · 2021-11-16T01:38:50Z

openwisp_radius/signals.py

@@ -0,0 +1,3 @@
+from django.dispatch import Signal
+
+radius_accounting_success = Signal(providing_args=['account_data'])


Please chagne account_data to accounting_data here and anywhere else, because account and accounting are two different concepts which must not be confused.

nemesifier · 2021-11-16T19:36:47Z

docs/source/user/api.rst

+- ``accounting_data``: accounting information
+
+This signal is emitted every time accounting information is added or updated.
+


Please let's add a new page as requested in the issue description because this will likely grow in the near future.

nemesifier · 2021-11-17T01:44:22Z

docs/source/developer/signals.rst

+
+- ``accounting_data``: accounting information
+
+This signal is emitted every time accounting information is added or updated.


Let's make sure the following concepts are extra clear:

the signal is emitted every time the accounting REST API endpoint completes successfully

the signal is emitted right before the response is returned

accounting_data is a dict

nemesifier · 2021-11-17T18:45:15Z

openwisp_radius/api/freeradius_views.py

@@ -445,6 +448,11 @@ def _data_to_acct_model(self, valid_data):
        valid_data['organization'] = acct_org
        return valid_data

+    def send_radius_accounting_signal(self, accounting_data):
+        radius_accounting_success.send(
+            sender=self.__class__, accounting_data=accounting_data


The sender is the view, I noticed only now, it makes sense, but we haven't specified this in the docs, so users would have to dig in the code to find out, which is means the documentation is not good enough. Please fix this.

I thought that it could be useful to pass the view instance (self) instead of the class, because that allows accessing a lot of information about the request and it can be useful to developers who may want to use this signal. Can you please update it here and in the docs?

codesankalp · 2021-11-18T12:18:56Z

openwisp_radius/api/freeradius_views.py

            return Response(None, status=201, headers=headers)
        else:
            serializer = self.get_serializer(instance, data=data, partial=False)
            serializer.is_valid(raise_exception=True)
            acct_data = self._data_to_acct_model(serializer.validated_data.copy())
            serializer.update(instance, acct_data)
+            self.send_radius_accounting_signal(acct_data)


@devkapilbansal Instead of acct_data, can we pass serializer.data?
Because while passing this data to celery I am getting:
kombu.exceptions.EncodeError: Object of type Organization is not JSON serializable

_data_to_acct_model removes status_type and adds organization. Also, status_type is more required in detecting the radius status i.e. Start.

Makes sense.

nemesifier · 2021-11-19T01:13:35Z

openwisp_radius/api/freeradius_views.py

@@ -445,6 +448,11 @@ def _data_to_acct_model(self, valid_data):
        valid_data['organization'] = acct_org
        return valid_data

+    def send_radius_accounting_signal(self, accounting_data):
+        radius_accounting_success.send(
+            sender=self.__class__, accounting_data=accounting_data


I thought that it could be useful to pass the view instance (self) instead of the class, because that allows accessing a lot of information about the request and it can be useful to developers who may want to use this signal. Can you please update it here and in the docs?

nemesifier

This looks good now 👍

I will do some manual testing and then merge it.

codesankalp · 2021-11-26T00:58:09Z

openwisp_radius/api/freeradius_views.py

@@ -445,6 +448,9 @@ def _data_to_acct_model(self, valid_data):
        valid_data['organization'] = acct_org
        return valid_data

+    def send_radius_accounting_signal(self, accounting_data):
+        radius_accounting_success.send(sender=self, accounting_data=accounting_data)


When we are passing self from here it is a view object which we can't specify as sender while connecting to receivers.
We must use __class__ to get the class view and not the view object.
cc: @nemesisdesign

@codesankalp we know it's a view instance, the change was done on purpose (django also passes view instances), what is the problem you're trying to describe? I don't understand.

Oh I think I understand now, in this case we don't need to take into account the sender though, because only this class will send this. Let's look at how django implements some of their request cycle signals and do it similarly (let's look if they use the view instance or class as sender).

@devkapilbansal Sankalp is right, let's do the following which should be more ortodox:

restore the previous sender (the class)

pass the request and view instance as an additional argument

update docs

nemesifier · 2021-11-26T21:15:21Z

openwisp_radius/signals.py

@@ -0,0 +1,3 @@
+from django.dispatch import Signal
+
+radius_accounting_success = Signal(providing_args=['accounting_data'])


providing_args doesn't play well with django 4 in which it was removed, please see the release notes of django 4 (lookfor providing_args) and update this code according to what is suggested there.

nemesifier · 2021-11-26T21:15:36Z

openwisp_radius/api/freeradius_views.py

@@ -445,6 +448,9 @@ def _data_to_acct_model(self, valid_data):
        valid_data['organization'] = acct_org
        return valid_data

+    def send_radius_accounting_signal(self, accounting_data):
+        radius_accounting_success.send(sender=self, accounting_data=accounting_data)


@devkapilbansal Sankalp is right, let's do the following which should be more ortodox:

restore the previous sender (the class)

pass the request and view instance as an additional argument

update docs

Closes #341

docs/source/developer/signals.rst

setup.py

devkapilbansal force-pushed the issues/341-emit-signal branch from 158e4aa to a410ad9 Compare November 15, 2021 19:33

nemesifier reviewed Nov 16, 2021

View reviewed changes

devkapilbansal marked this pull request as ready for review November 16, 2021 16:02

nemesifier requested changes Nov 16, 2021

View reviewed changes

nemesifier requested changes Nov 17, 2021

View reviewed changes

pandafy assigned devkapilbansal Nov 17, 2021

devkapilbansal force-pushed the issues/341-emit-signal branch from 450bdfd to 282a383 Compare November 17, 2021 18:28

nemesifier requested changes Nov 17, 2021

View reviewed changes

devkapilbansal force-pushed the issues/341-emit-signal branch from 282a383 to 027d497 Compare November 17, 2021 22:05

codesankalp reviewed Nov 18, 2021

View reviewed changes

nemesifier requested changes Nov 19, 2021

View reviewed changes

devkapilbansal force-pushed the issues/341-emit-signal branch from ef50247 to 2ce3de0 Compare November 19, 2021 13:15

nemesifier approved these changes Nov 25, 2021

View reviewed changes

codesankalp reviewed Nov 26, 2021

View reviewed changes

nemesifier requested changes Nov 26, 2021

View reviewed changes

devkapilbansal and others added 7 commits November 30, 2021 00:22

[feature] Emit signal in freeradius accounting view #341

855b626

Closes #341

[docs] Add docs about radius accounting success signal

b8afefd

[docs] Move signal docs to dedicated page

fb6be1c

[chores] Minor improvements

4aae1e4

[change] Send serializer data inside accounting signal

dbd7af5

[chores] Minor improvements

5067ab4

[change] Changed sender and passing view in signal

5942fbd

codesankalp force-pushed the issues/341-emit-signal branch from 2ce3de0 to 5942fbd Compare November 29, 2021 18:53

[change] Updated swapper version

9a1d2e2

devkapilbansal force-pushed the issues/341-emit-signal branch from 5942fbd to d8385e3 Compare November 29, 2021 19:08

codesankalp force-pushed the issues/341-emit-signal branch from d8385e3 to 9a1d2e2 Compare November 29, 2021 19:11

[docs] Updated signal.rst

3db0a2a

nemesifier requested changes Nov 29, 2021

View reviewed changes

docs/source/developer/signals.rst Outdated Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

[change] Updated setup.py and signals.rst

716a4a4

nemesifier approved these changes Nov 29, 2021

View reviewed changes

nemesifier merged commit 8124ff4 into master Nov 29, 2021

nemesifier deleted the issues/341-emit-signal branch November 29, 2021 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] Emit signal in freeradius accounting view #341 #350

[feature] Emit signal in freeradius accounting view #341 #350

devkapilbansal commented Nov 15, 2021

coveralls commented Nov 15, 2021 •

edited

Loading

nemesifier left a comment

nemesifier Nov 16, 2021

nemesifier Nov 16, 2021

nemesifier Nov 17, 2021

nemesifier Nov 17, 2021

nemesifier Nov 19, 2021

codesankalp Nov 18, 2021

nemesifier Nov 18, 2021

nemesifier Nov 19, 2021

nemesifier left a comment

codesankalp Nov 26, 2021

nemesifier Nov 26, 2021

nemesifier Nov 26, 2021

nemesifier Nov 26, 2021

nemesifier Nov 26, 2021

nemesifier Nov 26, 2021

		@@ -0,0 +1,3 @@
		from django.dispatch import Signal

		radius_accounting_success = Signal(providing_args=['account_data'])

		- ``accounting_data``: accounting information

		This signal is emitted every time accounting information is added or updated.

[feature] Emit signal in freeradius accounting view #341 #350

[feature] Emit signal in freeradius accounting view #341 #350

Conversation

devkapilbansal commented Nov 15, 2021

coveralls commented Nov 15, 2021 • edited Loading

nemesifier 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier 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

coveralls commented Nov 15, 2021 •

edited

Loading