[django] Generalize uuid/key/receive_url logic (openwisp/openwisp-utils#40) #109

waleko · 2020-01-16T18:20:29Z

Removed the logics which were duplicated
in the others modules and redefined it here to enable reusability and ease debugging.

Related to openwisp/openwisp-utils#40

waleko · 2020-01-16T18:21:00Z

There is a key field in Topology model. I changed it to

key = KeyField(unique=False,
               db_index=False,
               help_text=_('key needed to update topology from nodes '),
               verbose_name=_('key'),
               blank=True)

But now when I run makemigrations I get this error:
TypeError: Couldn't reconstruct field key on django_netjsongraph.Topology: __init__() missing 2 required positional arguments: 'unique' and 'db_index'.

Presumably, changing CharField to KeyField for old data somehow doesn't include unique and db_index arguments in the constructor of KeyField. You can see the whole commit in my commit here. I also tried to fake the migrations file and then running migrate, but I get almost the same error: TypeError: __init__() missing 2 required positional arguments: 'unique' and 'db_index'.

The easy fix is of course to ditch KeyField, and just have CharField, but I would only do it as a last resort. Can you please help?

nemesifier

In the repo there's a get_random_key which you should be able to remove

waleko · 2020-01-17T16:42:23Z

Travis fails for python 2.7 because django-model-utils>=4.0.0,<4.1.0 requires Django>=2.0.1

nemesifier

Ok no worries for the failing build on python 2.7 because i'm waiting for PR #99 to be merged which will fix it (@pawelplsi that's one of the reasons I'm insisting with finishing that PR).

I have only one minor request see below.

nemesifier · 2020-01-17T18:02:12Z

django_netjsongraph/migrations/0011_move_to_key_field.py

+            name='key',
+            field=openwisp_utils.base.KeyField(blank=True, default=openwisp_utils.utils.get_random_key, help_text='key needed to update topology from nodes ', max_length=64, validators=[django.core.validators.RegexValidator(re.compile('^[^\\s/\\.]+$'), code='invalid', message='This value must not contain spaces, dots or slashes.')], verbose_name='key'),
+        ),
+    ]


this migration file should not be needed now that you updated the old migration file, you can remove it

@nemesisdesign I updated old migration file, so that it's old generator (which was in django_netjsongraph.utils) wouldn't be needed, as I deleted it from there. But in the new migration I change the entire field type. So it seems to me, that we still need new migration file. I can of course update the old file once again to make it also KeyField, but I think it's better the way it is.

change the entire field in the old migration file

nemesifier

Great progress! Please rebase your changes on the current master and see my comments below.

nemesifier · 2020-01-20T16:35:18Z

.travis.yml

@@ -28,6 +28,9 @@ before_install:
 install:
  - pip install $DJANGO
  - if [[ $TRAVIS_PYTHON_VERSION == 2.7 ]]; then pip install -U "djangorestframework<3.10"; fi
+  # Temporary: remove when openwisp-utils#71 is merged
+  - pip install "https://github.com/waleko/openwisp-utils/tarball/issues/40" --upgrade


I released openwisp-utils 0.4.1 which contains your patch openwisp/openwisp-utils#71.
You can now remove this temporary addition.

nemesifier · 2020-01-20T16:36:05Z

django_netjsongraph/migrations/0011_move_to_key_field.py

+            name='key',
+            field=openwisp_utils.base.KeyField(blank=True, default=openwisp_utils.utils.get_random_key, help_text='key needed to update topology from nodes ', max_length=64, validators=[django.core.validators.RegexValidator(re.compile('^[^\\s/\\.]+$'), code='invalid', message='This value must not contain spaces, dots or slashes.')], verbose_name='key'),
+        ),
+    ]


change the entire field in the old migration file

nemesifier · 2020-01-20T16:36:19Z

requirements.txt

@@ -2,6 +2,6 @@ django>=1.11,<2.3
 djangorestframework>=3.3,<3.12
 django-model-utils
 netdiff>=0.6.0,<0.7
-openwisp-utils>=0.3,<0.4
+openwisp-utils>=0.4,<0.5


nemesifier · 2020-01-20T16:36:22Z

requirements-test.txt

@@ -1,4 +1,4 @@
 coveralls
 responses
 freezegun
-openwisp-utils[qa]>=0.3.0
+openwisp-utils[qa]>=0.4.0


nemesifier

Great work, thank you! 👍

waleko mentioned this pull request Jan 16, 2020

[admin] Removed logic duplicates #40 openwisp/openwisp-utils#71

Merged

waleko requested a review from nemesifier January 16, 2020 19:52

nemesifier reviewed Jan 17, 2020

View reviewed changes

waleko requested a review from nemesifier January 17, 2020 16:45

nemesifier suggested changes Jan 17, 2020

View reviewed changes

nemesifier suggested changes Jan 20, 2020

View reviewed changes

nemesifier mentioned this pull request Jan 20, 2020

[django] Generalize uuid/key logic openwisp/openwisp-utils#40 openwisp/django-netjsonconfig#143

Merged

[django] Generalize uuid/key/receive_url logic

110cb71

waleko requested a review from nemesifier January 22, 2020 15:58

nemesifier approved these changes Jan 22, 2020

View reviewed changes

nemesifier merged commit d47b251 into openwisp:master Jan 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[django] Generalize uuid/key/receive_url logic (openwisp/openwisp-utils#40) #109

[django] Generalize uuid/key/receive_url logic (openwisp/openwisp-utils#40) #109

waleko commented Jan 16, 2020

waleko commented Jan 16, 2020 •

edited

Loading

nemesifier left a comment

waleko commented Jan 17, 2020

nemesifier left a comment

nemesifier Jan 17, 2020

waleko Jan 17, 2020 •

edited

Loading

nemesifier Jan 20, 2020

nemesifier left a comment

nemesifier Jan 20, 2020

nemesifier Jan 20, 2020

nemesifier Jan 20, 2020

nemesifier Jan 20, 2020

nemesifier left a comment

[django] Generalize uuid/key/receive_url logic (openwisp/openwisp-utils#40) #109

[django] Generalize uuid/key/receive_url logic (openwisp/openwisp-utils#40) #109

Conversation

waleko commented Jan 16, 2020

waleko commented Jan 16, 2020 • edited Loading

nemesifier left a comment

Choose a reason for hiding this comment

waleko commented Jan 17, 2020

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier Jan 17, 2020

Choose a reason for hiding this comment

waleko Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

nemesifier Jan 20, 2020

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier Jan 20, 2020

Choose a reason for hiding this comment

nemesifier Jan 20, 2020

Choose a reason for hiding this comment

nemesifier Jan 20, 2020

Choose a reason for hiding this comment

nemesifier Jan 20, 2020

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

waleko commented Jan 16, 2020 •

edited

Loading

waleko Jan 17, 2020 •

edited

Loading