Enterprise flow updates and fixes (#3067)

* Add screenshot links to the enterprise feature creation page
* Make enterprise pages that require sign in use an intermediate component when not signed in and redirect to the initial page when signed in
* Move Rollout steps help text to the edit all page
* Rename "Start feature rollout" to "Rollout Step"
* Rename "Rollout stages" to "Rollout steps"
* Fix editors not showing on the feature details page for all features
* Fix enterprise categories only using the first category chosen after creating a new feature

api/api_specs.py

@@ -38,7 +38,7 @@ FEATURE_FIELD_DATA_TYPES: FIELD_INFO_DATA_TYPE = [
   ('devrel', 'list'),
   ('devtrial_instructions', 'str'),
   ('doc_links', 'list'),
-  ('editors_emails', 'list'),
+  ('editor_emails', 'list'),
   ('enterprise_feature_categories', 'list'),
   ('ergonomics_risks', 'str'),
   ('explainer_links', 'list'),

client-src/components.js

@@ -72,6 +72,7 @@ import './elements/chromedash-guide-metadata-page';
 import './elements/chromedash-guide-verify-accuracy-page';
 import './elements/chromedash-header';
 import './elements/chromedash-legend';
+import './elements/chromedash-login-required-page';
 import './elements/chromedash-metadata';
 import './elements/chromedash-myfeatures-page';
 import './elements/chromedash-preflight-dialog';

client-src/elements/chromedash-app.js

@@ -192,8 +192,10 @@ class ChromedashApp extends LitElement {
   }
 
   // Maybe set up new page, or if the URL is the same, we stay.
+  // If signin is required and a backup component is passed in signinRequiredComponentName,
+  // we render that component instead page.
   // Returns true if we are proceeding to the new page, false otherwise.
-  setupNewPage(ctx, componentName) {
+  setupNewPage(ctx, componentName, signinRequiredComponentName) {
     // If current page is ctx.path and a ctx.hash exists,
     // don't create a new element but instead
     // just scroll to the element identified by the hash.
@@ -218,9 +220,12 @@ class ChromedashApp extends LitElement {
         }
       }
     }
+    const signinRequired = ctx.querystring.search('loginStatus=False') > -1;
 
     // Loading new page.
-    this.pageComponent = document.createElement(componentName);
+    this.pageComponent = document.createElement(signinRequired && signinRequiredComponentName ?
+      signinRequiredComponentName :
+      componentName);
     this.setUnsavedChanges(false);
     this.removeBeforeUnloadHandler();
 
@@ -306,8 +311,14 @@ class ChromedashApp extends LitElement {
       this.hideSidebar();
     });
     page('/guide/enterprise/new', (ctx) => {
-      if (!this.setupNewPage(ctx, 'chromedash-guide-new-page')) return;
-      this.pageComponent.userEmail = this.user.email;
+      if (!this.setupNewPage(
+        ctx,
+        'chromedash-guide-new-page',
+        'chromedash-login-required-page')) return;
+
+      if (ctx.querystring.search('loginStatus=False') == -1) {
+        this.pageComponent.userEmail = this.user.email;
+      }
       this.pageComponent.isEnterpriseFeature = true;
       this.currentPage = ctx.path;
       this.hideSidebar();
@@ -401,7 +412,10 @@ class ChromedashApp extends LitElement {
       this.hideSidebar();
     });
     page('/enterprise/releasenotes', (ctx) => {
-      if (!this.setupNewPage(ctx, 'chromedash-enterprise-release-notes-page')) return;
+      if (!this.setupNewPage(
+        ctx,
+        'chromedash-enterprise-release-notes-page',
+        'chromedash-login-required-page')) return;
       this.pageComponent.user = this.user;
       this.contextLink = ctx.path;
       this.currentPage = ctx.path;

client-src/elements/chromedash-feature-detail.js

@@ -172,12 +172,6 @@ class ChromedashFeatureDetail extends LitElement {
         border: var(--spot-card-border);
         box-shadow: var(--spot-card-box-shadow);
       }
-
-      .enterprise-help-text > *, .enterprise-help-text li {
-        margin: revert;
-        padding: revert;
-        list-style: revert;
-      }
     `];
   }
 
@@ -318,7 +312,7 @@ class ChromedashFeatureDetail extends LitElement {
     let value = this.feature[fieldName];
     const fieldNameMapping = {
       owner: 'browsers.chrome.owners',
-      editors: 'browsers.chrome.editors',
+      editors: 'editors',
       search_tags: 'tags',
       spec_link: 'standards.spec',
       standard_maturity: 'standards.maturity.text',
@@ -624,11 +618,12 @@ class ChromedashFeatureDetail extends LitElement {
     if (!this.canEdit) {
       return nothing;
     }
+    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)}">
-      Add stage
+      ${text}
     </sl-button>`;
   }
 
@@ -636,26 +631,10 @@ class ChromedashFeatureDetail extends LitElement {
     return html`
       ${this.renderMetadataSection()}
       <h2>
-        <span>Development stages</span>
+        ${renderHTMLIf(!this.feature.is_enterprise_feature, html`<span>Development stages</span>`)}
+        ${renderHTMLIf(this.feature.is_enterprise_feature, html`<span>Rollout steps</span>`)}
         ${this.renderControls()}
       </h2>
-      ${renderHTMLIf(this.feature.is_enterprise_feature, html`
-        <section class="enterprise-help-text">
-          <p>The enterprise release notes focus on changes to the stable channel.
-          Please add a stage for each milestone where something is changing on the stable channel.
-          For finch rollouts, use the milestone where the rollout starts.
-          </p>
-          <p>
-            For example, you may only have a single stage where you roll out to 100% of users on milestone N.
-          </p>
-          <p>A more complex example might look like this:</p>
-          <ul>
-            <li>On milestone N-1, you introduce a flag for early testing of an upcoming change, or start a deprecation origin trial</li>
-            <li>On milestone N, you start a finch rollout of a feature at 1% and introduce an enterprise policy for it</li>
-            <li>On milestone N+3, you remove the enterprise policy</li>
-          </ul>
-        </section>
-      `)}
       ${this.feature.stages.map(feStage => this.renderProcessStage(feStage))}
       ${this.renderAddStageButton()}
     `;

client-src/elements/chromedash-guide-editall-page.js

@@ -5,7 +5,8 @@ import {
   showToastMessage,
   flattenSections,
   setupScrollToHash,
-  shouldShowDisplayNameField} from './utils.js';
+  shouldShowDisplayNameField,
+  renderHTMLIf} from './utils.js';
 import './chromedash-form-table';
 import './chromedash-form-field';
 import {
@@ -17,7 +18,7 @@ import {
 } from './form-definition';
 import {SHARED_STYLES} from '../css/shared-css.js';
 import {FORM_STYLES} from '../css/forms-css.js';
-import {STAGE_SHORT_NAMES, STAGE_SPECIFIC_FIELDS} from './form-field-enums.js';
+import {STAGE_SHORT_NAMES, STAGE_SPECIFIC_FIELDS, STAGE_ENT_ROLLOUT} from './form-field-enums.js';
 import {openAddStageDialog} from './chromedash-add-stage-dialog';
 
 
@@ -27,6 +28,11 @@ export class ChromedashGuideEditallPage extends LitElement {
       ...SHARED_STYLES,
       ...FORM_STYLES,
       css`
+      .enterprise-help-text > *, .enterprise-help-text li {
+        margin: revert;
+        padding: revert;
+        list-style: revert;
+      }
     `];
   }
 
@@ -151,6 +157,32 @@ export class ChromedashGuideEditallPage extends LitElement {
     return FORMS_BY_STAGE_TYPE[stageType] || null;
   }
 
+  getHelpTextForStage(stageType) {
+    switch (stageType) {
+      case STAGE_ENT_ROLLOUT:
+        return html`
+        <section class="enterprise-help-text">
+          <h3>Rollout steps</h3>
+          <p>The enterprise release notes focus on changes to the stable channel.
+          Please add a stage for each milestone where something is changing on the stable channel.
+          For finch rollouts, use the milestone where the rollout starts.
+          </p>
+          <p>
+            For example, you may only have a single stage where you roll out to 100% of users on milestone N.
+          </p>
+          <p>A more complex example might look like this:</p>
+          <ul>
+            <li>On milestone N-1, you introduce a flag for early testing of an upcoming change, or start a deprecation origin trial</li>
+            <li>On milestone N, you start a finch rollout of a feature at 1% and introduce an enterprise policy for it</li>
+            <li>On milestone N+3, you remove the enterprise policy</li>
+          </ul>
+        </section>
+      `;
+      default:
+        return nothing;
+    }
+  }
+
   renderStageSection(formattedFeature, sectionBaseName, feStage, stageFields) {
     if (!stageFields) return nothing;
 
@@ -193,8 +225,10 @@ export class ChromedashGuideEditallPage extends LitElement {
     });
     const id = `${STAGE_SHORT_NAMES[feStage.stage_type] || 'metadata'}${this.sameTypeRendered}`
       .toLowerCase();
+    const isEnterpriseFeatureRollout = formattedFeature.is_enterprise_feature &&
+      feStage.stage_type === STAGE_ENT_ROLLOUT;
     return html`
-    <h3 id="${id}">${sectionName}</h3>
+    ${renderHTMLIf(!isEnterpriseFeatureRollout, html`<h3 id="${id}">${sectionName}</h3>`)}
     <section class="flat_form">
       ${formFieldEls}
     </section>
@@ -222,12 +256,19 @@ export class ChromedashGuideEditallPage extends LitElement {
     let allFormFields = [...fieldsOnly];
 
 
+    let previousStageType = null;
     for (const feStage of feStages) {
       const stageForm = this.getStageForm(feStage.stage_type);
       if (!stageForm) {
         continue;
       }
 
+      if (formattedFeature.is_enterprise_feature &&
+          feStage.stage_type !== previousStageType) {
+        formsToRender.push(this.getHelpTextForStage(feStage.stage_type));
+        previousStageType = feStage.stage_type;
+      }
+
       fieldsOnly = flattenSections(stageForm);
       formsToRender.push(this.renderStageSection(
         formattedFeature, stageForm.name, feStage, fieldsOnly));
@@ -266,10 +307,11 @@ export class ChromedashGuideEditallPage extends LitElement {
   }
 
   renderAddStageButton() {
+    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)}">
-      Add stage
+      ${text}
     </sl-button>`;
   }
 

client-src/elements/chromedash-login-required-page.js

@@ -0,0 +1,13 @@
+import {LitElement, html} from 'lit';
+
+export class ChromedashLoginRequiredPage extends LitElement {
+  render() {
+    return html`
+    <div>
+        Please login to see the content of this page.
+    </div>
+    `;
+  }
+}
+
+customElements.define('chromedash-login-required-page', ChromedashLoginRequiredPage);

client-src/elements/form-definition.js

@@ -125,6 +125,7 @@ export const ENTERPRISE_NEW_FEATURE_FORM_FIELDS = [
   'owner',
   'editors',
   'enterprise_feature_categories',
+  'screenshot_links',
 ];
 
 // The fields that are available to every feature.
@@ -384,10 +385,10 @@ const FLAT_PREPARE_TO_SHIP_FIELDS = {
 
 // All fields relevant to the enterprise prepare to ship stage.
 export const FLAT_ENTERPRISE_PREPARE_TO_SHIP_FIELDS = {
-  name: 'Start feature rollout',
+  name: 'Rollout step',
   sections: [
     {
-      name: 'Start feature rollout',
+      name: 'Rollout step',
       fields: [
         'rollout_impact',
         'rollout_milestone',

framework/basehandlers.py

@@ -562,6 +562,11 @@ def get_spa_template_data(handler_obj, defaults):
   """Check permissions then let spa.html do its thing."""
   # Check if the page requires user to sign in
   if defaults.get('require_signin') and not handler_obj.get_current_user():
+    if defaults.get('is_enterprise_page'):
+      common_data = handler_obj.get_common_data()
+      if 'loginStatus=False' in common_data['current_path']:
+        return {}
+      return flask.redirect('?loginStatus=False'), handler_obj.get_headers()
     return flask.redirect(settings.LOGIN_PAGE_URL), handler_obj.get_headers()
 
   # Check if the page requires create feature permission
@@ -589,7 +594,7 @@ def get_spa_template_data(handler_obj, defaults):
       handler_obj.abort(403, msg='Cannot perform admin actions')
 
   # Validate the user has a google or chromium account and redirect if needed.
-  if defaults.get('require_google_or_chromium_account'):
+  if defaults.get('is_enterprise_page'):
     user = handler_obj.get_current_user()
     # Should have already done the require_signin check.
     # If for reason, we don't let's treat it as the main 403 case.

framework/basehandlers_test.py

@@ -1142,6 +1142,27 @@ class GetSPATemplateDataTests(testing_config.CustomTestCase):
     self.assertEqual(
         settings.LOGIN_PAGE_URL, actual_redirect.headers['location'])
 
+  def test_get_spa_template_data__signin_missing_enterprise(self):
+    """This page requires sign in, but user is anon."""
+    testing_config.sign_out()
+    with test_app.test_request_context('/must_be_signed_in'):
+      defaults = {'require_signin': True, 'is_enterprise_page':True}
+      actual_redirect, actual_headers = basehandlers.get_spa_template_data(
+          self.handler, defaults)
+
+    self.assertEqual(302, actual_redirect.status_code)
+    self.assertEqual(
+        '?loginStatus=False', actual_redirect.headers['location'])
+
+  def test_get_spa_template_data__signin_missing_enterprise_after_redirect(self):
+    """This page requires sign in, but user is anon."""
+    testing_config.sign_out()
+    with test_app.test_request_context('/must_be_signed_in?loginStatus=False'):
+      defaults = {'require_signin': True, 'is_enterprise_page':True}
+      actual = basehandlers.get_spa_template_data(self.handler, defaults)
+
+    self.assertEqual({}, actual)
+
   def test_get_spa_template_data__signin_ok(self):
     """This page requires sign in, and user is signed in."""
     testing_config.sign_in('user@example.com', 111)

internals/processes.py

@@ -167,7 +167,7 @@ PI_ENTERPRISE_POLICIES = ProgressItem('Enterprise policies', 'enterprise_policie
 # This is a stage that can be inserted in the stages of any non-enterprise
 # features that are marked as breaking changes.
 FEATURE_ROLLOUT_STAGE = ProcessStage(
-      'Start feature rollout',
+      'Rollout step',
       '',
       [PI_ROLLOUT_IMPACT,
        PI_ROLLOUT_MILESTONE,
@@ -532,7 +532,7 @@ DEPRECATION_STAGES = [
 # Thise are the stages for a feature that has the enterprise feature type.
 ENTERPRISE_STAGES = [
   ProcessStage(
-      'Start feature rollout',
+      'Rollout step',
       '',
       [PI_ROLLOUT_IMPACT,
        PI_ROLLOUT_MILESTONE,

main.py

@@ -156,7 +156,7 @@ spa_page_routes = [
   Route('/guide/new', guide.FeatureCreateHandler,
       defaults={'require_create_feature': True}),
   Route('/guide/enterprise/new', guide.EnterpriseFeatureCreateHandler,
-      defaults={'require_create_feature': True}),
+      defaults={'require_signin': True, 'require_create_feature': True, 'is_enterprise_page': True}),
   Route('/guide/edit/<int:feature_id>', guide.FeatureEditHandler,
       defaults={'require_edit_feature': True}),
   Route('/guide/stage/<int:feature_id>/<int:intent_stage>/<int:stage_id>',
@@ -188,7 +188,7 @@ spa_page_routes = [
   Route('/enterprise'),
   Route(
     '/enterprise/releasenotes',
-    defaults={'require_signin': True, 'require_google_or_chromium_account': True,}),
+    defaults={'require_signin': True, 'is_enterprise_page': True}),
   # Admin pages
   Route('/admin/blink', defaults={'require_admin_site': True, 'require_signin': True}),
 ]

pages/guide.py

@@ -127,7 +127,7 @@ class EnterpriseFeatureCreateHandler(FeatureCreateHandler):
     # Write for new FeatureEntry entity.
     feature_entry = FeatureEntry(
         category=int(core_enums.MISC),
-        enterprise_feature_categories=self.split_input(
+        enterprise_feature_categories=self.form.getlist(
             'enterprise_feature_categories'),
         name=self.form.get('name'),
         feature_type=feature_type,