From 050c5ebee83e2f009da8baccb64fb593d136d60d Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Tue, 15 Jun 2021 19:07:47 -0400 Subject: [PATCH] Simplify rating classes initialization (see note) 1. Remove id parameter from constructor (this is created by the db). 2. Remove `item` instance attribute: it's not used and is meaningless. 3. Make user and item parameters in the constructor required: a rating cannot exist without a rater or the item being rated. 4. For the same reason as in #2, make the `set_item()` method private and only call it from the constructor. 5. Simplify mapping test: just use the constructor. --- lib/galaxy/managers/ratable.py | 2 +- lib/galaxy/model/__init__.py | 22 +++++++++--------- lib/galaxy/model/item_attrs.py | 5 +---- test/unit/data/test_galaxy_mapping.py | 32 +++++++++++++-------------- 4 files changed, 28 insertions(+), 33 deletions(-) diff --git a/lib/galaxy/managers/ratable.py b/lib/galaxy/managers/ratable.py index b1bf4a08e674..b86c5505e3dc 100644 --- a/lib/galaxy/managers/ratable.py +++ b/lib/galaxy/managers/ratable.py @@ -49,7 +49,7 @@ def rate(self, item, user, value, flush=True): # TODO?: update and create to RatingsManager (if not overkill) rating = self.rating(item, user, as_int=False) if not rating: - rating = self.rating_assoc(user=user) + rating = self.rating_assoc(user, item) self.associate(rating, item) rating.rating = value diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index 9e44a9ad66b5..b9f160f36fa9 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -6874,48 +6874,48 @@ class LibraryDatasetCollectionAnnotationAssociation(RepresentById): # Item rating classes. class ItemRatingAssociation: - def __init__(self, id=None, user=None, item=None, rating=0): - self.id = id + def __init__(self, user, item, rating=0): self.user = user - self.item = item self.rating = rating + self._set_item(item) - def set_item(self, item): + def _set_item(self, item): """ Set association's item. """ + raise NotImplementedError() class HistoryRatingAssociation(ItemRatingAssociation, RepresentById): - def set_item(self, history): + def _set_item(self, history): self.history = history class HistoryDatasetAssociationRatingAssociation(ItemRatingAssociation, RepresentById): - def set_item(self, history_dataset_association): + def _set_item(self, history_dataset_association): self.history_dataset_association = history_dataset_association class StoredWorkflowRatingAssociation(ItemRatingAssociation, RepresentById): - def set_item(self, stored_workflow): + def _set_item(self, stored_workflow): self.stored_workflow = stored_workflow class PageRatingAssociation(ItemRatingAssociation, RepresentById): - def set_item(self, page): + def _set_item(self, page): self.page = page class VisualizationRatingAssociation(ItemRatingAssociation, RepresentById): - def set_item(self, visualization): + def _set_item(self, visualization): self.visualization = visualization class HistoryDatasetCollectionRatingAssociation(ItemRatingAssociation, RepresentById): - def set_item(self, dataset_collection): + def _set_item(self, dataset_collection): self.dataset_collection = dataset_collection class LibraryDatasetCollectionRatingAssociation(ItemRatingAssociation, RepresentById): - def set_item(self, dataset_collection): + def _set_item(self, dataset_collection): self.dataset_collection = dataset_collection diff --git a/lib/galaxy/model/item_attrs.py b/lib/galaxy/model/item_attrs.py index 473b3c77ae97..fde16e715154 100644 --- a/lib/galaxy/model/item_attrs.py +++ b/lib/galaxy/model/item_attrs.py @@ -43,10 +43,7 @@ def rate_item(self, db_session, user, item, rating, webapp_model=None): if not item_rating: # User has not yet rated item; create rating. item_rating_assoc_class = self._get_item_rating_assoc_class(item, webapp_model=webapp_model) - item_rating = item_rating_assoc_class() - item_rating.user = user - item_rating.set_item(item) - item_rating.rating = rating + item_rating = item_rating_assoc_class(user, item, rating) db_session.add(item_rating) db_session.flush() elif item_rating.rating != rating: diff --git a/test/unit/data/test_galaxy_mapping.py b/test/unit/data/test_galaxy_mapping.py index 430b333dc0aa..27b9b3a0a126 100644 --- a/test/unit/data/test_galaxy_mapping.py +++ b/test/unit/data/test_galaxy_mapping.py @@ -130,52 +130,50 @@ def persist_and_check_annotation(annotation_class, **kwds): def test_ratings(self): model = self.model - u = model.User(email="rater@example.com", password="password") + user_email = "rater@example.com" + u = model.User(email=user_email, password="password") self.persist(u) - def persist_and_check_rating(rating_class, **kwds): - rating_association = rating_class() - rating_association.rating = 5 - rating_association.user = u - for key, value in kwds.items(): - setattr(rating_association, key, value) + def persist_and_check_rating(rating_class, item): + rating = 5 + rating_association = rating_class(u, item, rating) self.persist(rating_association) self.expunge() - stored_annotation = self.query(rating_class).all()[0] - assert stored_annotation.rating == 5 - assert stored_annotation.user.email == "rater@example.com" + stored_rating = self.query(rating_class).all()[0] + assert stored_rating.rating == rating + assert stored_rating.user.email == user_email sw = model.StoredWorkflow() sw.user = u self.persist(sw) - persist_and_check_rating(model.StoredWorkflowRatingAssociation, stored_workflow=sw) + persist_and_check_rating(model.StoredWorkflowRatingAssociation, sw) h = model.History(name="History for Rating", user=u) self.persist(h) - persist_and_check_rating(model.HistoryRatingAssociation, history=h) + persist_and_check_rating(model.HistoryRatingAssociation, h) d1 = model.HistoryDatasetAssociation(extension="txt", history=h, create_dataset=True, sa_session=model.session) self.persist(d1) - persist_and_check_rating(model.HistoryDatasetAssociationRatingAssociation, hda=d1) + persist_and_check_rating(model.HistoryDatasetAssociationRatingAssociation, d1) page = model.Page() page.user = u self.persist(page) - persist_and_check_rating(model.PageRatingAssociation, page=page) + persist_and_check_rating(model.PageRatingAssociation, page) visualization = model.Visualization() visualization.user = u self.persist(visualization) - persist_and_check_rating(model.VisualizationRatingAssociation, visualization=visualization) + persist_and_check_rating(model.VisualizationRatingAssociation, visualization) dataset_collection = model.DatasetCollection(collection_type="paired") history_dataset_collection = model.HistoryDatasetCollectionAssociation(collection=dataset_collection) self.persist(history_dataset_collection) - persist_and_check_rating(model.HistoryDatasetCollectionRatingAssociation, history_dataset_collection=history_dataset_collection) + persist_and_check_rating(model.HistoryDatasetCollectionRatingAssociation, history_dataset_collection) library_dataset_collection = model.LibraryDatasetCollectionAssociation(collection=dataset_collection) self.persist(library_dataset_collection) - persist_and_check_rating(model.LibraryDatasetCollectionRatingAssociation, library_dataset_collection=library_dataset_collection) + persist_and_check_rating(model.LibraryDatasetCollectionRatingAssociation, library_dataset_collection) def test_display_name(self):