Code review comment for lp:~cjohnston/ubuntu-ci-services-itself/ts-ticket-models

On 12/10/2013 01:23 PM, Chris Johnston wrote:
> Chris Johnston has proposed merging lp:~cjohnston/ubuntu-ci-services-itself/ts-ticket-models into lp:ubuntu-ci-services-itself.

> === added file 'ticket_system/ticket/models.py'

> +class Ticket(models.Model):
> + class Meta:
> + db_table = 'ticket'
> +
> + WORKFLOW_STEPS = (
> + ("PACKAGE_BUILDING", "Package building"),
> + ("IMAGE_BUILDING", "Image building"),
> + ("IMAGE_TESTING", "Image testing"),
> + ("PACKAGE_PUBLISHING", "Package publishing"),
> + )
> + WORKFLOW_STEP_STATUSES = (
> + ("PKG_BUILD_WAITING", "Not started"),
> + ("PKG_BUILD_INPROGRESS", "In progress"),
> + ("PKG_BUILD_DONE", "Completed"),
> + ("PKG_BUILD_FAILED", "Failed"),
> + ("IMAGE_BUILD_WAITING", "Not started"),
> + ("IMAGE_BUILD_INPROGRESS", "In progress"),
> + ("IMAGE_BUILD_DONE", "Completed"),
> + ("IMAGE_BUILD_FAILED", "Failed"),
> + ("IMAGE_TESTS_WAITING", "Not started"),
> + ("IMAGE_TESTS_INPROGRESS", "In progress"),
> + ("IMAGE_TESTS_PASSED", "Passed"),
> + ("IMAGE_TESTS_FAILED", "Failed"),
> + ("PKG_PUBLISHING_DONE", "Completed"),
> + ("PKG_PUBLISHING_FAILED", "Failed"),
> + )

> + current_workflow_step = models.CharField(choices=WORKFLOW_STEPS,
> + max_length=4096)
> + status = models.CharField(choices=WORKFLOW_STEP_STATUSES,
> + max_length=4096)

Would it make sense to use models.IntegerField and change these values
from strings to integers. I think that would then allow you do
eventually have logic like:

  if ticket.status < Ticket.IMAGE_BUILD_DONE:
      print("No image available for ticket")

I'd define these status as multiples of 10 like:
   PKG_BUILD_WAITING = 0
   PKG_BUILD_INPROGRESS = 10

because I could forsee a day when we might need new steps in the process.

> +class SourcePackageUpload(models.Model):
> + class Meta:
> + db_table = 'sourcepackageupload'
> +
> + sourcepackage = models.ForeignKey(SourcePackage)
> + version = models.CharField(max_length=4096)
> +
> + def __unicode__(self):
> + return "{} {}".format(self.sourcepackage, self.version)
> +
> +
> +class SubTicket(models.Model):
> + class Meta:
> + db_table = 'subticket'
> +
> + WORKFLOW_STEPS = (
> + ("WAITING", "Not started"),
> + ("PACKAGE_BUILDING", "Package building"),
> + ("COMPLETE", "Completed"),
> + )
> + WORKFLOW_STEP_STATUSES = (
> + ("PKG_BUILD_WAITING", "Not started"),
> + ("PKG_BUILD_INPROGRESS", "In progress"),
> + ("PKG_BUILD_DONE", "Completed"),
> + ("PKG_BUILD_FAILED", "Failed"),
> + )

same point about integer field here.

> + assignee = models.ForeignKey(Person)

I assume we want subtickets to be able to have different owners than the
main ticket?

> + sourcepackageupload = models.ForeignKey(SourcePackageUpload)

> +class Artifact(models.Model):

> + ticket_component = models.ForeignKey(SubTicket, blank=True, null=True)
> + sourcepackageupload = models.ForeignKey(
> + SourcePackageUpload, null=True, blank=True,
> + verbose_name='Source Package Upload')

This feels like we might not have a relation defined properly. It seems
like an Artifact shouldn't need the "ticket_component" attribute. I
think a SubTicket has a SourcePackageUpload which has artifacts. So
adding this ticket_component here feels like something isn't properly
normalized. I'm not sure the intent but it seems like it should be one
of these options:

= 1 ======
You add to SourcePackageUpload:
   subticket = models.ForeignKey(SubTicket)

Remove the the "ticket_component" attribute from Artifact.

The drawback here, is that a SubTicket could have more than
SourcePackageUploads. Maybe a OneToOneField is needed instead?

= 2 =====

You don't need a SourcePackage upload class. You just make
"source_package" and "source_package_version" as attributes to
SubTicket. Then Arfifact just has a foreign key to SubTicket.

= 3 =====

This is already correct, and I'm missing something.