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.