From 7ee39ccf886e890f8914860e9edbab201b377418 Mon Sep 17 00:00:00 2001 From: Daniel Smith <56164590+DanielRyanSmith@users.noreply.github.com> Date: Thu, 5 Jan 2023 16:02:13 -0800 Subject: [PATCH] make trial extension fields editable (#2624) --- api/converters.py | 19 ++++++++- api/stages_api.py | 11 +++++ internals/core_enums.py | 7 +++ pages/guide.py | 95 +++++++++++++++++++++++++++++------------ 4 files changed, 102 insertions(+), 30 deletions(-) diff --git a/api/converters.py b/api/converters.py index 40ccbbcb..6189a7cd 100644 --- a/api/converters.py +++ b/api/converters.py @@ -250,8 +250,6 @@ def _prep_stage_gate_info( # Write a list of stages associated with the feature. d['stages'] = [] - # TODO(danielrsmith): This approach should be removed or refactored when - # functionality for creating multiple stages is added. for s in stages: d['stages'].append(stage_to_json_dict(s, fe.feature_type)) # Keep major stages for referencing additional fields. @@ -271,6 +269,21 @@ def _prep_stage_gate_info( return major_stages +# This function is a workaround to reference extension stage info on +# origin trial stages. +# TODO(danielrsmith): Remove this once users can manually add trial extension +# stages. +def _add_ot_extension_fields(d: dict): + """Adds info from the trial extension stage to the OT stage dict.""" + extension_stage = Stage.query( + Stage.ot_stage_id == d['id']).get() + if extension_stage is None: + return + d['experiment_extension_reason'] = extension_stage.experiment_extension_reason + d['intent_to_extend_experiment_url'] = ( + extension_stage.intent_thread_url) + + def stage_to_json_dict( stage: Stage, feature_type: int | None=None) -> dict[str, Any]: """Convert a stage entity into a JSON dict.""" @@ -298,9 +311,11 @@ def stage_to_json_dict( d['experiment_goals'] = stage.experiment_goals d['experiment_risks'] = stage.experiment_risks d['origin_trial_feedback_url'] = stage.origin_trial_feedback_url + _add_ot_extension_fields(d) milestone_field_names = MilestoneSet.OT_MILESTONE_FIELD_NAMES elif d['stage_type'] == STAGE_TYPES_EXTEND_ORIGIN_TRIAL[feature_type]: d['experiment_extension_reason'] = stage.experiment_extension_reason + d['intent_to_extend_experiment_url'] = stage.intent_thread_url d['ot_stage_id'] = stage.ot_stage_id elif d['stage_type'] == STAGE_TYPES_SHIPPING[feature_type]: d['intent_to_ship_url'] = stage.intent_thread_url diff --git a/api/stages_api.py b/api/stages_api.py index 446490c4..b11fdf2f 100644 --- a/api/stages_api.py +++ b/api/stages_api.py @@ -170,6 +170,17 @@ class StagesAPI(basehandlers.APIHandler): self._update_stage_vals( stage, feature.feature_type, use_stage_type=True, create_gate=True) + # Create an extension stage if creating an origin trial. + # This is a workaround to edit extension stage fields using the old + # paradigm of 1 field per feature. + # TODO(danielrsmith): refactor to only create extension stages as needed. + if stage.stage_type == core_enums.STAGE_TYPES_ORIGIN_TRIAL[ + feature.feature_type]: + extension_stage = Stage(feature_id=feature_id, + stage_type=core_enums.OT_EXTENSION_STAGE_TYPES_MAPPING[ + stage.stage_type], ot_stage_id=stage.key.integer_id()) + extension_stage.put() + # Changing stage values means the cached feature should be invalidated. lookup_key = FeatureEntry.feature_cache_key( FeatureEntry.DEFAULT_CACHE_KEY, feature_id) diff --git a/internals/core_enums.py b/internals/core_enums.py index 712f53f0..1f32129d 100644 --- a/internals/core_enums.py +++ b/internals/core_enums.py @@ -325,6 +325,13 @@ STAGE_TYPES_ROLLOUT: dict[int, Optional[int]] = { FEATURE_TYPE_ENTERPRISE_ID: STAGE_ENT_ROLLOUT } +# Origin trial stage types mapped to extension stage types. +OT_EXTENSION_STAGE_TYPES_MAPPING: dict[int, int] = { + STAGE_BLINK_ORIGIN_TRIAL: STAGE_BLINK_EXTEND_ORIGIN_TRIAL, + STAGE_FAST_ORIGIN_TRIAL: STAGE_FAST_EXTEND_ORIGIN_TRIAL, + STAGE_DEP_DEPRECATION_TRIAL: STAGE_DEP_EXTEND_DEPRECATION_TRIAL, +} + # Mapping of original field names to the new stage types the fields live on. STAGE_TYPES_BY_FIELD_MAPPING: dict[str, dict[int, Optional[int]]] = { 'finch_url': STAGE_TYPES_SHIPPING, diff --git a/pages/guide.py b/pages/guide.py index beee4c42..9836b5ba 100644 --- a/pages/guide.py +++ b/pages/guide.py @@ -111,6 +111,11 @@ class FeatureCreateHandler(basehandlers.FlaskHandler): ot_stage_id: Optional[int] = None for stage_type, gate_type in stages_gates: stage = Stage(feature_id=feature_id, stage_type=stage_type) + # If we are creating a trial extension gate, + # then the trial extension stage was just created. + # Associate the origin trial stage id with the extension stage. + if gate_type == core_enums.GATE_EXTEND_ORIGIN_TRIAL: + stage.ot_stage_id = ot_stage_id stage.put() # Not all stages have gates. If a gate is specified, create it. if gate_type: @@ -120,12 +125,6 @@ class FeatureCreateHandler(basehandlers.FlaskHandler): ot_stage_id = stage.key.integer_id() gate = Gate(feature_id=feature_id, stage_id=stage.key.integer_id(), gate_type=gate_type, state=Gate.PREPARING) - - # If we are creating a trial extension gate, - # then the trial extension stage was just created. - # Associate the origin trial stage id with the extension stage. - if gate_type == core_enums.GATE_EXTEND_ORIGIN_TRIAL: - stage.ot_stage_id = ot_stage_id gate.put() @@ -206,26 +205,24 @@ class FeatureEditHandler(basehandlers.FlaskHandler): # Field name, data type STAGE_FIELDS: list[tuple[str, str]] = [ - ('intent_to_implement_url', 'link'), - ('intent_to_ship_url', 'link'), ('ready_for_trial_url', 'link'), - ('intent_to_experiment_url', 'link'), - ('intent_to_extend_experiment_url', 'link'), ('origin_trial_feedback_url', 'link'), ('finch_url', 'link'), ('experiment_goals', 'str'), ('experiment_risks', 'str'), - ('experiment_extension_reason', 'str'), ('rollout_milestone', 'int'), ('rollout_platforms', 'split_str'), ('rollout_details', 'str'), ('enterprise_policies', 'split_str'), ] + OT_EXTENSION_FIELDS: list[tuple[str, str]] = [ + ('intent_to_extend_experiment_url', 'link'), + ('experiment_extension_reason', 'str')] + INTENT_FIELDS: list[str] = [ 'intent_to_implement_url', 'intent_to_experiment_url', - 'intent_to_extend_experiment_url', 'intent_to_ship_url' ] @@ -405,7 +402,8 @@ class FeatureEditHandler(basehandlers.FlaskHandler): # If a stage_id is supplied, we make changes to only that specific stage. if stage_update_items and stage_id: - self.update_single_stage(stage_id, stage_update_items, changed_fields) + self.update_single_stage( + stage_id, fe.feature_type, stage_update_items, changed_fields) # Otherwise, we find the associated stages and make changes (edit-all). elif stage_update_items: self.update_multiple_stages(feature_id, feature.feature_type, @@ -445,6 +443,34 @@ class FeatureEditHandler(basehandlers.FlaskHandler): redirect_url = '/guide/edit/' + str(key.integer_id()) return self.redirect(redirect_url) + # TODO(danielrsmith): This method should be removed and extension stage + # fields should be handled the same as other stages once users can create + # trial extension stages. + def _handle_ot_extension_stage_changes(self, ot_stage_id: int, + changed_fields: list[tuple[str, Any, Any]], + extension_stage: Stage | None=None, + is_from_editall: bool=False): + if extension_stage is None: + extension_stage = Stage.query(Stage.ot_stage_id == ot_stage_id).get() + if extension_stage is None: + return + + for field, field_type in self.OT_EXTENSION_FIELDS: + # Update the field's name if it has been renamed. + old_field_name = field + field = self.RENAMED_FIELD_MAPPING.get(field, field) + old_val = getattr(extension_stage, field) + new_val = None + if is_from_editall: + new_val = self._get_field_val( + f'{old_field_name}__{ot_stage_id}', field_type) + else: + new_val = self._get_field_val(old_field_name, field_type) + if old_val != new_val: + setattr(extension_stage, field, new_val) + changed_fields.append((old_field_name, old_val, new_val)) + extension_stage.put() + def update_multiple_stages(self, feature_id: int, feature_type: int, update_items: list[tuple[str, Any]], changed_fields: list[tuple[str, Any, Any]]) -> None: @@ -500,7 +526,7 @@ class FeatureEditHandler(basehandlers.FlaskHandler): for stage in stages_by_type: stage.put() - def update_single_stage(self, stage_id: int, + def update_single_stage(self, stage_id: int, feature_type: int, update_items: list[tuple[str, Any]], changed_fields: list[tuple[str, Any, Any]]) -> None: """Update the fields of the stage of a given ID.""" @@ -522,6 +548,12 @@ class FeatureEditHandler(basehandlers.FlaskHandler): (changed_field, stage_to_update.intent_thread_url, intent_thread_val)) setattr(stage_to_update, 'intent_thread_url', intent_thread_val) + # TODO(danielrsmith): Remove this and handle extension fields properly + # once the ability to add a trial extension is available to users. + if (stage_to_update.stage_type == + core_enums.STAGE_TYPES_ORIGIN_TRIAL[feature_type]): + self._handle_ot_extension_stage_changes(stage_id, changed_fields) + for field, new_val in update_items: # Update the field's name if it has been renamed. old_field_name = field @@ -549,6 +581,14 @@ class FeatureEditHandler(basehandlers.FlaskHandler): if not stage: self.abort(404, msg=f'No stage {id} found') + # TODO(danielrsmith): Remove this and handle extension fields properly + # once the ability to add a trial extension is available to users. + if (stage.stage_type == + core_enums.STAGE_TYPES_EXTEND_ORIGIN_TRIAL[feature_type]): + self._handle_ot_extension_stage_changes( + stage.ot_stage_id, changed_fields, + extension_stage=stage, is_from_editall=True) + # Update the stage-specific fields. for field, field_type in self.STAGE_FIELDS: # To differentiate stages that have the same fields, the stage ID @@ -560,29 +600,28 @@ class FeatureEditHandler(basehandlers.FlaskHandler): setattr(stage, new_field_name, new_val) changed_fields.append((field, old_val, new_val)) - # Determine if 'intent_thread_url' field needs to be changed. - intent_thread_val = None - changed_field = None - for field in self.INTENT_FIELDS: - field_val = self._get_field_val(f'{field}__{id}', 'link') - if field_val is not None: - intent_thread_val = field_val - changed_field = field - break - if changed_field is not None: - changed_fields.append( - (changed_field, stage.intent_thread_url, intent_thread_val)) - setattr(stage, 'intent_thread_url', intent_thread_val) - + intent_thread_field= None milestone_fields = [] # Determine if the stage type is one with specific milestone fields. + if stage.stage_type == core_enums.STAGE_TYPES_PROTOTYPE[feature_type]: + intent_thread_field = 'intent_to_implement_url' if stage.stage_type == core_enums.STAGE_TYPES_DEV_TRIAL[feature_type]: milestone_fields = self.DEV_TRIAL_MILESTONE_FIELDS if stage.stage_type == core_enums.STAGE_TYPES_ORIGIN_TRIAL[feature_type]: + intent_thread_field = 'intent_to_experiment_url' milestone_fields = self.OT_MILESTONE_FIELDS if stage.stage_type == core_enums.STAGE_TYPES_SHIPPING[feature_type]: + intent_thread_field = 'intent_to_ship_url' milestone_fields = self.SHIPPING_MILESTONE_FIELDS + # Determine if 'intent_thread_url' field needs to be changed. + if intent_thread_field: + old_val = stage.intent_thread_url + new_val = self._get_field_val(f'{intent_thread_field}__{id}', 'link') + if old_val != new_val: + changed_fields.append((intent_thread_field, old_val, new_val)) + setattr(stage, 'intent_thread_url', new_val) + for field, milestone_field in milestone_fields: field_with_id = f'{field}__{id}' old_val = None