Merge remote-tracking branch 'origin/main' into dlaliberte-intentprev…

…iew-1030

api/converters.py

@@ -610,6 +610,7 @@ def gate_value_to_json_dict(gate: Gate) -> dict[str, Any]:
       'gate_type': gate.gate_type,
       'team_name': appr_def.team_name if appr_def else 'Team',
       'gate_name': appr_def.name if appr_def else 'Gate',
+      'escalation_email': appr_def.escalation_email if appr_def else None,
       'state': gate.state,
       'requested_on': requested_on,  # YYYY-MM-DD HH:MM:SS or None
       'responded_on': responded_on,  # YYYY-MM-DD HH:MM:SS or None

api/converters_test.py

@@ -509,6 +509,7 @@ def test_minimal(self):
       'gate_type': 3,
       'team_name': appr_def.team_name,
       'gate_name': appr_def.name,
+      'escalation_email': None,
       'state': 4,
       'requested_on': None,
       'responded_on': None,
@@ -543,6 +544,7 @@ def test_maxmimal(self, mock_now):
       'gate_type': 34,
       'team_name': appr_def.team_name,
       'gate_name': appr_def.name,
+      'escalation_email': '[email protected]',
       'state': 4,
       'requested_on': '2022-12-14 01:02:03',
       'responded_on': None,

api/reviews_api.py

@@ -16,15 +16,34 @@
 import datetime
 import logging
 import re
-from typing import Any, Optional
+from typing import Any, Optional, Tuple
 
 from api import converters
 from framework import basehandlers
 from framework import permissions
+from framework.users import User
 from internals import approval_defs, notifier_helpers
+from internals.core_models import FeatureEntry
 from internals.review_models import Gate, Vote
 
 
+def get_user_feature_and_gate(handler, kwargs) -> Tuple[
+    User, FeatureEntry, Gate, int, int]:
+  """Get common parameters from the request."""
+  feature_id: int = kwargs['feature_id']
+  gate_id: int = kwargs['gate_id']
+  fe: FeatureEntry = handler.get_specified_feature(feature_id=feature_id)
+
+  user: User = handler.get_current_user(required=True)
+  gate: Gate = Gate.get_by_id(gate_id)
+  if not gate:
+    handler.abort(404, msg='Gate not found')
+  if gate.feature_id != feature_id:
+    handler.abort(400, msg='Mismatched feature and gate')
+
+  return user, fe, gate, feature_id, gate_id
+
+
 class VotesAPI(basehandlers.APIHandler):
   """Users may see the set of votes on a feature, and add their own,
   if allowed."""
@@ -40,33 +59,26 @@ def do_get(self, **kwargs) -> dict[str, list[dict[str, Any]]]:
 
   def do_post(self, **kwargs) -> dict[str, str]:
     """Set a user's vote value for the specified feature and gate."""
-    feature_id = kwargs['feature_id']
-    gate_id = kwargs['gate_id']
-    feature = self.get_specified_feature(feature_id=feature_id)
+    user, fe, gate, feature_id, gate_id = get_user_feature_and_gate(
+        self, kwargs)
     new_state = self.get_int_param('state', validator=Vote.is_valid_state)
 
-    user = self.get_current_user(required=True)
-    gate = Gate.get_by_id(gate_id)
-    if not gate:
-      self.abort(404, msg='Gate not found')
-    if gate.feature_id != feature_id:
-      self.abort(400, msg='Mismatched feature and gate')
-
     old_votes = Vote.get_votes(
         feature_id=feature_id, gate_id=gate_id, set_by=user.email())
     old_state = old_votes[0].state if old_votes else Vote.NO_RESPONSE
-    self.require_permissions(user, feature, gate, new_state)
+    self.require_permissions(user, fe, gate, new_state)
 
     # Note: We no longer write Approval entities.
     approval_defs.set_vote(feature_id, None, new_state,
         user.email(), gate_id)
 
     if new_state in (Vote.REVIEW_REQUESTED, Vote.NA_REQUESTED):
