Skip to content
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

Removing environment pagination #356

Closed

Conversation

PhilipNeffOnGithub
Copy link

@PhilipNeffOnGithub PhilipNeffOnGithub commented Jan 19, 2024

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:

  • Removes size and page number from the api call, as well as the corresponding functions.
    Changes were tested locally.

@pavithraes pavithraes added needs: review 👀 area: user experience 👩🏻‍💻 Items impacting the end-user experience labels Jan 26, 2024
@trallard
Copy link
Collaborator

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)?

@trallard
Copy link
Collaborator

@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

@trallard trallard requested a review from gabalafou January 31, 2024 12:56
@gabalafou
Copy link
Contributor

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?

@gabalafou
Copy link
Contributor

@nkaretnikov can you help me figure out how to generate 100s of conda store environments in my local dev environment?

@nkaretnikov
Copy link
Contributor

nkaretnikov commented Feb 21, 2024

@gabalafou Sure! Take a look at these new tests being added: conda-incubator/conda-store#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.

@gabalafou gabalafou self-assigned this Mar 26, 2024
@PhilipNeffOnGithub
Copy link
Author

Hello again! Any update on this review?

@trallard
Copy link
Collaborator

trallard commented Apr 9, 2024

Sorry for the radio silence and thanks for the ping.
@gabalafou could you please have another look at this PR please?

@gabalafou
Copy link
Contributor

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?

@gabalafou
Copy link
Contributor

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.

@gabalafou gabalafou closed this Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

Only first page of GET /environment is loaded
5 participants