Skip to content

Commit

Permalink
Add semantic check for consistency between the feature name and type …
Browse files Browse the repository at this point in the history
…when type is deprecation (#3536)

* Semantic check between feature name and type

* Correct semantic check for feature name and type

* Style updates
  • Loading branch information
dlaliberte authored Dec 7, 2023
1 parent 2818780 commit c6c44de
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 18 deletions.
59 changes: 44 additions & 15 deletions client-src/elements/chromedash-guide-new-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ import {
NEW_FEATURE_FORM_FIELDS,
ENTERPRISE_NEW_FEATURE_FORM_FIELDS,
} from './form-definition';
import {ALL_FIELDS} from './form-field-specs';
import {SHARED_STYLES} from '../css/shared-css.js';
import {FORM_STYLES} from '../css/forms-css.js';
import {setupScrollToHash} from './utils';
import {setupScrollToHash, formatFeatureChanges} from './utils';


export class ChromedashGuideNewPage extends LitElement {
Expand Down Expand Up @@ -50,12 +51,14 @@ export class ChromedashGuideNewPage extends LitElement {
return {
userEmail: {type: String},
isEnterpriseFeature: {type: Boolean},
fieldValues: {type: Array},
};
}

constructor() {
super();
this.userEmail = '';
this.fieldValues = [];
}

/* Add the form's event listener after Shoelace event listeners are attached
Expand All @@ -74,6 +77,7 @@ export class ChromedashGuideNewPage extends LitElement {

handleFormSubmit(event, hiddenTokenField) {
event.preventDefault();
formatFeatureChanges(this.fieldValues, this.featureId);

// get the XSRF token and update it if it's expired before submission
window.csClient.ensureTokenIsValid().then(() => {
Expand All @@ -82,6 +86,19 @@ export class ChromedashGuideNewPage extends LitElement {
});
}

// Handler to update form values when a field update event is fired.
handleFormFieldUpdate(event) {
const value = event.detail.value;
// Index represents which form was updated.
const index = event.detail.index;
if (index >= this.fieldValues.length) {
throw new Error('Out of bounds index when updating field values.');
}
// The field has been updated, so it is considered touched.
this.fieldValues[index].touched = true;
this.fieldValues[index].value = value;
};

renderSubHeader() {
return html`
<div id="subheader" style="display:block">
Expand Down Expand Up @@ -120,26 +137,38 @@ export class ChromedashGuideNewPage extends LitElement {
NEW_FEATURE_FORM_FIELDS;
const postAction = this.isEnterpriseFeature ? '/guide/enterprise/new' : '/guide/new';

const renderFormField = (field, className) => {
const featureJSONKey = ALL_FIELDS[field].name || field;
const value = newFeatureInitialValues[field];
const index = this.fieldValues.length;
this.fieldValues.push({
name: featureJSONKey,
touched: false,
value, // stageId
});
return html`
<chromedash-form-field
name=${field}
index=${index}
value=${value}
.fieldValues=${this.fieldValues}
?forEnterprise=${this.isEnterpriseFeature}
@form-field-update="${this.handleFormFieldUpdate}"
class="${className || ''}"></chromedash-form-field>
</chromedash-form-field>
`;
};

return html`
<section id="stage_form">
<form name="overview_form" method="post" action=${postAction}>
<input type="hidden" name="token">
<chromedash-form-table ${ref(this.registerHandlers)}>
${this.renderWarnings()}
${formFields.map((field) => html`
<chromedash-form-field
name=${field}
value=${newFeatureInitialValues[field]}
?forEnterprise=${this.isEnterpriseFeature}>
</chromedash-form-field>
`)}
${!this.isEnterpriseFeature ? html`
<chromedash-form-field
name="feature_type_radio_group"
class="choices">
</chromedash-form-field>` :
nothing}
${formFields.map((field) => renderFormField(field))}
${!this.isEnterpriseFeature ?
renderFormField('feature_type_radio_group', 'choices') : nothing}
</chromedash-form-table>
<input
type="submit"
Expand Down
16 changes: 14 additions & 2 deletions client-src/elements/form-field-specs.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
WEB_DEV_VIEWS,
} from './form-field-enums';

import {checkMilestoneStartEnd} from './utils.js';
import {checkFeatureNameAndType, checkMilestoneStartEnd} from './utils.js';

/* Patterns from https://www.oreilly.com/library/view/regular-expressions-cookbook/9781449327453/ch04s01.html
* Removing single quote ('), backtick (`), and pipe (|) since they are risky unless properly escaped everywhere.
Expand Down Expand Up @@ -110,6 +110,10 @@ export const ALL_FIELDS = {
<li>CSS Flexbox: intrinsic size algorithm</li>
<li>Permissions-Policy header</li>
</ul>`,
check: (_value, getFieldValue) => {
return checkFeatureNameAndType(getFieldValue);
},
dependents: ['feature_type', 'feature_type_radio_group'],
},

'summary': {
Expand Down Expand Up @@ -250,11 +254,15 @@ export const ALL_FIELDS = {
choices: FEATURE_TYPES,
label: 'Feature type',
help_text: html`
Feature type chosen at time of creation.
Feature type chosen at time of creation.
<br/>
<p style="color: red"><strong>Note:</strong> The feature type field
cannot be changed. If this field needs to be modified, a new feature
would need to be created.</p>`,
check: (_value, getFieldValue) => {
return checkFeatureNameAndType(getFieldValue);
},
dependents: ['name'],
},

'feature_type_radio_group': {
Expand All @@ -269,6 +277,10 @@ export const ALL_FIELDS = {
<p style="color: red"><strong>Note:</strong> The feature type field
cannot be changed. If this field needs to be modified, a new feature
would need to be created.</p>`,
check: (_value, getFieldValue) => {
return checkFeatureNameAndType(getFieldValue);
},
dependents: ['name'],
},

'set_stage': {
Expand Down
25 changes: 24 additions & 1 deletion client-src/elements/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import {markupAutolinks} from './autolink.js';
import {nothing, html} from 'lit';
import {STAGE_FIELD_NAME_MAPPING} from './form-field-enums';
import {STAGE_FIELD_NAME_MAPPING, FEATURE_TYPES} from './form-field-enums';

let toastEl;

Expand Down Expand Up @@ -400,3 +400,26 @@ export function checkMilestoneStartEnd(startEndPair, getFieldValue) {
}
}
}


export function checkFeatureNameAndType(getFieldValue) {
const name = (getFieldValue('name') || '').toLowerCase();
const featureType = Number(getFieldValue('feature_type') || '0');
const isdeprecationName =
(name.includes('deprecat') || name.includes('remov'));
const isdeprecationType =
(featureType === FEATURE_TYPES.FEATURE_TYPE_DEPRECATION_ID[0]);
if (isdeprecationName !== isdeprecationType) {
if (isdeprecationName) {
return {
warning: `If the feature name contains "deprecate" or "remove",
the feature type should be "Feature deprecation"`,
};
} else {
return {
warning: `If the feature type is "Feature deprecation",
the feature name should contain "deprecate" or "remove"`,
};
}
}
}

0 comments on commit c6c44de

Please sign in to comment.