Code review comment for lp:~edwin-grubbs/launchpad/bug-602385-register-project-from-sourcepackage-page

> On Thu, 2010-08-05 at 18:04 +0000, Edwin Grubbs wrote:
> > Thanks for the review.
> >
> > > It would be nice to have some tests for get_register_upstream_url() -
> making
> > > sure dashes are converted properly, etc.
>
> > The method uses urllib.urlencode(), which is reliable at encoding url
> parameters. I can add a unit test for that if you still want one, but we
> generally haven't tested that.
> I'm not concerned about the URL encoding, but about e.g. replacing
> dashes in the source package name with spaces in the project title (as
> is done on the first line of get_register_upstream_url), especially
> since this function might get more complex in the future as more fields
> are pre-filled-in.

I added tests for get_register_upstream_url().

> > > Since it's likely that the user doesn't want to create a project for their
> own
> > > page this way, it might be a good idea to enable the "I do not want to
> > > maintain this project" checkbox when registering a project this way.
> > This was commented on in bug 602385, and we thought that the user should
> make the decision not to own a project explicitly.
> Fair enough.
>
> > > The summary seems to be "<packagename>: summary" now, should the
> packagename
> > > perhaps be stripped out ?
> > The summary is definitely the part that I'm least happy about. The source
> package may or may not have any summary info depending on whether it has
> binary packages. If there are multiple binary packages, it will be even
> uglier, looking like this:
> > <packagename>: summary
> > <packagename>: summary2
> > Do you think I could stop populating the summary this way and just make the
> user enter it?
> I hadn't considered that we are basing the new project on a source
> package and not on a binary package. As an optimization, I think it
> would still be nice to strip out the "packagename:" bit if there is only
> a single binary package for the source package, but I don't see a good
> alternative in the other situation.
>
> Certainly, having this field pre-filled-in, even when there are multiple
> binary packages, is better than having an empty field imho.

I removed the parts before the colon. I also eliminated duplicates, since it would look really ugly to have a summary like the one from this sourcepackage.

https://edge.launchpad.net/ubuntu/lucid/+source/linux

I added tests for this also.

> > > It would also be nice to fill in the description field from the
> > multi-line > Debian description, or perhaps that is something that can
> > be addressed later. I think this would have to be done in a follow up
> > branch. As bug 602385 indicates, it's also desired to automatically
> > connect the source package to the new project instead of just
> > redirecting back to the source package. Do you think the Debian
> > description or some other field would be good for filling in the
> > summary field also?
> Using the Debian description to fill in the description field would also
> be useful, but the same problem as above applies here as well
> unfortunately as there can be multiple binary packages, each with their
> own description.

That would be nice. I'll look into it and open a bug if it seems easy to do.

-Edwin