-
-
Notifications
You must be signed in to change notification settings - Fork 188
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 device groups #203 #492
Conversation
7e03a07
to
bad4070
Compare
bad4070
to
c9aed4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress @pandafy! Here's a first code-only review.
0e524a7
to
6014c98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one failing test.
A group can be specified for devices, i.e. DeviceGroup. DeviceGroup contains metadata about group of deivces in form of JSON. JSONSchema can be set systemwide for validation of entered metadata. Added REST API endpoint for listing, creating and retrieving DeviceGroups. Closes #203
0dc4dd5
to
4d036d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The screenshot shows "Configuration menu" and "Advanced mode" buttons, which shouldn't be included, is that screenshot outdated or are those buttons not disabled? It should be possible to remove them, see how it's done in credentials for reference.
…om JSONSchema Editor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the metadata without any schema defined look weird to you as well?
Device group detail API URL
Getting the detail of a group returns me the groups, wierd..
Also, there's no DELETE/PUT/PATCH for group detail but only get and POST, seems a replica of the list view.
Menu
Please also register a menu group, we'll adapt this in #493.
pagination_class = ListViewPagination | ||
|
||
|
||
class DeviceGroupDetailView(ProtectedAPIMixin, ListCreateAPIView): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you're inherting ListCreateAPIView
.
Please spend a bit more time testing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I caught this on in #500. Forgot to push changes on this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API is missing the UUID of the group, without that it's not possible to determine the detail URL easily when browsing the API.
The registration of a new menu item is pending.
Also, I think I understood more the issue with the config editor.
I think we should set the default schema to be an object which allows any property, not an empty schema.
I'll take care of the rest with the CSS on that part.
c67fdf1
to
8de60b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the admin page of the device, the label of the group field is "Device group", but since we are in the device edit page we don't need to repeat the word "Device" there, can you ensure it's named just "Group"? I think it could be done at model level to simplify things, it's always gonna be like that (the group attribute of a device is the device group).
README.rst
Outdated
@@ -628,8 +628,8 @@ Allows to specify backend URL for API requests, if the frontend is hosted separa | |||
Allows to specify a `list` of tuples for adding commands as described in | |||
`'How to add commands" <#how-to-add-commands>`_ section. | |||
|
|||
``OPENWISP_CONTROLLER_DEVICE_GROUP_SCHEMA`` | |||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, why this change? It was better before!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes it consistent with VPNCLIENT
and explicitly show that DeviceGroup is it's own separate thing.
9cbcf24
to
e2e725a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert 8de60b2 as discussed recently.
This reverts commit 8de60b2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the reverting.
A group can be specified for devices, i.e. DeviceGroup.
DeviceGroup contains metadata about group of deivces in form of JSON.
JSONSchema can be set systemwide for validation of entered metadata.
Added REST API endpoint for listing, creating and retrieving DeviceGroups.
Closes #203
DeviceGroup
orGroup
model and admin (maybe we should call itDeviceGroup
to avoid confusion with other models that are called Group, like django'sauth.Group
model), the model shall have fields like (organization
, usingOrgMixin
),name
,description
,meta_data
,created
,modified
.Device
, the field should be aForeignKey
toDeviceGroup
.group
field of a device is change and document the new signal.DeviceGroup.meta_data
and document it.meta_data
field is JSON formatted field which will leverage the work we did with JSON schema to show the UI, by default the schema will be empty and allow any property so the user can add what they need.config.Group
./api/v1/controller/group/
,/api/v1/controller/group/{id}/
, authentication and permission checks should be consistent with the REST API endpoints added recently in [feature] REST API for main controller features #379 #386 and the ones being added in [api] Rest API for PKI app #455.