Hi Julian,

This is a nice branch and I'm glad you've asked for a UI review before
asking for a code review.  I have some questions/suggestions, but
nothing major.

  review needsfixing

On Fri, 2010-03-26 at 14:02 +0000, Julian Edwards wrote:
> You have been requested to review the proposed merge of lp:~julian-edwards/launchpad/ppa-deletion-ui into lp:launchpad.
> 
> Adds a trivial UI for PPA deletion.

Since you've done changes after the m-p creation, would you care to run
'make lint' one more time?

> === modified file 'lib/lp/soyuz/browser/archive.py'
> --- lib/lp/soyuz/browser/archive.py	2010-03-12 06:23:04 +0000
> +++ lib/lp/soyuz/browser/archive.py	2010-03-26 13:59:31 +0000
> @@ -404,13 +405,32 @@
>          archive = view.context
>          if not archive.private:
>              link.enabled = False
> +        if archive.status in (
> +            ArchiveStatus.DELETING, ArchiveStatus.DELETED):
> +            link.enabled = False
> +        return link
>  
>          return link

Am I reading this wrong or is there an extra 'return link' above?  Maybe
'make lint' catches this?

>  
>      @enabled_with_permission('launchpad.Edit')
>      def edit(self):
>          text = 'Change details'
> -        return Link('+edit', text, icon='edit')
> +        link = Link('+edit', text, icon='edit')
> +        view = self.context
> +        if view.context.status in (
> +            ArchiveStatus.DELETING, ArchiveStatus.DELETED):
> +            link.enabled = False
> +        return link
> +
> +    @enabled_with_permission('launchpad.Edit')
> +    def delete_ppa(self):
> +        text = 'Delete PPA'
> +        link = Link('+delete', text, icon='trash-icon')
> +        view = self.context
> +        if view.context.status in (
> +            ArchiveStatus.DELETING, ArchiveStatus.DELETED):
> +            link.enabled = False
> +        return link
>  
>      def builds(self):
>          text = 'View all builds'
> @@ -442,6 +462,10 @@
>          # archives without any sources.
>          if self.context.is_copy or not self.context.has_sources:
>              link.enabled = False
> +        view = self.context
> +        if view.context.status in (
> +            ArchiveStatus.DELETING, ArchiveStatus.DELETED):
> +            link.enabled = False
>          return link
>  
>      @enabled_with_permission('launchpad.AnyPerson')
> @@ -458,7 +482,12 @@
>      @enabled_with_permission('launchpad.Edit')
>      def edit_dependencies(self):
>          text = 'Edit PPA dependencies'
> -        return Link('+edit-dependencies', text, icon='edit')
> +        link = Link('+edit-dependencies', text, icon='edit')
> +        view = self.context
> +        if view.context.status in (
> +            ArchiveStatus.DELETING, ArchiveStatus.DELETED):
> +            link.enabled = False
> +        return link

You could have a method/function which takes an archive and returns
whether or not it's (being) deleted, to avoid all the duplication above.
Also, you can pass enabled=False in the link constructor, simplifying
things a bit more. e.g.

    def is_archive_alive(archive):
        """Return True if the given archive has not been deleted."""
        return archive.staus not in 
            ArchiveStatus.DELETING, ArchiveStatus.DELETED)

And then you create the Link object

    return Link(...,
                enabled=is_archive_alive(view.context))

>  
> 
>  class ArchiveNavigationMenu(NavigationMenu, ArchiveMenuMixin):
> @@ -620,6 +649,19 @@
>          return list(copy_requests)
>  
> 
> +    @property
> +    def disabled_warning_message(self):
> +        """Return an appropriate message if the archive is disabled."""
> +        if self.context.enabled:
> +            return None
> +
> +        if self.context.status in (
> +            ArchiveStatus.DELETED, ArchiveStatus.DELETING):

And you could even use the above function here. :)

Although by now I'm starting to wonder if it shouldn't be a property of
the model class?

