launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #17335
[Merge] lp:~wgrant/launchpad/no-ic-ff into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/no-ic-ff into lp:launchpad.
Commit message:
Drop the code.inline_diff_comments.enabled feature flag. It's been enabled everywhere for months.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/no-ic-ff/+merge/231301
Drop the code.inline_diff_comments.enabled feature flag. It's been enabled everywhere for months.
--
https://code.launchpad.net/~wgrant/launchpad/no-ic-ff/+merge/231301
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/no-ic-ff into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2014-05-28 22:03:06 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2014-08-19 05:11:39 +0000
@@ -624,10 +624,6 @@
implements(IBranchMergeProposalActionMenu)
- related_features = {
- 'code.inline_diff_comments.enabled': True,
- }
-
schema = ClaimButton
def initialize(self):
@@ -642,8 +638,6 @@
if getFeatureFlag("longpoll.merge_proposals.enabled"):
cache.objects['merge_proposal_event_key'] = subscribe(
self.context).event_key
- if getFeatureFlag("code.inline_diff_comments.enabled"):
- cache.objects['inline_diff_comments'] = True
@action('Claim', name='claim')
def claim_action(self, action, data):
=== modified file 'lib/lp/code/browser/codereviewcomment.py'
--- lib/lp/code/browser/codereviewcomment.py 2014-06-04 18:09:03 +0000
+++ lib/lp/code/browser/codereviewcomment.py 2014-08-19 05:11:39 +0000
@@ -40,7 +40,6 @@
from lp.services.comments.browser.messagecomment import MessageComment
from lp.services.comments.interfaces.conversation import IComment
from lp.services.config import config
-from lp.services.features import getFeatureFlag
from lp.services.librarian.interfaces import ILibraryFileAlias
from lp.services.propertycache import (
cachedproperty,
@@ -257,11 +256,6 @@
comment = ''
return {'comment': comment}
- def setUpFields(self):
- super(CodeReviewCommentAddView, self).setUpFields()
- if not getFeatureFlag('code.inline_diff_comments.enabled'):
- self.form_fields.omit('publish_inline_comments')
-
@property
def is_reply(self):
"""True if this comment is a reply to another comment, else False."""
=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-07-08 15:25:12 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-08-19 05:11:39 +0000
@@ -261,9 +261,6 @@
};
namespace.setup_inline_comments = function(previewdiff_id) {
- if (LP.cache.inline_diff_comments !== true) {
- return;
- }
// Store the current diff ID locally.
namespace.current_previewdiff_id = previewdiff_id;
@@ -372,9 +369,6 @@
Y.extend(InlineCommentToggle, Y.Widget, {
renderUI: function() {
- if (LP.cache.inline_diff_comments !== true) {
- return;
- }
var ui = Y.Node.create('<li><label>' +
'<input type="checkbox" checked="checked" id="show-ic"/>' +
' Show comments</label></li>');
@@ -511,9 +505,6 @@
* @method update_on_new_comment
*/
update_on_new_comment: function() {
- if (LP.cache.inline_diff_comments !== true) {
- return;
- }
namespace.cleanup_comments();
namespace.populate_comments();
namespace.populate_drafts();
@@ -634,11 +625,6 @@
*/
renderUI: function() {
var self = this;
- if (LP.cache.inline_diff_comments !== true) {
- var preview_diff_uri = (LP.cache.context.web_link + '/++diff');
- self._updateDiff(preview_diff_uri);
- return;
- }
var pub_drafts_anchor = Y.one('[id="field.review_type"]');
if (pub_drafts_anchor !== null) {
=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-07-08 15:25:12 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-08-19 05:11:39 +0000
@@ -19,7 +19,6 @@
LP.links = {
me : 'something'
};
- LP.cache.inline_diff_comments = true;
},
tearDown: function () {},
@@ -182,22 +181,7 @@
Y.Assert.areEqual('diff-line-2', line.next().get('id'));
},
- test_feature_flag_off: function() {
- var called = false;
- add_doubleclick_handler = function() {
- called = true;
- };
- module.add_doubleclick_handler = add_doubleclick_handler;
- module.current_previewdiff_id = null;
-
- LP.cache.inline_diff_comments = false;
- module.setup_inline_comments(1);
-
- Y.Assert.isFalse(called);
- Y.Assert.isNull(module.current_previewdiff_id);
- },
-
- test_feature_flag: function() {
+ test_logged_in: function() {
var called = false;
add_doubleclick_handler = function() {
called = true;
@@ -211,7 +195,7 @@
Y.Assert.areEqual(1, module.current_previewdiff_id);
},
- test_feature_not_logged_in: function() {
+ test_not_logged_in: function() {
// Draft inline-comments are not loaded and doubleclick-handlers
// are not set for anonymous requests.
var doubleclick_called = false;
@@ -250,7 +234,6 @@
"~foo/bar/foobr/+merge/1/preview_diffs")
};
- LP.cache.inline_diff_comments = true;
// Disable/Instrument inlinecomments hook.
var self = this;
self.inline_comments_requested_id = null;
@@ -301,24 +284,6 @@
responseHeaders: {'Content-Type': 'text/html'}});
},
- test_diff_nav_feature_flag_disabled: function() {
- // When rendered with the corresponding feature-flag disabled,
- // Diff Navigator only updates the diff content with the latest
- // previewdiff and does not create the navigator (<select>),
- // does not fetch the inline comments nor render the
- // InlineToggle (show/hide inline comments) widget.
- LP.cache.inline_diff_comments = false;
- this.diffnav.render();
- mockio = this.diffnav.get('lp_client').io_provider;
- Y.Assert.areEqual(1, mockio.requests.length);
- Y.Assert.areSame(
- "/~foo/bar/foobr/+merge/1/++diff",
- mockio.last_request.url);
- this.reply_diffcontent();
- Y.Assert.isNull(this.diffnav.get('previewdiff_id'));
- Y.Assert.isNull(Y.one('.diff-content').one('#show-ic'));
- },
-
test_diff_nav_rendering: function() {
// When rendered the Diff Navigator fetches the available diffs
// collection and builds the selector, fetches and displays
=== modified file 'lib/lp/code/mail/tests/test_codereviewcomment.py'
--- lib/lp/code/mail/tests/test_codereviewcomment.py 2014-05-21 09:33:04 +0000
+++ lib/lp/code/mail/tests/test_codereviewcomment.py 2014-08-19 05:11:39 +0000
@@ -22,10 +22,8 @@
from lp.services.messages.interfaces.message import IMessageSet
from lp.services.webapp import canonical_url
from lp.testing import (
- feature_flags,
login,
login_person,
- set_feature_flag,
TestCaseWithFactory,
)
from lp.testing.layers import LaunchpadFunctionalLayer
@@ -240,10 +238,6 @@
See `build_inline_comments_section` tests for formatting details.
"""
- # Enabled corresponding feature flag.
- self.useContext(feature_flags())
- set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
-
comment = self.makeCommentWithInlineComments(
inline_comments={'2': u'Is this from Planet Earth\xa9 ?'})
mailer = CodeReviewCommentMailer.forCreation(comment)
@@ -266,21 +260,6 @@
]
self.assertEqual(expected_lines, ctrl.body.splitlines()[1:10])
- def test_generateEmailWithInlineComments_feature_disabled(self):
- """Inline comments are not considered if the flag is not enabled."""
- # Do not enable (set, actually) the corresponding feature flag
- # ('code.inline_diff_comments.enabled').
- content = 'CoNtEnT'
- comment = self.makeCommentWithInlineComments(content=content)
- mailer = CodeReviewCommentMailer.forCreation(comment)
- commenter = comment.branch_merge_proposal.registrant
- ctrl = mailer.generateEmail(
- commenter.preferredemail.email, commenter)
- # Only the comment content (footer is ignored) is included in
- # email body.
- self.assertEqual(
- [content], ctrl.body.splitlines()[:-3])
-
def makeComment(self, email_message):
message = getUtility(IMessageSet).fromEmail(email_message.as_string())
bmp = self.factory.makeBranchMergeProposal()
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2014-05-21 02:41:19 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2014-08-19 05:11:39 +0000
@@ -76,7 +76,6 @@
IncrementalDiff,
PreviewDiff,
)
-from lp.services.features import getFeatureFlag
from lp.registry.interfaces.person import (
IPerson,
IPersonSet,
@@ -789,16 +788,15 @@
message, vote, review_type, original_email=None,
_notify_listeners=_notify_listeners, _validate=False)
- if getFeatureFlag("code.inline_diff_comments.enabled"):
- if inline_comments:
- assert previewdiff_id is not None, (
- 'Inline comments must be associated with a '
- 'previewdiff ID.')
- previewdiff = self.getPreviewDiff(previewdiff_id)
- getUtility(ICodeReviewInlineCommentSet).ensureDraft(
- previewdiff, owner, inline_comments)
- getUtility(ICodeReviewInlineCommentSet).publishDraft(
- previewdiff, owner, comment)
+ if inline_comments:
+ assert previewdiff_id is not None, (
+ 'Inline comments must be associated with a '
+ 'previewdiff ID.')
+ previewdiff = self.getPreviewDiff(previewdiff_id)
+ getUtility(ICodeReviewInlineCommentSet).ensureDraft(
+ previewdiff, owner, inline_comments)
+ getUtility(ICodeReviewInlineCommentSet).publishDraft(
+ previewdiff, owner, comment)
return comment
@@ -914,8 +912,6 @@
def saveDraftInlineComment(self, previewdiff_id, person, comments):
"""See `IBranchMergeProposal`."""
- if not getFeatureFlag("code.inline_diff_comments.enabled"):
- return
previewdiff = self.getPreviewDiff(previewdiff_id)
getUtility(ICodeReviewInlineCommentSet).ensureDraft(
previewdiff, person, comments)
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py 2014-06-19 10:03:29 +0000
+++ lib/lp/code/model/tests/test_branch.py 2014-08-19 05:11:39 +0000
@@ -1416,8 +1416,6 @@
preview_diff = self.factory.makePreviewDiff(
merge_proposal=merge_proposal)
transaction.commit()
- self.useFixture(FeatureFixture({
- 'code.inline_diff_comments.enabled': 'enabled'}))
merge_proposal.saveDraftInlineComment(
previewdiff_id=preview_diff.id,
person=self.user,
@@ -1432,8 +1430,6 @@
preview_diff = self.factory.makePreviewDiff(
merge_proposal=merge_proposal)
transaction.commit()
- self.useFixture(FeatureFixture({
- 'code.inline_diff_comments.enabled': 'enabled'}))
merge_proposal.createComment(
owner=self.user,
subject='Delete me!',
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2014-06-10 16:13:03 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2014-08-19 05:11:39 +0000
@@ -68,12 +68,10 @@
from lp.services.webapp import canonical_url
from lp.testing import (
ExpectedException,
- feature_flags,
launchpadlib_for,
login,
login_person,
person_logged_in,
- set_feature_flag,
TestCaseWithFactory,
verifyObject,
WebServiceTestCase,
@@ -2072,12 +2070,12 @@
self.assertNotIn(r1, partial_revisions)
-class TestBranchMergeProposalInlineCommentsBase(TestCaseWithFactory):
+class TestBranchMergeProposalInlineComments(TestCaseWithFactory):
layer = LaunchpadFunctionalLayer
def setUp(self):
- super(TestBranchMergeProposalInlineCommentsBase, self).setUp()
+ super(TestBranchMergeProposalInlineComments, self).setUp()
# Create a testing IPerson, IPreviewDiff and IBranchMergeProposal
# for tests. Log in as the testing IPerson.
self.person = self.factory.makePerson()
@@ -2104,46 +2102,6 @@
return self.bmp.getDraftInlineComments(
previewdiff_id, person)
-
-class TestBranchMergeProposalInlineCommentsDisabled(
- TestBranchMergeProposalInlineCommentsBase):
-
- layer = LaunchpadFunctionalLayer
-
- def test_save_drafts(self):
- # 'saveDraftInlineComment' does not record draft inline comments
- # if the corresponding feature-flag is not set.
- self.bmp.saveDraftInlineComment(
- previewdiff_id=self.previewdiff.id,
- person=self.person,
- comments={'10': 'No game'})
- self.assertIsNone(self.getDraft())
-
- def test_publish(self):
- # `createComment` does not publish given inline comments
- # if the corresponding feature-flag is not set. The MP comment
- # is created. The MP comment itself is recorded.
- self.bmp.createComment(
- owner=self.bmp.registrant,
- subject='Testing!',
- previewdiff_id=self.previewdiff.id,
- inline_comments={'11': 'foo'},
- )
- self.assertEqual(0, len(self.getInlineComments()))
- self.assertEqual(1, self.bmp.all_comments.count())
-
-
-class TestBranchMergeProposalInlineCommentsEnabled(
- TestBranchMergeProposalInlineCommentsBase):
-
- layer = LaunchpadFunctionalLayer
-
- def setUp(self):
- super(TestBranchMergeProposalInlineCommentsEnabled, self).setUp()
- # Enabled corresponding feature flag.
- self.useContext(feature_flags())
- set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
-
def test_save_drafts(self):
# Draft inline comments, passed as a dictionary keyed by diff line
# number, can be stored for the current user using
@@ -2167,9 +2125,8 @@
DiffNotFound, self.bmp.saveDraftInlineComment, **kwargs)
def test_publish(self):
- # Existing (draft) inline comments can only be published associated
- # with an `ICodeReviewComment` if the feature flag
- # 'code.inline_diff_comments.enabled' is set.
+ # Existing (draft) inline comments can be published associated
+ # with an `ICodeReviewComment`.
self.bmp.createComment(
owner=self.bmp.registrant,
subject='Testing!',
@@ -2346,11 +2303,6 @@
def test_saveDraftInlineComment_with_no_previewdiff(self):
# Failure on context diff mismatch.
-
- # Enabled inline_diff feature.
- self.useContext(feature_flags())
- set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
-
bmp = self.factory.makeBranchMergeProposal()
ws_bmp = self.wsObject(bmp, user=bmp.target_branch.owner)
@@ -2362,11 +2314,6 @@
# Creating and retrieving draft inline comments.
# These operations require an logged in user with permission
# to view the BMP.
-
- # Enabled inline_diff feature.
- self.useContext(feature_flags())
- set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
-
previewdiff = self.factory.makePreviewDiff()
proposal = previewdiff.branch_merge_proposal
@@ -2382,11 +2329,6 @@
def test_getInlineComment(self):
# Publishing and retrieving inline comments.
-
- # Enabled inline_diff feature.
- self.useContext(feature_flags())
- set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
-
previewdiff = self.factory.makePreviewDiff()
proposal = previewdiff.branch_merge_proposal
user = proposal.target_branch.owner
=== modified file 'lib/lp/code/stories/branches/xx-code-review-comments.txt'
--- lib/lp/code/stories/branches/xx-code-review-comments.txt 2014-05-29 07:27:25 +0000
+++ lib/lp/code/stories/branches/xx-code-review-comments.txt 2014-08-19 05:11:39 +0000
@@ -198,12 +198,7 @@
# Created a new review comment with associated inline comments
# on the superseded and on the new MP.
- >>> from lp.services.features.testing import FeatureFixture
- >>> fixture = FeatureFixture(
- ... {'code.inline_diff_comments.enabled': 'enabled'})
-
>>> login('foo.bar@xxxxxxxxxxxxx')
- >>> fixture.setUp()
>>> previewdiff = factory.makePreviewDiff(merge_proposal=merge_proposal)
>>> new_previewdiff = factory.makePreviewDiff(
... merge_proposal=new_merge_proposal)
@@ -219,7 +214,6 @@
... previewdiff_id=new_previewdiff.id,
... inline_comments={'1': 'Yes!'}
... )
- >>> fixture.cleanUp()
>>> logout()
# helper for printing review comment 'data-' attributes.
=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py 2014-06-27 12:50:55 +0000
+++ lib/lp/scripts/tests/test_garbo.py 2014-08-19 05:11:39 +0000
@@ -111,10 +111,8 @@
from lp.soyuz.model.livefsbuild import LiveFSFile
from lp.soyuz.model.reporting import LatestPersonSourcePackageReleaseCache
from lp.testing import (
- feature_flags,
+ FakeAdapterMixin,
person_logged_in,
- set_feature_flag,
- FakeAdapterMixin,
TestCase,
TestCaseWithFactory,
)
@@ -632,10 +630,7 @@
mp1_diff_draft = self.factory.makePreviewDiff(
merge_proposal=mp1, date_created=now - timedelta(hours=1))
mp1_diff = self.factory.makePreviewDiff(merge_proposal=mp1)
- # Enabled 'inline_diff_comments' feature flag and attach comments
- # on the old diffs, so they are kept in DB.
- self.useContext(feature_flags())
- set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
+ # Attach comments on the old diffs, so they are kept in DB.
mp1.createComment(
owner=mp1.registrant, previewdiff_id=mp1_diff_comment.id,
subject='Hold this diff!', inline_comments={'1': '!!!'})
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py 2014-07-31 04:48:07 +0000
+++ lib/lp/services/features/flags.py 2014-08-19 05:11:39 +0000
@@ -221,12 +221,6 @@
'disabled',
'',
''),
- ('code.inline_diff_comments.enabled',
- 'boolean',
- 'If true, reviewers can comment on parts of merge proposal diffs.',
- 'disabled',
- 'Inline diff comments',
- ''),
('soyuz.ppa.separate_long_descriptions',
'boolean',
'If true, PPAs will create an i18n/Translations-en file',
Follow ups