+      approval_defs.auto_assign_reviewer(gate)
       notifier_helpers.notify_approvers_of_reviews(
-          feature, gate, new_state, user.email())
+          fe, gate, new_state, user.email())
     else:
       notifier_helpers.notify_subscribers_of_vote_changes(
-          feature, gate, user.email(), new_state, old_state)
+          fe, gate, user.email(), new_state, old_state)
 
     # Callers don't use the JSON response for this API call.
     return {'message': 'Done'}
@@ -102,16 +114,48 @@ def do_get(self, **kwargs) -> dict[str, Any]:
     if len(gates) == 0:
       return {
           'gates': [],
-          'possible_assignee_emails': {}
           }
 
     dicts = [converters.gate_value_to_json_dict(g) for g in gates]
-    possible_assignees_by_gate_type: dict[int, list[str]] = {
-        gate_type: approval_defs.get_approvers(gate_type)
-        for gate_type in approval_defs.APPROVAL_FIELDS_BY_ID
-        }
+    for g in dicts:
+      approvers = approval_defs.get_approvers(g['gate_type'])
+      g['possible_assignee_emails'] = approvers
 
     return {
         'gates': dicts,
-        'possible_assignee_emails': possible_assignees_by_gate_type
         }
+
+  def do_post(self, **kwargs) -> dict[str, str]:
+    """Set the assignees for a gate."""
+    user, fe, gate, feature_id, gate_id = get_user_feature_and_gate(
+        self, kwargs)
+    assignees = self.get_param('assignees')
+
+    self.require_permissions(user, fe, gate)
+    self.validate_assignees(assignees, fe, gate)
+    old_assignees = gate.assignee_emails
+    gate.assignee_emails = assignees
+    gate.put()
+    notifier_helpers.notify_assignees(
+        fe, gate, user.email(), old_assignees, assignees)
+
+    # Callers don't use the JSON response for this API call.
+    return {'message': 'Done'}
+
+  def require_permissions(self, user, feature, gate):
+    """Abort the request if the user lacks permission to set assignees."""
+    is_editor = permissions.can_edit_feature(user, feature.key.integer_id())
+    approvers = approval_defs.get_approvers(gate.gate_type)
+    is_approver = permissions.can_review_gate(user, feature, gate, approvers)
+
+    if not is_editor and not is_approver:
+      self.abort(403, msg='User lacks permission to assign reviews')
+
+  def validate_assignees(self, assignees, fe, gate):
+    """Each assignee must have permission to review."""
+    approvers = approval_defs.get_approvers(gate.gate_type)
+    for email in assignees:
+      reviewer = User(email=email)
+      is_approver = permissions.can_review_gate(reviewer, fe, gate, approvers)
+      if not is_approver:
+        self.abort(400, 'Assignee is not a reviewer')

api/reviews_api_test.py

@@ -326,6 +326,7 @@ def test_do_get__success(self, mock_get_approvers):
                 "gate_type": 1,
                 "team_name": "API Owners",
                 "gate_name": "Intent to Prototype",
+                "escalation_email": None,
                 "state": 1,
                 "requested_on": None,
                 "responded_on": None,
@@ -335,22 +336,10 @@ def test_do_get__success(self, mock_get_approvers):
                 'slo_initial_response': 5,
                 'slo_initial_response_took': None,
                 'slo_initial_response_remaining': None,
+                'possible_assignee_emails': ['[email protected]'],
             },
         ],
-        "possible_assignee_emails": {
-            1: ["[email protected]"],
-            2: ["[email protected]"],
-            3: ["[email protected]"],
-            4: ["[email protected]"],
-            32: ["[email protected]"],
-            34: ["[email protected]"],
-            42: ["[email protected]"],
-            44: ["[email protected]"],
-            54: ["[email protected]"],
-            62: ["[email protected]"],
-            64: ["[email protected]"],
-            74: ["[email protected]"],
-        }}
+        }
 
     self.assertEqual(actual, expected)
 
@@ -364,6 +353,5 @@ def test_do_get__empty_gates(self, mock_get_approvers):
 
     expected = {
         'gates': [],
-        'possible_assignee_emails': {}
     }
     self.assertEqual(actual, expected)

client-src/css/forms-css.js

