Display SLO for review resolutions. (#4662)

* Display SLO for review resolutions.

* Revert undesired package changes.

* Revert screenshot changes due to New Year.

* lint-fix

* Tweak display and fix remaining days calculation

api/converters.py

@@ -641,19 +641,31 @@ def gate_value_to_json_dict(gate: Gate) -> dict[str, Any]:
   next_action = str(gate.next_action) if gate.next_action else None
   requested_on = str(gate.requested_on) if gate.requested_on else None
   responded_on = str(gate.responded_on) if gate.responded_on else None
+  needs_work_started_on = (
+      str(gate.needs_work_started_on) if gate.needs_work_started_on else None)
   appr_def = approval_defs.APPROVAL_FIELDS_BY_ID.get(gate.gate_type)
   slo_initial_response = approval_defs.DEFAULT_SLO_LIMIT
+  slo_resolve = approval_defs.DEFAULT_SLO_RESOLVE_LIMIT
   if appr_def:
     slo_initial_response = appr_def.slo_initial_response
+    slo_resolve = appr_def.slo_resolve
   slo_initial_response_remaining = None
   slo_initial_response_took = None
+  slo_resolve_remaining = None
+  slo_resolve_took = None
   if requested_on:
     if responded_on:
       slo_initial_response_took = slo.weekdays_between(
           gate.requested_on, gate.responded_on)
     else:
       slo_initial_response_remaining = slo.remaining_days(
           gate.requested_on, slo_initial_response)
+    if gate.resolved_on:
+      slo_resolve_took = max(0, slo.weekdays_between(
+          gate.requested_on, gate.resolved_on) - (gate.needs_work_elapsed or 0))
+    else:
+      slo_resolve_remaining = slo.remaining_days(
+          gate.requested_on, slo_resolve) + (gate.needs_work_elapsed or 0)
 
   return {
       'id': gate.key.integer_id(),
@@ -672,4 +684,9 @@ def gate_value_to_json_dict(gate: Gate) -> dict[str, Any]:
       'slo_initial_response': slo_initial_response,
       'slo_initial_response_took': slo_initial_response_took,
       'slo_initial_response_remaining': slo_initial_response_remaining,
+      'slo_resolve': slo_resolve,
+      'slo_resolve_took': slo_resolve_took,
+      'slo_resolve_remaining': slo_resolve_remaining,
+      # YYYY-MM-DD HH:MM:SS or None
+      'needs_work_started_on': needs_work_started_on,
       }

api/converters_test.py

@@ -542,6 +542,10 @@ def test_minimal(self):
       'slo_initial_response': appr_def.slo_initial_response,
       'slo_initial_response_took': None,
       'slo_initial_response_remaining': None,
+      'slo_resolve': approval_defs.DEFAULT_SLO_RESOLVE_LIMIT,
+      'slo_resolve_took': None,
+      'slo_resolve_remaining': None,
+      'needs_work_started_on': None,
       }
     self.assertEqual(expected, actual)
 
@@ -577,6 +581,10 @@ def test_maxmimal(self, mock_now):
       'slo_initial_response': appr_def.slo_initial_response,
       'slo_initial_response_took': None,  # Review is still in-progress.
       'slo_initial_response_remaining': -1,  # One weekday overdue.
+      'slo_resolve': approval_defs.DEFAULT_SLO_RESOLVE_LIMIT,
+      'slo_resolve_took': None,
+      'slo_resolve_remaining': 3,
+      'needs_work_started_on': None,
       }
     self.assertEqual(expected, actual)
 

api/reviews_api_test.py

@@ -341,11 +341,16 @@ def test_do_get__success(self, mock_get_approvers):
                 'slo_initial_response': 5,
                 'slo_initial_response_took': None,
                 'slo_initial_response_remaining': None,
+                'slo_resolve': 10,
+                'slo_resolve_took': None,
+                'slo_resolve_remaining': None,
+                'needs_work_started_on': None,
                 'possible_assignee_emails': ['[email protected]'],
             },
         ],
         }
 
+    self.maxDiff = None
     self.assertEqual(actual, expected)
 
   @mock.patch('internals.approval_defs.get_approvers')

client-src/elements/chromedash-gate-chip.ts

