Smplify raiting resource property

10 files changed, 133 insertions, 169 deletions

diff --git a/sugar_network/db/index.py b/sugar_network/db/index.py
index 824423a..aa75f80 100644
--- a/sugar_network/db/index.py
+++ b/sugar_network/db/index.py
@@ -201,7 +201,7 @@ class IndexReader(object):
                     parser.add_prefix(name, prop.prefix)
                 parser.add_prefix('', prop.prefix)
                 if prop.slot is not None and \
-                        prop.typecast in [int, float, bool]:
+                        prop.sortable_serialise is not None:
                     value_range = xapian.NumberValueRangeProcessor(
                             prop.slot, name + ':')
                     parser.add_valuerangeprocessor(value_range)
@@ -359,14 +359,14 @@ class IndexWriter(IndexReader):
                     else properties.get(name, prop.default)
 
             if prop.slot is not None:
-                if prop.typecast in [int, float, bool]:
-                    value_ = xapian.sortable_serialise(value)
+                if prop.sortable_serialise is not None:
+                    slotted_value = xapian.sortable_serialise(
+                            prop.sortable_serialise(value))
+                elif prop.localized:
+                    slotted_value = toolkit.gettext(value) or ''
                 else:
-                    if prop.localized:
-                        value_ = toolkit.gettext(value) or ''
-                    else:
-                        value_ = next(_fmt_prop_value(prop, value))
-                document.add_value(prop.slot, value_)
+                    slotted_value = next(_fmt_prop_value(prop, value))
+                document.add_value(prop.slot, slotted_value)
 
             if prop.prefix or prop.full_text:
                 for value_ in _fmt_prop_value(prop, value):
diff --git a/sugar_network/db/metadata.py b/sugar_network/db/metadata.py
index d22bb6f..7cba5ce 100644
--- a/sugar_network/db/metadata.py
+++ b/sugar_network/db/metadata.py
@@ -116,67 +116,47 @@ class Property(object):
     """Basic class to collect information about document property."""
 
     def __init__(self, name, acl=ACL.PUBLIC, typecast=None,
-            parse=None, fmt=None, default=None):
+            parse=None, fmt=None, default=None, sortable_serialise=None):
+        """
+        :param name:
+            property name;
+        :param acl:
+            access to the property,
+            might be an ORed composition of `db.ACCESS_*` constants;
+        :param typecast:
+            cast property value before storing in the system;
+            supported values are `None` (strings), `int` (intergers),
+            `float` (floats), `bool` (booleans repesented by symbols
+            `0` and `1`),  a sequence of strings (property value should
+            confirm one of values from the sequencei);
+        :param parse:
+            parse property value from a string;
+        :param fmt:
+            format property value to a string or a list of strings;
+        :param default:
+            default property value or None;
+        :param sortable_serialise:
+            cast property value before storing as a srotable value.
+
+        """
         if typecast is bool:
             if fmt is None:
                 fmt = lambda x: '1' if x else '0'
             if parse is None:
                 parse = lambda x: str(x).lower() in ('true', '1', 'on', 'yes')
+        if sortable_serialise is None and typecast in [int, float, bool]:
+            sortable_serialise = typecast
+
         self.setter = None
         self.on_get = lambda self, x: x
         self.on_set = None
-        self._name = name
-        self._acl = acl
-        self._typecast = typecast
-        self._parse = parse
-        self._fmt = fmt
-        self._default = default
-
-    @property
-    def name(self):
-        """Property name."""
-        return self._name
-
-    @property
-    def acl(self):
-        """Specify access to the property.
-
-        Value might be ORed composition of `db.ACCESS_*`
-        constants.
-
-        """
-        return self._acl
-
-    @property
-    def typecast(self):
-        """Cast property value before storing in the system.
-
-        Supported values are:
-
-        * `None`, string values
-        * `int`, interger values
-        * `float`, float values
-        * `bool`, boolean values repesented by symbols `0` and `1`
-        * sequence of strings, property value should confirm one of values
-          from the sequence
-
-        """
-        return self._typecast
-
-    @property
-    def parse(self):
-        """Parse property value from a string."""
-        return self._parse
-
-    @property
-    def fmt(self):
-        """Format property value to a string or a list of strings."""
-        return self._fmt
-
-    @property
-    def default(self):
-        """Default property value or None."""
-        return self._default
+        self.name = name
+        self.acl = acl
+        self.typecast = typecast
+        self.parse = parse
+        self.fmt = fmt
+        self.default = default
+        self.sortable_serialise = sortable_serialise
 
     def assert_access(self, mode):
         """Is access to the property permitted.
@@ -200,11 +180,13 @@ class StoredProperty(Property):
     def __init__(self, name, localized=False, typecast=None, fmt=None,
             **kwargs):
         """
