From 055f180d20bfe70c0edd7f550e0332b0a79e228f Mon Sep 17 00:00:00 2001
From: carlfeberhard <carlfeberhard@gmail.com>
Date: Fri, 22 Jan 2016 14:33:01 -0500
Subject: [PATCH 1/4] Managers: fix sharable.list_published's incorrect use of
 query, add manager getter to base serializer, deserializer, and filter
 parser; Managers, ratable: implement and attach to history

---
 lib/galaxy/managers/annotatable.py         |  2 +-
 lib/galaxy/managers/base.py                | 49 ++++++++---
 lib/galaxy/managers/datasets.py            |  4 +-
 lib/galaxy/managers/deletable.py           |  6 +-
 lib/galaxy/managers/hdas.py                |  2 +-
 lib/galaxy/managers/histories.py           | 19 +++--
 lib/galaxy/managers/pages.py               |  2 +-
 lib/galaxy/managers/ratable.py             | 98 +++++++++++++++++++---
 lib/galaxy/managers/sharable.py            | 19 +++--
 lib/galaxy/managers/visualizations.py      |  2 +-
 lib/galaxy/webapps/galaxy/api/histories.py |  2 +-
 11 files changed, 154 insertions(+), 51 deletions(-)

diff --git a/lib/galaxy/managers/annotatable.py b/lib/galaxy/managers/annotatable.py
index 0b25a947db9..7e2b74d9b17 100644
--- a/lib/galaxy/managers/annotatable.py
+++ b/lib/galaxy/managers/annotatable.py
@@ -71,7 +71,7 @@ class AnnotatableDeserializerMixin( object ):
         if `val` is None.
         """
         val = self.validate.nullable_basestring( key, val )
-        return self.manager.annotate( item, val, user=user, flush=False )
+        return self.manager().annotate( item, val, user=user, flush=False )
 
 
 # TODO: I'm not entirely convinced this (or tags) are a good idea for filters since they involve a/the user
diff --git a/lib/galaxy/managers/base.py b/lib/galaxy/managers/base.py
index 13820752c79..62ad1dc5791 100644
--- a/lib/galaxy/managers/base.py
+++ b/lib/galaxy/managers/base.py
@@ -460,7 +460,6 @@ class ModelManager( object ):
             self.session().flush()
         return item
 
-    # TODO: yagni?
     def associate( self, associate_with, item, foreign_key_name=None ):
         """
         Generically associate `item` with `associate_with` based on `foreign_key_name`.
@@ -469,12 +468,15 @@ class ModelManager( object ):
         setattr( associate_with, foreign_key_name, item )
         return item
 
+    def _foreign_key( self, associated_model_class, foreign_key_name=None ):
+        foreign_key_name = foreign_key_name or self.foreign_key_name
+        return getattr( associated_model_class, foreign_key_name )
+
     def query_associated( self, associated_model_class, item, foreign_key_name=None ):
         """
         Generically query other items that have been associated with this `item`.
         """
-        foreign_key_name = foreign_key_name or self.foreign_key_name
-        foreign_key = getattr( associated_model_class, foreign_key_name )
+        foreign_key = self._foreign_key( associated_model_class, foreign_key_name=foreign_key_name )
         return self.session().query( associated_model_class ).filter( foreign_key == item )
 
     # a rename of sql DELETE to differentiate from the Galaxy notion of mark_as_deleted
@@ -520,14 +522,17 @@ class ModelSerializer( object ):
         keys_to_serialize = [ 'id', 'name', 'attr1', 'attr2', ... ]
         item_dict = MySerializer.serialize( my_item, keys_to_serialize )
     """
+    #: the class used to create this serializer's generically accessible model_manager
+    model_manager_class = None
     #: 'service' to use for getting urls - use class var to allow overriding when testing
     url_for = staticmethod( routes.url_for )
 
-    def __init__( self, app ):
+    def __init__( self, app, manager=None ):
         """
         Set up serializer map, any additional serializable keys, and views here.
         """
         self.app = app
+        self._manager = manager
 
         # a list of valid serializable keys that can use the default (string) serializer
         #   this allows us to: 'mention' the key without adding the default serializer
@@ -546,6 +551,13 @@ class ModelSerializer( object ):
         self.views = {}
         self.default_view = None
 
+    def manager( self ):
+        """Return an appropriate manager if it exists, instantiate if not."""
+        if not self._manager:
+            # TODO: pass this serializer to it
+            self._manager = self.model_manager_class( self.app )
+        return self._manager
+
     def add_serializers( self ):
         """
         Register a map of attribute keys -> serializing functions that will serialize
@@ -681,11 +693,12 @@ class ModelDeserializer( object ):
 
     # TODO:?? a larger question is: which should be first? Deserialize then validate - or - validate then deserialize?
 
-    def __init__( self, app ):
+    def __init__( self, app, manager=None ):
         """
         Set up deserializers and validator.
         """
         self.app = app
+        self._manager = None
 
         self.deserializers = {}
         self.deserializable_keyset = set([])
@@ -693,10 +706,12 @@ class ModelDeserializer( object ):
         # a sub object that can validate incoming values
         self.validate = ModelValidator( self.app )
 
-        # create a generically accessible manager for the model this deserializer works with/for
-        self.manager = None
-        if self.model_manager_class:
-            self.manager = self.model_manager_class( self.app )
+    def manager( self ):
+        """Return an appropriate manager if it exists, instantiate if not."""
+        if not self._manager:
+            # TODO: pass this deserializer to it
+            self._manager = self.model_manager_class( self.app )
+        return self._manager
 
     def add_deserializers( self ):
         """
@@ -865,6 +880,9 @@ class ModelFilterParser( object ):
     These might be safely be replaced in the future by creating SQLAlchemy
     hybrid properties or more thoroughly mapping derived values.
     """
+    #: the class used to create this deserializer's generically accessible model_manager
+    model_manager_class = None
+
     # ??: this class kindof 'lives' in both the world of the controllers/param-parsing and to models/orm
     # (as the model informs how the filter params are parsed)
     # I have no great idea where this 'belongs', so it's here for now
@@ -872,11 +890,12 @@ class ModelFilterParser( object ):
     #: model class
     model_class = None
 
-    def __init__( self, app ):
+    def __init__( self, app, manager=None ):
         """
         Set up serializer map, any additional serializable keys, and views here.
         """
         self.app = app
+        self._manager = manager
 
         # dictionary containing parsing data for ORM/SQLAlchemy-based filters
         # ..note: although kind of a pain in the ass and verbose, opt-in/whitelisting allows more control
@@ -889,6 +908,13 @@ class ModelFilterParser( object ):
         # set up both of the above
         self._add_parsers()
 
