Merge lp:~edwin-grubbs/launchpad/bug-602385-register-project-from-sourcepackage-page into lp:launchpad
| Status: | Merged |
|---|---|
| Approved by: | Edwin Grubbs on 2010-08-12 |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 11326 |
| Proposed branch: | lp:~edwin-grubbs/launchpad/bug-602385-register-project-from-sourcepackage-page |
| Merge into: | lp:launchpad |
| Diff against target: |
695 lines (+401/-13) 10 files modified
lib/canonical/launchpad/browser/multistep.py (+19/-0) lib/canonical/launchpad/webapp/launchpadform.py (+3/-0) lib/lp/registry/browser/product.py (+30/-3) lib/lp/registry/browser/sourcepackage.py (+47/-4) lib/lp/registry/browser/tests/sourcepackage-views.txt (+14/-3) lib/lp/registry/browser/tests/test_sourcepackage_views.py (+114/-0) lib/lp/registry/model/sourcepackage.py (+0/-2) lib/lp/registry/stories/packaging/xx-sourcepackage-packaging.txt (+89/-1) lib/lp/registry/templates/sourcepackage-edit-packaging.pt (+20/-0) lib/lp/soyuz/tests/test_publishing.py (+65/-0) |
| To merge this branch: | bzr merge lp:~edwin-grubbs/launchpad/bug-602385-register-project-from-sourcepackage-page |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Jelmer Vernooij (community) | 2010-08-05 | Approve on 2010-08-05 | |
| Registry Administrators | 2010-08-05 | Pending | |
|
Review via email:
|
|||
Description of the Change
Summary
-------
This branch makes it easy to register an upstream project from a source
package by prefilling the project registration form with data from the
source package.
Implementation details
-------
Added link to $sourcepackage/
$sourcepackage/
lib/
lib/
lib/
lib/
The makeBinaryPacka
binarypackagere
lib/
When +edit-packaging became a MultiStepView, some of the tests
quit exercising the view completely, since no errors were expected, and
the view had just stopped doing anything. I added a check inside the
MultiStepView to help prevent this from happening.
lib/
lib/
lib/
Drive-by lint fixes.
lib/
Tests
-----
./bin/test -vv -t '/sourcepackage
Demo and Q/A
------------
The summary field is not always populated, since the
only info a source package has is the summaries from its binary
packages that might not exist yet.
* Open http://
* Select "Register the upstream project" radio button.
* Click the "Link to Upstream Project" button.
* The form should be prefilled.
* Enter the license.
* Submit the form.
* You should now be redirected to the source package page.
* Open http://
* Click the "Register the upstream project" link.
* The form should be prefilled.
* Enter the license.
* Submit the form.
* You should now be redirected to the +edit-packaging page.
| Jelmer Vernooij (jelmer) wrote : | # |
| Jelmer Vernooij (jelmer) wrote : | # |
The summary seems to be "<packagename>: summary" now, should the packagename perhaps be stripped out ?
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.
| Edwin Grubbs (edwin-grubbs) wrote : | # |
Hi Jelmer,
Thanks for the review.
> It would be nice to have some tests for get_register_
> 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.
> get_register_
> None - when does that situation occur?
The SourcePackage.
> 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.
> 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?
> 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?
| Jelmer Vernooij (jelmer) wrote : | # |
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_
> > 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_
since this function might get more complex in the future as more fields
are pre-filled-in.
> > 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.
> > 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.
Thanks for following up, nice work.
review approve
Cheers,
Jelmer
| Edwin Grubbs (edwin-grubbs) wrote : | # |
> 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_
> 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_
> since this function might get more complex in the future as more fields
> are pre-filled-in.
I added tests for get_register_
> > > 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:/
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 ...

It would be nice to have some tests for get_register_ upstream_ url() - making sure dashes are converted properly, etc.
get_register_ upstream_ url() handles the case of source_ package. summary being None - when does that situation occur?
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.