[fix] Ensure recent items are shown first in the REST API #1044
Conversation
|
@nemesifier |
stktyagi
force-pushed
the
issue/1040-added-recency-tests
branch
3 times, most recently
from
72c4e9e to
d079110
Compare
|
updated commit message to prevent build failure. |
nemesifier
changed the title
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
github-project-automation
bot
moved this to In progress
in OpenWISP Priorities for next releases
nemesifier
left a comment
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
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 openwisp#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 openwisp#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 openwisp#1040
stktyagi
force-pushed
the
issue/1040-added-recency-tests
branch
from
f915d33 to
40f7b91
Compare
|
@nemesifier greetings, |
nemesifier
left a comment
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.
github-project-automation
bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
github-project-automation
bot
moved this from Reviewer approved
to Done
in OpenWISP Priorities for next releases
3 participants
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.