[fix] Ensure recent items are shown first in the REST API - #1044
Conversation
|
@nemesifier |
|
updated commit message to prevent build failure. |
nemesifier
left a comment
There was a problem hiding this comment.
@stktyagi it looks good to me! I just want to find some time to double check this is the only endpoint where we need to fix this.
nemesifier
left a comment
There was a problem hiding this comment.
Other endpoints I found that are suffering from the same issue:
/api/v1/controller/group//api/v1/controller/device/
Can we take advantage and fix those as well?
Regarding the tests, I recommend extending the existing tests for these endpoints with a subtest which checks the ordering, let's try to reduce the new lines to the bare minimuim required to test the ordering effectively.
Please also update your local copy of openwisp-utils to the latest master, reinstall it to update the dependencies and run openwisp-qa-format again as there's been slight changes to the QA dependencies and configuration (see openwisp/openwisp-utils#473 and openwisp/openwisp-utils#456).
See also my comments below.
Fixed bug where device connections were not properly filtered in the API response with recency with new tests added. Fixes #1040
Fixed bug where device connections were not properly filtered in the API response with recency with new tests added. All the command list endpoint models were updated. Fixes #1040
Added and tested recency tests for config /group and /device endpoints. Updated tests(added as subtests) for previously fixed and recently fixed endpoints. Fixes #1040
|
@nemesifier greetings, |
nemesifier
left a comment
There was a problem hiding this comment.
I must have made some confusion during the previous review because I see that device and device group are already ordered properly. Well, that's good to know, thanks for adding the tests! I did some minor simplifications, should be ready to merge.
| r = self.client.get(path) | ||
| self.assertEqual(r.status_code, 200) | ||
| with self.subTest("should return devices ordered by most recent first"): | ||
| Device.objects.all().delete() |
There was a problem hiding this comment.
Why is this needed? We could avoid it and add only 2 more devices so we end up with 3.
Checklist
Reference to Existing Issue
Closes #1040.
Description of Changes
Added ordering by creation date (descending) to the Base command view to ensure consistent and expected command ordering and checked all other list apis to ensure recency.
Tests to supported the same added.