Updating methods for custom field choices #115

nautics889 · 2023-04-16T02:03:13Z

Since Nautobot API has two different endpoints for custom fields and custom field choices (/api/extras/custom-fields/ and /api/extras/custom-field-choices/ respectively), it might seem some ambiguous that we have a method custom_choices() which actually sends a request to /custom-fields/.

Rename:

App.custom_choices() → App.custom_fields() (sends a request to /extras/custom-fields/)
AppCustomChoicesTestCase → AppCustomFieldsTestCase (tests for sending a request to /extras/custom-fields/)

Add:

App.custom_field_choices() (sends a request to /extras/custom-field-choices/)
AppCustomFieldChoicesTestCase (tests for sending a request to /extras/custom-field-choices/)

I also add checking of called URL in the tests. I presume it's a point to be discussed, because without this checking there's a possibility that tests don't display an error when the method being tested sends request to wrong URL. Considering we have mocks all over test module. So as for me, it'd be good to add such assertions in other places of test modules.

Add method `custom_field_choices()` in `App` class. Rename method `custom_choices()` to `custom_fields()`. Update docstings for both methods according to returning data.

Add test case `AppCustomFieldChoicesTestCase` for `custom_field_choices()` method. Rename test case for `custom_fields()` method. Add using fixtures for mentioned test cases.

Add missing JSON files with fixtures for tests for getting custom fields.

nautics889 · 2023-04-16T15:50:42Z

IDK why tests are failing on URL assertion, when i run it locally, those tests are passed well.
Either via pytest directy:

$ pytest
...
tests/test_app.py::AppCustomFieldsTestCase::test_custom_fields PASSED                                                                                                                                     [  2%]
tests/test_app.py::AppCustomFieldChoicesTestCase::test_custom_field_choices PASSED                                                                                                                        [  2%]
...

or via invoke

$ export NAUTOBOT_URL="http://localhost:8000"
$ invoke tests
...
tests/test_app.py::AppCustomFieldsTestCase::test_custom_fields PASSED                                                                                                                                     [  2%]
tests/test_app.py::AppCustomFieldChoicesTestCase::test_custom_field_choices PASSED                                                                                                                        [  2%]
...

Working on it...

Fix test cases `AppCustomFieldsTestCase` and `AppCustomFieldChoicesTestCase` for python 3.7 stable. Update logic for checking passed arguments of mock's `call_args`.

nautics889 · 2023-04-16T19:59:36Z

Done, tests are fixed.
The problem was that i'd added checking for passed URL like that:

expect_url in mock_obj.call_args.args

However .args and .kwargs attributes are only available from 3.8 version.

That's why tests were failing only on 3.7.

jvanderaa · 2023-05-09T18:43:39Z

@pszulczewski with what you have been seeing, this look good?

pszulczewski

Good work, just some cosmetic changes around examples, mocking and variable names.

pszulczewski · 2023-05-10T08:33:13Z

pynautobot/core/app.py

+        >>> 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']


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.

pszulczewski · 2023-05-10T08:35:24Z

pynautobot/core/app.py

