Closed PhilipNeffOnGithub closed 3 days ago
Thanks for your contribution @PhilipNeffOnGithub, it is much appreciated. To move along the PR review would it be possible for you to add screenshots of the Before and After (with pagination limits vs without - this PR)?
@gabalafou can you please have a look at this PR and the corresponding issue? My feeling is that this PR will be more of a mitigation for now but longer term we would like to look at the UI itself and incorporate an actual pagination or other display behaviour as I can imagine there will be a point where a user has loads of environments (hundreds)
cc/ @smeragoel for visibility
Sorry it's taken me so long to get to this.
The code suggests that pagination should work. I would prefer to see if it's fixable before removing the functionality. Or perhaps we could consider other stop-gap solutions, such as a toggle or user preference to load all rather than paginate?
@nkaretnikov can you help me figure out how to generate 100s of conda store environments in my local dev environment?
@gabalafou Sure! Take a look at these new tests being added: https://github.com/conda-incubator/conda-store/pull/760.
These show how to log in and create an environment. See test_admin_user_can_create_environment
and specifically this part and what API
does internally:
namespace = utils.API.gen_random_namespace()
api = utils.API(base_url=base_url, token=token)
api.create_namespace(namespace)
response = api.create_environment(namespace, specification_path)
Let me know if you have more questions.
Hello again! Any update on this review?
Sorry for the radio silence and thanks for the ping. @gabalafou could you please have another look at this PR please?
Apologies for the delay. It took me some time to set up a local dev server with 100+ environments.
When I test this PR locally, it doesn't work. I'm wondering if the server was updated at some point to automatically enforce pagination because when I run the server locally with this PR applied, I only get the first 100 environments in the UI. And when I inspect the API request and response, it looks like so:
GET /conda-store/api/v1/environment/?search=
{
"status": "ok",
"data": [
{
"id": 94,
"namespace": {
"id": 1,
"name": "default",
"metadata_": {
},
"role_mappings": []
},
"name": "test_env_10009",
"current_build_id": 94,
"current_build": null,
"description": ""
},
... 98 other environments ...
{
"id": 64,
"namespace": {
"id": 1,
"name": "default",
"metadata_": {
},
"role_mappings": []
},
"name": "test_env_91627",
"current_build_id": 64,
"current_build": null,
"description": ""
}
],
"message": null,
"page": 1,
"size": 100,
"count": 115
}
Note that if I remove the ?search
query param, I get the same result.
Is this PR meant to be combined with a backend PR to remove API pagination?
I think the underlying issue, #312, that this PR addresses was fixed in #391, so I'm closing this for now. We can re-open if necessary.
Fixes #312 Fixes issue 312, where only the first page of environments get loaded
Description
It seems that the pagination of environments does not happen, however the environments are still loaded as if they are, with only 100 per page. So, if a user has over 100 environments, the UI will only show the first 100 (in alphabetical order).
This pull request: