Fix performance issues for shortcut menu

8 files changed, 158 insertions, 49 deletions

diff --git a/archaeological_context_records/models.py b/archaeological_context_records/models.py
index 518610aa5..cd0dd4268 100644
--- a/archaeological_context_records/models.py
+++ b/archaeological_context_records/models.py
@@ -269,14 +269,16 @@ class ContextRecord(BaseHistorizedItem, ImageModel, OwnPerms,
             Q(history_creator=user)
 
     @classmethod
-    def get_owns(cls, user, menu_filtr=None, limit=None):
+    def get_owns(cls, user, menu_filtr=None, limit=None,
+                 values=None, get_short_menu_class=None):
         replace_query = None
         if menu_filtr and 'operation' in menu_filtr:
             replace_query = Q(operation=menu_filtr['operation'])
         owns = super(ContextRecord, cls).get_owns(
             user, replace_query=replace_query,
-            limit=limit)
-        return sorted(owns, key=lambda x: x.cached_label)
+            limit=limit, values=values,
+            get_short_menu_class=get_short_menu_class)
+        return cls._return_get_owns(owns, values, get_short_menu_class)
 
     def full_label(self):
         return unicode(self)
diff --git a/archaeological_files/models.py b/archaeological_files/models.py
index 638b19d18..15e65579c 100644
--- a/archaeological_files/models.py
+++ b/archaeological_files/models.py
@@ -340,12 +340,16 @@ class File(ClosedItem, BaseHistorizedItem, OwnPerms, ValueGetter,
         cache.set(cache_key, has_adminact, settings.CACHE_TIMEOUT)
         return has_adminact
 
-    def get_short_menu_class(self):
-        cache_key, val = get_cache(self.__class__, [self.pk,
-                                                    'short_class_name'])
+    @classmethod
+    def get_short_menu_class(cls, pk):
+        cache_key, val = get_cache(cls, [pk, 'short_class_name'])
         if val:
             return val
-        return self.update_short_menu_class(cache_key)
+        q = cls.objects.filter(pk=pk)
+        if not q.count():
+            return ''
+        item = q.all()[0]
+        return item.update_short_menu_class(cache_key)
 
     def update_short_menu_class(self, cache_key=None):
         if not cache_key:
@@ -366,9 +370,12 @@ class File(ClosedItem, BaseHistorizedItem, OwnPerms, ValueGetter,
         return cls
 
     @classmethod
-    def get_owns(cls, user, menu_filtr=None, limit=None):
-        owns = super(File, cls).get_owns(user, limit=limit)
-        return sorted(owns, key=lambda x: x.cached_label)
+    def get_owns(cls, user, menu_filtr=None, limit=None, values=None,
+                 get_short_menu_class=False):
+        owns = super(File, cls).get_owns(
+            user, limit=limit, values=values,
+            get_short_menu_class=get_short_menu_class)
+        return cls._return_get_owns(owns, values, get_short_menu_class)
 
     def get_values(self, prefix=''):
         values = super(File, self).get_values(prefix=prefix)
diff --git a/archaeological_finds/models_finds.py b/archaeological_finds/models_finds.py
index e8b6135a8..588edb5cf 100644
--- a/archaeological_finds/models_finds.py
+++ b/archaeological_finds/models_finds.py
@@ -687,18 +687,17 @@ class Find(BaseHistorizedItem, ImageModel, OwnPerms, ShortMenuItem):
             Q(history_creator=user)
 
     @classmethod
-    def get_owns(cls, user, menu_filtr=None, limit=None):
+    def get_owns(cls, user, menu_filtr=None, limit=None,
+                 values=None, get_short_menu_class=None):
         replace_query = None
         if menu_filtr and 'contextrecord' in menu_filtr:
             replace_query = Q(
                 base_finds__context_record=menu_filtr['contextrecord']
             )
         owns = super(Find, cls).get_owns(
-            user, replace_query=replace_query,
-            limit=limit)
-        return sorted(
-            owns, key=lambda x: x.cached_label
-            if hasattr(x, 'cached_label') else unicode(x))
+            user, replace_query=replace_query, limit=limit, values=values,
+            get_short_menu_class=get_short_menu_class)
+        return cls._return_get_owns(owns, values, get_short_menu_class)
 
     def _generate_cached_label(self):
         return unicode(self)
diff --git a/archaeological_finds/models_treatments.py b/archaeological_finds/models_treatments.py
index d8d51e28c..e94d1c272 100644
--- a/archaeological_finds/models_treatments.py
+++ b/archaeological_finds/models_treatments.py
@@ -171,7 +171,8 @@ class Treatment(BaseHistorizedItem, ImageModel, OwnPerms, ShortMenuItem):
             & Q(end_date__isnull=True)
 
     @classmethod
-    def get_owns(cls, user, menu_filtr=None, limit=None):
+    def get_owns(cls, user, menu_filtr=None, limit=None, values=None,
+                 get_short_menu_class=None):
         replace_query = None
         if menu_filtr:
             if 'treatmentfile' in menu_filtr:
@@ -184,10 +185,9 @@ class Treatment(BaseHistorizedItem, ImageModel, OwnPerms, ShortMenuItem):
                 else:
                     replace_query = q
         owns = super(Treatment, cls).get_owns(
-            user, replace_query=replace_query, limit=limit)
-        return sorted(
-            owns, key=lambda x: x.cached_label
-            if hasattr(x, 'cached_label') else unicode(x))
+            user, replace_query=replace_query, limit=limit,
+            values=values, get_short_menu_class=get_short_menu_class)
+        return cls._return_get_owns(owns, values, get_short_menu_class)
 
     def get_query_operations(self):
         return Operation.objects.filter(
@@ -531,11 +531,12 @@ class TreatmentFile(ClosedItem, BaseHistorizedItem, OwnPerms, ValueGetter,
                                       'name') if getattr(self, attr)])
 
     @classmethod
-    def get_owns(cls, user, menu_filtr=None, limit=None):
-        owns = super(TreatmentFile, cls).get_owns(user, limit=limit)
-        return sorted(
-            owns, key=lambda x: x.cached_label
-            if hasattr(x, 'cached_label') else unicode(x))
+    def get_owns(cls, user, menu_filtr=None, limit=None, values=None,
+                 get_short_menu_class=None):
+        owns = super(TreatmentFile, cls).get_owns(
+            user, limit=limit, values=values,
+            get_short_menu_class=get_short_menu_class)
+        return cls._return_get_owns(owns, values, get_short_menu_class)
 
     def _generate_cached_label(self):
         items = [unicode(getattr(self, k))
diff --git a/archaeological_operations/models.py b/archaeological_operations/models.py
index 4a900c276..30cd61010 100644
--- a/archaeological_operations/models.py
+++ b/archaeological_operations/models.py
@@ -379,16 +379,17 @@ class Operation(ClosedItem, BaseHistorizedItem, ImageModel, OwnPerms,
         ordering = ('cached_label',)
 
     @classmethod
-    def get_owns(cls, user, menu_filtr=None, limit=None):
+    def get_owns(cls, user, menu_filtr=None, limit=None, values=None,
+                 get_short_menu_class=None):
         replace_query = None
         if menu_filtr and 'file' in menu_filtr:
             replace_query = Q(associated_file=menu_filtr['file'])
+
         owns = super(Operation, cls).get_owns(
             user, replace_query=replace_query,
-            limit=limit)
-        # owns = owns.annotate(null_count=Count('operation_code'))
-        # return owns.order_by("common_name", "-year", "operation_code")
-        return sorted(owns, key=lambda x: x.cached_label)
+            limit=limit, values=values,
+            get_short_menu_class=get_short_menu_class)
+        return cls._return_get_owns(owns, values, get_short_menu_class)
 
     def __unicode__(self):
         if self.cached_label:
diff --git a/ishtar_common/models.py b/ishtar_common/models.py
index 6fb7852ae..7015c70a5 100644
--- a/ishtar_common/models.py
+++ b/ishtar_common/models.py
@@ -226,7 +226,7 @@ class OwnPerms:
         query = self.get_query_owns(user)
         if not query:
             return False
-        query = query & Q(pk=self.pk)
+        query &= Q(pk=self.pk)
         return self.__class__.objects.filter(query).count()
 
     @classmethod
@@ -240,28 +240,57 @@ class OwnPerms:
         return cls.objects.filter(query).count()
 
     @classmethod
-    def get_owns(cls, user, replace_query={}, limit=None):
+    def _return_get_owns(cls, owns, values, get_short_menu_class,
+                         label_key='cached_label'):
+        if not values:
+            if not get_short_menu_class:
+                return sorted(owns, key=lambda x: getattr(x, label_key))
+            return sorted(owns, key=lambda x: getattr(x[0], label_key))
+        if not get_short_menu_class:
+            return sorted(owns, key=lambda x: x[label_key])
+        return sorted(owns, key=lambda x: x[0][label_key])
+
+    @classmethod
+    def get_owns(cls, user, replace_query={}, limit=None, values=None,
+                 get_short_menu_class=False):
         """
         Get Own items
         """
         if isinstance(user, User):
             user = IshtarUser.objects.get(user_ptr=user)
         if user.is_anonymous():
-            return cls.objects.filter(pk__isnull=True)
+            returned = cls.objects.filter(pk__isnull=True)
+            if values:
+                returned = []
+            return returned
         items = []
         if hasattr(cls, 'BASKET_MODEL'):
             items = list(cls.BASKET_MODEL.objects.filter(user=user).all())
         query = cls.get_query_owns(user)
         if not query and not replace_query:
-            return cls.objects.filter(pk__isnull=True)
+            returned = cls.objects.filter(pk__isnull=True)
+            if values:
+                returned = []
+            return returned
         if query:
             q = cls.objects.filter(query)
         if replace_query:
             q = cls.objects.filter(replace_query)
+        if values:
+            q = q.values(*values)
         if limit:
             items += list(q.order_by('-pk')[:limit])
         else:
             items += list(q.order_by(*cls._meta.ordering).all())
+        if get_short_menu_class:
+            if values:
+                if 'id' not in values:
+                    raise NotImplementedError(
+                        "Call of get_owns with get_short_menu_class option and"
+                        " no 'id' in values is not implemented")
+                items = [(i, cls.get_short_menu_class(i['id'])) for i in items]
+            else:
+                items = [(i, cls.get_short_menu_class(i.pk)) for i in items]
         return items
 
 
@@ -643,7 +672,8 @@ class Basket(models.Model):
     def __unicode__(self):
         return self.label
 
-    def get_short_menu_class(self):
+    @classmethod
+    def get_short_menu_class(cls, pk):
         return 'basket'
 
     @property
@@ -968,7 +998,8 @@ def post_delete_record_relation(sender, instance, **kwargs):
 
 
 class ShortMenuItem(object):
-    def get_short_menu_class(self):
+    @classmethod
+    def get_short_menu_class(cls, pk):
         return ''
 
 
diff --git a/ishtar_common/tests.py b/ishtar_common/tests.py
index 10584e4f2..cdf6ce330 100644
--- a/ishtar_common/tests.py
+++ b/ishtar_common/tests.py
@@ -334,6 +334,70 @@ class MergeTest(TestCase):
                          init_mc)
 
 
+class ShortMenuTest(TestCase):
+    def setUp(self):
+        from archaeological_operations.models import OperationType
+        self.username = 'username666'
+        self.password = 'dcbqj7xnjkxnjsknx!@%'
+        self.user = User.objects.create_superuser(
+            self.username, "nomail@nomail.com", self.password)
+        self.other_user = User.objects.create_superuser(
+            'John', "nomail@nomail.com", self.password)
+        self.ope_type = OperationType.objects.create()
+
+    def testNotConnected(self):
+        c = Client()
+        response = c.get(reverse('shortcut-menu'))
+        # no content if not logged
+        self.assertFalse("shortcut-menu" in response.content)
+        c = Client()
+        c.login(username=self.username, password=self.password)
+        # no content because the user owns no object
+        response = c.get(reverse('shortcut-menu'))
+        self.assertFalse("shortcut-menu" in response.content)
+        from archaeological_operations.models import Operation
+        Operation.objects.create(
+            operation_type=self.ope_type,
+            history_modifier=self.user)
+        # content is here
+        response = c.get(reverse('shortcut-menu'))
+        self.assertTrue("shortcut-menu" in response.content)
+
+    def testOperation(self):
+        from archaeological_operations.models import Operation
+        c = Client()
+        c.login(username=self.username, password=self.password)
+        ope = Operation.objects.create(
+            operation_type=self.ope_type,
+            history_modifier=self.other_user,
+            year=2042, operation_code=54
+        )
+        # not available at first
+        response = c.get(reverse('shortcut-menu'))
+        self.assertFalse(str(ope.cached_label) in response.content)
+
+        # available because is the creator
+        ope.history_creator = self.user
+        ope.save()
+        response = c.get(reverse('shortcut-menu'))
+        self.assertTrue(str(ope.cached_label) in response.content)
+
+        # available because is in charge
+        ope.history_creator = self.other_user
+        ope.in_charge = self.user.ishtaruser.person
+        ope.save()
+        response = c.get(reverse('shortcut-menu'))
+        self.assertTrue(str(ope.cached_label) in response.content)
+
+        # available because is the scientist
+        ope.history_creator = self.other_user
+        ope.in_charge = None
+        ope.scientist = self.user.ishtaruser.person
+        ope.save()
+        response = c.get(reverse('shortcut-menu'))
+        self.assertTrue(str(ope.cached_label) in response.content)
+
+
 class ImportTest(TestCase):
     def testDeleteRelated(self):
         town = models.Town.objects.create(name='my-test')
