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

[feature] Added REST API for wifisession and wificlients #374 #382

Merged

Conversation

Aryamanz29
Copy link
Member

@Aryamanz29 Aryamanz29 commented May 27, 2022

Blockers

Closes #374

Peek.2022-05-27.16-24.mp4

GIF/Demo

Checks:

  • I have manually tested the proposed changes
  • I have updated the documentation (e.g. README.rst)

@coveralls
Copy link

coveralls commented May 27, 2022

Coverage Status

Coverage: 98.943% (+0.03%) from 98.909% when pulling 95fa60a on Aryamanz29:issue-374/wifi-client-session-rest-api into 15977c7 on openwisp:master.

@Aryamanz29
Copy link
Member Author

I need your review (when you're free) @pandafy @nemesisdesign. This will let me know that I'm going on the right direction with this PR or not 😄

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Looks good, please add at least one test for each endpoint.

README.rst Outdated Show resolved Hide resolved
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

I found some problems during testing.

Multitenancy test gave error

I tried logging in with a non superuser to test multitenancy but when I tried opening the wifi session list API endpoint I got this exception:

FieldError at /api/v1/monitoring/wifi-session/
Cannot resolve keyword 'organization' into field. Choices are: device, device_id, id, interface_name, modified, ssid, start_time, stop_time, wifi_client, wifi_client_id

The same happens for the wifi-client endpoint. Please test this on your end and look at the implementation of the multitenancy in other API endpoints, refer to https://github.com/openwisp/openwisp-users#organization_field and the other DRF utilities provided in OpenWISP Users.

Filters invalid name

Do you know why there's this "[invalid name]" here?

Screenshot from 2022-05-30 16-28-24

Browsable API selects for foreign keys

The wifi session API endpoint shows a select for "device" and "wifi client", however, these selects will be huge on production systems, slowing down the response time in the browsable API or even causing it to fail with a timeout error. For this reason, can you find a way to display this field using only the primary key?

See https://www.django-rest-framework.org/topics/browsable-api/#handling-choicefield-with-large-numbers-of-items for more info.

openwisp_monitoring/device/api/serializers.py Outdated Show resolved Hide resolved
openwisp_monitoring/device/api/serializers.py Outdated Show resolved Hide resolved
@Aryamanz29
Copy link
Member Author

Aryamanz29 commented Jun 1, 2022

Todo :

  • Nested serializer for wificlient.
  • Fix browsable API selects for foreign keys
  • Need to fix browsable API [Invalid name] attribute bug.
  • Need to fix "Multitenancy test error" (It might take time, I'll explore openwisp-users first)
  • At least one test for the each endpoint.
  • Mention used API filters in README.
    -------------------------------------------------------------
  • Add tests for API filters.
  • It is possible to create WifiSession with device_id from another organization. This is undesired. Add a failing test for this.
  • Remove WifiClients redundant device and device__organization filters.
  • Filtering with mac_address is not useful. It will always return a single WifiClient object. We can remove that.
  • Refactor WifiSessionCreateUpdateSerializer with Meta.extra_kwargs.
  • Remove redundant comments and update README.
  • Use organization_slug for filtering https://github.com/openwisp/openwisp-controller#list-locations
  • Use comparison (gt, gte, lt, lte) for filtering with time.

@Aryamanz29 Aryamanz29 force-pushed the issue-374/wifi-client-session-rest-api branch 4 times, most recently from 268f065 to 40845bb Compare June 2, 2022 20:45
Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

Good work @Aryamanz29! This PR is coming along well.

Other requested changes

  • It is possible to create WifiSession with device_id from another organization. This is undesired. Add a failing test for this.
  • Add tests for REST API filters

openwisp_monitoring/device/api/views.py Outdated Show resolved Hide resolved
openwisp_monitoring/device/api/views.py Outdated Show resolved Hide resolved
openwisp_monitoring/device/api/serializers.py Outdated Show resolved Hide resolved
openwisp_monitoring/device/tests/test_api.py Outdated Show resolved Hide resolved
openwisp_monitoring/device/tests/test_api.py Outdated Show resolved Hide resolved
openwisp_monitoring/device/api/serializers.py Outdated Show resolved Hide resolved
openwisp_monitoring/device/tests/test_api.py Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@Aryamanz29 Aryamanz29 force-pushed the issue-374/wifi-client-session-rest-api branch from 433d6ca to 3074fea Compare June 9, 2022 15:19
@Aryamanz29 Aryamanz29 marked this pull request as ready for review June 10, 2022 17:04
@Aryamanz29 Aryamanz29 self-assigned this Jun 10, 2022
@Aryamanz29 Aryamanz29 added the enhancement New feature or request label Jun 10, 2022
Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

I would add dedicated tests for multi-tenancy for each model:

  • test_wifi_clients_multi_tenancy
  • test_wifi_session_multi_tenancy
    And test each operation (GET, POST, PUT, PATCH) within a subtest in these test cases.

openwisp_monitoring/device/api/serializers.py Outdated Show resolved Hide resolved
openwisp_monitoring/device/tests/test_api.py Outdated Show resolved Hide resolved
openwisp_monitoring/device/tests/test_api.py Outdated Show resolved Hide resolved
- Added nested serializer.
- Used select_related() to enhance API performance.
- Added missing fields like modified, created, device_name, org_name etc.
… input

- Fixed browsable API selects for foreign keys, Changed select field to input.
- Fixed browsable API [Invalid name] attribute bug, Explicitly provided label for related model.
- Fixed multitenancy test error.
- Added use of available API filter in README.
- Changed ordering in WifiClient class Meta to prevent DRF
"UnorderedObjectListWarning: Pagination may yield inconsistent results with an unordered object_list"
- Improved tests.
- Mentioned device_group in the README.
- Added extra_kwargs in WifiSessionCreateUpdateSerializer.
- Chaneged device_organization filter to organization_slug filter.
- Removed unused filters from README.
- Added comparison (gt, gte, lt, lte) for filtering with time.
- Added tests for API filters.
@Aryamanz29 Aryamanz29 force-pushed the issue-374/wifi-client-session-rest-api branch from eb9d404 to 385d69c Compare June 14, 2022 07:57
@Aryamanz29 Aryamanz29 force-pushed the issue-374/wifi-client-session-rest-api branch from a36e71c to cdf00bb Compare March 13, 2023 12:38
Aryamanz29 added a commit to Aryamanz29/openwisp-monitoring that referenced this pull request May 8, 2023
…i session endpoint

As per to code review:
openwisp#382 (comment)

- Removed wifi client endpoints.
- Exposed only list wifi session endpoint.
- Updated tests and docs.
Aryamanz29 added a commit to Aryamanz29/openwisp-monitoring that referenced this pull request May 8, 2023
…i session endpoint

As per the code review:
openwisp#382 (comment)

- Removed wifi client endpoints.
- Exposed only list wifi session endpoint.
- Updated tests and docs.
@Aryamanz29 Aryamanz29 force-pushed the issue-374/wifi-client-session-rest-api branch from 475c786 to 51af136 Compare May 8, 2023 10:50
…i session endpoint

As per the code review:
openwisp#382 (comment)

- Removed wifi client endpoints.
- Exposed only list wifi session endpoint.
- Updated tests and docs.
@Aryamanz29 Aryamanz29 force-pushed the issue-374/wifi-client-session-rest-api branch from 51af136 to 1c673f0 Compare May 8, 2023 11:00
… views of the admin view

- Improved list and detail api response (similar to admin).
- Added api tests.
@Aryamanz29 Aryamanz29 force-pushed the issue-374/wifi-client-session-rest-api branch from cfcec98 to 5301a53 Compare May 24, 2023 13:01
@Aryamanz29 Aryamanz29 force-pushed the issue-374/wifi-client-session-rest-api branch 2 times, most recently from 029ea5e to 5f03f3a Compare May 25, 2023 08:12
@Aryamanz29
Copy link
Member Author

Updates

Below is a comparison between the WiFi session admin (list and detail view) and the WiFi session API (detail and list view):

Screenshot from 2023-05-25 16-05-33

Screenshot from 2023-05-25 15-58-35

Screenshot from 2023-05-25 15-59-07

Screenshot from 2023-05-25 15-58-57

@nemesisdesign Now, I have a question regarding how we should include WiFi client field information in the API response. Should we use a nested serializer (as demonstrated in the API detail view) or should we directly add the client serializer fields (as displayed in the API list) to the response?

@Aryamanz29 Aryamanz29 force-pushed the issue-374/wifi-client-session-rest-api branch from 5f03f3a to 9832b80 Compare May 25, 2023 10:59
- `ValidatedModelSerializer` and `FilterSerializerByOrgManaged`
is not required for read-only API views.
@Aryamanz29 Aryamanz29 force-pushed the issue-374/wifi-client-session-rest-api branch from 9832b80 to 15260e2 Compare May 25, 2023 11:14
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@Aryamanz29 the nested serialzier solution looks nice and clean!

@nemesifier nemesifier merged commit 5dd00cc into openwisp:master May 29, 2023
9 checks passed
@Aryamanz29 Aryamanz29 deleted the issue-374/wifi-client-session-rest-api branch May 30, 2023 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Add REST API for wifi sessions and wifi clients
4 participants