Merge pull request #123 from eoyilmaz/121-add-reviewversion-attribute

121 add reviewversion attribute

alembic/versions/a2007ad7f535_added_review_version_id_column.py

@@ -0,0 +1,33 @@
+"""Added Review.version_id column
+
+Revision ID: a2007ad7f535
+Revises: 91ed52b72b82
+Create Date: 2024-11-26 11:36:07.776169
+"""
+
+from alembic import op
+
+import sqlalchemy as sa
+
+
+# revision identifiers, used by Alembic.
+revision = "a2007ad7f535"
+down_revision = "91ed52b72b82"
+
+
+def upgrade():
+    """Upgrade the tables."""
+    op.add_column("Reviews", sa.Column("version_id", sa.Integer(), nullable=True))
+    op.create_foreign_key(
+        "Reviews_version_id_fkey",
+        "Reviews",
+        "Versions",
+        ["version_id"],
+        ["id"],
+    )
+
+
+def downgrade():
+    """Downgrade the tables."""
+    op.drop_constraint("Reviews_version_id_fkey", "Reviews", type_="foreignkey")
+    op.drop_column("Reviews", "version_id")

src/stalker/db/setup.py

@@ -36,7 +36,7 @@
 logger: logging.Logger = log.get_logger(__name__)
 
 # TODO: Try to get it from the API (it was not working inside a package before)
-alembic_version: str = "91ed52b72b82"
+alembic_version: str = "a2007ad7f535"
 
 
 def setup(settings: Optional[Dict[str, Any]] = None) -> None:

src/stalker/models/review.py

@@ -40,16 +40,30 @@ class Review(SimpleEntity, ScheduleMixin, StatusMixin):
     Each Review instance with the same :attr:`.review_number` for a
     :class:`.Task` represents a set of reviews.
 
-    Creating a review will automatically cap the schedule timing value of the
-    related task to the total logged time logs for that task and then extend
-    the timing values according to the review schedule values.
+    .. version-added:: 1.0.0
+
+      Review -> Version relation
+
+      Versions can now be attached to reviews.
+
+    Review instances, alongside the :class:`.Task` can also optionally hold a
+    :class:`.Version` instance. This allows the information of which
+    :class:`.Version` instance has been reviewed as a part of the review
+    process to be much cleaner, and when the Review history is investigated,
+    it will be much easier to identify which :class:`.Version` the review was
+    about.
 
     Args:
         task (Task): A :class:`.Task` instance that this review is related to.
-            It cannot be skipped.
+            It can be skipped if a :class:`.Version` instance has been given.
+
+        version (Version): A :class:`.Version` instance that this review
+            instance is related to. The :class:`.Version` and the
+            :class:`.Task` should be related, a ``ValueError`` will be raised
+            if they are not.
 
-        review_number (int): This number represents the revision set id that this
-            Review instance belongs to.
+        review_number (int): This number represents the revision set id that
+            this Review instance belongs to.
 
         reviewer (User): One of the responsible of the related Task. There will
             be only one Review instances with the same review_number for every
@@ -89,6 +103,16 @@ class Review(SimpleEntity, ScheduleMixin, StatusMixin):
         doc="The :class:`.Task` instance that this Review is created for",
     )
 