diff --git a/ishtar_common/views.py b/ishtar_common/views.py
index b0817fc59..dbbc3d538 100644
--- a/ishtar_common/views.py
+++ b/ishtar_common/views.py
@@ -256,7 +256,6 @@ def shortcut_menu(request):
             model_name = model.SLUG
             current = model_name in request.session \
                 and request.session[model_name]
-
             dct['menu'].append((
                 lbl, model_name, current or 0,
                 JQueryAutoComplete(
@@ -275,23 +274,28 @@ def shortcut_menu(request):
         cls = ''
         current = model_name in request.session and request.session[model_name]
         items = []
-        for item in model.get_owns(request.user,
-                                   menu_filtr=current_selected_item,
-                                   limit=100):
-            pk = unicode(item.pk)
-            if item.IS_BASKET:
+        current_items = []
+        for item, shortmenu_class in model.get_owns(
+                request.user, menu_filtr=current_selected_item, limit=100,
+                values=['id', 'cached_label'], get_short_menu_class=True):
+            pk = unicode(item['id'])
+            if shortmenu_class == 'basket':
                 pk = "basket-" + pk
+            # prevent duplicates
+            if pk in current_items:
+                continue
+            current_items.append(pk)
             selected = pk == current
             if selected:
-                cls = item.get_short_menu_class()
-                new_selected_item = item
-            items.append((pk, shortify(unicode(item), 60),
-                          selected, item.get_short_menu_class()))
+                cls = shortmenu_class
+                new_selected_item = pk
+            items.append((pk, shortify(item['cached_label'], 60),
+                          selected, shortmenu_class))
         # selected is not in owns - add it to the list
         if not new_selected_item and current:
             try:
                 item = model.objects.get(pk=int(current))
-                new_selected_item = item
+                new_selected_item = item.pk
                 items.append((item.pk, shortify(unicode(item), 60),
                               True, item.get_short_menu_class()))
             except (model.DoesNotExist, ValueError):
@@ -323,7 +327,7 @@ def get_current_items(request):
 
 def unpin(request, item_type):
     if item_type not in ('find', 'contextrecord', 'operation', 'file',
-                          'treatment', 'treatmentfile'):
+                         'treatment', 'treatmentfile'):
         logger.warning("unpin unknow type: {}".format(item_type))
         return HttpResponse('nok')
     request.session['treatment'] = ''