+    def manager( self ):
+        """Return an appropriate manager if it exists, instantiate if not."""
+        if not self._manager:
+            # TODO: pass this parser to it
+            self._manager = self.model_manager_class( self.app )
+        return self._manager
+
     def _add_parsers( self ):
         """
         Set up, extend, or alter `orm_filter_parsers` and `fn_filter_parsers`.
@@ -907,6 +933,7 @@ class ModelFilterParser( object ):
         """
         Parse string 3-tuples (attr, op, val) into orm or functional filters.
         """
+        # TODO: allow defining the default filter op in this class (and not 'eq' in base/controller.py)
         parsed = []
         for ( attr, op, val ) in filter_tuple_list:
             filter_ = self.parse_filter( attr, op, val )
@@ -936,7 +963,7 @@ class ModelFilterParser( object ):
         # by convention, assume most val parsers raise ValueError
         except ValueError, val_err:
             raise exceptions.RequestParameterInvalidException( 'unparsable value for filter',
-                                                               column=attr, operation=op, value=val, ValueError=str( val_err ) )
+                column=attr, operation=op, value=val, ValueError=str( val_err ) )
 
         # if neither of the above work, raise an error with how-to info
         # TODO: send back all valid filter keys in exception for added user help
diff --git a/lib/galaxy/managers/datasets.py b/lib/galaxy/managers/datasets.py
index 44e5dbe4b21..4339b782df2 100644
--- a/lib/galaxy/managers/datasets.py
+++ b/lib/galaxy/managers/datasets.py
@@ -234,11 +234,11 @@ class DatasetDeserializer( base.ModelDeserializer, deletable.PurgableDeserialize
         `permissions` dictionary, where `permissions` is in the form:
             { 'manage': [ <role id 1>, ... ], 'access': [ <role id 2>, ... ] }
         """
-        self.manager.permissions.manage.error_unless_permitted( dataset, user )
+        self.manager().permissions.manage.error_unless_permitted( dataset, user )
         self._validate_permissions( permissions, **context )
         manage = self._list_of_roles_from_ids( permissions[ 'manage' ] )
         access = self._list_of_roles_from_ids( permissions[ 'access' ] )
-        self.manager.permissions.set( dataset, manage, access, flush=False )
+        self.manager().permissions.set( dataset, manage, access, flush=False )
         return permissions
 
     def _validate_permissions( self, permissions, **context ):
diff --git a/lib/galaxy/managers/deletable.py b/lib/galaxy/managers/deletable.py
index dc55eea5baf..9351a35305e 100644
--- a/lib/galaxy/managers/deletable.py
+++ b/lib/galaxy/managers/deletable.py
@@ -52,9 +52,9 @@ class DeletableDeserializerMixin( object ):
             return item.deleted
         # TODO:?? flush=False?
         if new_deleted:
-            self.manager.delete( item, flush=False )
+            self.manager().delete( item, flush=False )
         else:
-            self.manager.undelete( item, flush=False )
+            self.manager().undelete( item, flush=False )
         return item.deleted
 
 
@@ -103,7 +103,7 @@ class PurgableDeserializerMixin( DeletableDeserializerMixin ):
             return item.purged
         # do we want to error if something attempts to 'unpurge'?
         if new_purged:
-            self.manager.purge( item, flush=False )
+            self.manager().purge( item, flush=False )
         return item.purged
 
 
