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

Fix get_route_for_model for ContentTypes #3341

Merged
merged 4 commits into from Feb 22, 2023

Conversation

timizuoebideri1
Copy link
Contributor

Closes: #3080

What's Changed

Running get_route_for_model(ConentType, api=True...) would yield content-types-api:contenttype-list rather than the newly created endpoint for contenttypes because ContentType is not a core model in nautobot (extras-api:contenttype-list)

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

Copy link
Member

@bryanculver bryanculver left a comment

Choose a reason for hiding this comment

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

LGTM. Curious to know why we can't use the Django default for ContentType. Doesn't appear we do anything fancy with our ContentType view. Is their default not read-only?

@glennmatthews
Copy link
Contributor

LGTM. Curious to know why we can't use the Django default for ContentType. Doesn't appear we do anything fancy with our ContentType view. Is their default not read-only?

There is no default REST API provided by the contenttypes app AFAIK. We provide one for the ContentType model through our extras app.

Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

  1. Do we need a similar fix for the Group model and the users/groups/ REST URL?
  2. Would you mind looking into how difficult it would be to fix Unable to query content_types via GraphQL #3300 in this same PR?

nautobot/utilities/utils.py Outdated Show resolved Hide resolved
@bryanculver
Copy link
Member

LGTM. Curious to know why we can't use the Django default for ContentType. Doesn't appear we do anything fancy with our ContentType view. Is their default not read-only?

There is no default REST API provided by the contenttypes app AFAIK. We provide one for the ContentType model through our extras app.

DOH. Didn't think beyond the get_route_for_model being our invention.

Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
@timizuoebideri1
Copy link
Contributor Author

  1. I suppose we'll need a similar fix for 'Group'.
  2. I have little to no idea how GraphQL works, so it may take longer because I need to learn how it works first.

@glennmatthews
Copy link
Contributor

  • Would you mind looking into how difficult it would be to fix Unable to query content_types via GraphQL #3300 in this same PR?
    2. I have little to no idea how GraphQL works, so it may take longer because I need to learn how it works first.

No worries, that can be deferred since it's a separate issue. Thanks!

@@ -0,0 +1 @@
Fix the issue with the ContentType reverse return, i.e get_route_for_model(ContentType, api=True...).
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps:

Suggested change
Fix the issue with the ContentType reverse return, i.e get_route_for_model(ContentType, api=True...).
Fixed missing `get_route_for_model()` logic for the `ContentType` and `Group` models.

Comment on lines +236 to +239
self.assertEqual(get_route_for_model(ContentType, "list", api=True), "extras-api:contenttype-list")
self.assertEqual(get_route_for_model(ContentType, "detail", api=True), "extras-api:contenttype-detail")
self.assertEqual(get_route_for_model(Group, "list", api=True), "users-api:group-list")
self.assertEqual(get_route_for_model(Group, "detail", api=True), "users-api:group-detail")
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I suppose for the api=False case we don't actually have viable routes for either of these models, do we? I guess it's inline with how get_route_for_model currently works that it just tries to guess the route name, but doesn't actually verify that it's valid though. Ah well.

@timizuoebideri1 timizuoebideri1 merged commit d6717c3 into develop Feb 22, 2023
@timizuoebideri1 timizuoebideri1 deleted the fix_api_list_contentypes branch February 22, 2023 13:48
@timizuoebideri1 timizuoebideri1 mentioned this pull request Feb 23, 2023
7 tasks
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.

Missing REST API listing for content types
4 participants