Allow editing of first_notification_milestone for current release. (#6127)

Author: jrobbins

api/features_api_test.py

@@ -925,8 +925,14 @@ def test_patch__enterprise_first_notice_wrong_non_enterprise_feature(self, mock_
   @mock.patch('api.channels_api.construct_specified_milestones_details')
   def test_patch__enterprise_first_notice_enterprise_feature(self, mock_call):
     """PATCH request successful with provided first_enterprise_notification_milestone."""
-    stable_date = _datetime_to_str(datetime.now().replace(year=datetime.now().year + 1, day=1))
-    mock_call.return_value = { 100: { 'version': 100, 'stable_date': stable_date } }
+    stable_date = _datetime_to_str(datetime.now().replace(
+        year=datetime.now().year, day=1))
+    next_stable_date = _datetime_to_str(datetime.now().replace(
+        year=datetime.now().year + 1, day=1))
+    mock_call.return_value = {
+        100: { 'version': 100, 'stable_date': stable_date },
+        101: { 'version': 101, 'stable_date': next_stable_date },
+    }
 
     # Signed-in user with permissions.
     testing_config.sign_in('admin@example.com', 123567890)
@@ -957,8 +963,14 @@ def test_patch__enterprise_first_notice_enterprise_feature(self, mock_call):
   @mock.patch('api.channels_api.construct_specified_milestones_details')
   def test_patch__enterprise_first_notice_newly_breaking_feature(self, mock_call):
     """PATCH request successful with provided first_enterprise_notification_milestone."""
-    stable_date = _datetime_to_str(datetime.now().replace(year=datetime.now().year + 1, day=1))
-    mock_call.return_value = { 100: { 'version': 100, 'stable_date': stable_date } }
+    stable_date = _datetime_to_str(datetime.now().replace(
+        year=datetime.now().year, day=1))
+    next_stable_date = _datetime_to_str(datetime.now().replace(
+        year=datetime.now().year + 1, day=1))
+    mock_call.return_value = {
+        100: { 'version': 100, 'stable_date': stable_date },
+        101: { 'version': 101, 'stable_date': next_stable_date },
+    }
 
     # Signed-in user with permissions.
     testing_config.sign_in('admin@example.com', 123567890)

internals/enterprise_helpers.py

@@ -85,19 +85,22 @@ def is_update_first_notification_milestone(feature: FeatureEntry, new_fields: di
     return False
   milestone = int(milestone)
 
-  # We cannot set a milestone that has already been released
-  milestone_details = channels_api.construct_specified_milestones_details(milestone, milestone)
-  if (milestone not in milestone_details or
-      _str_to_datetime(milestone_details[milestone]['stable_date']) <= datetime.now()):
+  # We don't allow setting an old milestone, but allow current and future.
+  next_milestone = milestone + 1
+  next_milestone_details = channels_api.construct_specified_milestones_details(
+      next_milestone, next_milestone)
+  if (next_milestone not in next_milestone_details or
+      _str_to_datetime(next_milestone_details[next_milestone]['stable_date']) <= datetime.now()):
     return False
 
-  # We cannot update the milestone of a feature that has already been announced
+  # We don't allow changing the existing milestone value if it was in old release notes.
   if feature.first_enterprise_notification_milestone != None:
-    milestone = feature.first_enterprise_notification_milestone
-    previous_milestone_details = channels_api.construct_specified_milestones_details(milestone,
-                                                                                     milestone)
-    if (milestone in previous_milestone_details and
-        _str_to_datetime(previous_milestone_details[milestone]['stable_date']) <= datetime.now()):
+    existing_milestone = feature.first_enterprise_notification_milestone
+    existing_next = existing_milestone + 1
+    existing_next_details = channels_api.construct_specified_milestones_details(
+        existing_next, existing_next)
+    if (existing_next in existing_next_details and
+        _str_to_datetime(existing_next_details[existing_next]['stable_date']) <= datetime.now()):
       return False
 
   if feature.feature_type == FEATURE_TYPE_ENTERPRISE_ID:

internals/enterprise_helpers_test.py

@@ -53,12 +53,12 @@ def tearDown(self):
   @mock.patch('api.channels_api.construct_specified_milestones_details')
   def test__needs_default_first_notification_milestone__new_feature(self, mock_specified_milestones):
     mock_specified_milestones.return_value = {
-        99: { 
-          'version': 99, 
+        99: {
+          'version': 99,
           'stable_date': self.now.replace(year=self.now.year - 1, day=1).strftime(DATETIME_FORMAT)
         },
-        100: { 
-          'version': 100, 
+        100: {
+          'version': 100,
           'stable_date': self.now.replace(year=self.now.year + 1, day=1).strftime(DATETIME_FORMAT)
         },
     }
@@ -124,7 +124,7 @@ def test__needs_default_first_notification_milestone__new_feature(self, mock_spe
 
   @mock.patch('api.channels_api.construct_specified_milestones_details')
   def test__needs_default_first_notification_milestone__update(self, mock_specified_milestones):
-    
+
     mock_specified_milestones.return_value =  {
         99: {
           'version': 99,
@@ -187,7 +187,7 @@ def test__needs_default_first_notification_milestone__update(self, mock_specifie
     self.assertFalse(needs_default_first_notification_milestone(
       self.normal_feature,
       { 'enterprise_impact': ENTERPRISE_IMPACT_LOW, 'first_enterprise_notification_milestone': 100}))
-  
+
     # Breaking feature becoming normal feature missing the milestone
     self.assertFalse(needs_default_first_notification_milestone(
       self.breaking_feature, { 'enterprise_impact': ENTERPRISE_IMPACT_NONE}))
@@ -223,19 +223,23 @@ def test__is_update_first_notification_milestone(self,
           'version': 99,
           'stable_date': self.now.replace(year=self.now.year - 1, day=1).strftime(DATETIME_FORMAT)
         },
-        100: {
+        100: {  # Current milestone on stable channel
           'version': 100,
-          'stable_date': self.now.replace(year=self.now.year + 1, day=1).strftime(DATETIME_FORMAT)
+          'stable_date': self.now.replace(year=self.now.year, day=1).strftime(DATETIME_FORMAT)
         },
         101: {
           'version': 101,
+          'stable_date': self.now.replace(year=self.now.year + 1, day=1).strftime(DATETIME_FORMAT)
+        },
+        102: {
+          'version': 102,
           'stable_date': self.now.replace(year=self.now.year + 2, day=1).strftime(DATETIME_FORMAT)
         },
     }
     mock_channel_details.return_value = {
       'beta': {
         'version': 100,
-        'stable_date': self.now.replace(year=self.now.year + 1, day=1).strftime(DATETIME_FORMAT)
+        'stable_date': self.now.replace(year=self.now.year, day=1).strftime(DATETIME_FORMAT)
       }
     }
 
@@ -289,7 +293,7 @@ def test__is_update_first_notification_milestone(self,
     self.assertTrue(is_update_first_notification_milestone(
       self.normal_feature,
       { 'enterprise_impact': ENTERPRISE_IMPACT_LOW, 'first_enterprise_notification_milestone': 100}))
-  
+
     # Breaking feature becoming normal feature missing the milestone
     self.assertFalse(is_update_first_notification_milestone(
       self.breaking_feature, { 'enterprise_impact': ENTERPRISE_IMPACT_NONE}))
@@ -368,7 +372,7 @@ def test__should_remove_first_notice_milestone(self, mock_specified_milestones):
           'stable_date': now.replace(year=now.year + 2, day=1).strftime(DATETIME_FORMAT)
         },
     }
-    
+
     # Enterprise feature with no changes and no existing milestone
     self.assertFalse(should_remove_first_notice_milestone(self.enterprise_feature, {}))