launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05279
[Merge] lp:~wallyworld/launchpad/remove-picker-feature-flags-855904 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/remove-picker-feature-flags-855904 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #855904 in Launchpad itself: "Remove person and target picker flags"
https://bugs.launchpad.net/launchpad/+bug/855904
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/remove-picker-feature-flags-855904/+merge/79897
Remove the following feature flags (the associated features are now out of beta and enabled for all users):
1008 - ('disclosure.picker_enhancements.enabled',
1010 - ('Enables the display of extra details in the person picker.'),
1012 - ('disclosure.picker_expander.enabled',
1014 - ('Enables the expanding of extra details in the person picker.'),
1016 - ('disclosure.personpicker_affiliation.enabled',
1018 - ('Enables display of affiliation details in the person picker.'),
1020 - ('disclosure.person_affiliation_rank.enabled',
1022 - ('Enables ranking by pillar affiliation in the person picker.'),
1024 - ('disclosure.target_picker_enhancements.enabled',
1026 - ('Enables the display and use of the enhanced target pickers.'),
--
https://code.launchpad.net/~wallyworld/launchpad/remove-picker-feature-flags-855904/+merge/79897
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/remove-picker-feature-flags-855904 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/webapp/tests/cookie-authentication.txt'
--- lib/canonical/launchpad/webapp/tests/cookie-authentication.txt 2011-05-31 22:41:22 +0000
+++ lib/canonical/launchpad/webapp/tests/cookie-authentication.txt 2011-10-20 00:58:24 +0000
@@ -2,12 +2,6 @@
on http instead of https, it cannot read the secure cookie on https,
so it cannot tell that it will end up overwriting the existing cookie.
- >>> # Enable the picker_enhancements feature to test the commenter name.
- >>> from lp.services.features.testing import FeatureFixture
- >>> feature_flag = {'disclosure.picker_enhancements.enabled': 'on'}
- >>> flags = FeatureFixture(feature_flag)
- >>> flags.setUp()
-
>>> from canonical.testing.layers import BaseLayer
>>> feeds_root_url = BaseLayer.appserver_root_url('feeds')
>>> browser.open('%s/announcements.atom' % feeds_root_url)
@@ -73,6 +67,3 @@
# XXX stub 20060816 bug=56601: We need to test that for https: URLs, the
# secure attribute is set on the cookie.
-
- >>> # Clean up the feature flag.
- >>> flags.cleanUp()
=== modified file 'lib/canonical/launchpad/webapp/tests/no-anonymous-session-cookies.txt'
--- lib/canonical/launchpad/webapp/tests/no-anonymous-session-cookies.txt 2011-05-31 22:41:22 +0000
+++ lib/canonical/launchpad/webapp/tests/no-anonymous-session-cookies.txt 2011-10-20 00:58:24 +0000
@@ -13,12 +13,6 @@
Now let's log in and show that the session cookie is set.
- >>> # Enable the picker_enhancements feature to test the commenter name.
- >>> from lp.services.features.testing import FeatureFixture
- >>> feature_flag = {'disclosure.picker_enhancements.enabled': 'on'}
- >>> flags = FeatureFixture(feature_flag)
- >>> flags.setUp()
-
>>> import datetime
>>> import pytz
>>> now = datetime.datetime.now(pytz.UTC).replace(microsecond=0)
@@ -39,9 +33,6 @@
>>> print extract_text(find_tag_by_id(browser.contents, 'logincontrol'))
Foo Bar (name16) ...
- >>> # Clean up the feature flag.
- >>> flags.cleanUp()
-
# Open a page again so that we see the cookie for a launchpad.dev request
# and not a testopenid.dev request (as above).
>>> browser.open(root_url)
=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py 2011-07-15 00:10:51 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py 2011-10-20 00:58:24 +0000
@@ -569,13 +569,6 @@
layer = AppServerLayer
def test_replay_attacks_do_not_succeed(self):
- # Enable the picker_enhancements feature to test the commenter name.
- from lp.services.features.testing import FeatureFixture
- feature_flag = {'disclosure.picker_enhancements.enabled': 'on'}
- flags = FeatureFixture(feature_flag)
- flags.setUp()
- self.addCleanup(flags.cleanUp)
-
browser = Browser(mech_browser=MyMechanizeBrowser())
browser.open('%s/+login' % self.layer.appserver_root_url())
# On a JS-enabled browser this page would've been auto-submitted
=== modified file 'lib/lp/answers/stories/question-workflow.txt'
--- lib/lp/answers/stories/question-workflow.txt 2011-07-01 15:12:32 +0000
+++ lib/lp/answers/stories/question-workflow.txt 2011-10-20 00:58:24 +0000
@@ -1,11 +1,6 @@
Question Workflow
=================
- >>> from lp.services.features.testing import FeatureFixture
- >>> feature_flag = {'disclosure.picker_enhancements.enabled': 'on'}
- >>> flags = FeatureFixture(feature_flag)
- >>> flags.setUp()
-
The status of a question changes based on the action done by users on
it. To demonstrate the workflow, we will use the existing question #2 on
the Firefox product which was filed by 'Sample Person'.
@@ -461,7 +456,3 @@
>>> user_browser.getLink('Ask a question').click()
>>> print user_browser.title
Ask a question about...
-
-Clean up the feature flag.
-
- >>> flags.cleanUp()
=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py 2011-10-13 05:27:16 +0000
+++ lib/lp/app/browser/tales.py 2011-10-20 00:58:24 +0000
@@ -1244,18 +1244,7 @@
The link text uses both the display name and Launchpad id to clearly
indicate which user profile is linked.
"""
- if bool(getFeatureFlag('disclosure.picker_enhancements.enabled')):
- text = self.unique_displayname(None)
- # XXX sinzui 2011-05-31: Remove this next line when the feature
- # flag is removed.
- view_name = None
- elif view_name == 'id-only':
- # XXX sinzui 2011-05-31: remove this block and /id-only from
- # launchpad-loginstatus.pt whwn the feature flag is removed.
- text = self._context.name
- view_name = None
- else:
- text = self._context.displayname
+ text = self.unique_displayname(None)
return self._makeLink(view_name, 'mainsite', text)
=== modified file 'lib/lp/app/browser/tests/test_vocabulary.py'
--- lib/lp/app/browser/tests/test_vocabulary.py 2011-10-05 05:08:36 +0000
+++ lib/lp/app/browser/tests/test_vocabulary.py 2011-10-20 00:58:24 +0000
@@ -37,7 +37,6 @@
from lp.app.errors import UnexpectedFormData
from lp.registry.interfaces.irc import IIrcIDSet
from lp.registry.interfaces.series import SeriesStatus
-from lp.services.features.testing import FeatureFixture
from lp.testing import (
login_person,
TestCaseWithFactory,
@@ -197,12 +196,6 @@
layer = DatabaseFunctionalLayer
- def setUp(self):
- super(TestDistributionSourcePackagePickerEntrySourceAdapter,
- self).setUp()
- flag = {'disclosure.target_picker_enhancements.enabled': 'on'}
- self.useFixture(FeatureFixture(flag))
-
def getPickerEntry(self, dsp):
return get_picker_entry(dsp, object())
@@ -276,11 +269,6 @@
layer = DatabaseFunctionalLayer
- def setUp(self):
- super(TestProductPickerEntrySourceAdapter, self).setUp()
- flag = {'disclosure.target_picker_enhancements.enabled': 'on'}
- self.useFixture(FeatureFixture(flag))
-
def getPickerEntry(self, product):
return get_picker_entry(product, object())
@@ -334,11 +322,6 @@
layer = DatabaseFunctionalLayer
- def setUp(self):
- super(TestProjectGroupPickerEntrySourceAdapter, self).setUp()
- flag = {'disclosure.target_picker_enhancements.enabled': 'on'}
- self.useFixture(FeatureFixture(flag))
-
def getPickerEntry(self, projectgroup):
return get_picker_entry(projectgroup, object())
@@ -393,11 +376,6 @@
layer = DatabaseFunctionalLayer
- def setUp(self):
- super(TestDistributionPickerEntrySourceAdapter, self).setUp()
- flag = {'disclosure.target_picker_enhancements.enabled': 'on'}
- self.useFixture(FeatureFixture(flag))
-
def getPickerEntry(self, distribution):
return get_picker_entry(distribution, object())
@@ -532,14 +510,6 @@
def test_json_entries(self):
# The results are JSON encoded.
- feature_flag = {
- 'disclosure.picker_enhancements.enabled': 'on',
- 'disclosure.picker_expander.enabled': 'on',
- 'disclosure.personpicker_affiliation.enabled': 'on',
- }
- flags = FeatureFixture(feature_flag)
- flags.setUp()
- self.addCleanup(flags.cleanUp)
team = self.factory.makeTeam(name='xpting-team')
person = self.factory.makePerson(name='xpting-person')
creation_date = datetime(
=== modified file 'lib/lp/app/browser/vocabulary.py'
--- lib/lp/app/browser/vocabulary.py 2011-10-05 05:08:36 +0000
+++ lib/lp/app/browser/vocabulary.py 2011-10-20 00:58:24 +0000
@@ -50,7 +50,6 @@
from lp.registry.interfaces.sourcepackagename import ISourcePackageName
from lp.registry.model.pillaraffiliation import IHasAffiliation
from lp.registry.model.sourcepackagename import getSourcePackageDescriptions
-from lp.services.features import getFeatureFlag
from lp.soyuz.interfaces.archive import IArchive
# XXX: EdwinGrubbs 2009-07-27 bug=405476
@@ -156,11 +155,8 @@
super(PersonPickerEntrySourceAdapter, self)
.getPickerEntries(term_values, context_object))
- personpicker_affiliation_enabled = kwarg.get(
- 'personpicker_affiliation_enabled', False)
affiliated_context = IHasAffiliation(context_object, None)
- if (affiliated_context is not None
- and personpicker_affiliation_enabled):
+ if affiliated_context is not None:
# If a person is affiliated with the associated_object then we
# can display a badge.
badges = affiliated_context.getAffiliationBadges(term_values)
@@ -172,10 +168,8 @@
label=badge_info.label,
role=badge_info.role))
- picker_expander_enabled = kwarg.get('picker_expander_enabled', False)
for person, picker_entry in izip(term_values, picker_entries):
- if picker_expander_enabled:
- picker_entry.details = []
+ picker_entry.details = []
if person.preferredemail is not None:
if person.hide_email_addresses:
@@ -187,39 +181,29 @@
picker_entry.description = '<email address hidden>'
picker_entry.metadata = get_person_picker_entry_metadata(person)
- enhanced_picker_enabled = kwarg.get(
- 'enhanced_picker_enabled', False)
- if enhanced_picker_enabled:
- # We will display the person's name (launchpad id) after their
- # displayname.
- picker_entry.alt_title = person.name
- # We will linkify the person's name so it can be clicked to
- # open the page for that person.
- picker_entry.alt_title_link = canonical_url(
- person, rootsite='mainsite')
- # We will display the person's irc nick(s) after their email
- # address in the description text.
- irc_nicks = None
- if person.ircnicknames:
- irc_nicks = ", ".join(
- [IRCNicknameFormatterAPI(ircid).displayname()
- for ircid in person.ircnicknames])
- if irc_nicks and not picker_expander_enabled:
- if picker_entry.description:
- picker_entry.description = ("%s (%s)" %
- (picker_entry.description, irc_nicks))
- else:
- picker_entry.description = "%s" % irc_nicks
- if picker_expander_enabled:
- if irc_nicks:
- picker_entry.details.append(irc_nicks)
- if person.is_team:
- picker_entry.details.append(
- 'Team members: %s' % person.all_member_count)
- else:
- picker_entry.details.append(
- 'Member since %s' % DateTimeFormatterAPI(
- person.datecreated).date())
+ # We will display the person's name (launchpad id) after their
+ # displayname.
+ picker_entry.alt_title = person.name
+ # We will linkify the person's name so it can be clicked to
+ # open the page for that person.
+ picker_entry.alt_title_link = canonical_url(
+ person, rootsite='mainsite')
+ # We will display the person's irc nick(s) after their email
+ # address in the description text.
+ irc_nicks = None
+ if person.ircnicknames:
+ irc_nicks = ", ".join(
+ [IRCNicknameFormatterAPI(ircid).displayname()
+ for ircid in person.ircnicknames])
+ if irc_nicks:
+ picker_entry.details.append(irc_nicks)
+ if person.is_team:
+ picker_entry.details.append(
+ 'Team members: %s' % person.all_member_count)
+ else:
+ picker_entry.details.append(
+ 'Member since %s' % DateTimeFormatterAPI(
+ person.datecreated).date())
return picker_entries
@@ -257,33 +241,30 @@
.getPickerEntries(term_values, context_object, **kwarg))
for target, picker_entry in izip(term_values, entries):
picker_entry.description = self.getDescription(target)
- enhanced = bool(getFeatureFlag(
- 'disclosure.target_picker_enhancements.enabled'))
- if enhanced:
- picker_entry.details = []
- summary = picker_entry.description
- if len(summary) > 45:
- index = summary.rfind(' ', 0, 45)
- first_line = summary[0:index + 1]
- second_line = summary[index:]
- else:
- first_line = summary
- second_line = ''
+ picker_entry.details = []
+ summary = picker_entry.description
+ if len(summary) > 45:
+ index = summary.rfind(' ', 0, 45)
+ first_line = summary[0:index + 1]
+ second_line = summary[index:]
+ else:
+ first_line = summary
+ second_line = ''
- if len(second_line) > 90:
- index = second_line.rfind(' ', 0, 90)
- second_line = second_line[0:index + 1]
- picker_entry.description = first_line
- if second_line:
- picker_entry.details.append(second_line)
- picker_entry.alt_title = target.name
- picker_entry.alt_title_link = canonical_url(
- target, rootsite='mainsite')
- picker_entry.target_type = self.target_type
- maintainer = self.getMaintainer(target)
- if maintainer is not None:
- picker_entry.details.append(
- 'Maintainer: %s' % self.getMaintainer(target))
+ if len(second_line) > 90:
+ index = second_line.rfind(' ', 0, 90)
+ second_line = second_line[0:index + 1]
+ picker_entry.description = first_line
+ if second_line:
+ picker_entry.details.append(second_line)
+ picker_entry.alt_title = target.name
+ picker_entry.alt_title_link = canonical_url(
+ target, rootsite='mainsite')
+ picker_entry.target_type = self.target_type
+ maintainer = self.getMaintainer(target)
+ if maintainer is not None:
+ picker_entry.details.append(
+ 'Maintainer: %s' % self.getMaintainer(target))
return entries
@@ -404,12 +385,6 @@
def __init__(self, context, request):
self.context = context
self.request = request
- self.enhanced_picker_enabled = bool(
- getFeatureFlag('disclosure.picker_enhancements.enabled'))
- self.picker_expander_enabled = bool(
- getFeatureFlag('disclosure.picker_expander.enabled'))
- self.personpicker_affiliation_enabled = bool(
- getFeatureFlag('disclosure.personpicker_affiliation.enabled'))
def __call__(self):
name = self.request.form.get('name')
@@ -466,11 +441,7 @@
for adapter_class, term_values in picker_entry_terms.items():
picker_entries = adapter_cache[adapter_class].getPickerEntries(
term_values,
- self.context,
- enhanced_picker_enabled=self.enhanced_picker_enabled,
- picker_expander_enabled=self.picker_expander_enabled,
- personpicker_affiliation_enabled=(
- self.personpicker_affiliation_enabled))
+ self.context)
for term_value, picker_entry in izip(term_values, picker_entries):
picker_term_entries[term_value] = picker_entry
=== modified file 'lib/lp/app/doc/tales.txt'
--- lib/lp/app/doc/tales.txt 2011-07-01 11:39:14 +0000
+++ lib/lp/app/doc/tales.txt 2011-10-20 00:58:24 +0000
@@ -433,18 +433,9 @@
>>> print test_tales("person/fmt:link", person=mark)
<a ...http://launchpad.dev/~mark...
- >>> # Enable the picker_enhancements feature to test the commenter name.
- >>> from lp.services.features.testing import FeatureFixture
- >>> feature_flag = {'disclosure.picker_enhancements.enabled': 'on'}
- >>> flags = FeatureFixture(feature_flag)
- >>> flags.setUp()
-
>>> print test_tales("person/fmt:link-display-name-id", person=mark)
<a ...http://launchpad.dev/~mark...>Mark Shuttleworth (mark)</a>
- >>> # Clean up the feature flag.
- >>> flags.cleanUp()
-
>>> print test_tales("team/fmt:link", team=ubuntu_team)
<a ...http://launchpad.dev/~ubuntu-team...
=== modified file 'lib/lp/app/templates/launchpad-loginstatus.pt'
--- lib/lp/app/templates/launchpad-loginstatus.pt 2011-06-01 03:48:14 +0000
+++ lib/lp/app/templates/launchpad-loginstatus.pt 2011-10-20 00:58:24 +0000
@@ -16,7 +16,7 @@
<li class="no-events">No AJAX events detected</li>
</ul>
</div>
- <a tal:replace="structure request/lp:person/fmt:link-display-name-id/id-only" /> •
+ <a tal:replace="structure request/lp:person/fmt:link-display-name-id" /> •
<input type="submit" name="logout" value="Log Out" />
</form>
</div>
=== modified file 'lib/lp/app/tests/test_tales.py'
--- lib/lp/app/tests/test_tales.py 2011-09-27 14:22:53 +0000
+++ lib/lp/app/tests/test_tales.py 2011-10-20 00:58:24 +0000
@@ -131,12 +131,6 @@
def test_link_display_name_id(self):
"""The link to the user profile page using displayname and id."""
person = self.factory.makePerson()
- # Enable the picker_enhancements feature to test the commenter name.
- from lp.services.features.testing import FeatureFixture
- feature_flag = {'disclosure.picker_enhancements.enabled': 'on'}
- flags = FeatureFixture(feature_flag)
- flags.setUp()
- self.addCleanup(flags.cleanUp)
formatter = getAdapter(person, IPathAdapter, 'fmt')
result = formatter.link_display_name_id(None)
expected = '<a href="%s" class="sprite person">%s (%s)</a>' % (
=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py 2011-09-13 05:23:16 +0000
+++ lib/lp/app/widgets/popup.py 2011-10-20 00:58:24 +0000
@@ -22,7 +22,6 @@
from canonical.launchpad.webapp.vocabulary import IHugeVocabulary
from lp.app.browser.stringformatter import FormattersAPI
from lp.app.browser.vocabulary import get_person_picker_entry_metadata
-from lp.services.features import getFeatureFlag
from lp.services.propertycache import cachedproperty
@@ -238,16 +237,7 @@
include_create_team_link = False
show_assign_me_button = True
show_remove_button = False
-
- @property
- def picker_type(self):
- # This is a method for now so we can block the use of the new
- # person picker js behind our picker_enhancments feature flag.
- if bool(getFeatureFlag('disclosure.picker_enhancements.enabled')):
- picker_type = 'person'
- else:
- picker_type = 'default'
- return picker_type
+ picker_type = 'person'
@property
def selected_value_metadata(self):
=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-activity.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-activity.txt 2011-09-08 22:57:34 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-activity.txt 2011-10-20 00:58:24 +0000
@@ -3,12 +3,6 @@
The bug activity page is where you find the "changelog" of a bug.
- >>> # Enable the picker_enhancements feature to test the commenter name.
- >>> from lp.services.features.testing import FeatureFixture
- >>> feature_flag = {'disclosure.picker_enhancements.enabled': 'on'}
- >>> flags = FeatureFixture(feature_flag)
- >>> flags.setUp()
-
>>> anon_browser.open(
... 'http://bugs.launchpad.dev/debian/+source/'
... 'mozilla-firefox/+bug/3/+activity')
@@ -303,7 +297,3 @@
affects:
mozilla-firefox (Ubuntu) => linux-source-2.6.15 (Ubuntu)
--------
-
-Clean up the feature flag.
-
- >>> flags.cleanUp()
=== modified file 'lib/lp/bugs/stories/bugs/xx-numbered-comments.txt'
--- lib/lp/bugs/stories/bugs/xx-numbered-comments.txt 2011-05-31 16:43:11 +0000
+++ lib/lp/bugs/stories/bugs/xx-numbered-comments.txt 2011-10-20 00:58:24 +0000
@@ -1,12 +1,6 @@
Comment numbering
=================
- >>> # Enable the picker_enhancements feature to test the commenter name.
- >>> from lp.services.features.testing import FeatureFixture
- >>> feature_flag = {'disclosure.picker_enhancements.enabled': 'on'}
- >>> flags = FeatureFixture(feature_flag)
- >>> flags.setUp()
-
Bug respondents frequently refer to other comments by number,
eg. "applied patch in comment #34". This number is stable, and part of
the permalink URL. To help users find the relevant comments more
@@ -36,7 +30,3 @@
And so it should be displayed again.
#6: Dave Miller (justdave)
This title, however, is the same as the bug title
-
-Clean up the feature flag.
-
- >>> flags.cleanUp()
=== modified file 'lib/lp/bugs/stories/bugs/xx-remote-bug-comments.txt'
--- lib/lp/bugs/stories/bugs/xx-remote-bug-comments.txt 2011-05-31 16:43:11 +0000
+++ lib/lp/bugs/stories/bugs/xx-remote-bug-comments.txt 2011-10-20 00:58:24 +0000
@@ -1,12 +1,6 @@
Remote bug comments
===================
- >>> # Enable the picker_enhancements feature to test the commenter name.
- >>> from lp.services.features.testing import FeatureFixture
- >>> feature_flag = {'disclosure.picker_enhancements.enabled': 'on'}
- >>> flags = FeatureFixture(feature_flag)
- >>> flags.setUp()
-
Comments imported into Launchpad from external bug trackers are
formatted for display in a way that distinguishes them from comments
entered within Launchpad itself.
@@ -122,7 +116,3 @@
... attrs={'class': 'boardCommentFooter'})
>>> 'Reply' in extract_text(footer)
False
-
-Clean up the feature flag.
-
- >>> flags.cleanUp()
=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py 2011-09-13 18:34:47 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py 2011-10-20 00:58:24 +0000
@@ -111,7 +111,6 @@
from lp.code.model.branchtarget import PersonBranchTarget
from lp.code.model.sourcepackagerecipe import get_buildable_distroseries_set
from lp.registry.interfaces.series import SeriesStatus
-from lp.services.features import getFeatureFlag
from lp.services.fields import PersonChoice
from lp.services.propertycache import cachedproperty
from lp.soyuz.interfaces.archive import ArchiveDisabled
@@ -258,18 +257,10 @@
@property
def person_picker(self):
- # If we are using the enhanced picker, we need to ensure the vocab
- # gives us terms showing just the displyname rather than displayname
- # plus Luanchpad id since the enhanced picker provides this extra
- # information itself.
- enhanced_picker_enabled = bool(
- getFeatureFlag('disclosure.picker_enhancements.enabled'))
- if enhanced_picker_enabled:
- vocabulary = 'UserTeamsParticipationPlusSelfSimpleDisplay'
- else:
- vocabulary = 'UserTeamsParticipationPlusSelf'
+ vocabulary = 'UserTeamsParticipationPlusSelfSimpleDisplay'
field = copy_field(
- ISourcePackageRecipe['owner'], vocabularyName=vocabulary)
+ ISourcePackageRecipe['owner'],
+ vocabularyName='UserTeamsParticipationPlusSelfSimpleDisplay')
return InlinePersonEditPickerWidget(
self.context, field,
format_link(self.context.owner),
=== modified file 'lib/lp/code/stories/branches/xx-code-review-comments.txt'
--- lib/lp/code/stories/branches/xx-code-review-comments.txt 2011-06-30 17:33:37 +0000
+++ lib/lp/code/stories/branches/xx-code-review-comments.txt 2011-10-20 00:58:24 +0000
@@ -12,11 +12,6 @@
>>> merge_proposal_url = canonical_url(merge_proposal)
>>> merge_proposal_path = canonical_url(
... merge_proposal, force_local_path=True)
- >>> # Enable the picker_enhancements feature to test the commenter name.
- >>> from lp.services.features.testing import FeatureFixture
- >>> feature_flag = {'disclosure.picker_enhancements.enabled': 'on'}
- >>> flags = FeatureFixture(feature_flag)
- >>> flags.setUp()
>>> logout()
>>> eric_browser = setupBrowser(auth="Basic eric@xxxxxxxxxxx:test")
@@ -194,7 +189,3 @@
Testing commits in conversation
4. By ... on 2009-09-12
and it works!
-
-Clean up the feature flag.
-
- >>> flags.cleanUp()
=== modified file 'lib/lp/registry/doc/vocabularies.txt'
--- lib/lp/registry/doc/vocabularies.txt 2011-10-02 07:02:37 +0000
+++ lib/lp/registry/doc/vocabularies.txt 2011-10-20 00:58:24 +0000
@@ -743,12 +743,6 @@
ValidPersonOrTeam
.................
-To use the new sorting functionality, we need to turn on the feature flag.
- >>> from lp.services.features.testing import FeatureFixture
- >>> feature_flag = {'disclosure.picker_enhancements.enabled': 'on'}
- >>> flags = FeatureFixture(feature_flag)
- >>> flags.setUp()
-
All 'valid' persons or teams. This is currently defined as people with a
password, a preferred email address and not merged (Person.merged is
None). It also includes all public teams and private teams the user has
@@ -958,9 +952,6 @@
... print person.name
symbolic
-Clean up the feature flag.
- >>> flags.cleanUp()
-
ValidOwner
..........
=== modified file 'lib/lp/registry/tests/test_person_vocabularies.py'
--- lib/lp/registry/tests/test_person_vocabularies.py 2011-08-22 12:13:22 +0000
+++ lib/lp/registry/tests/test_person_vocabularies.py 2011-10-20 00:58:24 +0000
@@ -24,7 +24,6 @@
)
from lp.registry.interfaces.karma import IKarmaCacheManager
from lp.registry.vocabularies import ValidPersonOrTeamVocabulary
-from lp.services.features.testing import FeatureFixture
from lp.testing import (
StormStatementRecorder,
TestCaseWithFactory,
@@ -33,12 +32,6 @@
from lp.testing.matchers import HasQueryCount
-PERSON_AFFILIATION_RANK_FLAG = {
- 'disclosure.picker_enhancements.enabled': 'on',
- 'disclosure.person_affiliation_rank.enabled': 'on',
- }
-
-
class VocabularyTestBase:
vocabulary_name = None
@@ -88,7 +81,6 @@
value, person.id, None, **kwargs)
def test_people_with_karma_sort_higher(self):
- self.useFixture(FeatureFixture(PERSON_AFFILIATION_RANK_FLAG))
exact_person = self.factory.makePerson(
name='fooix', displayname='Fooix Bar')
prefix_person = self.factory.makePerson(
@@ -125,8 +117,7 @@
expected,
removeSecurityProxy(
self.getVocabulary(context))._karma_context_constraint)
- with FeatureFixture(PERSON_AFFILIATION_RANK_FLAG):
- self.searchVocabulary(context, 'foo')
+ self.searchVocabulary(context, 'foo')
def test_product_karma_context(self):
self.assertKarmaContextConstraint(
@@ -147,9 +138,8 @@
person = self.factory.makePerson()
irc = getUtility(IIrcIDSet).new(
person, 'somenet', 'MiXeD' + self.factory.getUniqueString())
- with FeatureFixture(PERSON_AFFILIATION_RANK_FLAG):
- self.assertContentEqual(
- [person], self.searchVocabulary(person, irc.nickname.lower()))
+ self.assertContentEqual(
+ [person], self.searchVocabulary(person, irc.nickname.lower()))
def _person_filter_tests(self, person):
results = self.searchVocabulary(None, '', 'PERSON')
@@ -159,14 +149,11 @@
self.assertEqual([person], list(results))
def test_person_filter(self):
- # Test that the person filter only returns people
- # (with and without feature flag).
+ # Test that the person filter only returns people.
person = self.factory.makePerson(
name="fredperson", email="fredperson@xxxxxxx")
self.factory.makeTeam(
name="fredteam", email="fredteam@xxxxxxx")
- with FeatureFixture(PERSON_AFFILIATION_RANK_FLAG):
- self._person_filter_tests(person)
self._person_filter_tests(person)
def _team_filter_tests(self, team):
@@ -178,13 +165,11 @@
def test_team_filter(self):
# Test that the team filter only returns teams.
- # (with and without feature flag).
self.factory.makePerson(
name="fredperson", email="fredperson@xxxxxxx")
team = self.factory.makeTeam(
name="fredteam", email="fredteam@xxxxxxx")
- with FeatureFixture(PERSON_AFFILIATION_RANK_FLAG):
- self._team_filter_tests(team)
+ self._team_filter_tests(team)
self._team_filter_tests(team)
@@ -213,8 +198,7 @@
(person.id, person.preferredemail) for person in people)
Store.of(people[0]).invalidate()
- with FeatureFixture(PERSON_AFFILIATION_RANK_FLAG):
- results = list(self.searchVocabulary(None, u'foobar'))
+ results = list(self.searchVocabulary(None, u'foobar'))
with StormStatementRecorder() as recorder:
self.assertEquals(4, len(results))
for person in results:
=== modified file 'lib/lp/registry/tests/test_product_vocabularies.py'
--- lib/lp/registry/tests/test_product_vocabularies.py 2011-06-08 03:44:07 +0000
+++ lib/lp/registry/tests/test_product_vocabularies.py 2011-10-20 00:58:24 +0000
@@ -7,16 +7,12 @@
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.registry.vocabularies import ProductVocabulary
-from lp.services.features.testing import FeatureFixture
from lp.testing import (
celebrity_logged_in,
TestCaseWithFactory,
)
-PICKER_ENHANCEMENTS_FLAG = {'disclosure.picker_enhancements.enabled': 'on'}
-
-
class TestProductVocabulary(TestCaseWithFactory):
"""Test that the ProductVocabulary behaves as expected."""
layer = DatabaseFunctionalLayer
@@ -66,8 +62,7 @@
name='foo-bar', displayname='Foo bar', summary='quux')
quux_product = self.factory.makeProduct(
name='foo-quux', displayname='Foo quux')
- with FeatureFixture(PICKER_ENHANCEMENTS_FLAG):
- result = self.vocabulary.search('quux')
+ result = self.vocabulary.search('quux')
self.assertEqual(
[quux_product, bar_product], list(result))
@@ -77,8 +72,7 @@
name='the-quux', displayname='The quux')
quux_product = self.factory.makeProduct(
name='quux', displayname='The quux')
- with FeatureFixture(PICKER_ENHANCEMENTS_FLAG):
- result = self.vocabulary.search('quux')
+ result = self.vocabulary.search('quux')
self.assertEqual(
[quux_product, the_quux_product], list(result))
=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py 2011-10-05 15:11:38 +0000
+++ lib/lp/registry/vocabularies.py 2011-10-20 00:58:24 +0000
@@ -197,7 +197,6 @@
from lp.registry.model.sourcepackagename import SourcePackageName
from lp.registry.model.teammembership import TeamParticipation
from lp.services.database import bulk
-from lp.services.features import getFeatureFlag
from lp.services.propertycache import (
cachedproperty,
get_property_cache,
@@ -211,11 +210,6 @@
_table = Person
- def __init__(self, context=None):
- super(BasePersonVocabulary, self).__init__(context)
- self.enhanced_picker_enabled = bool(
- getFeatureFlag('disclosure.picker_enhancements.enabled'))
-
def toTerm(self, obj):
"""Return the term for this object."""
try:
@@ -304,13 +298,10 @@
fti_query = quote(query)
sql = "active = 't' AND (name LIKE %s OR fti @@ ftq(%s))" % (
like_query, fti_query)
- if getFeatureFlag('disclosure.picker_enhancements.enabled'):
- order_by = (
- '(CASE name WHEN %s THEN 1 '
- ' ELSE rank(fti, ftq(%s)) END) DESC, displayname, name'
- % (fti_query, fti_query))
- else:
- order_by = self._orderBy
+ order_by = (
+ '(CASE name WHEN %s THEN 1 '
+ ' ELSE rank(fti, ftq(%s)) END) DESC, displayname, name'
+ % (fti_query, fti_query))
return self._table.select(sql, orderBy=order_by, limit=100)
return self.emptySelectResults()
@@ -586,157 +577,6 @@
def _doSearch(self, text="", vocab_filter=None):
"""Return the people/teams whose fti or email address match :text:"""
- if self.enhanced_picker_enabled:
- return self._doSearchWithImprovedSorting(text, vocab_filter)
- else:
- return self._doSearchWithOriginalSorting(text, vocab_filter)
-
- def _doSearchWithOriginalSorting(self, text="", vocab_filter=None):
- private_query, private_tables = self._privateTeamQueryAndTables()
- exact_match = None
- extra_clauses = [self.extra_clause]
- if vocab_filter:
- extra_clauses.extend(vocab_filter.filter_terms)
-
- # Short circuit if there is no search text - all valid people and
- # teams have been requested. We still honour the vocab filter.
- if not text:
- tables = [
- Person,
- Join(self.cache_table_name,
- SQL("%s.id = Person.id" % self.cache_table_name)),
- ]
- tables.extend(private_tables)
- result = self.store.using(*tables).find(
- Person,
- And(
- Or(Person.visibility == PersonVisibility.PUBLIC,
- private_query,
- ),
- Person.merged == None,
- *extra_clauses
- )
- )
- else:
- # Do a full search based on the text given.
-
- # The queries are broken up into several steps for efficiency.
- # The public person and team searches do not need to join with the
- # TeamParticipation table, which is very expensive. The search
- # for private teams does need that table but the number of private
- # teams is very small so the cost is not great.
-
- # First search for public persons and teams that match the text.
- public_tables = [
- Person,
- LeftJoin(EmailAddress, EmailAddress.person == Person.id),
- ]
-
- # Create an inner query that will match public persons and teams
- # that have the search text in the fti, at the start of the email
- # address, or as their full IRC nickname.
- # Since we may be eliminating results with the limit to improve
- # performance, we sort by the rank, so that we will always get
- # the best results. The fti rank will be between 0 and 1.
- # Note we use lower() instead of the non-standard ILIKE because
- # ILIKE doesn't hit the indexes.
- # The '%%' is necessary because storm variable substitution
- # converts it to '%'.
- public_inner_textual_select = SQL("""
- SELECT id FROM (
- SELECT Person.id, 100 AS rank
- FROM Person
- WHERE name = ?
- UNION ALL
- SELECT Person.id, rank(fti, ftq(?))
- FROM Person
- WHERE Person.fti @@ ftq(?)
- UNION ALL
- SELECT Person.id, 10 AS rank
- FROM Person, IrcId
- WHERE IrcId.person = Person.id
- AND lower(IrcId.nickname) = ?
- UNION ALL
- SELECT Person.id, 1 AS rank
- FROM Person, EmailAddress
- WHERE EmailAddress.person = Person.id
- AND lower(email) LIKE ? || '%%'
- AND EmailAddress.status IN (?, ?)
- ) AS public_subquery
- ORDER BY rank DESC
- LIMIT ?
- """, (text, text, text, text, text,
- EmailAddressStatus.VALIDATED.value,
- EmailAddressStatus.PREFERRED.value,
- self.LIMIT))
-
- public_result = self.store.using(*public_tables).find(
- Person,
- And(
- Person.id.is_in(public_inner_textual_select),
- Person.visibility == PersonVisibility.PUBLIC,
- Person.merged == None,
- Or( # A valid person-or-team is either a team...
- # Note: 'Not' due to Bug 244768.
- Not(Person.teamowner == None),
- # Or a person who has a preferred email address.
- EmailAddress.status == EmailAddressStatus.PREFERRED),
- ))
- # The public query doesn't need to be ordered as it will be done
- # at the end.
- public_result.order_by()
-
- # Next search for the private teams.
- private_query, private_tables = self._privateTeamQueryAndTables()
- private_tables = [Person] + private_tables
-
- # Searching for private teams that match can be easier since we
- # are only interested in teams. Teams can have email addresses
- # but we're electing to ignore them here.
- private_result = self.store.using(*private_tables).find(
- Person,
- And(
- SQL('Person.fti @@ ftq(?)', [text]),
- private_query,
- )
- )
-
- private_result.order_by(SQL('rank(fti, ftq(?)) DESC', [text]))
- private_result.config(limit=self.LIMIT)
-
- combined_result = public_result.union(private_result)
- # Eliminate default ordering.
- combined_result.order_by()
- # XXX: BradCrittenden 2009-04-26 bug=217644: The use of Alias and
- # _get_select() is a work-around for .count() not working
- # with the 'distinct' option.
- subselect = Alias(combined_result._get_select(), 'Person')
- exact_match = (Person.name == text)
- result = self.store.using(subselect).find(
- (Person, exact_match),
- *extra_clauses)
- # XXX: BradCrittenden 2009-05-07 bug=373228: A bug in Storm prevents
- # setting the 'distinct' and 'limit' options in a single call to
- # .config(). The work-around is to split them up. Note the limit has
- # to be after the call to 'order_by' for this work-around to be
- # effective.
- result.config(distinct=True)
- if exact_match is not None:
- # A DISTINCT requires that the sort parameters appear in the
- # select, but it will break the vocabulary if it returns a list of
- # tuples instead of a list of Person objects, so we create
- # another subselect to sort after the DISTINCT is done.
- distinct_subselect = Alias(result._get_select(), 'Person')
- result = self.store.using(distinct_subselect).find(Person)
- result.order_by(
- Desc(exact_match), Person.displayname, Person.name)
- else:
- result.order_by(Person.displayname, Person.name)
- result.config(limit=self.LIMIT)
- return result
-
- def _doSearchWithImprovedSorting(self, text="", vocab_filter=None):
- """Return the people/teams whose fti or email address match :text:"""
private_query, private_tables = self._privateTeamQueryAndTables()
extra_clauses = [self.extra_clause]
@@ -889,8 +729,7 @@
*extra_clauses),
)
# Better ranked matches go first.
- if (getFeatureFlag('disclosure.person_affiliation_rank.enabled')
- and self._karma_context_constraint):
+ if self._karma_context_constraint:
rank_order = SQL("""
rank * COALESCE(
(SELECT LOG(karmavalue) FROM KarmaCache
@@ -980,14 +819,11 @@
self.extra_clause)
result = self.store.using(*tables).find(Person, query)
else:
- if self.enhanced_picker_enabled:
- name_match_query = SQL("""
- Person.name LIKE ? || '%%'
- OR lower(Person.displayname) LIKE ? || '%%'
- OR Person.fti @@ ftq(?)
- """, [text, text, text]),
- else:
- name_match_query = SQL("Person.fti @@ ftq(%s)" % quote(text))
+ name_match_query = SQL("""
+ Person.name LIKE ? || '%%'
+ OR lower(Person.displayname) LIKE ? || '%%'
+ OR Person.fti @@ ftq(?)
+ """, [text, text, text]),
email_storm_query = self.store.find(
EmailAddress.personID,
@@ -2049,12 +1885,7 @@
obj.__class__.__name__, obj.id)
obj = obj.pillar
- enhanced = bool(getFeatureFlag(
- 'disclosure.target_picker_enhancements.enabled'))
- if enhanced:
- title = '%s' % obj.title
- else:
- title = '%s (%s)' % (obj.title, obj.pillar_category)
+ title = '%s' % obj.title
return SimpleTerm(obj, obj.name, title)
def getTermByToken(self, token):
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py 2011-10-14 05:00:27 +0000
+++ lib/lp/services/features/flags.py 2011-10-20 00:58:24 +0000
@@ -113,26 +113,6 @@
'Enables the use of the new DistributionSourcePackage vocabulary for '
'the source and binary package name pickers.',
''),
- ('disclosure.picker_enhancements.enabled',
- 'boolean',
- ('Enables the display of extra details in the person picker.'),
- ''),
- ('disclosure.picker_expander.enabled',
- 'boolean',
- ('Enables the expanding of extra details in the person picker.'),
- ''),
- ('disclosure.personpicker_affiliation.enabled',
- 'boolean',
- ('Enables display of affiliation details in the person picker.'),
- ''),
- ('disclosure.person_affiliation_rank.enabled',
- 'boolean',
- ('Enables ranking by pillar affiliation in the person picker.'),
- ''),
- ('disclosure.target_picker_enhancements.enabled',
- 'boolean',
- ('Enables the display and use of the enhanced target pickers.'),
- ''),
('disclosure.private_bug_visibility_rules.enabled',
'boolean',
('Enables the application of additional privacy filter terms in bug '
