Merge lp://staging/~julian-edwards/launchpad/ppa-deletion-ui into lp://staging/launchpad/db-devel

<rockstar> bigjools, so, I don't feel like I know enough about Soyuz to really grok what I'm trying to review here (and there's no movie/screen shot). How can I get to the view?
<bigjools> rockstar: go to http://launchpad.dev/~cprov/+archive/ppa
<bigjools> sorry I assume too much :)
<rockstar> bigjools, :) It's okay.
<rockstar> bigjools, so, once a PPA is requested for deletion, can it be uploaded to?
<rockstar> Also, "Delete PPA" shouldn't show if the deletion request has already been made.
<bigjools> OTP, will type when I can :)
<rockstar> I also wonder if a red notification for "Deletion in progress" is probably better, since it's more likely to grab your attention.
<rockstar> Although the red often means "It's broken. It's broken! It's BROKEN!!!!"

So, the more I think about it, I think blue is the wrong notification color. We need some way of saying "THIS PPA IS GOING AWAY." I'd suggest we make it a red box.

<rockstar> bigjools, also, the "Delete PPA" link shouldn't show if the deletion has already been requested.
<bigjools> rockstar: I thought about that, it was easier to leave it and make the page template different!
<bigjools> generally I really hate links appearing and disappearing with no explanation :/
<bigjools> but I can change it
<bigjools> one day I hope we'll present disabled links
<rockstar> bigjools, grey it out then?
<bigjools> rockstar: does Link do that?
<rockstar> bigjools, I don't think so, but it should... :)
<bigjools> heh
<rockstar> bigjools, basically, you shouldn't be able to click it if it's no longer deletable (because it's already deleted)
<bigjools> I didn't think about it too hard because it won't be around longer than 5 minutes
<bigjools> in any case we're changing tack slightly, and only removing the repository, the PPA will be disabled and hidden
<rockstar> bigjools, yeah, I figured that, but to the user, it's "deleted"
<bigjools> rockstar: yeah
<bigjools> rockstar: so I'll make the whole thing disappear and redirect to the user's profile page, what do you think?
<rockstar> bigjools, yeah, that's probably a good idea.
<rockstar> And then the info notification is something like "The PPA deletion has been requested."
<bigjools> right
<bigjools> in red?
<bigjools> :)
<rockstar> bigjools, no, at that point, blue is fine.
<bigjools> rockstar: cool, thanks for the pre-imp

Great points Paul and Julian.

I'm just taking a look now, as by the time Paul's day starts, mine will be ending. The UI looks great and the redirection seems natural. I've got notes below, but basically I'm happy to ui=me - unless Paul disagrees of course - with the Delete menu item moved as below, and the Edit/Copy/Delete menu items disabled when the PPA is deleted/deleting.

Great stuff!

Details:
When first browsing:

https://launchpad.dev/~cprov/+archive/ppa

I was a bit surprised to see the Delete action as the first presented.

{{{
11:49 < noodles> bigjools: did you intend for the Delete link to be the first presented in the menu? Or is it just because you've listed it alphabetically in the ArchiveIndexActionsMenu?
11:49 < bigjools> noodles: that did cross my mind, I was considering moving it but was waiting for someone to comment first
11:49 < noodles> I'm wondering if it should be the last action (ie. after 'manage_subscribers' on ArchiveIndexActionsMenu.links)
11:49 < bigjools> and you did :)
11:50 < noodles> Great.
}}}

I agree, when deleting the archive, redirecting to the profile and including the blue info box is a good solution. Works well.

At that point though, I'm wasn't sure there was any reason for presenting the link on the profile page allowing me to view the empty PPA, but Julian clarified that - the history is still viewable at:
https://launchpad.dev/~cprov/+archive/ppa/+packages?field.status_filter=

{{{
11:56 < noodles> bigjools: Is there any reason to present the PPA after the deletion has been requested? Why have a disabled-like PPA link on the profile that can be clicked on and viewed in it's empty state with the "This archive has been deleted" message?
11:57 < bigjools> so you can go in and view its history even after deletion
11:58 < bigjools> StevenK: looks good
11:58 < noodles> bigjools: ah, I misunderstood. So it will stay there permanently? OK.
11:58 < bigjools> StevenK: when that's done, you need to run process-accepted to make it do the re-upload between the librarians
11:59 < bigjools> noodles: yes - this is what wgrant wanted, and is the simpler first step before complete obliteration of all archive objects
}}}

which leaves me wondering whether, rather than saying (on the PPA index page), "This archive has been deleted.", we said "This archive has been deleted ([link]View deleted packages[/link])." which would (1) indicate that there is useful information still available and (2) save you from having to know to click on the Packages link and update the filter to be able to see them. See what you both think. (it would be nice if this extra link only appeared on the index page)

Note: you can use view.archive_label instead of 'archive' in your message if you want it to automatically use "PPA" or "archive".

The only other thoughts I had were around the presentation of the menu options, such as "Copy packages", "Delete packages", "Change details" and "Edit dependencies".

I'm assuming we want to disable the last 3 (currently I can click on Change details and "re-enable" my PPA, which then means that the "This PPA has been deleted" msg no longer appears, even though I assume it's still deleted). Note, disabl...

Ok guys, how is it looking now?

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...

On Friday 26 March 2010 15:09:22 Guilherme Salgado wrote:
> 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.

Thanks for the review Salgado - answers inline as usual!

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

There's no lint, thanks for reminding me.

> > === 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?

Yes it's wrong and no it doesn't!

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

I've done that now, dunno why I didn't do it before really!

> > + @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...

It is indeed wrong, I've fixed it, thanks for noticing.

> > +Now, all the publications are DELTETED, the archive is disabled and the
>
> typo: DELTETED

Fixed.

> > + def delete(self, deleted_by):
> > + """See `IArchive`."""
>
> Would it make sense to assert that self.status is not DELETED or
> DELETING here?

Yep, done.

> > + >>> 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. :)

See, I know this. Why did I do it wrong? :)

> > + <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...

Nearly free :)

I was hacking it up to see what I wanted and didn't get around to changing it
to an LPForm. All fixed now!

> 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?

It's being done in a different branch - the publisher will remove it.

Cheers
J

 review approve code
 status approved

Because the backend didn't make it last cycle, this change is now going to be delayed a whole cycle as it can't appear on edge unless the backend is in place :(