@@ -78,6 +78,19 @@ export const FORM_STYLES = [
       margin-top: 2em;
     }
 
+    .fade-in {
+      animation:fadeIn 0.5s linear;
+    }
+    
+    @keyframes fadeIn {
+      0% {
+        opacity:0
+      }
+      100% {
+        opacity:1;
+      }
+    }
+
     #metadata, .stage_form, .flat_form, .final_buttons {
       max-width: 67em;
       padding: 1em;

client-src/elements/chromedash-admin-blink-component-listing.js

@@ -84,7 +84,6 @@ export class ChromedashAdminBlinkComponentListing extends LitElement {
 
   static get properties() {
     return {
-      _client: {attribute: false},
       editing: {type: Boolean, reflect: true},
       component: {type: Object},
       index: {type: Number},

client-src/elements/chromedash-admin-blink-page.js

@@ -64,10 +64,7 @@ export class ChromedashAdminBlinkPage extends LitElement {
     return {
       loading: {type: Boolean},
       user: {type: Object},
-      _client: {attribute: false},
       _editMode: {type: Boolean},
-      components: {type: Array},
-      usersMap: {type: Object},
     };
   }
 

client-src/elements/chromedash-app.js

@@ -298,7 +298,6 @@ class ChromedashApp extends LitElement {
       this.pageComponent.user = this.user;
       this.pageComponent.contextLink = this.contextLink;
       this.pageComponent.selectedGateId = this.selectedGateId;
-      this.pageComponent.rawQuery = parseRawQuery(ctx.querystring);
       this.pageComponent.appTitle = this.appTitle;
       this.currentPage = ctx.path;
       if (this.pageComponent.featureId != this.gateColumnRef.value?.feature?.id) {

client-src/elements/chromedash-enterprise-release-notes-page.js

@@ -366,7 +366,7 @@ export class ChromedashEnterpriseReleaseNotesPage extends LitElement {
       ${features.map(f => html`
       <section class="feature">
         <strong>${f.name}</strong>
-        <p class="toremove">< To remove - Owners: ${f.browsers.chrome.owners.join(', ')} - Editors: ${(f.editors || []).join(', ')}  - Last Updated: ${f.updated.when} ></p>
+        <p class="toremove">< To remove - <a target="_blank" href="/feature/${f.id}">Feature details</a> - Owners: ${f.browsers.chrome.owners.join(', ')} - Editors: ${(f.editors || []).join(', ')}  - Last Updated: ${f.updated.when} ></p>
         <p class="summary preformatted">${f.summary}</p>
         <ul>
         ${f.stages.map(s => html`

client-src/elements/chromedash-enterprise-release-notes-page_test.js

@@ -344,7 +344,8 @@ describe('chromedash-enterprise-release-notes-page', () => {
           'feature with two consecutive rollout stages',
           features[0].querySelector('strong').textContent);
         assert.equal(
-          '< To remove - Owners: owner1 - Editors: editor1  - Last Updated: feature 4 updated >',
+          '< To remove - Feature details - ' +
+          'Owners: owner1 - Editors: editor1  - Last Updated: feature 4 updated >',
           features[0].querySelector('.toremove').textContent);
         assert.equal(
           'feature 4 summary',
@@ -370,7 +371,8 @@ describe('chromedash-enterprise-release-notes-page', () => {
           'feature with one rollout stages',
           features[1].querySelector('strong').textContent);
         assert.equal(
-          '< To remove - Owners: owner - Editors: editor1, editor2  - Last Updated: updated when >',
+          '< To remove - Feature details - ' +
+          'Owners: owner - Editors: editor1, editor2  - Last Updated: updated when >',
           features[1].querySelector('.toremove').textContent);
         assert.equal(
           'feature 3 summary',
@@ -392,7 +394,8 @@ describe('chromedash-enterprise-release-notes-page', () => {
           'normal feature with shipping stage',
           features[2].querySelector('strong').textContent);
         assert.equal(
-          '< To remove - Owners: owner - Editors: editor1, editor2  - Last Updated: updated when >',
+          '< To remove - Feature details - ' +
+          'Owners: owner - Editors: editor1, editor2  - Last Updated: updated when >',
           features[2].querySelector('.toremove').textContent);
         assert.equal(
           'normal feature summary',
@@ -424,7 +427,8 @@ describe('chromedash-enterprise-release-notes-page', () => {
           'feature with upcoming rollout stages',
           features[0].querySelector('strong').textContent);
         assert.equal(
-          '< To remove - Owners: owner - Editors: editor1, editor2  - Last Updated: updated when >',
+          '< To remove - Feature details - ' +
+          'Owners: owner - Editors: editor1, editor2  - Last Updated: updated when >',
           features[0].querySelector('.toremove').textContent);
         assert.equal(
           'feature 6 summary',
@@ -444,7 +448,8 @@ describe('chromedash-enterprise-release-notes-page', () => {
           'feature with past and future rollout stages',
           features[1].querySelector('strong').textContent);
         assert.equal(
-          '< To remove - Owners: owner - Editors: editor1, editor2  - Last Updated: updated when >',
+          '< To remove - Feature details - ' +
+          'Owners: owner - Editors: editor1, editor2  - Last Updated: updated when >',
           features[1].querySelector('.toremove').textContent);
         assert.equal(
           'feature 5 summary',

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

@@ -30,7 +30,9 @@ import '@polymer/iron-icon';
 import './chromedash-activity-log';
 import './chromedash-callout';
 import './chromedash-gate-chip';
-import {autolink, findProcessStage, flattenSections} from './utils.js';
+import {
+  autolink, findProcessStage, flattenSections, parseRawQuery,
+} from './utils.js';
 import {SHARED_STYLES} from '../css/shared-css.js';
 
 export const DETAILS_STYLES = [css`
@@ -71,7 +73,6 @@ class ChromedashFeatureDetail extends LitElement {
       dismissedCues: {type: Array},
       anyCollapsed: {type: Boolean},
       selectedGateId: {type: Number},
-      rawQuery: {type: Object},
       openStage: {type: Number},
     };
   }
@@ -89,7 +90,6 @@ class ChromedashFeatureDetail extends LitElement {
     this.previousStageTypeRendered = 0;
     this.sameTypeRendered = 0;
     this.selectedGateId = 0;
-    this.rawQuery = {};
     this.openStage = 0;
   }
 
@@ -193,14 +193,11 @@ class ChromedashFeatureDetail extends LitElement {
   }
 
   intializeGateColumn() {
-    if (!this.rawQuery) {
+    const rawQuery = parseRawQuery(window.location.search);
+    if (!rawQuery.hasOwnProperty('gate')) {
       return;
     }
-
-    if (!this.rawQuery.hasOwnProperty('gate')) {
-      return;
-    }
-    const gateVal = this.rawQuery['gate'];
+    const gateVal = rawQuery['gate'];
     const foundGates = this.gates.filter(g => g.id == gateVal);
     if (!foundGates.length) {
       return;
@@ -678,7 +675,7 @@ class ChromedashFeatureDetail extends LitElement {
         originTrialsURL = `https://developer.chrome.com/origintrials/#/view_trial/${feStage.origin_trial_id}`;
       }
       return html`
-        <sl-button 
+        <sl-button
           size="small"
           variant="primary"
           href=${originTrialsURL}

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

@@ -105,7 +105,6 @@ export class ChromedashFeaturePage extends LitElement {
       starred: {type: Boolean},
       loading: {attribute: false},
       selectedGateId: {type: Number},
-      rawQuery: {type: Object},
     };
   }
 
@@ -124,7 +123,6 @@ export class ChromedashFeaturePage extends LitElement {
     this.starred = false;
     this.loading = true;
     this.selectedGateId = 0;
-    this.rawQuery = {};
   }
 
   connectedCallback() {
@@ -582,7 +580,6 @@ export class ChromedashFeaturePage extends LitElement {
         .comments=${this.comments}
         .process=${this.process}
         .dismissedCues=${this.dismissedCues}
-        .rawQuery=${this.rawQuery}
         .featureLinks=${this.featureLinks}
         selectedGateId=${this.selectedGateId}
        >