Fixes #96: Improve string representation of sortedm2m relationships #101

rohithasrk · 2017-03-17T08:14:13Z

Fixes #96

This PR aims to improve the string representation of SortedM2M relationship when deleting one from an admin panel. I basically added a base_class parameter, a class which is inherited by the through-model of a sortedm2m relationship. This is basically so that the users of django-sortedm2m can add their own __unicode__ method and improve the string representation.

For ex., in example/testapp/models.py we've passed a base_class argument when a relationship is being established. Check the image below,

@gregmuellegger : Please review.

nemesifier

Hey @rohithasrk. great work!

I left you comments on parts that need improvement.

TODO

In order to improve the pull request so it can be merge you should:

address the inline comments
add at least an assertion in the test suite that checks the m2mprint model __str__ method
squash your commits into one
add a reference to this new feature in the README (a few lines of text are enough, don't worry about the english grammar/style, I will help you, just show that you are willing to put effort into this)

Automated tests

Regarding the tests, I suggest to add an assertion here:

https://github.com/gregmuellegger/django-sortedm2m/blob/master/sortedm2m_tests/test_field.py

This means you should probably move your new model to this file:

https://github.com/gregmuellegger/django-sortedm2m/blob/master/sortedm2m_tests/models.py

nemesifier · 2017-03-20T19:07:20Z

example/testapp/models.py

@@ -9,10 +9,13 @@ class Car(models.Model):
    def __unicode__(self):
        return self.plate



follow pep8 conventions and add an additional blank line

Shall I use tools like autopep8 to correct all pep8 errors?

In OpenWISP we use flake8 and it is enforced in the build, but not sure is enforced here. For the moment I suggest you to focus on fixing these small issues I pointed out, it will be easier for you.

Okay. Thanks :)

nemesifier · 2017-03-20T19:10:44Z

example/testapp/models.py

@@ -9,10 +9,13 @@ class Car(models.Model):
    def __unicode__(self):
        return self.plate

+class m2mprint:
+    def __unicode__(self):


