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.

Sign up for GitHub

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

Updating methods for custom field choices #115

Merged
merged 7 commits into from
Jul 27, 2023
Merged

Updating methods for custom field choices #115

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
afedd94
fix: Update methods for custom fields (#114)
nautics889 Apr 15, 2023
8085e23
fix: Tests for methods for custom fields (#114)
nautics889 Apr 16, 2023
69ef638
fix: Fixtures for tests (custom fields) (#114)
nautics889 Apr 16, 2023
b09543a
fix: Failing test for getting custom fields (#114)
nautics889 Apr 16, 2023
e8b586c
refactor: Update docstrings and naming (#114)
nautics889 Jul 15, 2023
e5efc1a
fix: backward compatibility
nautics889 Jul 23, 2023
f401a23
refactor: update `custom_choices()` (#114)
nautics889 Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions pynautobot/core/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,18 @@ def choices(self):

return self._choices

def custom_choices(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have .custom_choices() call self.custom_fields() so it is backwards compatible?

@jvanderaa @pszulczewski do we have any specific patterns of notifying the end user the method is deprecated?

Copy link
Contributor Author

@nautics889 nautics889 Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

BTW, this is exactly the question that i had gotten, but i thought it depends on versioning. I guess if it's ok to wait new minor feature release (x.y.z) for this update, it can be as is. Otherwise if it's better to keep as it was, i'm ready to upload a new commit, containing the original method implemetation and both of new, but renamed. Just let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have a specific pattern today @joewesch. I think we should add in something for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to have it straight for here.

If there is a breaking change, we will do a major release, following SemVer. There is an up coming major release for supporting Nautobot 2.0 if we want to get that in with this.

On top of it we should start to look at adding the policy in with 2.0 release.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to add .custom_choices() to call self.custom_fields() as @joewesch suggested and we will keep it in 1.x release train and remove in 2.0.
We can also add logging and call logger.warning() that the method will be deprecated in 2.x.

Copy link
Contributor Author

@nautics889 nautics889 Jul 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think it would be good to add .custom_choices() to call self.custom_fields() as @joewesch suggested and we will keep it in 1.x release train and remove in 2.0. We can also add logging and call logger.warning() that the method will be deprecated in 2.x.

Done

I've restored original method as it was, except adding deprecation warning log. Original tests are left removed for obvious reasons i presume (or should they be restored as well?)

def custom_fields(self):
"""Returns custom-fields response from app

:Returns: Raw response from Nautobot's custom-fields endpoint.
:Raises: :py:class:`.RequestError` if called for an invalid endpoint.
:Example:

>>> nb.extras.custom_choices()
{'Testfield1': {'Testvalue2': 2, 'Testvalue1': 1},
'Testfield2': {'Othervalue2': 4, 'Othervalue1': 3}}
>>> custom_fields_list = nb.extras.custom_fields()
>>> print(custom_fields_list[0]['label'])
Test custom field for rack
>>> print(custom_fields_list[0]['content_types'])
['dcim.rack']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
>>> custom_fields_list = nb.extras.custom_fields()
>>> print(custom_fields_list[0]['label'])
Test custom field for rack
>>> print(custom_fields_list[0]['content_types'])
['dcim.rack']
>>> nb.extras.custom_fields()
[
{
"id": "5b39ba88-e5ab-4be2-89f5-5a016473b53c",
"display": "Test custom field",
"url": "http://localhost:8000/api/extras/custom-fields/5b39ba88-e5ab-4be2-89f5-5a016473b53c/",
"content_types": ["dcim.rack"],
"type": {"value": "integer", "label": "Integer"},
"label": "Test custom field",
"name": "test_custom_field",
"slug": "test_custom_field",
"description": "",
"required": False,
"filter_logic": {"value": "loose", "label": "Loose"},
"default": None,
"weight": 100,
"validation_minimum": None,
"validation_maximum": None,
"validation_regex": "",
"created": "2023-04-15",
"last_updated": "2023-04-15T17:45:11.839431Z",
"notes_url": "http://localhost:8000/api/extras/custom-fields/5b39ba88-e5ab-4be2-89f5-5a016473b53c/notes/",
},
]

Examples should show what is really returned from the method.
Think about someone who is looking at this for the first time.

"""
custom_fields = Request(
base="{}/{}/custom-fields/".format(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to convert that to f-string. Refactor is welcomed when it simplifies the code.

Expand All @@ -103,6 +105,30 @@ def custom_choices(self):
).get()
return custom_fields
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return custom_fields
return self.custom_fields()

You need just that after logger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ✓


def custom_field_choices(self):
"""Returns custom-field-choices response from app

:Returns: Raw response from Nautobot's custom-field-choices endpoint.
:Raises: :py:class:`.RequestError` if called for an invalid endpoint.
:Example:


>>> custom_field_choices_list = nb.extras.custom_field_choices()
>>> print(custom_field_choices_list[0]['value'])
First option
>>> print(custom_field_choices_list[0]['field']['name'])
test_custom_field
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
>>> custom_field_choices_list = nb.extras.custom_field_choices()
>>> print(custom_field_choices_list[0]['value'])
First option
>>> print(custom_field_choices_list[0]['field']['name'])
test_custom_field
>>> nb.extras.custom_field_choices()
[
{
"id": "5b39ba88-e5ab-4be2-89f5-5a016473b53c",
"display": "First option",
"url": "http://localhost:8000/api/extras/custom-field-choices/5b39ba88-e5ab-4be2-89f5-5a016473b53c/",
"field": {
"display": "Test custom field 2",
"id": "5b39ba88-e5ab-4be2-89f5-5a016473b53c",
"url": "http://localhost:8000/api/extras/custom-fields/5b39ba88-e5ab-4be2-89f5-5a016473b53c/",
"name": "test_custom_field_2"
},
"value": "First option",
"weight": 100,
"created": "2023-04-15",
"last_updated": "2023-04-15T18:11:57.163237Z"
},
]

Same here, give full example of what this method is returning.

"""
custom_fields = Request(
base="{}/{}/custom-field-choices/".format(
self.api.base_url,
self.name,
),
token=self.api.token,
http_session=self.api.http_session,
).get()
return custom_fields
Copy link
Contributor

@pszulczewski pszulczewski May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
custom_fields = Request(
base="{}/{}/custom-field-choices/".format(
self.api.base_url,
self.name,
),
token=self.api.token,
http_session=self.api.http_session,
).get()
return custom_fields
cf_choices = Request(
base=f"{self.api.base_url}/{self.name}/custom-field-choices/",
token=self.api.token,
http_session=self.api.http_session,
).get()
return cf_choices

Change custom_fields to cf_choices + refactor to f-string.


def config(self):
"""Returns config response from app

Expand Down
47 changes: 47 additions & 0 deletions tests/fixtures/extras/custom_field_choices.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
[
{
"id": "5b39ba88-e5ab-4be2-89f5-5a016473b53c",
"display": "First option",
"url": "http://localhost:8000/api/extras/custom-field-choices/5b39ba88-e5ab-4be2-89f5-5a016473b53c/",
"field": {
"display": "Test custom field 2",
"id": "5b39ba88-e5ab-4be2-89f5-5a016473b53c",
"url": "http://localhost:8000/api/extras/custom-fields/5b39ba88-e5ab-4be2-89f5-5a016473b53c/",
"name": "test_custom_field_2"
},
"value": "First option",
"weight": 100,
"created": "2023-04-15",
"last_updated": "2023-04-15T18:11:57.163237Z"
},
{
"id": "5b39ba88-e5ab-4be2-89f5-5a016473b53c",
"display": "Second option",
"url": "http://localhost:8000/api/extras/custom-field-choices/5b39ba88-e5ab-4be2-89f5-5a016473b53c/",
"field": {
"display": "Test custom field 2",
"id": "5b39ba88-e5ab-4be2-89f5-5a016473b53c",
"url": "http://localhost:8000/api/extras/custom-fields/5b39ba88-e5ab-4be2-89f5-5a016473b53c/",
"name": "test_custom_field_2"
},
"value": "Second option",
"weight": 100,
"created": "2023-04-15",
"last_updated": "2023-04-15T18:11:57.169962Z"
},
{
"id": "5b39ba88-e5ab-4be2-89f5-5a016473b53c",
"display": "Third option",
"url": "http://localhost:8000/api/extras/custom-field-choices/5b39ba88-e5ab-4be2-89f5-5a016473b53c/",
"field": {
"display": "Test custom field 2",
"id": "5b39ba88-e5ab-4be2-89f5-5a016473b53c",
"url": "http://localhost:8000/api/extras/custom-fields/5b39ba88-e5ab-4be2-89f5-5a016473b53c/",
"name": "test_custom_field_2"
},
"value": "Third option",
"weight": 100,
"created": "2023-04-15",
"last_updated": "2023-04-15T18:11:57.174825Z"
}
]
60 changes: 60 additions & 0 deletions tests/fixtures/extras/custom_fields.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
[
{
"id": "5b39ba88-e5ab-4be2-89f5-5a016473b53c",
"display": "Test custom field",
"url": "http://localhost:8000/api/extras/custom-fields/5b39ba88-e5ab-4be2-89f5-5a016473b53c/",
"content_types": [
"dcim.rack"
],
"type": {
"value": "integer",
"label": "Integer"
},
"label": "Test custom field",
"name": "test_custom_field",
"slug": "test_custom_field",
"description": "",
"required": false,
"filter_logic": {
"value": "loose",
"label": "Loose"
},
"default": null,
"weight": 100,
"validation_minimum": null,
"validation_maximum": null,
"validation_regex": "",
"created": "2023-04-15",
"last_updated": "2023-04-15T17:45:11.839431Z",
"notes_url": "http://localhost:8000/api/extras/custom-fields/5b39ba88-e5ab-4be2-89f5-5a016473b53c/notes/"
},
{
"id": "5b39ba88-e5ab-4be2-89f5-5a016473b53c",
"display": "Test custom field 2",
"url": "http://localhost:8000/api/extras/custom-fields/5b39ba88-e5ab-4be2-89f5-5a016473b53c/",
"content_types": [
"dcim.rack"
],
"type": {
"value": "select",
"label": "Selection"
},
"label": "Test custom field 2",
"name": "test_custom_field_2",
"slug": "test_custom_field_2",
"description": "",
"required": false,
"filter_logic": {
"value": "loose",
"label": "Loose"
},
"default": null,
"weight": 100,
"validation_minimum": null,
"validation_maximum": null,
"validation_regex": "",
"created": "2023-04-15",
"last_updated": "2023-04-15T18:11:57.133408Z",
"notes_url": "http://localhost:8000/api/extras/custom-fields/5b39ba88-e5ab-4be2-89f5-5a016473b53c/notes/"
}
]
53 changes: 46 additions & 7 deletions tests/test_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,62 @@

import pynautobot

from .util import Response

host = "http://localhost:8000"

def_kwargs = {
"token": "abc123",
}


class AppCustomChoicesTestCase(unittest.TestCase):
class AppCustomFieldsTestCase(unittest.TestCase):
@patch(
"pynautobot.core.query.Request.get",
return_value={"Testfield1": {"TF1_1": 1, "TF1_2": 2}, "Testfield2": {"TF2_1": 3, "TF2_2": 4}},
"requests.sessions.Session.get",
side_effect=[Response(fixture="extras/custom_fields.json")],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
side_effect=[Response(fixture="extras/custom_fields.json")],
return_value=Response(fixture="extras/custom_fields.json"),

return_value is good enough, side_effect is for rising exceptions or iterables.

)
def test_custom_choices(self, *_):
def test_custom_fields(self, session_get_mock):
api = pynautobot.api(host, **def_kwargs)
choices = api.extras.custom_choices()
choices = api.extras.custom_fields()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
choices = api.extras.custom_fields()
cfs = api.extras.custom_fields()


session_get_mock.assert_called_once()
expect_url = "{}/extras/custom-fields/".format(api.base_url)
self.assertGreaterEqual(len(session_get_mock.call_args), 2)
url_passed_in_args = expect_url in session_get_mock.call_args[0]
url_passed_in_kwargs = expect_url == session_get_mock.call_args[1].get("url")
self.assertTrue(url_passed_in_args or url_passed_in_kwargs)

self.assertIsInstance(choices, list)
self.assertEqual(len(choices), 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.assertIsInstance(choices, list)
self.assertEqual(len(choices), 2)
self.assertIsInstance(cfs, list)
self.assertEqual(len(cfs), 2)

self.assertEqual(sorted(choices.keys()), ["Testfield1", "Testfield2"])
for field in choices:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for field in choices:
for field in cfs:

self.assertIsInstance(field.get("name"), str)
self.assertIsInstance(field.get("content_types"), list)
self.assertIsInstance(field.get("slug"), str)
self.assertIn("type", field)


class AppCustomFieldChoicesTestCase(unittest.TestCase):
@patch(
"requests.sessions.Session.get",
side_effect=[Response(fixture="extras/custom_field_choices.json")],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
side_effect=[Response(fixture="extras/custom_field_choices.json")],
return_value=Response(fixture="extras/custom_field_choices.json"),

)
def test_custom_field_choices(self, session_get_mock):
api = pynautobot.api(host, **def_kwargs)
choices = api.extras.custom_field_choices()

session_get_mock.assert_called_once()
expect_url = "{}/extras/custom-field-choices/".format(api.base_url)
self.assertGreaterEqual(len(session_get_mock.call_args), 2)
url_passed_in_args = expect_url in session_get_mock.call_args[0]
url_passed_in_kwargs = expect_url == session_get_mock.call_args[1].get("url")
self.assertTrue(url_passed_in_args or url_passed_in_kwargs)

self.assertIsInstance(choices, list)
self.assertEqual(len(choices), 3)
for choice in choices:
self.assertIsInstance(choice.get("field"), dict)
self.assertIsInstance(choice.get("value"), str)
self.assertIn(choice.get("value"), ("First option", "Second option", "Third option"))


class AppConfigTestCase(unittest.TestCase):
Expand All @@ -44,7 +83,7 @@ class PluginAppCustomChoicesTestCase(unittest.TestCase):
)
def test_custom_choices(self, *_):
api = pynautobot.api(host, **def_kwargs)
choices = api.plugins.test_plugin.custom_choices()
choices = api.plugins.test_plugin.custom_fields()
self.assertEqual(len(choices), 2)
self.assertEqual(sorted(choices.keys()), ["Testfield1", "Testfield2"])

Expand Down