[feature] Run batch user creation asynchronously #608 #646
Conversation
openwisp_radius/tasks.py
Outdated
| try: | ||
| batch = RadiusBatch.objects.get(pk=batch_id) | ||
| batch.status = "processing" | ||
| batch.save(update_fields=["status"]) |
There was a problem hiding this comment.
Let's move this logic as a class method and keep this file lean.
openwisp_radius/tasks.py
Outdated
| RadiusAccounting.objects.bulk_update(updated_sessions, fields=["groupname"]) | ||
|
|
||
|
|
||
| @shared_task |
There was a problem hiding this comment.
tests/openwisp2/settings.py
Outdated
| ASGI_APPLICATION = "openwisp2.routing.application" | ||
|
|
||
| if TESTING: | ||
| CHANNEL_LAYERS = {"default": {"BACKEND": "channels.layers.InMemoryChannelLayer"}} |
There was a problem hiding this comment.
Are we sure this wouldn't cause issues with selenium tests?
openwisp_radius/templates/openwisp-radius/admin/rad_batch_users_change_form.html
Show resolved
Hide resolved
There was a problem hiding this comment.
So far it seems to almost meet all requirements, I found one blunder regarding the notification not targeting the right object and a few details to improve. See below.
Notification points to org instead of batch object
I created a batch and called it "AsyncTest".
The notification looks like:
The batch user creation "[testing](http://localhost:8000/admin/openwisp_users/organization/51689c38-7a02-4609-9326-a79e1e7f8b1d/change/)" was successfully processed.
And points to the organization.
It should instead point to the Batch User Creation object.
New batch: hide status
The status line doesn't provide any value here and it would be better to hide it either with python code or CSS.
Existing batch
Current batch details page:
I think we should show the status field somewhere on top.
I am also not sure about the line "Completed: This batch operation has completed successfully." provides any real value.
I will leave more comments below about both.
Resolved it. |
There was a problem hiding this comment.
The status field in the admin change page was still being shown as last, so I fixed this in 6f5a12a (and pushed some other minor improvements).
During testing I found another minor issue I didn't see before, but it should be quick to fix:
when using the API to create a batch operation, the pdf_link in the 202 response refers to an empty PDF (since the batch hasn't been processed yet).
We need to change the get_pdf_link method to return the link only when the status is completed and add a test for this, I think we can add a subTest in the existing test_api_batch_pdf_link.
See my suggestions below to improve the readability and maintenability of the code.
The rest looks good so we can merge after this round of improvements.
|
|
||
| batch.refresh_from_db() | ||
| response_serializer = self.get_serializer(batch) | ||
|
|
There was a problem hiding this comment.
Please remove these blank lines as they don't provide any value.
| return Response(response.data, status=status.HTTP_201_CREATED) | ||
| return Response( | ||
| response_serializer.data, status=status.HTTP_201_CREATED | ||
| ) |
There was a problem hiding this comment.
Let's reduce repetition:
status_code = status.HTTP_202_ACCEPTED if is_async else status.HTTP_201_CREATED
return Response(
response_serializer.data, status=status_code
)| items_to_process = app_settings.BATCH_ASYNC_THRESHOLD | ||
|
|
||
| is_async = items_to_process >= app_settings.BATCH_ASYNC_THRESHOLD | ||
|
|
There was a problem hiding this comment.
Too many blank lines.
| notify.send( | ||
| type="generic_message", | ||
| level="error", | ||
| message=_(f"The batch creation operation for '{self.name}' failed."), |
There was a problem hiding this comment.
| message=_(f"The batch creation operation for '{self.name}' failed."), | |
| message=_(f"The batch creation operation for \"{self.name}\" failed."), |
| type="generic_message", | ||
| level="success", | ||
| message=_( | ||
| f"The batch creation operation for '{self.name}' " |
There was a problem hiding this comment.
| f"The batch creation operation for '{self.name}' " | |
| f"The batch creation operation for \"{self.name}\" " |
Closes #608