__unicode__ is deprecated (in python 3 there's only unicode strings). Use __str__ with django.utils.encoding.python_2_unicode_compatible.

Subclass the django base model class.

__unicode__ method is being used almost everywhere in this package. Shall I replace all the occurrences?

@rohithasrk I'm looking now.. wait a minute and I'll come back to you regarding this

I suggest you to move this new model to:
https://github.com/gregmuellegger/django-sortedm2m/blob/master/sortedm2m_tests/models.py

It will be easier for you to import that model in the test suite.

In the test suite you will have to ensure the representation of the model is what you expect and I'm pretty confident declaring __unicode__ only would not work on python3.
I leave the solution up to you, as long as the test works, I'm fine with it.

I pointed out that solution because in the OpenWISP modules I try to use python_2_unicode_compatible as much as possible but this package was probably developed before this utility was added to django.

nemesifier · 2017-03-20T19:11:11Z

sortedm2m/fields.py

@@ -212,17 +212,23 @@ class SortedManyToManyField(_ManyToManyField):
    '''
    Providing a many to many relation that remembers the order of related
    objects.
-
+    


you are adding spaces and changing a line that doesn't need to be changed, try to revert this change (insert a blank line without space, commit and then squash/fixup).

nemesifier · 2017-03-20T19:14:53Z

sortedm2m/fields.py

+        if self.base_class:
+            return type(str(name), (models.Model, self.base_class), attrs)
+        else:
+            return type(str(name), (models.Model,), attrs)


this whole if block can be improved. You are repeating str(name), (models.Model,) and attrs twice.

You could store that tuple in an auxiliary variable and insert the base class if needed.

Could you try this? You could also try a different idea if you like.

Yes. Seems good.

nemesifier · 2017-03-20T19:21:16Z

sortedm2m/fields.py


    def create_intermediate_model(self, klass):
        # Construct and return the new class.
        from_field_name, from_field = self.get_intermediate_model_from_field(klass)
        to_field_name, to_field = self.get_intermediate_model_to_field(klass)
        sort_value_field_name, sort_value_field = self.get_intermediate_model_sort_value_field(klass)
-
        meta = self.get_intermediate_model_meta_class(


more readable would be:

meta = self.get_intermediate_model_meta_class(klass, from_field_name, to_field_name, sort_value_field_name)

Yes. I agree

Using tools like autopep8 should help doing all these. Anyways, will fix them soon.

nemesifier · 2017-03-20T19:22:31Z

sortedm2m/fields.py

        meta = self.get_intermediate_model_meta_class(
            klass, from_field_name, to_field_name, sort_value_field_name)
-
+        


you are adding spaces and changing a line that doesn't need to be changed, try to revert this change (insert a blank line without space, commit and then squash/fixup).

My bad. Will fix them

rohithasrk · 2017-03-21T08:22:49Z

Thanks for the review @nemesisdesign. I'll update the PR soon

nemesifier · 2017-03-21T09:50:40Z

Regarding the tests, I suggest to add an assertion here:

https://github.com/gregmuellegger/django-sortedm2m/blob/master/sortedm2m_tests/test_field.py

This means you should probably move your new model to this file:

https://github.com/gregmuellegger/django-sortedm2m/blob/master/sortedm2m_tests/models.py

I will copy this text in the review text.

Federico

rohithasrk · 2017-03-24T11:42:21Z

@nemesisdesign : I've updated the PR by squashing the commits and modifying the commits with the changes you've suggested. Please review. Thanks

nemesifier · 2017-03-24T16:07:51Z

@rohithasrk great! There's only one thing missing, an assertion in the test suite. Did you get one of my last replies?

I see that you have created a model in the testapp but that is not enough. We should ensure that the test model is behaving as expected.

nemesifier · 2017-03-24T16:10:34Z

@gregmuellegger I think this PR is going to be ready soon

rohithasrk · 2017-03-24T22:19:08Z

@nemesisdesign : Added a test case too. I'm kinda new to writing tests, so please review. Thank you

rohithasrk · 2017-03-25T11:42:53Z

This test seems to be giving wrong results. I'll update the revised version in a while

rohithasrk · 2017-03-25T13:59:30Z

@nemesisdesign : I think this is fine. I've written a test comparing the __unicode__ methods and not the outputs of instances. This just checks if the the __unicode__ methods of the m2mprint and shelf_books is the same.

rohithasrk · 2017-03-27T16:51:31Z

@nemesisdesign @gregmuellegger I'm curious. Can this be merged?

nemesifier

@rohithasrk I can see you don't have a lot of experience with unit testing in general. No worries, I left you some comments that give you precise instructions on what to do. Please read carefully my comment and try to understand why I left them, if you have questions, please don't hesitate.

Implement these changes and you are done with this.

I'll ping @gregmuellegger to merge this as soon as he can.

nemesifier · 2017-03-27T17:17:49Z

sortedm2m_tests/models.py

+class m2mprint:
+
+    def __unicode__(self):
+        return unicode(self.book)


Please do something like the following:

class BaseCarThrough(object): def __str__(self): return "Relationship to {0}".format(self.book.name) def __unicode__(self): return u"{0}".format(str(self))

This throws and error saying the maximum recursion depth has exceeded. Which IMO, is because str(self) is not invoking the __str__ method. It is calling the __unicode__ method. @nemesisdesign

So, it's better to write them independently IMO. What say @nemesisdesign?

yes whatever it works as long as there is an assertion in the test that checks for the result is what we expect and the assertion works with python2 and python3.

nemesifier · 2017-03-27T17:18:07Z

sortedm2m_tests/models.py

 class Shelf(models.Model):
-    books = SortedManyToManyField('Book', related_name='shelves')
+    books = SortedManyToManyField('Book', related_name='shelves', base_class=m2mprint)


change m2mprint to BaseBookThrough

nemesifier · 2017-03-27T17:23:41Z

sortedm2m_tests/test_field.py

+        shelf_book = shelf.books.through
+
+        if hasattr(shelf_book, '__unicode__'):
+            self.assertEqual(shelf_book.__unicode__, m2mprint.__unicode__)


@rohithasrk, this is not good enough, you should do something like:

self.assertEqual(str(shelf), ''Relationship to {0}".format(shelf.name)

get rid of shelf_book = shelf.books.through.

Shouldn't it be shelf.book.name? As shelf doesn't have an attribute name?

@rohithasrk Yes

@nemesisdesign : shelf.book.name throws errors too, as shelf has no attribute book. That is why I've used shelf_book = shelf.books.through.

apply the suggestions I suggested to you on IRC.

Do something like:

relationship = shelf_book.objects.first() self.assertEqual(str(relationship), "<expected representation of the relationship here>")

nemesifier · 2017-03-27T17:25:42Z

sortedm2m_tests/test_field.py

+        if hasattr(shelf_book, '__unicode__'):
+            self.assertEqual(shelf_book.__unicode__, m2mprint.__unicode__)
+        else:
+            self.assertTrue(True)


Get rid of the if/else condition please.

BTW: adding stuff like self.assertTrue(True) is really bad. Don't do it!

@nemesisdesign, I added these if/else conditions just to check if the base_class parameter has been provided. Tests fail when base_class parameter isn't provided. It shouldn't be the case right?

I have not understood your last comment very well.
You have to write a test which checks the output of the __str__ method is what you expect. No need to add if/else blocks.

@nemesisdesign : I mean, should the test fail if a sortedm2m relationship doesn't have a base_class parameter? In my opinion, No.

As in the above mentioned changes, the test is failing. And that is why, I've used if/else conditions.

why should the test fail if you defined it in the model? Am I missing something?

nemesifier · 2017-03-27T18:16:49Z

example/testapp/models.py

@@ -10,9 +10,15 @@ def __unicode__(self):
        return self.plate


+class m2mprint:


apply the same suggestions I gave you in https://github.com/gregmuellegger/django-sortedm2m/pull/101/files#r108227092

nemesifier · 2017-03-28T09:19:05Z

sortedm2m_tests/test_field.py

+        shelf_book = shelf.books.through
+
+        if hasattr(shelf_book, '__unicode__'):
+            self.assertEqual(shelf_book.__unicode__, m2mprint.__unicode__)


apply the suggestions I suggested to you on IRC.

Do something like:

relationship = shelf_book.objects.first() self.assertEqual(str(relationship), "<expected representation of the relationship here>")

nemesifier · 2017-03-28T09:20:09Z

sortedm2m_tests/test_field.py

+        if hasattr(shelf_book, '__unicode__'):
+            self.assertEqual(shelf_book.__unicode__, m2mprint.__unicode__)
+        else:
+            self.assertTrue(True)


why should the test fail if you defined it in the model? Am I missing something?

nemesifier · 2017-03-28T09:21:14Z

sortedm2m_tests/models.py

+class m2mprint:
+
+    def __unicode__(self):
+        return unicode(self.book)


yes whatever it works as long as there is an assertion in the test that checks for the result is what we expect and the assertion works with python2 and python3.

nemesifier

I believe this is the last round of changes!

nemesifier · 2017-03-28T10:41:55Z

sortedm2m_tests/test_field.py

+    def test_base_class_str(self):
+        shelf = self.model.objects.create()
+        shelf.books.add(self.books[0])
+


remove blank line

nemesifier · 2017-03-28T10:42:00Z

sortedm2m_tests/test_field.py

+
+        through_model = shelf.books.through
+        instance = through_model.objects.first()
+


remove blank line

nemesifier · 2017-03-28T10:42:13Z

example/testapp/models.py

@@ -10,9 +10,15 @@ def __unicode__(self):
        return self.plate


+class BaseCarThrough(object):
+


remove blank line

nemesifier · 2017-03-28T10:46:05Z

sortedm2m_tests/test_field.py

+        shelf.books.add(self.books[0])
+
+        through_model = shelf.books.through
+        instance = through_model.objects.first()


unfortunately I forgot first() is not supported on old django versions.

Try rewriting this line as:

instance = through_model.objects.all()[0]

nemesifier

@gregmuellegger I think it's ready to be merged.

…nships

rohithasrk · 2017-06-13T11:00:52Z

@gregmuellegger Resolved the conflicts and pushed. I think this can be merged.

gregmuellegger · 2017-08-01T19:22:31Z

I'm deeply sorry for this contribution to being so long on the waiting! I was deeply dived into personal projects (like getting maried 💒) which was that I spent nearly no time in front of a computer.

Thank you for your patience and for the contribution and the amazing guidance that you @nemesisdesign gave here!

I will release this feature in a new release in the next days.

gregmuellegger · 2017-08-02T20:42:38Z

It is released! You can find your contribution as part of: https://pypi.python.org/pypi/django-sortedm2m

rohithasrk · 2017-08-02T20:57:18Z

Thank you @gregmuellegger

nemesifier · 2017-08-07T09:35:27Z

Thanks for merging and congratulations for your marriage!

This was referenced Mar 17, 2017

How to improve str() representation of relationships? Eg: when deleting from admin #96

Closed

[Config] get rid of monkey patching in sortedm2m openwisp/django-netjsonconfig#29

Closed

nemesifier suggested changes Mar 20, 2017

View reviewed changes

rohithasrk force-pushed the str-rep branch 2 times, most recently from aedd21a to fc511ab Compare March 24, 2017 11:36

rohithasrk force-pushed the str-rep branch from fc511ab to 5196edc Compare March 24, 2017 22:18

rohithasrk force-pushed the str-rep branch from 5196edc to 15c82a9 Compare March 25, 2017 13:56

nemesifier suggested changes Mar 27, 2017

View reviewed changes

nemesifier reviewed Mar 27, 2017

View reviewed changes

nemesifier suggested changes Mar 28, 2017

View reviewed changes

rohithasrk force-pushed the str-rep branch from 15c82a9 to 78758c7 Compare March 28, 2017 10:33

nemesifier suggested changes Mar 28, 2017

View reviewed changes

rohithasrk force-pushed the str-rep branch 2 times, most recently from 81adf42 to 3279680 Compare March 28, 2017 11:27

nemesifier approved these changes Mar 28, 2017

View reviewed changes

Fixes jazzband#96: Improve string representation of sortedm2m relatio…

b48481e

…nships

rohithasrk force-pushed the str-rep branch from 1e529a1 to b48481e Compare June 13, 2017 10:47

Merge branch 'master' into str-rep

7cd6b1e

gregmuellegger merged commit 2df098c into jazzband:master Aug 1, 2017

rohithasrk deleted the str-rep branch August 1, 2017 19:23

		@@ -9,10 +9,13 @@ class Car(models.Model):
		def __unicode__(self):
		return self.plate

		meta = self.get_intermediate_model_meta_class(
		klass, from_field_name, to_field_name, sort_value_field_name)

		@@ -10,9 +10,15 @@ def __unicode__(self):
		return self.plate


		class m2mprint:


		through_model = shelf.books.through
		instance = through_model.objects.first()

		@@ -10,9 +10,15 @@ def __unicode__(self):
		return self.plate


		class BaseCarThrough(object):

Fixes #96: Improve string representation of sortedm2m relationships #101

Fixes #96: Improve string representation of sortedm2m relationships #101

Conversation

rohithasrk commented Mar 17, 2017

nemesifier left a comment • edited Loading

Choose a reason for hiding this comment

TODO

Automated tests

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

nemesifier Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier Mar 21, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohithasrk commented Mar 21, 2017

nemesifier commented Mar 21, 2017

rohithasrk commented Mar 24, 2017

nemesifier commented Mar 24, 2017 • edited Loading

nemesifier commented Mar 24, 2017

rohithasrk commented Mar 24, 2017

rohithasrk commented Mar 25, 2017

rohithasrk commented Mar 25, 2017

rohithasrk commented Mar 27, 2017

nemesifier left a comment • edited Loading

Choose a reason for hiding this comment

nemesifier Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

rohithasrk Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier Mar 27, 2017 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier 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

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

rohithasrk commented Jun 13, 2017 • edited Loading

gregmuellegger commented Aug 1, 2017

gregmuellegger commented Aug 2, 2017

rohithasrk commented Aug 2, 2017

nemesifier commented Aug 7, 2017

nemesifier left a comment •

edited

Loading

nemesifier Mar 20, 2017 •

edited

Loading

nemesifier Mar 21, 2017 •

edited

Loading

nemesifier commented Mar 24, 2017 •

edited

Loading

nemesifier left a comment •

edited

Loading

nemesifier Mar 27, 2017 •

edited

Loading

rohithasrk Mar 27, 2017 •

edited

Loading

nemesifier Mar 27, 2017 •

edited

Loading

rohithasrk commented Jun 13, 2017 •

edited

Loading