+        :param: localized:
+            property value will be stored per locale;
         :param: **kwargs
             :class:`.Property` arguments
 
         """
-        self._localized = localized
+        self.localized = localized
 
         if localized:
             enforce(typecast is None,
@@ -216,11 +198,6 @@ class StoredProperty(Property):
 
         Property.__init__(self, name, typecast=typecast, fmt=fmt, **kwargs)
 
-    @property
-    def localized(self):
-        """Property value will be stored per locale."""
-        return self._localized
-
 
 class IndexedProperty(StoredProperty):
     """Property which needs to be indexed."""
@@ -228,6 +205,14 @@ class IndexedProperty(StoredProperty):
     def __init__(self, name, slot=None, prefix=None, full_text=False,
             boolean=False, **kwargs):
         """
+        :param slot:
+            Xapian document's slot number to add property value to;
+        :param prefix:
+            Xapian serach term prefix, if `None`, property is not a term;
+        :param full_text:
+            property takes part in full-text search;
+        :param boolean:
+            Xapian will use boolean search for this property;
         :param: **kwargs
             :class:`.StoredProperty` arguments
 
@@ -242,35 +227,16 @@ class IndexedProperty(StoredProperty):
                 'For %r property, either slot, prefix or full_text '
                 'need to be set',
                 name)