+        >>> print(custom_fields_list[0]['label'])
+        Test custom field for rack
+        >>> print(custom_fields_list[0]['content_types'])
+        ['dcim.rack']
        """
        custom_fields = Request(
            base="{}/{}/custom-fields/".format(


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

pszulczewski · 2023-05-10T08:45:30Z

pynautobot/core/app.py

+
+
+        >>> 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


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.

pszulczewski · 2023-05-10T08:48:25Z

pynautobot/core/app.py

+        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


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.

pszulczewski · 2023-05-10T08:50:53Z

tests/test_app.py

        api = pynautobot.api(host, **def_kwargs)
-        choices = api.extras.custom_choices()
+        choices = api.extras.custom_fields()


Suggested change

choices = api.extras.custom_fields()

cfs = api.extras.custom_fields()

pszulczewski · 2023-05-10T08:51:29Z

tests/test_app.py

-        "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")],


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.

pszulczewski · 2023-05-10T08:51:57Z

tests/test_app.py

+        self.assertIsInstance(choices, list)
        self.assertEqual(len(choices), 2)


Suggested change

self.assertIsInstance(choices, list)

self.assertEqual(len(choices), 2)

self.assertIsInstance(cfs, list)

self.assertEqual(len(cfs), 2)

pszulczewski · 2023-05-10T08:52:23Z

tests/test_app.py

        self.assertEqual(len(choices), 2)
-        self.assertEqual(sorted(choices.keys()), ["Testfield1", "Testfield2"])
+        for field in choices:


Suggested change

for field in choices:

for field in cfs:

pszulczewski · 2023-05-10T08:52:56Z

tests/test_app.py

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


Suggested change

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

return_value=Response(fixture="extras/custom_field_choices.json"),

* (docs) update docstrings for `custom_fields()` and `custom_field_choices()` methods * (refactor): use f-strings instead of `.format()` in `custom_fields()` and `custom_field_choices()` * (tests): update naming in tests * (tests): use `return_value` instead of `side_effect` for mocks

nautics889 · 2023-07-15T13:00:28Z

@pszulczewski
Apologies for the delayed response. Thank you for the review!

I have uploaded a commit with all the changes you've suggested.

update examples in docstrings both in custom_fields() and custom_field_choices() ✓
use f-strings for the methods instead of .format() ✓
update naming in related test-cases ✓
use return_value instead of side_effect for mocks in related test-cases ✓

All CI checks are passed.
I'd appreciate if you could take a look when you have time.

p. s. about docstrings: i had inteded to make them as you described at first, but i was afraid that would look too huge for a docstring =)

joewesch · 2023-07-17T13:48:49Z

pynautobot/core/app.py

@@ -82,22 +82,73 @@ def choices(self):

        return self._choices

-    def custom_choices(self):


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?

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.

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

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.

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.

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

* (fix): restore original method `custom_choices()` for application class to provide a compatibility with existing client code * (enhance): add logging a deprecation warning

pszulczewski

Brilliant! Thanks @nautics889

pszulczewski · 2023-07-24T12:27:55Z

pynautobot/core/app.py

@@ -103,6 +118,78 @@ def custom_choices(self):
        ).get()
        return custom_fields


Suggested change

return custom_fields

return self.custom_fields()

You need just that after logger.

* (refactor): call `custom_fields()` in `custom_choices()` since they represent essentially identical requests to Nautobot

nautics889 · 2023-07-25T18:53:57Z

@pszulczewski thank you for the feedback!

I've uploaded the last minor changes you've mentioned

You need just that after logger.

p. s. there was some problem with CI runners (or rather with dependencies being installed for tests as seems to me according to logs). Failed job: https://github.com/nautics889/pynautobot/actions/runs/5660539569/job/15336640442

joewesch · 2023-07-25T20:46:09Z

p. s. there was some problem with CI runners (or rather with dependencies being installed for tests as seems to me according to logs). Failed job: https://github.com/nautics889/pynautobot/actions/runs/5660539569/job/15336640442

Those are the actions in your fork. They are passing in this repo: https://github.com/nautobot/pynautobot/actions/runs/5660540032/job/15336460260

Your branch is 20 commits behind develop, so may be worth pulling in the latest changes....or just disable and ignore your own CI.

nautics889 · 2023-07-25T23:55:23Z

Those are the actions in your fork

Indeed, didn't notice, thank you

nautics889 added 3 commits April 16, 2023 04:21

fix: Update methods for custom fields (nautobot#114)

afedd94

Add method `custom_field_choices()` in `App` class. Rename method `custom_choices()` to `custom_fields()`. Update docstings for both methods according to returning data.

fix: Tests for methods for custom fields (nautobot#114)

8085e23

Add test case `AppCustomFieldChoicesTestCase` for `custom_field_choices()` method. Rename test case for `custom_fields()` method. Add using fixtures for mentioned test cases.

fix: Fixtures for tests (custom fields) (nautobot#114)

69ef638

Add missing JSON files with fixtures for tests for getting custom fields.

nautics889 force-pushed the nautics-114-custom-field-choices branch 2 times, most recently from a89d82f to 49c500f Compare April 16, 2023 18:50

fix: Failing test for getting custom fields (nautobot#114)

b09543a

Fix test cases `AppCustomFieldsTestCase` and `AppCustomFieldChoicesTestCase` for python 3.7 stable. Update logic for checking passed arguments of mock's `call_args`.

nautics889 force-pushed the nautics-114-custom-field-choices branch from 49c500f to b09543a Compare April 16, 2023 19:48

nautics889 changed the title ~~Nautics 114 custom field choices~~ Updating methods for custom field choices Apr 16, 2023

pszulczewski requested changes May 10, 2023

View reviewed changes

pszulczewski linked an issue May 10, 2023 that may be closed by this pull request

custom-fields and custom-field-choices are supposed to be different endpoint #114

Closed

joewesch reviewed Jul 17, 2023

View reviewed changes

nautics889 requested a review from pszulczewski July 21, 2023 21:06

fix: backward compatibility

e5efc1a

* (fix): restore original method `custom_choices()` for application class to provide a compatibility with existing client code * (enhance): add logging a deprecation warning

pszulczewski approved these changes Jul 24, 2023

View reviewed changes

pszulczewski requested changes Jul 24, 2023

View reviewed changes

refactor: update custom_choices() (nautobot#114)

f401a23

* (refactor): call `custom_fields()` in `custom_choices()` since they represent essentially identical requests to Nautobot

nautics889 requested a review from pszulczewski July 25, 2023 18:48

pszulczewski approved these changes Jul 25, 2023

View reviewed changes

joewesch approved these changes Jul 25, 2023

View reviewed changes

nautics889 requested a review from jvanderaa July 25, 2023 23:55

joewesch merged commit c76fb00 into nautobot:develop Jul 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating methods for custom field choices #115

Updating methods for custom field choices #115

nautics889 commented Apr 16, 2023 •

edited

Loading

nautics889 commented Apr 16, 2023 •

edited

Loading

nautics889 commented Apr 16, 2023

jvanderaa commented May 9, 2023

pszulczewski left a comment

pszulczewski May 10, 2023

pszulczewski May 10, 2023

pszulczewski May 10, 2023

pszulczewski May 10, 2023 •

edited

Loading

pszulczewski May 10, 2023

pszulczewski May 10, 2023

pszulczewski May 10, 2023

pszulczewski May 10, 2023

pszulczewski May 10, 2023

nautics889 commented Jul 15, 2023

joewesch Jul 17, 2023

nautics889 Jul 17, 2023 •

edited

Loading

jvanderaa Jul 17, 2023

jvanderaa Jul 17, 2023

pszulczewski Jul 23, 2023

nautics889 Jul 23, 2023 •

edited

Loading

pszulczewski left a comment

pszulczewski Jul 24, 2023

nautics889 Jul 25, 2023

nautics889 commented Jul 25, 2023 •

edited

Loading

joewesch commented Jul 25, 2023

nautics889 commented Jul 25, 2023 •

edited

Loading

-        >>> 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/",
+            },
+        ]

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

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

		self.assertIsInstance(choices, list)
		self.assertEqual(len(choices), 2)

		@@ -82,22 +82,73 @@ def choices(self):

		return self._choices

		def custom_choices(self):

		@@ -103,6 +118,78 @@ def custom_choices(self):
		).get()
		return custom_fields

Updating methods for custom field choices #115

Updating methods for custom field choices #115

Conversation

nautics889 commented Apr 16, 2023 • edited Loading

nautics889 commented Apr 16, 2023 • edited Loading

nautics889 commented Apr 16, 2023

jvanderaa commented May 9, 2023

pszulczewski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pszulczewski May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nautics889 commented Jul 15, 2023

Choose a reason for hiding this comment

nautics889 Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nautics889 Jul 23, 2023 • edited Loading

Choose a reason for hiding this comment

pszulczewski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nautics889 commented Jul 25, 2023 • edited Loading

joewesch commented Jul 25, 2023

nautics889 commented Jul 25, 2023 • edited Loading

nautics889 commented Apr 16, 2023 •

edited

Loading

nautics889 commented Apr 16, 2023 •

edited

Loading

pszulczewski May 10, 2023 •

edited

Loading

nautics889 Jul 17, 2023 •

edited

Loading

nautics889 Jul 23, 2023 •

edited

Loading

nautics889 commented Jul 25, 2023 •

edited

Loading

nautics889 commented Jul 25, 2023 •

edited

Loading