Allow users to add new stages without having to reload the edit all page and losing their uunsaved data (#3082)

- Modify the "Add stage dialog" to receive a custom submit handler that replaces the default behavior of creating a new stage.
- Modify the edit all page to add a stage on the client only via a custom submit handler that it passes to the "Add stage dialog"
- Modify the form field names of stages added only on the client so that they can be created on submit
- Modify the server so that it creates new stages for the stages that were only created on the client
This commit is contained in:
Yann Dago 2023-06-15 11:42:41 -04:00 коммит произвёл GitHub
Родитель c8fbdbe10f
Коммит 566bd362bb
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: 4AEE18F83AFDEB23
9 изменённых файлов: 179 добавлений и 25 удалений

Просмотреть файл

@ -109,22 +109,17 @@ class StagesAPI(basehandlers.APIHandler):
setattr(stage.milestones, field, body[field])
# Keep the gate type that might need to be created for the stage type.
gate_type: int | None = None
gate_type: int | None = stage_helpers.get_gate_for_stage(feature_type, s_type)
# Update type-specific fields.
if s_type == core_enums.STAGE_TYPES_DEV_TRIAL[feature_type]: # pragma: no cover
gate_type = core_enums.GATE_API_PROTOTYPE
if s_type == core_enums.STAGE_TYPES_ORIGIN_TRIAL[feature_type]:
self._add_given_stage_vals(stage, body, self.OT_FIELDS)
gate_type = core_enums.GATE_API_ORIGIN_TRIAL
if s_type == core_enums.STAGE_TYPES_EXTEND_ORIGIN_TRIAL[feature_type]:
self._add_given_stage_vals(stage, body, self.OT_EXTENSION_FIELDS)
gate_type = core_enums.GATE_API_EXTEND_ORIGIN_TRIAL
if s_type == core_enums.STAGE_TYPES_SHIPPING[feature_type]: # pragma: no cover
self._add_given_stage_vals(stage, body, self.SHIPPING_FIELDS)
gate_type = core_enums.GATE_API_SHIP
if s_type == core_enums.STAGE_TYPES_ROLLOUT[feature_type]: # pragma: no cover
self._add_given_stage_vals(stage, body, self.ENTERPRISE_FIELDS)

Просмотреть файл

@ -7,11 +7,14 @@ let addStageDialogEl;
let currentFeatureId;
export async function openAddStageDialog(featureId, featureType) {
if (!addStageDialogEl || currentFeatureId !== featureId) {
export async function openAddStageDialog(featureId, featureType, onSubmitCustomHandler) {
if (!addStageDialogEl ||
currentFeatureId !== featureId ||
onSubmitCustomHandler !== addStageDialogEl.onSubmitCustomHandler) {
addStageDialogEl = document.createElement('chromedash-add-stage-dialog');
addStageDialogEl.featureId = featureId;
addStageDialogEl.featureType = featureType;
addStageDialogEl.onSubmitCustomHandler = onSubmitCustomHandler;
document.body.appendChild(addStageDialogEl);
await addStageDialogEl.updateComplete;
}
@ -26,6 +29,7 @@ class ChromedashAddStageDialog extends LitElement {
featureId: {type: Number},
featureType: {type: Number},
canSubmit: {type: Boolean},
onSubmitCustomHandler: {type: Function},
};
}
@ -33,6 +37,7 @@ class ChromedashAddStageDialog extends LitElement {
super();
this.featureId = 0;
this.featureType = 0;
this.onSubmitCustomHandler = null;
this.canSubmit = false;
}
@ -78,6 +83,12 @@ class ChromedashAddStageDialog extends LitElement {
}
handleStageCreate() {
if (this.onSubmitCustomHandler) {
this.onSubmitCustomHandler({stage_type: Number(this.getStageSelectValue())});
this.onSubmitCustomHandler = null;
this.shadowRoot.querySelector('sl-dialog').hide();
return;
}
window.csClient.createStage(this.featureId, {stage_type: this.getStageSelectValue()})
.then(() => {
this.shadowRoot.querySelector('sl-dialog').hide();

Просмотреть файл

@ -16,6 +16,7 @@ export class ChromedashFormField extends LitElement {
loading: {type: Boolean},
fieldProps: {type: Object},
forEnterprise: {type: Boolean},
stageType: {type: Number | undefined},
componentChoices: {type: Object}, // just for the blink component select field
};
}
@ -29,6 +30,7 @@ export class ChromedashFormField extends LitElement {
this.disabled = false;
this.loading = false;
this.forEnterprise = false;
this.stageType = undefined;
this.componentChoices = {};
}
@ -102,7 +104,7 @@ export class ChromedashFormField extends LitElement {
// form field name can be specified in form-field-spec to match DB field name
let fieldName = this.fieldProps.name || this.name;
if (STAGE_SPECIFIC_FIELDS.has(fieldName) && this.stageId) {
fieldName = `${fieldName}__${this.stageId}`;
fieldName = this.stageType ? `${fieldName}__${this.stageId}__${this.stageType}__create` : `${fieldName}__${this.stageId}`;
}
// choices can be specified in form-field-spec or fetched from API
const choices = this.fieldProps.choices || this.componentChoices;

Просмотреть файл

@ -101,4 +101,32 @@ describe('chromedash-form-field', () => {
assert.include(renderElement.innerHTML, 'multiple');
assert.include(renderElement.innerHTML, 'cleareable');
});
it('renders a stage\'s field with the right form field name', async () => {
const component = await fixture(
html`
<chromedash-form-field name="rollout_platforms" stageId="1">
</chromedash-form-field>`);
assert.exists(component);
assert.instanceOf(component, ChromedashFormField);
const fieldRow = component.renderRoot.querySelector('tr');
assert.exists(fieldRow);
const renderElement = component.renderRoot;
assert.include(renderElement.innerHTML, 'name="rollout_platforms__1"');
});
it('renders a stage\'s field to be created with the right form field name', async () => {
const component = await fixture(
html`
<chromedash-form-field name="rollout_platforms" stageId="10" stageType="11">
</chromedash-form-field>`);
assert.exists(component);
assert.instanceOf(component, ChromedashFormField);
const fieldRow = component.renderRoot.querySelector('tr');
assert.exists(fieldRow);
const renderElement = component.renderRoot;
assert.include(renderElement.innerHTML, 'name="rollout_platforms__10__11__create"');
});
});

Просмотреть файл

@ -43,6 +43,7 @@ export class ChromedashGuideEditallPage extends LitElement {
loading: {type: Boolean},
appTitle: {type: String},
nextPage: {type: String},
nextStageToCreateId: {type: Number},
};
}
@ -55,6 +56,7 @@ export class ChromedashGuideEditallPage extends LitElement {
this.nextPage = '';
this.previousStageTypeRendered = 0;
this.sameTypeRendered = 0;
this.nextStageToCreateId = 0;
}
connectedCallback() {
@ -218,6 +220,7 @@ export class ChromedashGuideEditallPage extends LitElement {
<chromedash-form-field
name=${field}
stageId=${feStage.id}
stageType=${feStage.to_create ? feStage.stage_type : undefined}
value=${value}
?forEnterprise=${formattedFeature.is_enterprise_feature}>
</chromedash-form-field>
@ -298,6 +301,7 @@ export class ChromedashGuideEditallPage extends LitElement {
getAllStageIds() {
const stageIds = [];
this.feature.stages.forEach(feStage => {
if (feStage.to_create) return;
stageIds.push(feStage.id);
// Check if any trial extension exist, and collect their IDs as well.
const extensions = feStage.extensions || [];
@ -310,11 +314,21 @@ export class ChromedashGuideEditallPage extends LitElement {
const text = this.feature.is_enterprise_feature ? 'Add Step': 'Add Stage';
return html`
<sl-button size="small" @click="${
() => openAddStageDialog(this.feature.id, this.feature.feature_type_int)}">
() => openAddStageDialog(this.feature.id, this.feature.feature_type_int, this.AddNewStageToCreate.bind(this))}">
${text}
</sl-button>`;
}
AddNewStageToCreate(stageType) {
this.feature.stages.push({
...stageType,
to_create: true,
id: ++this.nextStageToCreateId});
this.feature.stages = this.feature.stages
.sort((a, b) => a.stage_type_int - b.stage_type_int);
this.feature = {...this.feature};
}
renderForm() {
const formattedFeature = formatFeatureForEdit(this.feature);
const stageIds = this.getAllStageIds();

Просмотреть файл

@ -24,8 +24,13 @@ from internals.core_enums import (
STAGE_TYPES_DEV_TRIAL,
STAGE_TYPES_ORIGIN_TRIAL,
STAGE_TYPES_EXTEND_ORIGIN_TRIAL,
STAGE_TYPES_SHIPPING)
STAGE_TYPES_SHIPPING,
GATE_API_SHIP,
GATE_API_EXTEND_ORIGIN_TRIAL,
GATE_API_ORIGIN_TRIAL,
GATE_API_PROTOTYPE)
from internals.core_models import FeatureEntry, MilestoneSet, Stage
from internals.review_models import Gate
# Type return value of get_stage_info_for_templates()
@ -38,6 +43,36 @@ class StageTemplateInfo(TypedDict):
should_render_mstone_table: bool
should_render_intents: bool
def create_feature_stage(feature_id: int, feature_type: int, stage_type: int) -> Stage:
# Create the stage.
stage = Stage(feature_id=feature_id, stage_type=stage_type)
stage.put()
# If we should create a gate and this is a stage that requires a gate,
# create it.
gate_type = get_gate_for_stage(feature_type, stage_type)
if gate_type is not None:
gate = Gate(feature_id=feature_id, stage_id=stage.key.id(), gate_type=gate_type,
state=Gate.PREPARING)
gate.put()
return stage
def get_gate_for_stage(feature_type, s_type) -> int | None:
# Update type-specific fields.
if s_type == STAGE_TYPES_DEV_TRIAL[feature_type]: # pragma: no cover
return GATE_API_PROTOTYPE
if s_type == STAGE_TYPES_ORIGIN_TRIAL[feature_type]:
return GATE_API_ORIGIN_TRIAL
if s_type == STAGE_TYPES_EXTEND_ORIGIN_TRIAL[feature_type]:
return GATE_API_EXTEND_ORIGIN_TRIAL
if s_type == STAGE_TYPES_SHIPPING[feature_type]: # pragma: no cover
return GATE_API_SHIP
return None
def get_feature_stages(feature_id: int) -> dict[int, list[Stage]]:
"""Return a dictionary of stages associated with a given feature."""

Просмотреть файл

@ -23,13 +23,14 @@ class StageHelpersTest(testing_config.CustomTestCase):
def setUp(self):
self.feature_entry_1 = FeatureEntry(id=1, name='fe one',
summary='summary', category=1, impl_status_chrome=1,
summary='summary', category=1, impl_status_chrome=1, feature_type=1,
standard_maturity=1, web_dev_views=1)
self.feature_entry_1.put()
stage_types = [core_enums.STAGE_DEP_PLAN, core_enums.STAGE_DEP_DEV_TRIAL,
core_enums.STAGE_DEP_DEPRECATION_TRIAL, core_enums.STAGE_DEP_SHIPPING,
core_enums.STAGE_DEP_REMOVE_CODE]
self.feature_id = self.feature_entry_1.key.integer_id()
self.feature_type = self.feature_entry_1.feature_type
for stage_type in stage_types:
stage = Stage(feature_id=self.feature_id, stage_type=stage_type)
stage.put()
@ -50,4 +51,33 @@ class StageHelpersTest(testing_config.CustomTestCase):
for stage_type, stages_list in stage_dict.items():
self.assertTrue(stage_type in expected_stage_types)
self.assertEqual(stages_list[0].stage_type, stage_type)
expected_stage_types.remove(stage_type)
expected_stage_types.remove(stage_type)
def test_create_feature_stage(self):
"""A dictionary with stages relevant to the feature should be present."""
stage_dict = stage_helpers.get_feature_stages(self.feature_id)
list_stages = stage_dict.items()
expected_stage_types = {410, 430, 450, 460, 470}
self.assertEqual(len(list_stages), 5)
for stage_type, stages_list in stage_dict.items():
self.assertTrue(stage_type in expected_stage_types)
self.assertEqual(stages_list[0].stage_type, stage_type)
expected_stage_types.remove(stage_type)
stage_helpers.create_feature_stage(
self.feature_id,
self.feature_type,
core_enums.STAGE_ENT_ROLLOUT)
stage_helpers.create_feature_stage(
self.feature_id,
self.feature_type,
core_enums.STAGE_ENT_SHIPPED)
stage_dict = stage_helpers.get_feature_stages(self.feature_id)
list_stages = stage_dict.items()
expected_stage_types = {410, 430, 450, 460, 470, 1061, 1070}
self.assertEqual(len(list_stages), 7)
for stage_type, stages_list in stage_dict.items():
self.assertTrue(stage_type in expected_stage_types)
self.assertEqual(stages_list[0].stage_type, stage_type)
expected_stage_types.remove(stage_type)

Просмотреть файл

@ -424,7 +424,7 @@ class FeatureEditHandler(basehandlers.FlaskHandler):
if stage_ids:
stage_ids_list = [int(id) for id in stage_ids.split(',')]
self.update_stages_editall(
fe.feature_type, stage_ids_list, changed_fields, form_fields)
feature_id, fe.feature_type, stage_ids_list, changed_fields, form_fields)
# If a stage_id is supplied, we make changes to only that specific stage.
elif stage_id:
for field, field_type in self.STAGE_FIELDS:
@ -443,7 +443,7 @@ class FeatureEditHandler(basehandlers.FlaskHandler):
extension_stage_ids = self.form.get('extension_stage_ids')
if extension_stage_ids:
stage_ids_list = [int(id) for id in extension_stage_ids.split(',')]
self.update_stages_editall(fe.feature_type, stage_ids_list, changed_fields, form_fields)
self.update_stages_editall(feature_id, fe.feature_type, stage_ids_list, changed_fields, form_fields)
# Update metadata fields.
now = datetime.now()
if self.form.get('accurate_as_of'):
@ -571,12 +571,28 @@ class FeatureEditHandler(basehandlers.FlaskHandler):
def update_stages_editall(
self,
feature_id: int,
feature_type: int,
stage_ids: list[int],
changed_fields: list[tuple[str, Any, Any]],
form_fields: list[str]) -> None:
"""Handle the updates for stages on the edit-all page."""
id_to_field_suffix = {}
ids_created = set()
for field in self.form.keys():
if not field.endswith('__create'):
continue
[name, id, stage_type, suffix] = field.split('__')
if stage_type is None:
continue
if id not in ids_created:
ids_created.add(id)
stage = stage_helpers.create_feature_stage(feature_id, feature_type, int(stage_type))
id_to_field_suffix[stage.key.id()] = f'{id}__{stage_type}__create'
stage_ids.append(stage.key.id())
for id in stage_ids:
suffix = id_to_field_suffix.get(id, id)
stage = Stage.get_by_id(id)
if not stage:
self.abort(404, msg=f'No stage {id} found')
@ -584,12 +600,12 @@ class FeatureEditHandler(basehandlers.FlaskHandler):
for field, field_type in self.STAGE_FIELDS:
# To differentiate stages that have the same fields, the stage ID
# is appended to the field name with 2 underscores.
field_with_id = f'{field}__{id}'
if not self.touched(field_with_id, form_fields):
field_with_suffix = f'{field}__{suffix}'
if not self.touched(field_with_suffix, form_fields):
continue
new_field_name = self.RENAMED_FIELD_MAPPING.get(field, field)
old_val = getattr(stage, new_field_name)
new_val = self._get_field_val(field_with_id, field_type)
new_val = self._get_field_val(field_with_suffix, field_type)
setattr(stage, new_field_name, new_val)
if old_val != new_val:
changed_fields.append((field, old_val, new_val))
@ -613,20 +629,20 @@ class FeatureEditHandler(basehandlers.FlaskHandler):
milestone_fields = self.SHIPPING_MILESTONE_FIELDS
# Determine if 'intent_thread_url' field needs to be changed.
intent_field_with_id = f'{intent_thread_field}__{id}'
if intent_thread_field and self.touched(intent_field_with_id, form_fields):
intent_field_with_suffix = f'{intent_thread_field}__{suffix}'
if intent_thread_field and self.touched(intent_field_with_suffix, form_fields):
old_val = stage.intent_thread_url
new_val = self._get_field_val(intent_field_with_id, 'link')
new_val = self._get_field_val(intent_field_with_suffix, '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}'
if not self.touched(field_with_id, form_fields):
field_with_suffix = f'{field}__{suffix}'
if not self.touched(field_with_suffix, form_fields):
continue
old_val = None
new_val = self._get_field_val(field_with_id, 'int')
new_val = self._get_field_val(field_with_suffix, 'int')
milestoneset_entity = stage.milestones
milestoneset_entity = getattr(stage, 'milestones')
if milestoneset_entity is None:

Просмотреть файл

@ -224,6 +224,14 @@ class FeatureEditHandlerTest(testing_config.CustomTestCase):
new_origin_trial_feedback_url = 'https://example.com/ot_intent'
new_intent_to_ship_url = 'https://example.com/shipping'
# Expected stage created
new_rollout_milestone = 50
new_rollout_details = 'Details'
new_rollout_impact = 3
new_second_rollout_milestone = 55
new_second_rollout_details = 'Details 1'
new_second_rollout_impact = 1
with test_app.test_request_context(
self.request_path, data={
'stages': '30,50,60',
@ -237,7 +245,14 @@ class FeatureEditHandlerTest(testing_config.CustomTestCase):
'experiment_risks__50': new_experiment_risks,
'origin_trial_feedback_url__50': new_origin_trial_feedback_url,
'intent_to_ship_url__60': new_intent_to_ship_url,
'rollout_milestone__1__1061__create': new_rollout_milestone,
'rollout_details__1__1061__create': new_rollout_details,
'rollout_impact__1__1061__create': new_rollout_impact,
'rollout_milestone__2__1061__create': new_second_rollout_milestone,
'rollout_details__2__1061__create': new_second_rollout_details,
'rollout_impact__2__1061__create': new_second_rollout_impact,
'feature_type': '1'
# TODO ad to creates
}):
actual_response = self.handler.process_post_data(
feature_id=self.fe_1.key.integer_id())
@ -257,7 +272,7 @@ class FeatureEditHandlerTest(testing_config.CustomTestCase):
# Ensure changes were made to Stage entities.
stages = stage_helpers.get_feature_stages(
self.fe_1.key.integer_id())
self.assertEqual(len(stages.keys()), 6)
self.assertEqual(len(stages.keys()), 7)
dev_trial_stage = stages.get(130)
origin_trial_stages = stages.get(150)
# Stage for shipping should have been created.
@ -277,6 +292,14 @@ class FeatureEditHandlerTest(testing_config.CustomTestCase):
int(new_shipped_milestone))
self.assertEqual(shipping_stages[0].intent_thread_url,
new_intent_to_ship_url)
# Check that rollout stages are created
rollout_stages = stages.get(1061)
self.assertEqual(rollout_stages[0].rollout_milestone, new_rollout_milestone)
self.assertEqual(rollout_stages[0].rollout_details, new_rollout_details)
self.assertEqual(rollout_stages[0].rollout_impact, new_rollout_impact)
self.assertEqual(rollout_stages[1].rollout_milestone, new_second_rollout_milestone)
self.assertEqual(rollout_stages[1].rollout_details, new_second_rollout_details)
self.assertEqual(rollout_stages[1].rollout_impact, new_second_rollout_impact)
def test_post__normal_valid_single_stage(self):
"""Allowed user can edit a feature."""