-        enforce(slot is None or _is_sloted_prop(kwargs.get('typecast')),
+        enforce(slot is None or _is_sloted_prop(kwargs.get('typecast')) or
+                kwargs.get('sortable_serialise'),
                 'Slot can be set only for properties for str, int, float, '
                 'bool types, or, for list of these types')
 
         StoredProperty.__init__(self, name, **kwargs)
-        self._slot = slot
-        self._prefix = prefix
-        self._full_text = full_text
-        self._boolean = boolean
-
-    @property
-    def slot(self):
-        """Xapian document's slot number to add property value to."""
-        return self._slot
-
-    @property
-    def prefix(self):
-        """Xapian serach term prefix, if `None`, property is not a term."""
-        return self._prefix
-
-    @property
-    def full_text(self):
-        """Property takes part in full-text search."""
-        return self._full_text
-
-    @property
-    def boolean(self):
-        """Xapian will use boolean search for this property."""
-        return self._boolean
+        self.slot = slot
+        self.prefix = prefix
+        self.full_text = full_text
+        self.boolean = boolean
 
 
 class BlobProperty(Property):
@@ -279,21 +245,15 @@ class BlobProperty(Property):
     def __init__(self, name, acl=ACL.PUBLIC,
             mime_type='application/octet-stream'):
         """
+        :param mime_type:
+            MIME type for BLOB content;
+            by default, MIME type is application/octet-stream;
         :param: **kwargs
             :class:`.Property` arguments
 
         """
         Property.__init__(self, name, acl=acl)
-        self._mime_type = mime_type
-
-    @property
-    def mime_type(self):
-        """MIME type for BLOB content.
-
-        By default, MIME type is application/octet-stream.
-
-        """
-        return self._mime_type
+        self.mime_type = mime_type
 
 
 def _is_sloted_prop(typecast):
diff --git a/sugar_network/model/context.py b/sugar_network/model/context.py
index 872d74d..85d9be8 100644
--- a/sugar_network/model/context.py
+++ b/sugar_network/model/context.py
@@ -115,25 +115,12 @@ class Context(db.Resource):
     def downloads(self, value):
         return value
 
-    @db.indexed_property(slot=3, typecast=model.RATINGS, default=0,
+    @db.indexed_property(slot=3, typecast=[], default=[0, 0],
+            sortable_serialise=lambda x: float(x[1]) / x[0] if x[0] else 0,
             acl=ACL.READ | ACL.CALC)
     def rating(self, value):
         return value
 
-    @db.stored_property(typecast=[], default=[0, 0], acl=ACL.READ | ACL.CALC)
-    def reviews(self, value):
-        if value is None:
-            return 0
-        else:
-            return value[0]
-
-    @reviews.setter
-    def reviews(self, value):
-        if isinstance(value, int):
-            return [value, 0]
-        else:
-            return value
-
     @db.stored_property(typecast=[], default=[], acl=ACL.PUBLIC | ACL.LOCAL)
     def dependencies(self, value):
         """Software dependencies.
diff --git a/sugar_network/model/post.py b/sugar_network/model/post.py
index d7a742c..602ad02 100644
--- a/sugar_network/model/post.py
+++ b/sugar_network/model/post.py
@@ -79,15 +79,8 @@ class Post(db.Resource):
     def downloads(self, value):
         return value
 
-    @db.indexed_property(slot=3, typecast=model.RATINGS, default=0,
+    @db.indexed_property(slot=3, typecast=[], default=[0, 0],
+            sortable_serialise=lambda x: float(x[1]) / x[0] if x[0] else 0,
             acl=ACL.READ | ACL.CALC)
     def rating(self, value):
         return value
-
-    @db.stored_property(typecast=[], default=[0, 0],
-            acl=ACL.READ | ACL.CALC)
-    def reviews(self, value):
-        if value is None:
-            return 0
-        else:
-            return value[0]
diff --git a/sugar_network/node/stats_node.py b/sugar_network/node/stats_node.py
index ed2c0bd..d37819b 100644
--- a/sugar_network/node/stats_node.py
+++ b/sugar_network/node/stats_node.py
@@ -118,7 +118,7 @@ class Sniffer(object):
         for resource, stats in self._stats.items():
             old = {
                     'downloads': 0,
-                    'reviews': (0, 0),
+                    'rating': (0, 0),
                     }
             directory = self._volume[resource]
             for guid, new in stats.objects.items():
@@ -131,12 +131,11 @@ class Sniffer(object):
                 patch = {}
                 if 'downloads' in new:
                     patch['downloads'] = new['downloads'] + old['downloads']
-                if 'reviews' in new:
-                    reviews, rating = old['reviews']
-                    reviews += new['reviews']
+                if 'votes' in new:
+                    votes, rating = old['rating']
+                    votes += new['votes']
                     rating += new['rating']
-                    patch['reviews'] = [reviews, rating]
-                    patch['rating'] = int(round(float(rating) / reviews))
+                    patch['rating'] = [votes, rating]
                 directory.update(guid, patch)
             stats.objects.clear()
 
@@ -296,7 +295,7 @@ class _PostStats(_ResourceStats):
                 stats = self._stats['post']
                 guid = request.content['topic']
             if stats:
-                stats.inc(guid, 'reviews')
+                stats.inc(guid, 'votes')
                 stats.inc(guid, 'rating', request.content.get('vote') or 0)
 
         elif request.method == 'GET' and request.prop == 'data':
diff --git a/tests/units/model/context.py b/tests/units/model/context.py
index b48db6f..f2de64d 100755
--- a/tests/units/model/context.py
+++ b/tests/units/model/context.py
@@ -5,7 +5,9 @@ from os.path import exists
 
 from __init__ import tests
 
+from sugar_network import db
 from sugar_network.node import obs
+from sugar_network.model.context import Context
 from sugar_network.client import IPCConnection
 from sugar_network.toolkit import coroutine, enforce
 
@@ -57,6 +59,25 @@ class ContextTest(tests.Test):
         assert exists('master/context/gu/guid/icon.blob')
         assert exists('master/context/gu/guid/preview.blob')
 
+    def test_RatingSort(self):
+        directory = db.Volume('db', [Context])['context']
+
+        directory.create({'guid': '1', 'type': 'activity', 'title': '', 'summary': '', 'description': '', 'rating': [0, 0]})
+        directory.create({'guid': '2', 'type': 'activity', 'title': '', 'summary': '', 'description': '', 'rating': [1, 2]})
+        directory.create({'guid': '3', 'type': 'activity', 'title': '', 'summary': '', 'description': '', 'rating': [1, 4]})
+        directory.create({'guid': '4', 'type': 'activity', 'title': '', 'summary': '', 'description': '', 'rating': [10, 10]})
+        directory.create({'guid': '5', 'type': 'activity', 'title': '', 'summary': '', 'description': '', 'rating': [30, 90]})
+
+        self.assertEqual(
+                ['1', '2', '3', '4', '5'],
+                [i.guid for i in directory.find()[0]])
+        self.assertEqual(
+                ['1', '4', '2', '5', '3'],
+                [i.guid for i in directory.find(order_by='rating')[0]])
+        self.assertEqual(
+                ['3', '5', '2', '4', '1'],
+                [i.guid for i in directory.find(order_by='-rating')[0]])
+
 
 if __name__ == '__main__':
     tests.main()
diff --git a/tests/units/model/post.py b/tests/units/model/post.py
index 91a0298..931bd66 100755
--- a/tests/units/model/post.py
+++ b/tests/units/model/post.py
@@ -3,6 +3,7 @@
 
 from __init__ import tests
 
+from sugar_network import db
 from sugar_network.client import Connection, keyfile
 from sugar_network.model.user import User
 from sugar_network.model.context import Context
@@ -41,6 +42,25 @@ class PostTest(tests.Test):
                 context,
                 client.get(['post', comment, 'context']))
 
+    def test_RatingSort(self):
+        directory = db.Volume('db', [Post])['post']
+
+        directory.create({'guid': '1', 'context': '', 'type': 'comment', 'title': '', 'message': '', 'rating': [0, 0]})
+        directory.create({'guid': '2', 'context': '', 'type': 'comment', 'title': '', 'message': '', 'rating': [1, 2]})
+        directory.create({'guid': '3', 'context': '', 'type': 'comment', 'title': '', 'message': '', 'rating': [1, 4]})
+        directory.create({'guid': '4', 'context': '', 'type': 'comment', 'title': '', 'message': '', 'rating': [10, 10]})
+        directory.create({'guid': '5', 'context': '', 'type': 'comment', 'title': '', 'message': '', 'rating': [30, 90]})
+
+        self.assertEqual(
+                ['1', '2', '3', '4', '5'],
+                [i.guid for i in directory.find()[0]])
+        self.assertEqual(
+                ['1', '4', '2', '5', '3'],
+                [i.guid for i in directory.find(order_by='rating')[0]])
+        self.assertEqual(
+                ['3', '5', '2', '4', '1'],
+                [i.guid for i in directory.find(order_by='-rating')[0]])
+
 
 if __name__ == '__main__':
     tests.main()
diff --git a/tests/units/node/node.py b/tests/units/node/node.py
index 22c0bd3..388b2ed 100755
--- a/tests/units/node/node.py
+++ b/tests/units/node/node.py
@@ -1217,28 +1217,24 @@ class NodeTest(tests.Test):
 
         self.assertEqual({
             'downloads': 2,
-            'rating': 1,
-            'reviews': [1, 1],
+            'rating': [1, 1],
             },
-            volume['context'].get('context_1').properties(['downloads', 'rating', 'reviews']))
+            volume['context'].get('context_1').properties(['downloads', 'rating']))
         self.assertEqual({
             'downloads': 1,
-            'rating': 4,
-            'reviews': [2, 7],
+            'rating': [2, 7],
             },
-            volume['context'].get('context_2').properties(['downloads', 'rating', 'reviews']))
+            volume['context'].get('context_2').properties(['downloads', 'rating']))
         self.assertEqual({
             'downloads': 2,
-            'rating': 2,
-            'reviews': [1, 2],
+            'rating': [1, 2],
             },
-            volume['post'].get('topic_1').properties(['downloads', 'rating', 'reviews']))
+            volume['post'].get('topic_1').properties(['downloads', 'rating']))
         self.assertEqual({
             'downloads': 1,
-            'rating': 0,
-            'reviews': [0, 0],
+            'rating': [0, 0],
             },
-            volume['post'].get('topic_2').properties(['downloads', 'rating', 'reviews']))
+            volume['post'].get('topic_2').properties(['downloads', 'rating']))
 
     def test_generate_node_stats_Deletes(self):
         node.stats_root.value = 'stats'
diff --git a/tests/units/node/stats_node.py b/tests/units/node/stats_node.py
index 666a183..eab7fb8 100755
--- a/tests/units/node/stats_node.py
+++ b/tests/units/node/stats_node.py
@@ -77,10 +77,8 @@ class StatsTest(tests.Test):
         self.assertEqual(3, stats._stats['post']['total'])
 
         stats.commit_objects()
-        self.assertEqual([2, 3], volume['context'].get('context')['reviews'])
-        self.assertEqual(2, volume['context'].get('context')['rating'])
-        self.assertEqual([1, 3], volume['post'].get('topic')['reviews'])
-        self.assertEqual(3, volume['post'].get('topic')['rating'])
+        self.assertEqual([2, 3], volume['context'].get('context')['rating'])
+        self.assertEqual([1, 3], volume['post'].get('topic')['rating'])
 
     def test_ContextDownloaded(self):
         volume = db.Volume('local', model.RESOURCES)
@@ -188,8 +186,7 @@ class StatsTest(tests.Test):
         volume['release'].create({'guid': 'release', 'context': 'context', 'license': 'GPLv3', 'version': '1', 'date': 0, 'stability': 'stable', 'notes': ''})
 
         self.assertEqual(0, volume['context'].get('context')['downloads'])
-        self.assertEqual([0, 0], volume['context'].get('context')['reviews'])
-        self.assertEqual(0, volume['context'].get('context')['rating'])
+        self.assertEqual([0, 0], volume['context'].get('context')['rating'])
 
         stats = Sniffer(volume, 'stats/node')
         request = Request(method='GET', path=['release', 'release', 'data'])
@@ -204,15 +201,13 @@ class StatsTest(tests.Test):
         stats.commit_objects()
 
         self.assertEqual(1, volume['context'].get('context')['downloads'])
-        self.assertEqual([1, 5], volume['context'].get('context')['reviews'])
-        self.assertEqual(5, volume['context'].get('context')['rating'])
+        self.assertEqual([1, 5], volume['context'].get('context')['rating'])
 
         stats.commit()
         stats.commit_objects()
 
         self.assertEqual(1, volume['context'].get('context')['downloads'])
-        self.assertEqual([1, 5], volume['context'].get('context')['reviews'])
-        self.assertEqual(5, volume['context'].get('context')['rating'])
+        self.assertEqual([1, 5], volume['context'].get('context')['rating'])
 
         stats = Sniffer(volume, 'stats/node')
         request = Request(method='GET', path=['release', 'release', 'data'])
@@ -226,8 +221,7 @@ class StatsTest(tests.Test):
         stats.commit_objects()
 
         self.assertEqual(2, volume['context'].get('context')['downloads'])
-        self.assertEqual([2, 6], volume['context'].get('context')['reviews'])
-        self.assertEqual(3, volume['context'].get('context')['rating'])
+        self.assertEqual([2, 6], volume['context'].get('context')['rating'])
 
     def test_CommitTopicStats(self):
         volume = db.Volume('local', model.RESOURCES)
@@ -236,8 +230,7 @@ class StatsTest(tests.Test):
         volume['post'].create({'guid': 'topic', 'type': 'object', 'context': 'context', 'title': '', 'message': ''})
 
         self.assertEqual(0, volume['post'].get('topic')['downloads'])
-        self.assertEqual([0, 0], volume['post'].get('topic')['reviews'])
-        self.assertEqual(0, volume['post'].get('topic')['rating'])
+        self.assertEqual([0, 0], volume['post'].get('topic')['rating'])
 
         stats = Sniffer(volume, 'stats/node')
         request = Request(method='GET', path=['post', 'topic', 'data'])
@@ -251,15 +244,13 @@ class StatsTest(tests.Test):
         stats.commit_objects()
 
         self.assertEqual(1, volume['post'].get('topic')['downloads'])
-        self.assertEqual([1, 5], volume['post'].get('topic')['reviews'])
-        self.assertEqual(5, volume['post'].get('topic')['rating'])
+        self.assertEqual([1, 5], volume['post'].get('topic')['rating'])
 
         stats.commit()
         stats.commit_objects()
 
         self.assertEqual(1, volume['post'].get('topic')['downloads'])
-        self.assertEqual([1, 5], volume['post'].get('topic')['reviews'])
-        self.assertEqual(5, volume['post'].get('topic')['rating'])
+        self.assertEqual([1, 5], volume['post'].get('topic')['rating'])
 
         request = Request(method='GET', path=['post', 'topic', 'data'])
         request.principal = 'user'
@@ -272,8 +263,7 @@ class StatsTest(tests.Test):
         stats.commit_objects()
 
         self.assertEqual(2, volume['post'].get('topic')['downloads'])
-        self.assertEqual([2, 6], volume['post'].get('topic')['reviews'])
-        self.assertEqual(3, volume['post'].get('topic')['rating'])
+        self.assertEqual([2, 6], volume['post'].get('topic')['rating'])
 
     def test_Suspend(self):
         stats_node_step.value = 5
diff --git a/tests/units/node/volume.py b/tests/units/node/volume.py
index 10a1cdd..512b3e0 100755
--- a/tests/units/node/volume.py
+++ b/tests/units/node/volume.py
@@ -462,10 +462,8 @@ class VolumeTest(tests.Test):
                 })],
             ],
             [[(j.name,) + i for i in j.get(j.last, j.last)] for j in Rrd('stats/node', 1)])
-        self.assertEqual(1, volume['context'].get('context')['rating'])
-        self.assertEqual([1, 1], volume['context'].get('context')['reviews'])
-        self.assertEqual(1, volume['post'].get('topic_1')['rating'])
-        self.assertEqual([1, 1], volume['post'].get('topic_1')['reviews'])
+        self.assertEqual([1, 1], volume['context'].get('context')['rating'])
+        self.assertEqual([1, 1], volume['post'].get('topic_1')['rating'])
 
         records = [
                 {'resource': 'post'},