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

Feature/464 permission admin view slow #508

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

danielmursa-dev
Copy link
Contributor

@danielmursa-dev danielmursa-dev commented Dec 31, 2024

Fixes #464

An internal administration endpoint admin/core/objecttype/<objecttype_id>/_versions/ was created to retrieve all versions related to the objecttype.

Then the FE part was modified, so that each time an objecttype is modifie/selectedd, it call a new request to the endpoint.

Copy link
Collaborator

@stevenbal stevenbal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of the following to make it possible to fetch the fields via the frontend (such that the form doesn't have to be saved to fetch them):

  • make an endpoint on the admin (that requires admin authentication) that performs the request for a specific object_type_id to retrieve the data_fields (e.g. /admin/objecttypes/<id>). That way we don't have to expose the token to the frontend and we only need to pass the id to the frontend
  • in react, call this new endpoint for the currently selected objecttype and display the result

@annashamray what do you think about this?

Copy link
Collaborator

@stevenbal stevenbal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, some minor questions/remarks

src/objects/core/admin.py Show resolved Hide resolved
src/objects/core/admin.py Outdated Show resolved Hide resolved
src/objects/core/admin.py Outdated Show resolved Hide resolved
src/objects/core/admin.py Outdated Show resolved Hide resolved
@danielmursa-dev danielmursa-dev requested review from stevenbal and removed request for alextreme January 27, 2025 13:19
Copy link
Collaborator

@annashamray annashamray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I've noticed now, but it's been implemented like this since the beginning:

The state of selected fields is not tracked:
If you change the objecttype in the dropdown and then change it back, the fields previous will not be selected, even if they are saved in the DB
Screencast from 28-01-25 16:32:04.webm

It's been like this probably forever, so could you please create an issue to fix it?
No need to fix it in this PR, but some cleanup would be nice

src/objects/core/admin.py Show resolved Hide resolved
src/objects/tests/admin/test_core_views.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@annashamray annashamray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create an issue for keeping track of selected fields after changing the objecttype

Copy link
Collaborator

@stevenbal stevenbal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments about the tests, looks good otherwise

object_type = ObjectTypeFactory.create()
url = reverse("admin:objecttype_versions", args=[object_type.pk])

with self.assertRaises(requests_mock.exceptions.NoMockAddress):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an exception that won't happen outside of tests, you can specify exc=... to let requests_mock raise a specific exception for a URL:

from requests.exceptions import HTTPError
m.get(f"{object_type.url}/versions", exc=HTTPError)

And I think in that case, the endpoint returns an empty list

url = reverse("admin:objecttype_versions", args=[100])
response = self.app.get(url, user=user)
self.assertEqual(response.status_code, 200)
self.assertEqual(len(response.json), 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's verify that it is an empty list (and not an object or something else with len == 0)

Suggested change
self.assertEqual(len(response.json), 0)
self.assertEqual(response.json, [])

self.assertEqual(len(response.json), 1)

# object_type does not exist
url = reverse("admin:objecttype_versions", args=[100])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependent on the amount of objecttypes created in other tests (or using --keepdb locally) this PK might be valid, so let's make sure it is unused

Suggested change
url = reverse("admin:objecttype_versions", args=[100])
url = reverse("admin:objecttype_versions", args=[object_type.pk+1])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Objecten API: Permission admin view is slow
3 participants