Fixes for get_name() and locale-driven sorting

[zmousm]

• Deduplicate and fix inconsistent get_name() methods
• Remove superfluous assignments to obj.__str__
• Remove now redundant str() (unicode()) wraps
• Replace method get_str_func() with nested function compat_strxfrm()
• Use setlocale() context manager and avoid hardcoding Greek sorting in contacts mgmt cmd
• Do not hardcode Greek sorting also in adminlist view
• Also use compat_strxfrm() in views

[vladimir-mencl-eresearch]

Hi @zmousm,

Thanks for this - looks like a great cleanup (also removing hacks I through in while working on Python3 compatibility).

Looks much cleaner now - thanks for that!

Cheers, Vlad

[vladimir-mencl-eresearch]

Hi Zenon,

I have rolled these changes out into our Production yesterday, and our PROD data triggered a an issue with the code.

We had an institution with two English names (which was an inconsistency, one of the names was in a different language). But it worked before this change, and now the /connect page started falling over with:

Exception Value: get() returned more than one Name_i18n -- it returned 2! 

The interesting part of the stack-trace is:

File "./edumanage/views.py" in connect
  1719.         dets.sort(key=lambda x: compat_strxfrm(
File "./edumanage/views.py" in <lambda>
  1720.             x.institution.get_name(lang=request.LANGUAGE_CODE)))
File "./edumanage/models.py" in get_name
  158.                 return manager.get(lang=lang).name

I agree we should not have two names in the same language in the first place, but there is no database constraint enforcing that.

What would you think would be a good way around this? Add a constraint? Add some sanity checks in get_name?

Thanks a lot in advance for getting back to me.

Cheers, Vlad

[zmousm]

Hi @vladimir-mencl-eresearch

We had an institution with two English names (which was an inconsistency, one of the names was in a different language). But it worked before this change, and now the /connect page started falling over with:

Exception Value: get() returned more than one Name_i18n -- it returned 2! 

The interesting part of the stack-trace is:

File "./edumanage/views.py" in connect
  1719.         dets.sort(key=lambda x: compat_strxfrm(
File "./edumanage/views.py" in <lambda>
  1720.             x.institution.get_name(lang=request.LANGUAGE_CODE)))
File "./edumanage/models.py" in get_name
  158.                 return manager.get(lang=lang).name

I agree we should not have two names in the same language in the first place, but there is no database constraint enforcing that.

What would you think would be a good way around this? Add a constraint? Add some sanity checks in get_name?

You are right. I am not sure how no constraints were added back in the day, but perhaps it would not work as expected with generic relations. Maybe one could add to Name_i18n.Meta:

unique_together = ('lang', 'content_type', 'object_id')

but I can't say for sure how this would work. It would also cause a violation if such duplicates already exist, such as in your case.

I also thought such issues are avoided through form validation, but I now see that only ensures there is always at least one name in English.

In any case, the least intrusive workaround, rather than a solution, would be an addition like this:

diff --git a/edumanage/models.py b/edumanage/models.py
index f692b41..1fc2376 100644
--- a/edumanage/models.py
+++ b/edumanage/models.py
@@ -158,6 +158,8 @@ class Name_i18n(models.Model):
                 return manager.get(lang=lang).name
             except Name_i18n.DoesNotExist:
                 return all_names()
+            except Name_i18n.MultipleObjectsReturned:
+                return manager.first(lang=lang).name
         return get_name

What do you think?

[zmousm]

Actually sorry, the qs in the patch should be:

manager.filter(lang=lang).first().name

But we could also join all names of a particular language, like so:

diff --git a/edumanage/models.py b/edumanage/models.py
index f692b41..b520f50 100644
--- a/edumanage/models.py
+++ b/edumanage/models.py
@@ -150,14 +150,19 @@ class Name_i18n(models.Model):
     def get_name_factory(reverse_accessor):
         def get_name(self, lang=None):
             manager = getattr(self, reverse_accessor)
-            def all_names():
-                return ', '.join([n.name for n in manager.all()])
+            def all_names(**kwargs):
+                names_qs = manager.filter(**kwargs) \
+                    if kwargs else \
+                    manager.all()
+                return ', '.join([n.name for n in names_qs])
             if not lang:
                 return all_names()
             try:
                 return manager.get(lang=lang).name
             except Name_i18n.DoesNotExist:
                 return all_names()
+            except Name_i18n.MultipleObjectsReturned:
+                return all_names(lang=lang)
         return get_name

[vladimir-mencl-eresearch]

Hi @zmousm , thanks for the quick reply.

Yes, this workaround (returning all names for the selected language) looks very reasonable to me.

And I'd think it would lead to "predictable" outcomes.

+1 for this.

[zmousm]

Workaround pushed to master

[vladimir-mencl-eresearch]

Thanks a lot for the quick fix!