Review: Approve code
Diff comments: > > === added file > 'lib/lp/app/widgets/templates/distributionsourcepackage-picker.pt' > --- lib/lp/app/widgets/templates/distributionsourcepackage-picker.pt > 1970-01-01 00:00:00 +0000 > +++ lib/lp/app/widgets/templates/distributionsourcepackage-picker.pt > 2016-07-22 16:51:33 +0000 > @@ -0,0 +1,29 @@ > +<tal:root > + xmlns:tal="http://xml.zope.org/namespaces/tal"; > + xmlns:metal="http://xml.zope.org/namespaces/metal"; > + omit-tag=""> > + > +<metal:form-picker use-macro="context/@@form-picker-macros/form-picker"> > + <script metal:fill-slot="add-picker" tal:content="structure string: > + LPJS.use('node', 'lp.app.picker', function(Y) { > + var config = ${view/json_config}; > + var distribution_name = '${view/distribution_name}'; > + var distribution_id = '${view/distribution_id}'; > + if (distribution_name !== '') { > + config.getContextPath = function() { > + return '/' + distribution_name; > + } > + } else if (distribution_id !== '') { > + config.getContextPath = function() { > + return '/' + Y.DOM.byId(distribution_id).value; > + }; > + } > + var show_widget_id = '${view/show_widget_id}'; > + Y.on('domready', function(e) { > + Y.lp.app.picker.addPicker(config, show_widget_id); > + }); > + }); > + "/> > +</metal:form-picker> > + > +</tal:root> This whole thing is awfully risky, but no worse than what you copied from. > > === modified file 'lib/lp/registry/vocabularies.py' > --- lib/lp/registry/vocabularies.py 2016-07-11 21:46:03 +0000 > +++ lib/lp/registry/vocabularies.py 2016-07-22 16:51:33 +0000 > @@ -2100,8 +2100,13 @@ > # Search already did the hard work of looking up binary > names. > cache = get_property_cache(dsp) > cache.binary_names = binary_names > - token = '%s/%s' % (dsp.distribution.name, dsp.name) > - return SimpleTerm(dsp, token, token) > + # XXX cjwatson 2016-07-22: It's a bit odd for the token to > + # return just the source package name and not the distribution > + # name as well, but at the moment this is always fed into a > + # package name box so things work much better this way. If we > + # ever do a true combined distribution/package picker, then this > + # may need to be revisited. > + return SimpleTerm(dsp, dsp.name, dsp.name) Won't this DTWT if parseToken sees a / and overrides the distribution? Perhaps it's worth ensuring in parseToken that the vocab always has a distribution and removing the override support, or is there some callsite that relies on that? > raise LookupError(distribution, spn_or_dsp) > > def getTerm(self, spn_or_dsp): -- https://code.launchpad.net/~cjwatson/launchpad/fix-dsp-vocab-picker/+merge/300782 Your team Launchpad code reviewers is subscribed to branch lp:launchpad. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