diff --git a/lib/galaxy/managers/hdas.py b/lib/galaxy/managers/hdas.py
index 19f24159a14..028af4d063c 100644
--- a/lib/galaxy/managers/hdas.py
+++ b/lib/galaxy/managers/hdas.py
@@ -443,7 +443,7 @@ class HDADeserializer( datasets.DatasetAssociationDeserializer,
 
     def __init__( self, app ):
         super( HDADeserializer, self ).__init__( app )
-        self.hda_manager = self.manager
+        self.hda_manager = self.manager()
 
     def add_deserializers( self ):
         super( HDADeserializer, self ).add_deserializers()
diff --git a/lib/galaxy/managers/histories.py b/lib/galaxy/managers/histories.py
index 7c85b0b3481..bfdb09acfc7 100644
--- a/lib/galaxy/managers/histories.py
+++ b/lib/galaxy/managers/histories.py
@@ -39,7 +39,6 @@ class HistoryManager( sharable.SharableModelManager, deletable.PurgableManagerMi
 
     def __init__( self, app, *args, **kwargs ):
         super( HistoryManager, self ).__init__( app, *args, **kwargs )
-
         self.hda_manager = hdas.HDAManager( app )
 
     def copy( self, history, user, **kwargs ):
@@ -148,6 +147,7 @@ class HistoryManager( sharable.SharableModelManager, deletable.PurgableManagerMi
             return desc( self.model_class.disk_size )
         if order_by_string == 'size-asc':
             return asc( self.model_class.disk_size )
+        # TODO: add functional/non-orm orders (such as rating)
         if default:
             return self.parse_order_by( default )
         raise glx_exceptions.RequestParameterInvalidException( 'Unkown order_by', order_by=order_by_string,
@@ -170,12 +170,13 @@ class HistorySerializer( sharable.SharableModelSerializer, deletable.PurgableSer
     """
     Interface/service object for serializing histories into dictionaries.
     """
+    model_manager_class = HistoryManager
     SINGLE_CHAR_ABBR = 'h'
 
-    def __init__( self, app ):
-        super( HistorySerializer, self ).__init__( app )
+    def __init__( self, app, **kwargs ):
+        super( HistorySerializer, self ).__init__( app, **kwargs )
 
-        self.history_manager = HistoryManager( app )
+        self.history_manager = self.manager()
         self.hda_manager = hdas.HDAManager( app )
         self.hda_serializer = hdas.HDASerializer( app )
 
@@ -207,8 +208,10 @@ class HistorySerializer( sharable.SharableModelSerializer, deletable.PurgableSer
             'state',
             'state_details',
             'state_ids',
-            # in the Historys' case, each of these views includes the keys from the previous
+            # 'community_rating',
+            # 'user_rating',
         ], include_keys_from='summary' )
+        # in the Historys' case, each of these views includes the keys from the previous
 
     # assumes: outgoing to json.dumps and sanitized
     def add_serializers( self ):
@@ -343,7 +346,7 @@ class HistoryDeserializer( sharable.SharableModelDeserializer, deletable.Purgabl
 
     def __init__( self, app ):
         super( HistoryDeserializer, self ).__init__( app )
-        self.history_manager = self.manager
+        self.history_manager = self.manager()
 
     def add_deserializers( self ):
         super( HistoryDeserializer, self ).add_deserializers()
@@ -355,9 +358,9 @@ class HistoryDeserializer( sharable.SharableModelDeserializer, deletable.Purgabl
         })
 
 
-class HistoryFilters( sharable.SharableModelFilters,
-                      deletable.PurgableFiltersMixin ):
+class HistoryFilters( sharable.SharableModelFilters, deletable.PurgableFiltersMixin ):
     model_class = model.History
+    model_manager_class = HistoryManager
 
     def _add_parsers( self ):
         super( HistoryFilters, self )._add_parsers()
diff --git a/lib/galaxy/managers/pages.py b/lib/galaxy/managers/pages.py
index 373007f581e..85961eb7278 100644
--- a/lib/galaxy/managers/pages.py
+++ b/lib/galaxy/managers/pages.py
@@ -65,7 +65,7 @@ class PageDeserializer( sharable.SharableModelDeserializer ):
 
     def __init__( self, app ):
         super( PageDeserializer, self ).__init__( app )
-        self.page_manager = self.manager
+        self.page_manager = self.manager()
 
     def add_deserializers( self ):
         super( PageDeserializer, self ).add_deserializers()
diff --git a/lib/galaxy/managers/ratable.py b/lib/galaxy/managers/ratable.py
index fdb2a3ad1de..fb3f28da86a 100644
--- a/lib/galaxy/managers/ratable.py
+++ b/lib/galaxy/managers/ratable.py
@@ -2,19 +2,55 @@
 Mixins for Ratable model managers and serializers.
 """
 
+from sqlalchemy.sql.expression import func
+from . import base
+
 import logging
 log = logging.getLogger( __name__ )
 
 
-# TODO: stub
 class RatableManagerMixin( object ):
+
     #: class of RatingAssociation (e.g. HistoryRatingAssociation)
     rating_assoc = None
 
-    # TODO: most of this seems to be covered by item_attrs.UsesItemRatings
+    def rating( self, item, user ):
+        """Returns the integer rating given to this item by the user."""
+        rating = self.query_associated( self.rating_assoc, item ).filter_by( user=user ).first()
+        if rating is not None:
+            # get the value if there's a rating
+            rating = rating.rating
+        return rating
 
-    # def by_user( self, trans, user, **kwargs ):
-    #    pass
+    def ratings( self, item, user ):
+        """Returns all ratings given to this item."""
+        return [ r.rating for r in item.ratings ]
+
+    def ratings_avg( self, item ):
+        """Returns the average of all ratings given to this item."""
+        foreign_key = self._foreign_key( self.rating_assoc )
+        return self.session().query( func.avg( self.rating_assoc.rating ) ).filter( foreign_key == item ).scalar()
+
+    def ratings_count( self, item ):
+        """Returns the average of all ratings given to this item."""
+        foreign_key = self._foreign_key( self.rating_assoc )
+        return self.session().query( func.count( self.rating_assoc.rating ) ).filter( foreign_key == item ).scalar()
+
+    def rate( self, item, user, value, flush=True ):
+        """Updates or creates a rating for this item and user. Returns the rating"""
+        # TODO?: possible generic update_or_create
+        # TODO?: update and create to RatingsManager (if not overkill)
+        rating = self.rating( item, user )
+        if not rating:
+            rating = self.rating_assoc( item=item, user=user )
+        rating.rating = value
+
+        self.session().add( rating )
+        if flush:
+            self.flush()
+        return rating
+
+    # TODO?: all ratings for a user
 
 
 class RatableSerializerMixin( object ):
@@ -24,22 +60,58 @@ class RatableSerializerMixin( object ):
         self.serializers[ 'community_rating' ] = self.serialize_community_rating
 
     def serialize_user_rating( self, item, key, user=None, **context ):
-        """
-        """
-        pass
+        """Returns the integer rating given to this item by the user."""
+        if not user:
+            raise base.ModelSerializingError( 'user_rating requires a user',
+                model_class=self.manager().model_class, id=self.serialize_id( item, 'id' ) )
+        return self.manager().rating( item, user )
 
     def serialize_community_rating( self, item, key, **context ):
         """
+        Returns a dictionary containing:
+            `average` the (float) average of all ratings of this object
+            `count` the number of ratings
         """
-        pass
+        # ??: seems like two queries (albeit in-sql functions) would slower
+        # than getting the rows and calc'ing both here with one query
+        manager = self.manager()
+        return {
+            'average' : manager.ratings_avg( item ),
+            'count'   : manager.ratings_count( item ),
+        }
 
 
 class RatableDeserializerMixin( object ):
 
     def add_deserializers( self ):
-        pass
-        # self.deserializers[ 'user_rating' ] = self.deserialize_rating
+        self.deserializers[ 'user_rating' ] = self.deserialize_rating
 
-    # def deserialize_rating( self, trans, item, key, val ):
-    #    val = self.validate.int_range( key, val, 0, 5 )
-    #    return self.set_rating...( trans, item, val, user=trans.user )
+    def deserialize_rating( self, item, key, val, user=None, **context ):
+        if not user:
+            raise base.ModelDeserializingError( 'user_rating requires a user',
+                model_class=self.manager().model_class, id=self.serialize_id( item, 'id' ) )
+        val = self.validate.int_range( key, val, 0, 5 )
+        return self.manager().rate( item, user, val, flush=False )
+
+
+class RatableFilterMixin( object ):
+
+    def _ratings_avg_accessor( self, item ):
+        return self.manager().ratings_avg( item )
+
+    def _add_parsers( self ):
+        """
+        Adds the following filters:
+            `community_rating`: filter
+        """
+        self.fn_filter_parsers.update({
+            'community_rating': {
+                'op': {
+                    'eq' : lambda i, v: self._ratings_avg_accessor( i ) == v,
+                    # TODO: default to greater than (currently 'eq' due to base/controller.py)
+                    'ge' : lambda i, v: self._ratings_avg_accessor( i ) >= v,
+                    'le' : lambda i, v: self._ratings_avg_accessor( i ) <= v,
+                },
+                'val' : float
+            }
+        })
diff --git a/lib/galaxy/managers/sharable.py b/lib/galaxy/managers/sharable.py
index 1cf77baa704..f81bec631e5 100644
--- a/lib/galaxy/managers/sharable.py
+++ b/lib/galaxy/managers/sharable.py
@@ -122,12 +122,13 @@ class SharableModelManager( base.ModelManager, secured.OwnableManagerMixin, secu
         filters = self._munge_filters( published_filter, filters )
         return self.query( filters=filters, **kwargs )
 
-    def list_published( self, **kwargs ):
+    def list_published( self, filters=None, **kwargs ):
         """
         Return a list of all published items.
         """
-        query = self._query_published( **kwargs )
-        return self.list( query=query, **kwargs )
+        published_filter = self.model_class.published == true()
+        filters = self._munge_filters( published_filter, filters )
+        return self.list( filters=filters, **kwargs )
 
     # .... user sharing
     # sharing is often done via a 3rd table btwn a User and an item -> a <Item>UserShareAssociation
@@ -382,9 +383,9 @@ class SharableModelDeserializer( base.ModelDeserializer,
             return val
 
         if val:
-            self.manager.publish( item, flush=False )
+            self.manager().publish( item, flush=False )
         else:
-            self.manager.unpublish( item, flush=False )
+            self.manager().unpublish( item, flush=False )
         return item.published
 
     def deserialize_importable( self, item, key, val, **context ):
@@ -395,9 +396,9 @@ class SharableModelDeserializer( base.ModelDeserializer,
             return val
 
         if val:
-            self.manager.make_importable( item, flush=False )
+            self.manager().make_importable( item, flush=False )
         else:
-            self.manager.make_non_importable( item, flush=False )
+            self.manager().make_non_importable( item, flush=False )
         return item.published
 
     # def deserialize_slug( self, item, val, **context ):
@@ -410,13 +411,13 @@ class SharableModelDeserializer( base.ModelDeserializer,
 
 
 class SharableModelFilters( base.ModelFilterParser,
-                            taggable.TaggableFilterMixin,
-                            annotatable.AnnotatableFilterMixin ):
+        taggable.TaggableFilterMixin, annotatable.AnnotatableFilterMixin, ratable.RatableFilterMixin ):
 
     def _add_parsers( self ):
         super( SharableModelFilters, self )._add_parsers()
         taggable.TaggableFilterMixin._add_parsers( self )
         annotatable.AnnotatableFilterMixin._add_parsers( self )
+        ratable.RatableFilterMixin._add_parsers( self )
 
         self.orm_filter_parsers.update({
             'importable'    : { 'op': ( 'eq' ), 'val': self.parse_bool },
diff --git a/lib/galaxy/managers/visualizations.py b/lib/galaxy/managers/visualizations.py
index 825caa229f2..1f52211dad2 100644
--- a/lib/galaxy/managers/visualizations.py
+++ b/lib/galaxy/managers/visualizations.py
@@ -67,7 +67,7 @@ class VisualizationDeserializer( sharable.SharableModelDeserializer ):
 
     def __init__( self, app ):
         super( VisualizationDeserializer, self ).__init__( app )
-        self.visualization_manager = self.manager
+        self.visualization_manager = self.manager()
 
     def add_deserializers( self ):
         super( VisualizationDeserializer, self ).add_deserializers()
diff --git a/lib/galaxy/webapps/galaxy/api/histories.py b/lib/galaxy/webapps/galaxy/api/histories.py
index 08648e0c857..df550fa8629 100644
--- a/lib/galaxy/webapps/galaxy/api/histories.py
+++ b/lib/galaxy/webapps/galaxy/api/histories.py
@@ -419,7 +419,7 @@ class HistoriesController( BaseAPIController, ExportsHistoryMixin, ImportsHistor
 
         self.history_deserializer.deserialize( history, payload, user=trans.user, trans=trans )
         return self.history_serializer.serialize_to_view( history,
-                                                          user=trans.user, trans=trans, **self._parse_serialization_params( kwd, 'detailed' ) )
+            user=trans.user, trans=trans, **self._parse_serialization_params( kwd, 'detailed' ) )
 
     @expose_api
     def archive_export( self, trans, id, **kwds ):

From 4d75d1c6418a363f56700b2f3afd9c32c6d60516 Mon Sep 17 00:00:00 2001
From: carlfeberhard <carlfeberhard@gmail.com>
Date: Fri, 29 Jan 2016 13:11:58 -0500
Subject: [PATCH 2/4] Managers, ratable: clean up, test, and fix

---
 lib/galaxy/managers/base.py               |  1 -
 lib/galaxy/managers/ratable.py            | 30 ++++---
 test/unit/managers/test_HistoryManager.py | 98 ++++++++++++++++++++---
 3 files changed, 107 insertions(+), 22 deletions(-)

diff --git a/lib/galaxy/managers/base.py b/lib/galaxy/managers/base.py
index 6eee35a96f5..ed33fd49476 100644
--- a/lib/galaxy/managers/base.py
+++ b/lib/galaxy/managers/base.py
@@ -24,7 +24,6 @@ attribute change to a model object.
 #   such as: a single flat class, serializers being singletons in the manager, etc.
 #   instead of the three separate classes. With no 'apparent' perfect scheme
 #   I'm opting to just keep them separate.
-
 import sqlalchemy
 import routes
 
diff --git a/lib/galaxy/managers/ratable.py b/lib/galaxy/managers/ratable.py
index fb3f28da86a..f47ddeb2fa8 100644
--- a/lib/galaxy/managers/ratable.py
+++ b/lib/galaxy/managers/ratable.py
@@ -14,22 +14,27 @@ class RatableManagerMixin( object ):
     #: class of RatingAssociation (e.g. HistoryRatingAssociation)
     rating_assoc = None
 
-    def rating( self, item, user ):
-        """Returns the integer rating given to this item by the user."""
-        rating = self.query_associated( self.rating_assoc, item ).filter_by( user=user ).first()
-        if rating is not None:
-            # get the value if there's a rating
-            rating = rating.rating
-        return rating
+    def rating( self, item, user, as_int=True ):
+        """Returns the integer rating given to this item by the user.
 
-    def ratings( self, item, user ):
+        Returns the full rating model if `as_int` is False.
+        """
+        rating = self.query_associated( self.rating_assoc, item ).filter_by( user=user ).first()
+        # most common case is assumed to be 'get the number'
+        if not as_int:
+            return rating
+        # get the int value if there's a rating
+        return rating.rating if rating is not None else None
+
+    def ratings( self, item ):
         """Returns all ratings given to this item."""
         return [ r.rating for r in item.ratings ]
 
     def ratings_avg( self, item ):
         """Returns the average of all ratings given to this item."""
         foreign_key = self._foreign_key( self.rating_assoc )
-        return self.session().query( func.avg( self.rating_assoc.rating ) ).filter( foreign_key == item ).scalar()
+        avg = self.session().query( func.avg( self.rating_assoc.rating ) ).filter( foreign_key == item ).scalar()
+        return avg or 0.0
 
     def ratings_count( self, item ):
         """Returns the average of all ratings given to this item."""
@@ -40,14 +45,15 @@ class RatableManagerMixin( object ):
         """Updates or creates a rating for this item and user. Returns the rating"""
         # TODO?: possible generic update_or_create
         # TODO?: update and create to RatingsManager (if not overkill)
-        rating = self.rating( item, user )
+        rating = self.rating( item, user, as_int=False )
         if not rating:
-            rating = self.rating_assoc( item=item, user=user )
+            rating = self.rating_assoc( user=user )
+            self.associate( rating, item )
         rating.rating = value
 
         self.session().add( rating )
         if flush:
-            self.flush()
+            self.session().flush()
         return rating
 
     # TODO?: all ratings for a user
diff --git a/test/unit/managers/test_HistoryManager.py b/test/unit/managers/test_HistoryManager.py
index 320fbb8e5f0..b98ec9f6809 100644
--- a/test/unit/managers/test_HistoryManager.py
+++ b/test/unit/managers/test_HistoryManager.py
@@ -17,8 +17,10 @@ from galaxy import exceptions
 
 from base import BaseTestCase
 
+from galaxy.managers import base
 from galaxy.managers.histories import HistoryManager
 from galaxy.managers.histories import HistorySerializer
+from galaxy.managers.histories import HistoryDeserializer
 from galaxy.managers.histories import HistoryFilters
 from galaxy.managers import hdas
 
@@ -334,6 +336,40 @@ class HistoryManagerTestCase( BaseTestCase ):
         self.assertEqual( self.history_manager.set_current_by_id( self.trans, history1.id ), history1 )
         self.assertEqual( self.history_manager.get_current( self.trans ), history1 )
 
+    def test_rating( self ):
+        user2 = self.user_manager.create( **user2_data )
+        manager = self.history_manager
+        item = manager.create( name='history1', user=user2 )
+
+        self.log( "should properly handle no ratings" )
+        self.assertEqual( manager.rating( item, user2 ), None )
+        self.assertEqual( manager.ratings( item ), [] )
+        self.assertEqual( manager.ratings_avg( item ), 0 )
+        self.assertEqual( manager.ratings_count( item ), 0 )
+
+        self.log( "should allow rating by user" )
+        manager.rate( item, user2, 5 )
+        self.assertEqual( manager.rating( item, user2 ), 5 )
+        self.assertEqual( manager.ratings( item ), [ 5 ] )
+        self.assertEqual( manager.ratings_avg( item ), 5 )
+        self.assertEqual( manager.ratings_count( item ), 1 )
+
+        self.log( "should allow updating" )
+        manager.rate( item, user2, 4 )
+        self.assertEqual( manager.rating( item, user2 ), 4 )
+        self.assertEqual( manager.ratings( item ), [ 4 ] )
+        self.assertEqual( manager.ratings_avg( item ), 4 )
+        self.assertEqual( manager.ratings_count( item ), 1 )
+
+        self.log( "should reflect multiple reviews" )
+        user3 = self.user_manager.create( **user3_data )
+        self.assertEqual( manager.rating( item, user3 ), None )
+        manager.rate( item, user3, 1 )
+        self.assertEqual( manager.rating( item, user3 ), 1 )
+        self.assertEqual( manager.ratings( item ), [ 4, 1 ] )
+        self.assertEqual( manager.ratings_avg( item ), 2.5 )
+        self.assertEqual( manager.ratings_count( item ), 2 )
+
 
 # =============================================================================
 # web.url_for doesn't work well in the framework
@@ -426,7 +462,7 @@ class HistorySerializerTestCase( BaseTestCase ):
         user2 = self.user_manager.create( **user2_data )
         history1 = self.history_manager.create( name='history1', user=user2 )
         all_keys = list( self.history_serializer.serializable_keyset )
-        serialized = self.history_serializer.serialize( history1, all_keys )
+        serialized = self.history_serializer.serialize( history1, all_keys, user=user2 )
 
         self.log( 'everything serialized should be of the proper type' )
         self.assertIsInstance( serialized[ 'size' ], int )
@@ -507,17 +543,57 @@ class HistorySerializerTestCase( BaseTestCase ):
         self.log( 'serialized should jsonify well' )
         self.assertIsJsonifyable( serialized )
 
+    def test_ratings( self ):
+        user2 = self.user_manager.create( **user2_data )
+        user3 = self.user_manager.create( **user3_data )
+        manager = self.history_manager
+        serializer = self.history_serializer
+        item = manager.create( name='history1', user=user2 )
 
-# # =============================================================================
-# class HistoryDeserializerTestCase( BaseTestCase ):
+        self.log( 'serialization should reflect no ratings' )
+        serialized = serializer.serialize( item, [ 'user_rating', 'community_rating' ], user=user2 )
+        self.assertEqual( serialized[ 'user_rating' ], None )
+        self.assertEqual( serialized[ 'community_rating' ][ 'count' ], 0 )
+        self.assertEqual( serialized[ 'community_rating' ][ 'average' ], 0.0 )
 
-#     def set_up_managers( self ):
-#         super( HistoryDeserializerTestCase, self ).set_up_managers()
-#         self.history_manager = HistoryManager( self.app )
-#         self.history_deserializer = HistoryDeserializer( self.app )
+        self.log( 'serialization should reflect ratings' )
+        manager.rate( item, user2, 1 )
+        manager.rate( item, user3, 4 )
+        serialized = serializer.serialize( item, [ 'user_rating', 'community_rating' ], user=user2 )
+        self.assertEqual( serialized[ 'user_rating' ], 1 )
+        self.assertEqual( serialized[ 'community_rating' ][ 'count' ], 2 )
+        self.assertEqual( serialized[ 'community_rating' ][ 'average' ], 2.5 )
+        self.assertIsJsonifyable( serialized )
 
-#     def test_base( self ):
-#         pass
+        self.log( 'serialization of user_rating without user should error' )
+        self.assertRaises( base.ModelSerializingError,
+            serializer.serialize, item, [ 'user_rating' ] )
+
+
+# =============================================================================
+class HistoryDeserializerTestCase( BaseTestCase ):
+
+    def set_up_managers( self ):
+        super( HistoryDeserializerTestCase, self ).set_up_managers()
+        self.history_manager = HistoryManager( self.app )
+        self.history_deserializer = HistoryDeserializer( self.app )
+
+    def test_ratings( self ):
+        user2 = self.user_manager.create( **user2_data )
+        manager = self.history_manager
+        deserializer = self.history_deserializer
+        item = manager.create( name='history1', user=user2 )
+
+        self.log( 'deserialization should allow ratings change' )
+        deserializer.deserialize( item, { 'user_rating' : 4 }, user=user2 )
+        self.assertEqual( manager.rating( item, user2 ), 4 )
+        self.assertEqual( manager.ratings( item ), [ 4 ] )
+        self.assertEqual( manager.ratings_avg( item ), 4 )
+        self.assertEqual( manager.ratings_count( item ), 1 )
+
+        self.log( 'deserialization should fail silently on community_rating' )
+        deserializer.deserialize( item, { 'community_rating' : 4 }, user=user2 )
+        self.assertEqual( manager.ratings_count( item ), 1 )
 
 
 # =============================================================================
@@ -752,6 +828,10 @@ class HistoryFiltersTestCase( BaseTestCase ):
         found = self.history_manager.list( filters=filters, offset=-1 )
         self.assertEqual( found, deleted_and_annotated )
 
+    # TODO: eq, ge, le
+    # def test_ratings( self ):
+    #     pass
+
 
 # =============================================================================
 if __name__ == '__main__':

From 05a3d04b60bf9512a2fd9ebe07bc978a295e6865 Mon Sep 17 00:00:00 2001
From: carlfeberhard <carlfeberhard@gmail.com>
Date: Mon, 8 Feb 2016 10:30:25 -0500
Subject: [PATCH 3/4] Managers: extract manager() code and move into class with
 property accessor

---
 lib/galaxy/managers/annotatable.py    |  2 +-
 lib/galaxy/managers/base.py           | 75 +++++++++++++--------------
 lib/galaxy/managers/datasets.py       |  4 +-
 lib/galaxy/managers/deletable.py      |  6 +--
 lib/galaxy/managers/hdas.py           |  2 +-
 lib/galaxy/managers/histories.py      |  4 +-
 lib/galaxy/managers/pages.py          |  2 +-
 lib/galaxy/managers/ratable.py        | 16 +++---
 lib/galaxy/managers/sharable.py       |  8 +--
 lib/galaxy/managers/visualizations.py |  2 +-
 10 files changed, 59 insertions(+), 62 deletions(-)

diff --git a/lib/galaxy/managers/annotatable.py b/lib/galaxy/managers/annotatable.py
index 7e2b74d9b17..0b25a947db9 100644
--- a/lib/galaxy/managers/annotatable.py
+++ b/lib/galaxy/managers/annotatable.py
@@ -71,7 +71,7 @@ class AnnotatableDeserializerMixin( object ):
         if `val` is None.
         """
         val = self.validate.nullable_basestring( key, val )
-        return self.manager().annotate( item, val, user=user, flush=False )
+        return self.manager.annotate( item, val, user=user, flush=False )
 
 
 # TODO: I'm not entirely convinced this (or tags) are a good idea for filters since they involve a/the user
diff --git a/lib/galaxy/managers/base.py b/lib/galaxy/managers/base.py
index ed33fd49476..9304f9cafed 100644
--- a/lib/galaxy/managers/base.py
+++ b/lib/galaxy/managers/base.py
@@ -493,6 +493,31 @@ class ModelManager( object ):
     #    return item
 
 
+# ---- code for classes that use one *main* model manager
+# TODO: this may become unecessary if we can access managers some other way (class var, app, etc.)
+class HasAModelManager( object ):
+    """
+    Mixin used where serializers, deserializers, filter parsers, etc.
+    need some functionality around the model they're mainly concerned with
+    and would perform that functionality with a manager.
+    """
+
+    #: the class used to create this serializer's generically accessible model_manager
+    model_manager_class = None
+
+    def __init__( self, app, manager=None, **kwargs ):
+        self._manager = manager
+
+    @property
+    def manager( self ):
+        """Return an appropriate manager if it exists, instantiate if not."""
+        # PRECONDITION: assumes self.app is assigned elsewhere
+        if not self._manager:
+            # TODO: pass this serializer to it
+            self._manager = self.model_manager_class( self.app )
+        return self._manager
+
+
 # ==== SERIALIZERS/to_dict,from_dict
 class ModelSerializingError( exceptions.InternalServerError ):
     """Thrown when request model values can't be serialized"""
@@ -514,7 +539,7 @@ class SkipAttribute( Exception ):
     pass
 
 
-class ModelSerializer( object ):
+class ModelSerializer( HasAModelManager ):
     """
     Turns models into JSONable dicts.
 
@@ -531,17 +556,15 @@ class ModelSerializer( object ):
         keys_to_serialize = [ 'id', 'name', 'attr1', 'attr2', ... ]
         item_dict = MySerializer.serialize( my_item, keys_to_serialize )
     """
-    #: the class used to create this serializer's generically accessible model_manager
-    model_manager_class = None
     #: 'service' to use for getting urls - use class var to allow overriding when testing
     url_for = staticmethod( routes.url_for )
 
-    def __init__( self, app, manager=None ):
+    def __init__( self, app, **kwargs ):
         """
         Set up serializer map, any additional serializable keys, and views here.
         """
+        super( ModelSerializer, self ).__init__( app, **kwargs )
         self.app = app
-        self._manager = manager
 
         # a list of valid serializable keys that can use the default (string) serializer
         #   this allows us to: 'mention' the key without adding the default serializer
@@ -560,13 +583,6 @@ class ModelSerializer( object ):
         self.views = {}
         self.default_view = None
 
-    def manager( self ):
-        """Return an appropriate manager if it exists, instantiate if not."""
-        if not self._manager:
-            # TODO: pass this serializer to it
-            self._manager = self.model_manager_class( self.app )
-        return self._manager
-
     def add_serializers( self ):
         """
         Register a map of attribute keys -> serializing functions that will serialize
@@ -692,22 +708,19 @@ class ModelSerializer( object ):
         return self.views[ view ][:]
 
 
-class ModelDeserializer( object ):
+class ModelDeserializer( HasAModelManager ):
     """
     An object that converts an incoming serialized dict into values that can be
     directly assigned to an item's attributes and assigns them.
     """
-    #: the class used to create this deserializer's generically accessible model_manager
-    model_manager_class = None
-
     # TODO:?? a larger question is: which should be first? Deserialize then validate - or - validate then deserialize?
 
-    def __init__( self, app, manager=None ):
+    def __init__( self, app, **kwargs ):
         """
         Set up deserializers and validator.
         """
+        super( ModelDeserializer, self ).__init__( app, **kwargs )
         self.app = app
-        self._manager = None
 
         self.deserializers = {}
         self.deserializable_keyset = set([])
@@ -715,13 +728,6 @@ class ModelDeserializer( object ):
         # a sub object that can validate incoming values
         self.validate = ModelValidator( self.app )
 
-    def manager( self ):
-        """Return an appropriate manager if it exists, instantiate if not."""
-        if not self._manager:
-            # TODO: pass this deserializer to it
-            self._manager = self.model_manager_class( self.app )
-        return self._manager
-
     def add_deserializers( self ):
         """
         Register a map of attribute keys -> functions that will deserialize data
@@ -787,7 +793,7 @@ class ModelDeserializer( object ):
         return self.default_deserializer( item, key, val, **context )
 
 
-class ModelValidator( object ):
+class ModelValidator( HasAModelManager ):
     """
     An object that inspects a dictionary (generally meant to be a set of
     new/updated values for the model) and raises an error if a value is
@@ -795,6 +801,7 @@ class ModelValidator( object ):
     """
 
     def __init__( self, app, *args, **kwargs ):
+        super( ModelValidator, self ).__init__( app, **kwargs )
         self.app = app
 
     def type( self, key, val, types ):
@@ -870,7 +877,7 @@ class ModelValidator( object ):
 
 
 # ==== Building query filters based on model data
-class ModelFilterParser( object ):
+class ModelFilterParser( HasAModelManager ):
     """
     Converts string tuples (partially converted query string params) of
     attr, op, val into either:
@@ -889,9 +896,6 @@ class ModelFilterParser( object ):
     These might be safely be replaced in the future by creating SQLAlchemy
     hybrid properties or more thoroughly mapping derived values.
     """
-    #: the class used to create this deserializer's generically accessible model_manager
-    model_manager_class = None
-
     # ??: this class kindof 'lives' in both the world of the controllers/param-parsing and to models/orm
     # (as the model informs how the filter params are parsed)
     # I have no great idea where this 'belongs', so it's here for now
@@ -899,12 +903,12 @@ class ModelFilterParser( object ):
     #: model class
     model_class = None
 
-    def __init__( self, app, manager=None ):
+    def __init__( self, app, **kwargs ):
         """
         Set up serializer map, any additional serializable keys, and views here.
         """
+        super( ModelFilterParser, self ).__init__( app, **kwargs )
         self.app = app
-        self._manager = manager
 
         # dictionary containing parsing data for ORM/SQLAlchemy-based filters
         # ..note: although kind of a pain in the ass and verbose, opt-in/whitelisting allows more control
@@ -917,13 +921,6 @@ class ModelFilterParser( object ):
         # set up both of the above
         self._add_parsers()
 
-    def manager( self ):
-        """Return an appropriate manager if it exists, instantiate if not."""
-        if not self._manager:
-            # TODO: pass this parser to it
-            self._manager = self.model_manager_class( self.app )
-        return self._manager
-
     def _add_parsers( self ):
         """
         Set up, extend, or alter `orm_filter_parsers` and `fn_filter_parsers`.
diff --git a/lib/galaxy/managers/datasets.py b/lib/galaxy/managers/datasets.py
index 4339b782df2..44e5dbe4b21 100644
--- a/lib/galaxy/managers/datasets.py
+++ b/lib/galaxy/managers/datasets.py
@@ -234,11 +234,11 @@ class DatasetDeserializer( base.ModelDeserializer, deletable.PurgableDeserialize
         `permissions` dictionary, where `permissions` is in the form:
             { 'manage': [ <role id 1>, ... ], 'access': [ <role id 2>, ... ] }
         """
-        self.manager().permissions.manage.error_unless_permitted( dataset, user )
+        self.manager.permissions.manage.error_unless_permitted( dataset, user )
         self._validate_permissions( permissions, **context )
         manage = self._list_of_roles_from_ids( permissions[ 'manage' ] )
         access = self._list_of_roles_from_ids( permissions[ 'access' ] )
-        self.manager().permissions.set( dataset, manage, access, flush=False )
+        self.manager.permissions.set( dataset, manage, access, flush=False )
         return permissions
 
     def _validate_permissions( self, permissions, **context ):
diff --git a/lib/galaxy/managers/deletable.py b/lib/galaxy/managers/deletable.py
index 9351a35305e..dc55eea5baf 100644
--- a/lib/galaxy/managers/deletable.py
+++ b/lib/galaxy/managers/deletable.py
@@ -52,9 +52,9 @@ class DeletableDeserializerMixin( object ):
             return item.deleted
         # TODO:?? flush=False?
         if new_deleted:
-            self.manager().delete( item, flush=False )
+            self.manager.delete( item, flush=False )
         else:
-            self.manager().undelete( item, flush=False )
+            self.manager.undelete( item, flush=False )
         return item.deleted
 
 
@@ -103,7 +103,7 @@ class PurgableDeserializerMixin( DeletableDeserializerMixin ):
             return item.purged
         # do we want to error if something attempts to 'unpurge'?
         if new_purged:
-            self.manager().purge( item, flush=False )
+            self.manager.purge( item, flush=False )
         return item.purged
 
 
diff --git a/lib/galaxy/managers/hdas.py b/lib/galaxy/managers/hdas.py
index 028af4d063c..19f24159a14 100644
--- a/lib/galaxy/managers/hdas.py
+++ b/lib/galaxy/managers/hdas.py
@@ -443,7 +443,7 @@ class HDADeserializer( datasets.DatasetAssociationDeserializer,
 
     def __init__( self, app ):
         super( HDADeserializer, self ).__init__( app )
-        self.hda_manager = self.manager()
+        self.hda_manager = self.manager
 
     def add_deserializers( self ):
         super( HDADeserializer, self ).add_deserializers()
diff --git a/lib/galaxy/managers/histories.py b/lib/galaxy/managers/histories.py
index 4c50543d398..bb98f634a7e 100644
--- a/lib/galaxy/managers/histories.py
+++ b/lib/galaxy/managers/histories.py
@@ -158,7 +158,7 @@ class HistorySerializer( sharable.SharableModelSerializer, deletable.PurgableSer
     def __init__( self, app, **kwargs ):
         super( HistorySerializer, self ).__init__( app, **kwargs )
 
-        self.history_manager = self.manager()
+        self.history_manager = self.manager
         self.hda_manager = hdas.HDAManager( app )
         self.hda_serializer = hdas.HDASerializer( app )
 
@@ -328,7 +328,7 @@ class HistoryDeserializer( sharable.SharableModelDeserializer, deletable.Purgabl
 
     def __init__( self, app ):
         super( HistoryDeserializer, self ).__init__( app )
-        self.history_manager = self.manager()
+        self.history_manager = self.manager
 
     def add_deserializers( self ):
         super( HistoryDeserializer, self ).add_deserializers()
diff --git a/lib/galaxy/managers/pages.py b/lib/galaxy/managers/pages.py
index 85961eb7278..373007f581e 100644
--- a/lib/galaxy/managers/pages.py
+++ b/lib/galaxy/managers/pages.py
@@ -65,7 +65,7 @@ class PageDeserializer( sharable.SharableModelDeserializer ):
 
     def __init__( self, app ):
         super( PageDeserializer, self ).__init__( app )
-        self.page_manager = self.manager()
+        self.page_manager = self.manager
 
     def add_deserializers( self ):
         super( PageDeserializer, self ).add_deserializers()
diff --git a/lib/galaxy/managers/ratable.py b/lib/galaxy/managers/ratable.py
index f47ddeb2fa8..217029ec577 100644
--- a/lib/galaxy/managers/ratable.py
+++ b/lib/galaxy/managers/ratable.py
@@ -27,7 +27,7 @@ class RatableManagerMixin( object ):
         return rating.rating if rating is not None else None
 
     def ratings( self, item ):
-        """Returns all ratings given to this item."""
+        """Returns a list of all rating values given to this item."""
         return [ r.rating for r in item.ratings ]
 
     def ratings_avg( self, item ):
@@ -37,7 +37,7 @@ class RatableManagerMixin( object ):
         return avg or 0.0
 
     def ratings_count( self, item ):
-        """Returns the average of all ratings given to this item."""
+        """Returns the number of ratings given to this item."""
         foreign_key = self._foreign_key( self.rating_assoc )
         return self.session().query( func.count( self.rating_assoc.rating ) ).filter( foreign_key == item ).scalar()
 
@@ -69,8 +69,8 @@ class RatableSerializerMixin( object ):
         """Returns the integer rating given to this item by the user."""
         if not user:
             raise base.ModelSerializingError( 'user_rating requires a user',
-                model_class=self.manager().model_class, id=self.serialize_id( item, 'id' ) )
-        return self.manager().rating( item, user )
+                model_class=self.manager.model_class, id=self.serialize_id( item, 'id' ) )
+        return self.manager.rating( item, user )
 
     def serialize_community_rating( self, item, key, **context ):
         """
@@ -80,7 +80,7 @@ class RatableSerializerMixin( object ):
         """
         # ??: seems like two queries (albeit in-sql functions) would slower
         # than getting the rows and calc'ing both here with one query
-        manager = self.manager()
+        manager = self.manager
         return {
             'average' : manager.ratings_avg( item ),
             'count'   : manager.ratings_count( item ),
@@ -95,15 +95,15 @@ class RatableDeserializerMixin( object ):
     def deserialize_rating( self, item, key, val, user=None, **context ):
         if not user:
             raise base.ModelDeserializingError( 'user_rating requires a user',
-                model_class=self.manager().model_class, id=self.serialize_id( item, 'id' ) )
+                model_class=self.manager.model_class, id=self.serialize_id( item, 'id' ) )
         val = self.validate.int_range( key, val, 0, 5 )
-        return self.manager().rate( item, user, val, flush=False )
+        return self.manager.rate( item, user, val, flush=False )
 
 
 class RatableFilterMixin( object ):
 
     def _ratings_avg_accessor( self, item ):
-        return self.manager().ratings_avg( item )
+        return self.manager.ratings_avg( item )
 
     def _add_parsers( self ):
         """
diff --git a/lib/galaxy/managers/sharable.py b/lib/galaxy/managers/sharable.py
index f81bec631e5..5826202ebf0 100644
--- a/lib/galaxy/managers/sharable.py
+++ b/lib/galaxy/managers/sharable.py
@@ -383,9 +383,9 @@ class SharableModelDeserializer( base.ModelDeserializer,
             return val
 
         if val:
-            self.manager().publish( item, flush=False )
+            self.manager.publish( item, flush=False )
         else:
-            self.manager().unpublish( item, flush=False )
+            self.manager.unpublish( item, flush=False )
         return item.published
 
     def deserialize_importable( self, item, key, val, **context ):
@@ -396,9 +396,9 @@ class SharableModelDeserializer( base.ModelDeserializer,
             return val
 
         if val:
-            self.manager().make_importable( item, flush=False )
+            self.manager.make_importable( item, flush=False )
         else:
-            self.manager().make_non_importable( item, flush=False )
+            self.manager.make_non_importable( item, flush=False )
         return item.published
 
     # def deserialize_slug( self, item, val, **context ):
diff --git a/lib/galaxy/managers/visualizations.py b/lib/galaxy/managers/visualizations.py
index 1f52211dad2..825caa229f2 100644
--- a/lib/galaxy/managers/visualizations.py
+++ b/lib/galaxy/managers/visualizations.py
@@ -67,7 +67,7 @@ class VisualizationDeserializer( sharable.SharableModelDeserializer ):
 
     def __init__( self, app ):
         super( VisualizationDeserializer, self ).__init__( app )
-        self.visualization_manager = self.manager()
+        self.visualization_manager = self.manager
 
     def add_deserializers( self ):
         super( VisualizationDeserializer, self ).add_deserializers()

From d07677bba16d535a67a2e4517f29ab28a8bb6c04 Mon Sep 17 00:00:00 2001
From: carlfeberhard <carlfeberhard@gmail.com>
Date: Mon, 8 Feb 2016 11:25:32 -0500
Subject: [PATCH 4/4] Managers: bit of cleanup for 05a3d04b

---
 lib/galaxy/managers/base.py           | 3 +++
 lib/galaxy/managers/configuration.py  | 3 ++-
 lib/galaxy/managers/datasets.py       | 5 ++++-
 lib/galaxy/managers/hdas.py           | 6 +++---
 lib/galaxy/managers/users.py          | 5 ++++-
 lib/galaxy/managers/visualizations.py | 3 ++-
 6 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/lib/galaxy/managers/base.py b/lib/galaxy/managers/base.py
index 9304f9cafed..613890a77ac 100644
--- a/lib/galaxy/managers/base.py
+++ b/lib/galaxy/managers/base.py
@@ -504,6 +504,8 @@ class HasAModelManager( object ):
 
     #: the class used to create this serializer's generically accessible model_manager
     model_manager_class = None
+    # examples where this doesn't really work are ConfigurationSerializer (no manager)
+    # and contents (2 managers)
 
     def __init__( self, app, manager=None, **kwargs ):
         self._manager = manager
@@ -515,6 +517,7 @@ class HasAModelManager( object ):
         if not self._manager:
             # TODO: pass this serializer to it
             self._manager = self.model_manager_class( self.app )
+            # this will error for unset model_manager_class'es
         return self._manager
 
 
diff --git a/lib/galaxy/managers/configuration.py b/lib/galaxy/managers/configuration.py
index 80d3a1f7742..49bcca7e257 100644
--- a/lib/galaxy/managers/configuration.py
+++ b/lib/galaxy/managers/configuration.py
@@ -16,6 +16,7 @@ log = logging.getLogger( __name__ )
 
 # TODO: for lack of a manager file for the config. May well be better in config.py? Circ imports?
 class ConfigSerializer( base.ModelSerializer ):
+    """Configuration (galaxy.ini) settings viewable by all users"""
 
     def __init__( self, app ):
         super( ConfigSerializer, self ).__init__( app )
@@ -75,7 +76,7 @@ class ConfigSerializer( base.ModelSerializer ):
 
 
 class AdminConfigSerializer( ConfigSerializer ):
-    # config attributes viewable by admin users
+    """Configuration attributes viewable only by admin users"""
 
     def add_serializers( self ):
         super( AdminConfigSerializer, self ).add_serializers()
diff --git a/lib/galaxy/managers/datasets.py b/lib/galaxy/managers/datasets.py
index 44e5dbe4b21..072c94c73dd 100644
--- a/lib/galaxy/managers/datasets.py
+++ b/lib/galaxy/managers/datasets.py
@@ -133,10 +133,11 @@ class DatasetRBACPermissions( object ):
 
 
 class DatasetSerializer( base.ModelSerializer, deletable.PurgableSerializerMixin ):
+    model_manager_class = DatasetManager
 
     def __init__( self, app ):
         super( DatasetSerializer, self ).__init__( app )
-        self.dataset_manager = DatasetManager( app )
+        self.dataset_manager = self.manager
         # needed for admin test
         self.user_manager = users.UserManager( app )
 
@@ -274,6 +275,8 @@ class DatasetAssociationManager( base.ModelManager,
     # Instead, a dataset association HAS a dataset but contains metadata specific to a library (lda) or user (hda)
     model_class = model.DatasetInstance
 
+    # NOTE: model_manager_class should be set in HDA/LDA subclasses
+
     def __init__( self, app ):
         super( DatasetAssociationManager, self ).__init__( app )
         self.dataset_manager = DatasetManager( app )
diff --git a/lib/galaxy/managers/hdas.py b/lib/galaxy/managers/hdas.py
index 19f24159a14..927f01de97a 100644
--- a/lib/galaxy/managers/hdas.py
+++ b/lib/galaxy/managers/hdas.py
@@ -243,12 +243,11 @@ class HDASerializer(  # datasets._UnflattenedMetadataDatasetAssociationSerialize
         datasets.DatasetAssociationSerializer,
         taggable.TaggableSerializerMixin,
         annotatable.AnnotatableSerializerMixin ):
-    # TODO: inherit from datasets.DatasetAssociationSerializer
-    # TODO: move what makes sense into DatasetSerializer
+    model_manager_class = HDAManager
 
     def __init__( self, app ):
         super( HDASerializer, self ).__init__( app )
-        self.hda_manager = HDAManager( app )
+        self.hda_manager = self.manager
 
         self.default_view = 'summary'
         self.add_view( 'summary', [
@@ -463,6 +462,7 @@ class HDADeserializer( datasets.DatasetAssociationDeserializer,
 class HDAFilterParser( datasets.DatasetAssociationFilterParser,
                        taggable.TaggableFilterMixin,
                        annotatable.AnnotatableFilterMixin ):
+    model_manager_class = HDAManager
     model_class = model.HistoryDatasetAssociation
 
     def _add_parsers( self ):
diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py
index 1d52ae5cc09..9ebda079bb9 100644
--- a/lib/galaxy/managers/users.py
+++ b/lib/galaxy/managers/users.py
@@ -239,13 +239,14 @@ class UserManager( base.ModelManager, deletable.PurgableManagerMixin ):
 
 
 class UserSerializer( base.ModelSerializer, deletable.PurgableSerializerMixin ):
+    model_manager_class = UserManager
 
     def __init__( self, app ):
         """
         Convert a User and associated data to a dictionary representation.
         """
         super( UserSerializer, self ).__init__( app )
-        self.user_manager = UserManager( app )
+        self.user_manager = self.manager
 
         self.default_view = 'summary'
         self.add_view( 'summary', [
@@ -288,6 +289,7 @@ class UserSerializer( base.ModelSerializer, deletable.PurgableSerializerMixin ):
 
 
 class CurrentUserSerializer( UserSerializer ):
+    model_manager_class = UserManager
 
     def serialize( self, user, keys, **kwargs ):
         """
@@ -324,6 +326,7 @@ class CurrentUserSerializer( UserSerializer ):
 
 
 class AdminUserFilterParser( base.ModelFilterParser, deletable.PurgableFiltersMixin ):
+    model_manager_class = UserManager
     model_class = model.User
 
     def _add_parsers( self ):
diff --git a/lib/galaxy/managers/visualizations.py b/lib/galaxy/managers/visualizations.py
index 825caa229f2..34368fb9186 100644
--- a/lib/galaxy/managers/visualizations.py
+++ b/lib/galaxy/managers/visualizations.py
@@ -42,11 +42,12 @@ class VisualizationSerializer( sharable.SharableModelSerializer ):
     """
     Interface/service object for serializing visualizations into dictionaries.
     """
+    model_manager_class = VisualizationManager
     SINGLE_CHAR_ABBR = 'v'
 
     def __init__( self, app ):
         super( VisualizationSerializer, self ).__init__( app )
-        self.visualizations_manager = VisualizationManager( app )
+        self.visualization_manager = self.manager
 
         self.default_view = 'summary'
         self.add_view( 'summary', [] )