Hi Curtis, thank you for this important work to get the link-to-upstream work ahead. I see things coming together and this is a part of it. However, I cannot yet approve this branch, please see my comments below. Also, I ask you to please seek an UI review, too, as mentioned elsewhere in my comments. Am 05.04.2010 18:37, schrieb Curtis Hovey: > > Allow users to say a project is not packaged in Ubuntu > ------------------------------------------------------ > > If you look at https://launchpad.net/launchpad-web you'll see suggestions for > packaging information for the project. Thing is, this project is not packaged > and probably never will be. There's no way of telling Launchpad that or > getting rid of the prompt for packaging on the screen. > > > Rules > ----- > > * Add IProduct.date_last_packaging_check to schema and interface. > * Add a rule to product-portlet-packages to suggest packages if > There are no packages and > The date_last_packaging_check is None or> now - 1 year > * Add an action to the form for the user to say: > This is Not Packaged in Ubuntu > * Replace the +ubuntupkg link with an option in the suggestions so > that to form has only two actions for the user. Choosing the new > option will redirect the user to +ubuntupkg > > > UI > -- > > The image at > > http://people.canonical.com/~curtis/package-suggestion-portlet.png > > shows two presentations of the Packages in Ubuntu portlet for unlinked > project > > * The version with suggestions shows a single suggestion and an > option to state that the project provides an unlisted package. > The user can choose to link to a package or state the project > has no Ubuntu packages. > * the version without suggests states that there were no matches, > but allows the user to still another package. The user can also > state that the project is not packaged in Ubuntu. I am not doing a UI review here but I think this branch needs one. Let me still add my thoughts here. In the second version it seems pointless to have just one static element in the radio button list. I think it would be much saner to not display the heading and the list in this case and have the "Link Ubuntu package" redirect to +ubuntupkg (which it does anyway). > > The user is redirect to//+ubuntupkg when he chooses > "An unlisted package" and (Link to Ubuntu Package). > > > QA > -- > > * Visit https://edge.launchpad.net/gdp (not packaged with suggestions). > * Choose (This is Not Packaged in Ubuntu). > * Verify the page does not suggest packages when it reloads. > * Visit https://edge.launchpad.net/gedit-class-browser > (not packaged without suggestions). > * Choose "An unlisted package" and (Link to Ubuntu Package). > * Verify you are on the +ubuntupkg form. Maybe add a step to have a LOSA apply some SQL to change date_last_packaging_check to be a year old, to test the expiry functionality? However, I had some problems trying this out on dev. * When clicking on "This is Not Packaged in Ubuntu" without selecting a radio button, the form returns an error about this. I think it is clear that it should ignore the radio button in this case. Maybe "This is Not Packaged in Ubuntu" should not be part of the form at all? * I don't seem to be able to get packages suggested. I unlinked the evolution product from the package but now I don't get a suggestion for that package again, although the names are identical. Am I doing something wrong? * Once the "This is Not Packaged in Ubuntu" button has been clicked, the form completely disappears from the Product home page. I understand that this is intended but I wonder if there shouldn't be some way to get it back if the button was clicked in error. Have a one-liner that says "This product is not packaged in Ubuntu (change)"? Probably subject of an UI review. > > > Lint > ---- > > Linting changed files: > database/sampledata/current-dev.sql > database/sampledata/current.sql > database/schema/patch-2207-96-0.sql > lib/lp/registry/configure.zcml > lib/lp/registry/browser/product.py > lib/lp/registry/browser/tests/product-portlet-packages-view.txt > lib/lp/registry/browser/tests/product-views.txt > lib/lp/registry/doc/product.txt > lib/lp/registry/interfaces/product.py > lib/lp/registry/model/product.py > lib/lp/registry/stories/product/xx-product-index.txt > lib/lp/registry/stories/webservice/xx-project-registry.txt > lib/lp/registry/templates/product-portlet-packages.pt > > > > Test > ---- > > * lib/lp/registry/doc/product.txt > * Added documentation for date_last_packaging_check. > * lib/lp/registry/stories/webservice/xx-project-registry.txt > * Verified that date_last_packaging_check is exported. > * lib/lp/registry/browser/tests/product-views.txt > * Removed redundant test. product-portlet-packages-view.txt did the > same tests. > * lib/lp/registry/browser/tests/product-portlet-packages-view.txt > * Revised the test to show that choosing "An unlisted package" > redirects to +ubuntupkg. Removed the verification of the link > to the form. > * Verified that form label states that the packages are in the > current Ubuntu series. > * Verified that the form is not rendered when the user is anonymous. > * Verified that the for is rendered when last_date_packaging_check > is None or older than a year, otherwise it is not rendered. > * Added a test to verify the form works. > * lib/lp/registry/stories/product/xx-product-index.txt > * Updated the test to verify what the user sees. > > > Implementation > -------------- > > * database/schema/patch-2207-96-0.sql > * Added date_last_packaging_check column to Product. > * database/sampledata/current-dev.sql > * regenerated sample data. > * database/sampledata/current.sql > * regenerated sample data. > * lib/lp/registry/configure.zcml > * Any logged in user can set last_date_packaging_check. Err, why is that??? What is special about this that is should be set by anybody? The form is only reachable for owners, anyway. > * lib/lp/registry/interfaces/product.py > * Added date_last_packaging_check > * lib/lp/registry/model/product.py > * Added date_last_packaging_check > * lib/lp/registry/browser/product.py > * Revised the rules to show the portlet--do not show it if > date_last_packaging_check is not< a year old. > * Revised the packages vocabulary to include an option to represent > another package. When the user chooses the option, he is redirected > to +ubuntupkg. > * Revised the field label to make it clear that the packages are in > the current Ubuntu series. > * Added a action to state that the project is not packaged. The > date_last_packaging_check is set to now. > * lib/lp/registry/templates/product-portlet-packages.pt > * Removed the link to +ubuntupkg. > * Moved the paragraph about no matches into the first form so that > the second form could be removed. > > === modified file 'database/sampledata/current-dev.sql' > === modified file 'database/sampledata/current.sql' > === added file 'database/schema/patch-2207-96-0.sql' Subject of DB review. > === modified file 'lib/lp/registry/browser/product.py' > --- lib/lp/registry/browser/product.py 2010-03-26 17:32:19 +0000 > +++ lib/lp/registry/browser/product.py 2010-04-05 16:37:23 +0000 > @@ -39,8 +39,11 @@ > > > from cgi import escape > +from datetime import datetime, timedelta > from operator import attrgetter > > +import pytz > + > from zope.component import getUtility > from zope.event import notify > from zope.app.form.browser import TextAreaWidget, TextWidget > @@ -977,6 +980,7 @@ > 'distributionsourcepackage', LaunchpadRadioWidget, > orientation='vertical') > suggestions = None > + other_package = object() > > @cachedproperty > def sourcepackages(self): > @@ -990,7 +994,14 @@ > @cachedproperty > def can_show_portlet(self): > """Are there packages, or can packages be suggested.""" > - return len(self.sourcepackages) > 0 or not config.launchpad.is_lpnet > + if len(self.sourcepackages) > 0: > + return True > + if self.user is None or config.launchpad.is_lpnet: I don't understand what "is_lpnet" has to do with this. A comment would be great here. > + return False > + last_check = self.context.date_last_packaging_check > + return ( > + last_check is None > + or last_check < datetime.now(tz=pytz.UTC) - timedelta(days=365)) > > def setUpFields(self): > """See `LaunchpadFormView`.""" > @@ -1009,20 +1020,34 @@ > description = """%s""" % ( > item_url, escape(package.name)) > vocab_terms.append(SimpleTerm(package, package.name, description)) > + description = 'An unlisted package' Should this really be capitalized? It's not a full sentence. > + vocab_terms.append( > + SimpleTerm(self.other_package, 'OTHER_PACKAGE', description)) > vocabulary = SimpleVocabulary(vocab_terms) > self.form_fields = form.Fields( > Choice(__name__='distributionsourcepackage', > - title=_('Ubuntu packages'), > + title=_('Ubuntu %s packages' % > + ubuntu.currentseries.displayname), The gettext function goes around the format string only AFAIK. title=_('Ubuntu %s packages') % ( ubuntu.currentseries.displayname), > default=None, > vocabulary=vocabulary, > required=True)) > > - @action(_('Link to this Ubuntu Package'), name='link') > + @action(_('This is Not Packaged in Ubuntu'), name='not-packaged') This looks a little bit too long. Wouldn't "Not packaged in Ubuntu" do? Also, I'd say "not packaged" should not be capitalized when found mid-sentence. > + def not_packaged(self, action, data): > + self.context.date_last_packaging_check = datetime.now(tz=pytz.UTC) > + self.next_url = self.request.getURL() > + > + @action(_('Link to Ubuntu Package'), name='link') "Link to Ubuntu package", I see no reason to capitalize "package". > def link(self, action, data): > product = self.context > dsp = data.get('distributionsourcepackage') > assert dsp is not None, "distributionsourcepackage was not specified" > product_series = product.development_focus > + if dsp is self.other_package: > + # The user wants to link an alternate package to this project. > + self.next_url = canonical_url( > + product_series, view_name="+ubuntupkg") > + return > ubuntu = getUtility(ILaunchpadCelebrities).ubuntu > product_series.setPackaging(ubuntu.currentseries, > dsp.sourcepackagename, > > === modified file 'lib/lp/registry/browser/tests/product-portlet-packages-view.txt' > --- lib/lp/registry/browser/tests/product-portlet-packages-view.txt 2010-03-26 16:12:55 +0000 > +++ lib/lp/registry/browser/tests/product-portlet-packages-view.txt 2010-04-05 16:37:23 +0000 [...] > @@ -63,8 +62,8 @@ > translations efficiently. > There are no unlinked source packages that are a good match. Can you > suggest one? > - Link to Ubuntu package > - > + Ubuntu Hoary packages: > + An unlisted package See discussion at the top. > > A distribution source package in a distribution other than ubuntu will > not be suggested. > @@ -93,11 +92,12 @@ > >>> view = create_initialized_view( > ... product, name="+portlet-packages", > ... principal=product.owner) > - >>> for dsp in view.suggestions: > - ... print dsp.name > + >>> view.suggestions > + [] > > A source package that does have a publishing history for the current > -Ubuntu series will be suggested. > +Ubuntu series will be suggested. The view as a distributionsourcepackage > +field. That is not a sentence, or is this s/as/has/ ? > > >>> spph = factory.makeSourcePackagePublishingHistory( > ... sourcepackagename=spn, distroseries=ubuntu.currentseries) [...] > @@ -184,6 +190,17 @@ > ... print dsp.name > bingo > > +The view is not rendered when there are no source packages and the user is > +anonymous. Hm, I don't see that "and" condition in "can_show_portlet", the conditions seem to be orthogonal. > + > + >>> login(ANONYMOUS) > + >>> view = create_initialized_view( > + ... product, name="+portlet-packages", principal=product.owner) > + >>> view.sourcepackages > + [] > + >>> view.can_show_portlet > + False > + > The can_show_portlet property indicates that the portlet can be rendered. The > portlet is not rendered if there are no source packages and the environment > is lpnet. Ah, here is a clue to the is_lpnet mystery. There has to be a good reason to let it behave differently on production and I think it should be mentioned here and/or in the actual code. Also, the whole "view.sourcepackages" property is not clear to me. Are those the packages, that are already linked to the product or are those the ones that are suggested to be linked? > @@ -191,6 +208,9 @@ > >>> config.launchpad.is_lpnet > False > > + >>> login_person(product.owner) > + >>> view = create_initialized_view( > + ... product, name="+portlet-packages", principal=product.owner) > >>> view.sourcepackages > [] > > @@ -219,15 +239,98 @@ > > >>> ignore = config.pop('test_data') > > +The can_show_portlet property is False when product.date_last_packaging_check > +is None or the date is more than one year ago. > + > + >>> from datetime import datetime, timedelta > + >>> import pytz > + > + >>> print product.date_last_packaging_check > + None > + > + >>> view = create_initialized_view( > + ... product, name="+portlet-packages", principal=product.owner) > + >>> view.can_show_portlet > + True > + > + >>> now = datetime.now(tz=pytz.UTC) > + >>> product.date_last_packaging_check = now > + >>> view = create_initialized_view( > + ... product, name="+portlet-packages", principal=product.owner) > + >>> view.can_show_portlet > + False > + > + ^ Extra newline... ;) > + >>> product.date_last_packaging_check = now - timedelta(days=366) > + >>> view = create_initialized_view( > + ... product, name="+portlet-packages", principal=product.owner) > + >>> view.can_show_portlet > + True > + > +The +portlet-packages view can set the product date_last_packaging_check > +attribute so that suggestions are disabled. > + > + >>> product.date_last_packaging_check = None > + > + >>> form = { > + ... 'field.actions.not-packaged': 'This is Not Packaged in Ubuntu', > + ... } > + >>> view = create_initialized_view( > + ... product, name="+portlet-packages", form=form) > + >>> view.errors > + [] > + > + >>> print product.date_last_packaging_check is not None > + True > + > +The +portlet-packages view can create a Packaging link. > + > + >>> product.sourcepackages > + [] > + > + >>> form = { > + ... 'field.distributionsourcepackage': 'bingo', > + ... 'field.actions.link': 'Link to this Ubuntu Package', > + ... } > + >>> view = create_initialized_view( > + ... product, name="+portlet-packages", form=form) > + >>> view.errors > + [] > + > + >>> for sp in product.sourcepackages: > + ... print sp.name > + bingo > + > + >>> from lp.registry.interfaces.packaging import ( > + ... IPackagingUtil, PackagingType) > + > + >>> packaging_util = getUtility(IPackagingUtil) > + >>> packaging_util.deletePackaging( > + ... product.development_focus, > + ... getUtility(ISourcePackageNameSet)['bingo'], > + ... ubuntu.currentseries) > + > +Choosing the other_package option issues a redirect to the +ubuntupkg. > + > + >>> form = { > + ... 'field.distributionsourcepackage': 'OTHER_PACKAGE', > + ... 'field.actions.link': 'Link to this Ubuntu Package', > + ... } > + >>> view = create_initialized_view( > + ... product, name="+portlet-packages", form=form) > + >>> view.errors > + [] > + > + >>> product.sourcepackages > + [] > + > + >>> print view.next_url > + http://launchpad.dev/bingo/trunk/+ubuntupkg > + > The view's sourcepackages property filters out obsolete packages and > reverses the order so that the latest packages for the current ubuntu > series are shown first. > > - >>> from lp.registry.interfaces.packaging import ( > - ... IPackagingUtil, PackagingType) > - > - >>> packaging_util = getUtility(IPackagingUtil) > - >>> series = ubuntu.currentseries > >>> spn = factory.makeSourcePackageName(name="a-obsolete-package") > >>> packaging_util.createPackaging( > ... product.development_focus, spn, ubuntu.currentseries, > > === modified file 'lib/lp/registry/browser/tests/product-views.txt' Ah, removing redundancy is good. Thank you! > === modified file 'lib/lp/registry/configure.zcml' > --- lib/lp/registry/configure.zcml 2010-03-19 11:33:44 +0000 > +++ lib/lp/registry/configure.zcml 2010-04-05 16:37:23 +0000 > @@ -1077,6 +1077,9 @@ > interface="lp.translations.interfaces.customlanguagecode.IHasCustomLanguageCodes"/> > + permission="launchpad.View" As mentioned earlier, I don't understand the reason for this. > + set_attributes="date_last_packaging_check"/> > + permission="launchpad.Driver" > interface="lp.registry.interfaces.product.IProductDriverRestricted"/> > > === modified file 'lib/lp/registry/doc/product.txt' > --- lib/lp/registry/doc/product.txt 2009-12-13 11:55:40 +0000 > +++ lib/lp/registry/doc/product.txt 2010-04-05 16:37:23 +0000 > @@ -205,7 +205,8 @@ > >>> evo.active = True > > > -== Package links == > +Package links > +------------- > > The packaging table allows us to list source and distro source packages > related to a certain upstream: > @@ -216,7 +217,28 @@ > >>> [(sp.name, sp.distribution.name) for sp in alsa.distrosourcepackages] > [(u'alsa-utils', u'debian'), (u'alsa-utils', u'ubuntu')] > > -== External Bug Tracker == > +The date_last_packaging_check attribute records the last date that the > +project was confirmed to have no Ubuntu packages. The project can be rejected > +at some later date. A value of None means that no user has ever confirmed that > +that the project has no Ubuntu packages. > + > + >>> print evo.date_last_packaging_check > + None > + > +Any logged in user can set the date. Again, why? > + > + >>> from datetime import datetime > + >>> from pytz import UTC > + > + >>> login("