> +            return "This %s has been deleted." % self.archive_label
> +        else:
> +            return "This %s has been disabled." % self.archive_label
> +
> +
>  class ArchiveSeriesVocabularyFactory:
>      """A factory for generating vocabularies of an archive's series."""
>  
> @@ -1917,3 +1959,40 @@
>          :rtype: bool
>          """
>          return self.context.owner.visibility == PersonVisibility.PRIVATE
> +
> +
> +class ArchiveDeleteView(LaunchpadView):
> +    """View class for deleting `IArchive`s"""
> +
> +    __used_for__ = IArchive
> +
> +    @property
> +    def page_title(self):
> +        return smartquote('Delete "%s"' % self.context.displayname)
> +
> +    @property
> +    def label(self):
> +        return self.page_title
> +
> +    @property
> +    def can_be_deleted(self):
> +        return self.context.status != ArchiveStatus.DELETING

Erm, can you delete an archive which has also been deleted (i.e. status
== DELETED)?  This doesn't seem consistent with the rules for enabling
the links above...

> +
> +    @property
> +    def waiting_for_deletion(self):
> +        return self.context.status == ArchiveStatus.DELETING
> +
> +    def delete_ppa(self):
> +        action = self.request.form.get('DELETE', None)
> +        # No action, return None to present the form again.
> +        if action is None:
> +            return
> +
> +        self.context.delete(self.user)
> +        self.request.response.addInfoNotification(
> +            "Deletion of '%s' has been requested and the repository will be "
> +            "removed shortly." % self.context.title)
> +
> +        # We redirect back to the PPA owner's profile page.
> +        self.request.response.redirect(canonical_url(self.context.owner))
> +

> === added file 'lib/lp/soyuz/doc/archive-deletion.txt'
> --- lib/lp/soyuz/doc/archive-deletion.txt	1970-01-01 00:00:00 +0000
> +++ lib/lp/soyuz/doc/archive-deletion.txt	2010-03-26 13:59:31 +0000
> +
> +    >>> Store.of(archive).invalidate()
> +
> +Now, all the publications are DELTETED, the archive is disabled and the

typo: DELTETED

> +status is DELETING to tell the publisher to remove the repository:
> +
> +    >>> publications = list(archive.getPublishedSources())
> +    >>> publications.extend(list(archive.getAllPublishedBinaries()))
> +    >>> for pub in publications:
> +    ...     print "%s, %s by %s" % (
> +    ...         pub.displayname, pub.status.name, pub.removed_by.name)
> +    foo 666 in breezy-autotest, DELETED by person-name12
> +    foo 666 in breezy-autotest, DELETED by person-name12
> +    foo-bin1 666 in breezy-autotest i386, DELETED by person-name12
> +    foo-bin1 666 in breezy-autotest hppa, DELETED by person-name12
> +    foo-bin2 666 in breezy-autotest i386, DELETED by person-name12
> +    foo-bin2 666 in breezy-autotest hppa, DELETED by person-name12
> +
> +    >>> print archive.enabled
> +    False
> +
> +    >>> print archive.status.name
> +    DELETING
> 

> === modified file 'lib/lp/soyuz/model/archive.py'
> --- lib/lp/soyuz/model/archive.py	2010-03-23 22:15:02 +0000
> +++ lib/lp/soyuz/model/archive.py	2010-03-26 13:59:31 +0000
> @@ -149,7 +149,8 @@
>          dbName='purpose', unique=False, notNull=True, schema=ArchivePurpose)
>  
>      status = EnumCol(
> -        dbName="status", unique=False, notNull=True, schema=ArchiveStatus)
> +        dbName="status", unique=False, notNull=True, schema=ArchiveStatus,
> +        default=ArchiveStatus.ACTIVE)
>  
>      _enabled = BoolCol(dbName='enabled', notNull=True, default=True)
>      enabled = property(lambda x: x._enabled)
> @@ -1406,6 +1407,22 @@
>          self._enabled = False
>          self._setBuildStatuses(JobStatus.SUSPENDED)
>  
> +    def delete(self, deleted_by):
> +        """See `IArchive`."""

Would it make sense to assert that self.status is not DELETED or
DELETING here?

> +        # Set all the publications to DELETED.
> +        statuses = (
> +            PackagePublishingStatus.PENDING,
> +            PackagePublishingStatus.PUBLISHED)
> +        sources = list(self.getPublishedSources(status=statuses))
> +        getUtility(IPublishingSet).requestDeletion(
> +            sources, removed_by=deleted_by,
> +            removal_comment="Removed when deleting archive")
> +
> +        # Mark the archive's status as DELETING so the repository can be
> +        # removed by the publisher.
> +        self.status = ArchiveStatus.DELETING
> +        self.disable()
> +
>  
>  class ArchiveSet:
>      implements(IArchiveSet)
> 
> === modified file 'lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt'
> --- lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt	2010-02-25 16:49:16 +0000
> +++ lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt	2010-03-26 13:59:31 +0000
[...]
> @@ -754,3 +756,78 @@
>      True
>  
> 
> +== Deleting a PPA ==
> +
> +Users with launchpad.Edit permission see a "Delete PPA" link in the
> +navigation menu.
> +
> +    >>> anon_browser.open("http://launchpad.dev/~no-priv/+archive/ppa")
> +    >>> print anon_browser.getLink("Delete PPA")
> +    Traceback (most recent call last):
> +    ...
> +    LinkNotFoundError
> +
> +    >>> no_priv_browser.open("http://launchpad.dev/~no-priv/+archive/ppa")
> +    >>> no_priv_browser.getLink("Delete PPA").click()
> +
> +Clicking this link takes the user to a page that allows deletion of a PPA:
> +
> +    >>> print no_priv_browser.title
> +    Delete “PPA for No Privileges Person” : PPA for No Privileges Person : No Privileges Person

The above line is too long, but since this is a doctest (and we run them
with the NORMALIZE_WHITESPACE flag) you can add line breaks anywhere in
it. :)

> +
> +The page contains a stern warning that this action is final and irreversible:
> +
> +    >>> print extract_text(find_main_content(no_priv_browser.contents))
> +    Delete “PPA for No Privileges Person”
> +    ...
> +    Deleting a PPA will destroy all of its packages, files and the
> +    repository area.
> +    This deletion is PERMANENT and cannot be undone.
> +    Are you sure ?
> +    ...
> +
> +If the user changes his mind, he can click on the cancel link to go back
> +a page:
> +
> +    >>> print no_priv_browser.getLink("Cancel").url
> +    http://launchpad.dev/~no-priv/+archive/ppa
> +
> +Otherwise, he has a button to press to confirm the deletion.
> +
> +    >>> no_priv_browser.getControl("Permanently delete PPA").click()
> +
> +This action will redirect the user back to his profile page, which will
> +contain a notification message that the deletion is in progress.
> +
> +    >>> print no_priv_browser.url
> +    http://launchpad.dev/~no-priv
> +
> +    >>> for msg in get_feedback_messages(no_priv_browser.contents):
> +    ...     print msg
> +    Deletion of 'PPA for No Privileges Person' has been requested and
> +    the repository will be removed shortly.
> +
> +The deleted PPA is still available to browse via a link on the profile page
> +so you can see its build history, etc.:
> +
> +    >>> no_priv_browser.getLink("PPA for No Privileges Person").click()
> +
> +However, most of the action links are removed for deleted PPAs, so you can
> +no longer "Delete packages", "Edit PPA dependencies", or "Change details".
> +
> +    >>> print no_priv_browser.getLink("Change details")
> +    Traceback (most recent call last):
> +    ...
> +    LinkNotFoundError
> +    
> +    >>> print no_priv_browser.getLink("Edit PPA dependencies")
> +    Traceback (most recent call last):
> +    ...
> +    LinkNotFoundError
> +
> +    >>> no_priv_browser.getLink("View package details").click()
> +    >>> print no_priv_browser.getLink("Delete packages")
> +    Traceback (most recent call last):
> +    ...
> +    LinkNotFoundError
> +
> 
> === added file 'lib/lp/soyuz/templates/archive-delete.pt'
> --- lib/lp/soyuz/templates/archive-delete.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/soyuz/templates/archive-delete.pt	2010-03-26 13:59:31 +0000
> @@ -0,0 +1,37 @@
> +<html
> +  xmlns="http://www.w3.org/1999/xhtml"
> +  xmlns:tal="http://xml.zope.org/namespaces/tal"
> +  xmlns:metal="http://xml.zope.org/namespaces/metal"
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> +  metal:use-macro="view/macro:page/main_only"
> +  i18n:domain="launchpad"
> +>
> +<body>
> +
> +  <div metal:fill-slot="main"
> +       tal:define="message view/delete_ppa; error view/error|nothing">
> +
> +    <div class="top-portlet"
> +         tal:condition="view/can_be_deleted">
> +      <p>Deleting a PPA will destroy all of its packages,
> +         files and the repository area.</p>
> +      <p>This deletion is PERMANENT and cannot be undone.</p>
> +      <p>Are you sure ?</p>
> +
> +      <form name="DELETE" action="" method="POST">
> +        <input name="DELETE" type="hidden" value="1"/>
> +        <input type="submit" value="Permanently delete PPA"/>
> +        or <a tal:attributes="href context/fmt:url">Cancel</a>

Why did you choose to hand-craft this form?  Using a LaunchpadFormView
you'd even get the cancel link for free when using an @action...

> +      </form>
> +    </div>
> +
> +    <div class="top-portlet"
> +         tal:condition="view/waiting_for_deletion">
> +      <p>This archive is marked for deletion and will be removed
> +         shortly.</p>
> +    </div>
> +
> +  </div> <!-- main -->
> +
> +</body>
> +</html>
> 

Now I'm left wondering where's the code that will actually delete the
PPA repository and set its status to DELETED?  Does it exist or is that
something you'll write in another branch?

-- 
Guilherme Salgado <email address hidden>