@@ -49,9 +49,13 @@ export interface GateDict {
   assignee_emails: string[];
   next_action?: string;
   additional_review: boolean;
-  slo_initial_response: string;
-  slo_initial_response_took: string;
+  slo_initial_response: number;
+  slo_initial_response_took: number;
   slo_initial_response_remaining: number;
+  slo_resolve: number;
+  slo_resolve_took: number;
+  slo_resolve_remaining: number;
+  needs_work_started_on: string;
   possible_assignee_emails: string[];
 }
 

client-src/elements/chromedash-gate-column.ts

@@ -605,64 +605,80 @@ export class ChromedashGateColumn extends LitElement {
     `;
   }
 
-  renderSLODetails() {
-    return html`
-      Reviewers are encouraged to provide an initial review status update or a
-      comment within this number of days. The full review may take longer.
-    `;
-  }
-
   dayPhrase(count) {
     return String(count) + (count == 1 ? ' day' : ' days');
   }
 
-  renderSLOSummary() {
-    const limit = this.gate?.slo_initial_response;
-    const took = this.gate?.slo_initial_response_took;
-    const remaining = this.gate?.slo_initial_response_remaining;
-    let msg: typeof nothing | TemplateResult = nothing;
-    let iconName = '';
-    let className = '';
-
+  renderSLOSummary(limit: number, remaining: number, took: number) {
     if (typeof took === 'number') {
-      msg = html`took ${this.dayPhrase(took)} for initial response`;
+      return html`took ${this.dayPhrase(took)}`;
     } else if (typeof remaining === 'number') {
-      iconName = 'clock_loader_60_20px';
+      let msg = html`due today`;
+      let className = '';
       if (remaining > 0) {
         msg = html`${this.dayPhrase(remaining)} remaining`;
       } else if (remaining < 0) {
         className = 'overdue';
         msg = html`${this.dayPhrase(-remaining)} overdue`;
-      } else {
-        msg = html`initial response is due today`;
       }
-    } else if (typeof limit === 'number') {
-      return html`
-        Reviewer SLO: ${this.dayPhrase(limit)} for initial response
-      `;
-    }
-
-    if (msg === nothing) {
-      return nothing;
-    } else {
-      const icon = iconName
-        ? html`<sl-icon library="material" name="${iconName}"></sl-icon>`
-        : nothing;
       return html`
-        Reviewer SLO status: <span class="${className}">${icon} ${msg}</span>
+        <span class="${className}">
+          <sl-icon library="material" name="clock_loader_60_20px"></sl-icon>
+          ${msg}
+        </span>
       `;
+    } else if (typeof limit === 'number') {
+      return html`${this.dayPhrase(limit)} allowed`;
     }
+    return nothing;
   }
 
   renderSLOStatus() {
-    const summary = this.renderSLOSummary();
-    if (summary === nothing) return nothing;
-    return html`
+    const initialLimit = this.gate?.slo_initial_response;
+    const initialRemaining = this.gate?.slo_initial_response_remaining;
+    const initialTook = this.gate?.slo_initial_response_took;
+    const resolveLimit = this.gate?.slo_resolve;
+    const resolveRemaining = this.gate?.slo_resolve_remaining;
+    const resolveTook = this.gate?.slo_resolve_took;
+    const needsWorkStartedOn = this.gate?.needs_work_started_on;
+
+    const initialLine = html`
       <details>
-        <summary>${summary}</summary>
-        ${this.renderSLODetails()}
+        <summary>
+          SLO initial response:
+          ${this.renderSLOSummary(initialLimit, initialRemaining, initialTook)}
+        </summary>
+        Reviewers are encouraged to provide an initial review status update or a
+        comment within this number of weekdays.
       </details>
     `;
+    let resolveLine: TemplateResult | typeof nothing = html`
+      <details>
+        <summary>
+          SLO resolution:
+          ${this.renderSLOSummary(resolveLimit, resolveRemaining, resolveTook)}
+        </summary>
+        Reviewers are encouraged to resolve the review within this number of
+        weekdays. If a reviewer responds with "Needs work", this clock pauses
+        until a feature owner clicks "Re-request review".
+      </details>
+    `;
+    let needsWorkLine: TemplateResult | typeof nothing = nothing;
+    if (typeof needsWorkStartedOn === 'string') {
+      resolveLine = nothing;
+      needsWorkLine = html`
+        <details>
+          <summary>
+            SLO resolution: Needs work since ${needsWorkStartedOn.split(' ')[0]}
+          </summary>
+          A reviewer has asked the feature owner to do needed work. Check the
+          comments for a description of the needed work. The SLO clock is paused
+          until a feature owner clicks "Re-request review".
+        </details>
+      `;
+    }
+
+    return html`${initialLine} ${resolveLine} ${needsWorkLine}`;
   }
 
   renderGateRationale() {

gen/js/chromestatus-openapi/src/models/Gate.ts

@@ -115,6 +115,30 @@ export interface Gate {
      * @memberof Gate
      */
     slo_initial_response_remaining?: number;
+    /**
+     * DEFAULT_SLO_RESOLVE_LIMIT is 10 in approval_defs.py
+     * @type {number}
+     * @memberof Gate
+     */
+    slo_resolve?: number;
+    /**
+     * 
+     * @type {number}
+     * @memberof Gate
+     */
+    slo_resolve_took?: number;
+    /**
+     * 
+     * @type {number}
+     * @memberof Gate
+     */
+    slo_resolve_remaining?: number;
+    /**
+     * 
+     * @type {Date}
+     * @memberof Gate
+     */
+    needs_work_started_on?: Date;
     /**
      * 
      * @type {Array<string>}
@@ -156,6 +180,10 @@ export function GateFromJSONTyped(json: any, ignoreDiscriminator: boolean): Gate
         'slo_initial_response': json['slo_initial_response'] == null ? undefined : json['slo_initial_response'],
         'slo_initial_response_took': json['slo_initial_response_took'] == null ? undefined : json['slo_initial_response_took'],
         'slo_initial_response_remaining': json['slo_initial_response_remaining'] == null ? undefined : json['slo_initial_response_remaining'],
+        'slo_resolve': json['slo_resolve'] == null ? undefined : json['slo_resolve'],
+        'slo_resolve_took': json['slo_resolve_took'] == null ? undefined : json['slo_resolve_took'],
+        'slo_resolve_remaining': json['slo_resolve_remaining'] == null ? undefined : json['slo_resolve_remaining'],
+        'needs_work_started_on': json['needs_work_started_on'] == null ? undefined : (new Date(json['needs_work_started_on'])),
         'possible_assignee_emails': json['possible_assignee_emails'] == null ? undefined : json['possible_assignee_emails'],
     };
 }
@@ -182,6 +210,10 @@ export function GateToJSON(value?: Gate | null): any {
         'slo_initial_response': value['slo_initial_response'],
         'slo_initial_response_took': value['slo_initial_response_took'],
         'slo_initial_response_remaining': value['slo_initial_response_remaining'],
+        'slo_resolve': value['slo_resolve'],
+        'slo_resolve_took': value['slo_resolve_took'],
+        'slo_resolve_remaining': value['slo_resolve_remaining'],
+        'needs_work_started_on': value['needs_work_started_on'] == null ? undefined : ((value['needs_work_started_on']).toISOString()),
         'possible_assignee_emails': value['possible_assignee_emails'],
     };
 }

gen/js/chromestatus-openapi/src/models/GateInfo.ts

@@ -67,6 +67,12 @@ export interface GateInfo {
      * @memberof GateInfo
      */
     slo_initial_response?: number;
+    /**
+     * DEFAULT_SLO_RESOLVE_LIMIT is 10 in approval_defs.py
+     * @type {number}
+     * @memberof GateInfo
+     */
+    slo_resolve?: number;
 }
 
 /**
@@ -94,6 +100,7 @@ export function GateInfoFromJSONTyped(json: any, ignoreDiscriminator: boolean):
         'team_name': json['team_name'] == null ? undefined : json['team_name'],
         'escalation_email': json['escalation_email'] == null ? undefined : json['escalation_email'],
         'slo_initial_response': json['slo_initial_response'] == null ? undefined : json['slo_initial_response'],
+        'slo_resolve': json['slo_resolve'] == null ? undefined : json['slo_resolve'],
     };
 }
 
@@ -111,6 +118,7 @@ export function GateInfoToJSON(value?: GateInfo | null): any {
         'team_name': value['team_name'],
         'escalation_email': value['escalation_email'],
         'slo_initial_response': value['slo_initial_response'],
+        'slo_resolve': value['slo_resolve'],
     };
 }
 

gen/js/chromestatus-openapi/src/models/PostVoteRequest.ts

@@ -20,7 +20,7 @@ import { mapValues } from '../runtime';
  */
 export interface PostVoteRequest {
     /**
-     * The vote value to set. 0 for abstain, 1 for approve, 2 for abstain.
+     * The vote value to set. E.g., 2 to request a review, 5 to approve.
      * @type {number}
      * @memberof PostVoteRequest
      */