+    version_id: Mapped[Optional[int]] = mapped_column(
+        "version_id", ForeignKey("Versions.id")
+    )
+
+    version: Mapped[Optional["Version"]] = relationship(
+        primaryjoin="Reviews.c.version_id==Versions.c.id",
+        uselist=False,
+        back_populates="reviews",
+    )
+
     reviewer_id: Mapped[int] = mapped_column(
         ForeignKey("Users.id"),
         nullable=False,
@@ -105,6 +129,7 @@ class Review(SimpleEntity, ScheduleMixin, StatusMixin):
     def __init__(
         self,
         task: Optional["Task"] = None,
+        version: Optional["Version"] = None,
         reviewer: Optional["User"] = None,
         description: str = "",
         **kwargs: Dict[str, Any],
@@ -115,6 +140,7 @@ def __init__(
         StatusMixin.__init__(self, **kwargs)
 
         self.task = task
+        self.version = version
         self.reviewer = reviewer
 
         # set the status to NEW
@@ -132,15 +158,14 @@ def _validate_task(
         """Validate the given task value.
 
         Args:
-            key (str): The name of the validated column.
-            task (Task): The task value to be validated.
+            task (Union[None, Task]): The task value to be validated.
 
         Raises:
             TypeError: If the given task value is not a Task instance.
             ValueError: If the given task is not a leaf task.
 
         Returns:
-            Task: The validated Task instance.
+            Union[None, Task]: The validated Task instance.
         """
         if task is None:
             return task
@@ -165,6 +190,38 @@ def _validate_task(
 
         return task
 
+    @validates("version")
+    def _validate_version(
+        self, key: str, version: Union[None, "Version"]
+    ) -> Union[None, "Version"]:
+        """Validate the given version value.
+
+        Args:
+            key (str): The name of the validated column.
+            version (Union[None, Version]): The version value to be validated.
+        """
+        if version is None:
+            return version
+
+        from stalker.models.version import Version
+
+        if not isinstance(version, Version):
+            raise TypeError(
+                f"{self.__class__.__name__}.version should be a Version "
+                f"instance, not {version.__class__.__name__}: '{version}'"
+            )
+
+        if self.task is not None:
+            if version.task != self.task:
+                raise ValueError(
+                    f"{self.__class__.__name__}.version should be a Version "
+                    f"instance related to this Task: {version}"
+                )
+        else:
+            self.task = version.task
+
+        return version
+
     @validates("reviewer")
     def _validate_reviewer(self, key: str, reviewer: "User") -> "User":
         """Validate the given reviewer value.

src/stalker/models/studio.py

@@ -871,7 +871,9 @@ def _validate_working_hours_value(self, value: List) -> List:
         err = (
             "{}.working_hours value should be a list of lists of two "
             "integers and the range of integers should be between 0-1440, "
-            "not {}: '{}'".format(self.__class__.__name__, value.__class__.__name__, value)
+            "not {}: '{}'".format(
+                self.__class__.__name__, value.__class__.__name__, value
+            )
         )
 
         if not isinstance(value, list):

src/stalker/models/task.py

@@ -2578,8 +2578,8 @@ def create_time_log(
         This will ease creating TimeLog instances for task.
 
         Args:
-            resource (User): The :class:`stalker.models.auth.User` instance as the
-                resource for the TimeLog.
+            resource (User): The :class:`stalker.models.auth.User` instance as
+                the resource for the TimeLog.
             start (datetime.datetime): The start date and time.
             end (datetime.datetime): The end date and time.
 
@@ -2591,7 +2591,7 @@ def create_time_log(
         return TimeLog(task=self, resource=resource, start=start, end=end)
         # also updating parent statuses are done in TimeLog._validate_task
 
-    def request_review(self) -> List[Review]:
+    def request_review(self, version: Optional["Version"] = None) -> List[Review]:
         """Create and return Review instances for each of the responsible of this task.
 
         Also set the task status to PREV.
@@ -2601,15 +2601,20 @@ def request_review(self) -> List[Review]:
 
         Only applicable to leaf tasks.
 
+        Args:
+            version (Optional[Version]): An optional :class:`.Version` instance
+                can also be passed. The :class:`.Version` should be related to
+                this :class:`.Task`.
+
         Raises:
-            StatusError: If the current task status is not WIP a StatusError will be
-                raised as the task has either not been started on being worked yet,
-                it is already on review, a dependency might be under review or this is
-                stopped, hold or completed.
+            StatusError: If the current task status is not WIP a StatusError
+                will be raised as the task has either not been started on being
+                worked yet, it is already on review, a dependency might be
+                under review or this is stopped, hold or completed.
 
         Returns:
-            List[Review]: The list of :class:`stalker.models.review.Review` instances
-                created.
+            List[Review]: The list of :class:`stalker.models.review.Review`
+                instances created.
         """
         # check task status
         with DBSession.no_autoflush:
@@ -2627,7 +2632,7 @@ def request_review(self) -> List[Review]:
         # create Review instances for each Responsible of this task
         reviews = []
         for responsible in self.responsible:
-            reviews.append(Review(task=self, reviewer=responsible))
+            reviews.append(Review(task=self, version=version, reviewer=responsible))
 
         # update the status to PREV
         self.status = prev

src/stalker/models/variant.py

@@ -7,7 +7,6 @@
 from stalker.models.task import Task
 
 
-
 class Variant(Task):
     """A Task derivative to keep track of Variants in a Task hierarchy.
 
@@ -36,4 +35,4 @@ class Variant(Task):
         "id",
         ForeignKey("Tasks.id"),
         primary_key=True,
-    )
+    )

src/stalker/models/version.py

@@ -16,6 +16,7 @@
 from stalker.log import get_logger
 from stalker.models.link import Link
 from stalker.models.mixins import DAGMixin
+from stalker.models.review import Review
 from stalker.models.task import Task
 from stalker.utils import walk_hierarchy
 
@@ -138,6 +139,10 @@ class Version(Link, DAGMixin):
         """,
     )
 
+    reviews: Mapped[Optional[List[Review]]] = relationship(
+        primaryjoin="Reviews.c.version_id==Versions.c.id"
+    )
+
     is_published: Mapped[Optional[bool]] = mapped_column(default=False)
     created_with: Mapped[Optional[str]] = mapped_column(String(256))
 
@@ -619,6 +624,14 @@ def walk_inputs(self, method: int = 0) -> Generator[None, "Version", None]:
         for v in walk_hierarchy(self, "inputs", method=method):
             yield v
 
+    def request_review(self):
+        """Call the self.task.request_review().
+
+        This is a shortcut to the Task.request_review() method of the related
+        task.
+        """
+        return self.task.request_review(version=self)
+
 
 # VERSION INPUTS
 Version_Inputs = Table(

tests/db/test_db.py

@@ -271,11 +271,9 @@ def test_daily_status_list_initialization(setup_postgresql_db):
 
 def test_variant_status_list_initialization(setup_postgresql_db):
     """Variant statuses are correctly created."""
-    variant_status_list = (
-        StatusList.query
-        .filter(StatusList.target_entity_type == "Variant")
-        .first()
-    )
+    variant_status_list = StatusList.query.filter(
+        StatusList.target_entity_type == "Variant"
+    ).first()
     assert isinstance(variant_status_list, StatusList)
     assert variant_status_list.name == "Variant Statuses"
     expected_status_names = [
@@ -4271,8 +4269,7 @@ def test_persistence_of_review(setup_postgresql_db):
         start=datetime.datetime.now(pytz.utc) + datetime.timedelta(2),
         end=datetime.datetime.now(pytz.utc) + datetime.timedelta(3),
     )
-    rev1 = Review(task=task2, reviewer=user1, schedule_timing=1, schedule_unit="h")
-    DBSession.add_all(
+    DBSession.save(
         [
             task1,
             child_task1,
@@ -4283,10 +4280,20 @@ def test_persistence_of_review(setup_postgresql_db):
             time_log3,
             user1,
             user2,
-            rev1,
         ]
     )
-    DBSession.commit()
+
+    version1 = Version(task=task2)
+    DBSession.save(version1)
+
+    rev1 = Review(
+        task=task2,
+        reviewer=user1,
+        version=version1,
+        schedule_timing=1,
+        schedule_unit="h",
+    )
+    DBSession.save(rev1)
 
     created_by = rev1.created_by
     date_created = rev1.date_created
@@ -4296,6 +4303,7 @@ def test_persistence_of_review(setup_postgresql_db):
     schedule_unit = rev1.schedule_unit
     task = rev1.task
     updated_by = rev1.updated_by
+    version = rev1.version
 
     del rev1
 
@@ -4312,6 +4320,7 @@ def test_persistence_of_review(setup_postgresql_db):
     assert rev1_db.updated_by == updated_by
     assert rev1_db.schedule_timing == schedule_timing
     assert rev1_db.schedule_unit == schedule_unit
+    assert rev1_db.version == version
 
     # delete tests