Code review comment for lp:~julian-edwards/launchpad/ppa-deletion-ui

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