Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a menu to add xfn gates. #3539

Merged
merged 2 commits into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 60 additions & 1 deletion api/reviews_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@
import logging
import re
from typing import Any, Optional, Tuple
from google.cloud import ndb

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.core_enums import *
from internals.core_models import FeatureEntry, Stage
from internals.review_models import Gate, Vote


Expand Down Expand Up @@ -159,3 +161,60 @@ def validate_assignees(self, assignees, fe, gate):
is_approver = permissions.can_review_gate(reviewer, fe, gate, approvers)
if not is_approver:
self.abort(400, 'Assignee is not a reviewer')


class XfnGatesAPI(basehandlers.APIHandler):

def do_get(self, **kwargs):
"""Reject unneeded GET requests without triggering Error Reporting."""
self.abort(405, valid_methods=['POST'])

def do_post(self, **kwargs) -> dict[str, str]:
"""Add a full set of cross-functional gates to a stage."""
feature_id: int = kwargs['feature_id']
fe: FeatureEntry | None = self.get_specified_feature(feature_id=feature_id)
if fe is None:
self.abort(404, msg=f'Feature {feature_id} not found')
stage_id: int = kwargs['stage_id']
stage: Stage | None = Stage.get_by_id(stage_id)
if stage is None:
self.abort(404, msg=f'Stage {stage_id} not found')

user: User = self.get_current_user(required=True)
is_editor = fe and permissions.can_edit_feature(user, fe.key.integer_id())
is_approver = approval_defs.fields_approvable_by(user)
if not is_editor and not is_approver:
self.abort(403, msg='User lacks permission to create gates')

count = self.create_xfn_gates(feature_id, stage_id)
return {'message': f'Created {count} gates'}

def get_needed_gate_types(self) -> list[int]:
"""Return a list of gate types normally used to ship a new feature."""
needed_gate_tuples = STAGES_AND_GATES_BY_FEATURE_TYPE[
FEATURE_TYPE_INCUBATE_ID]
for stage_type, gate_types in needed_gate_tuples:
if stage_type == STAGE_BLINK_SHIPPING:
return gate_types
raise ValueError('Could not find expected list of gate types')

def create_xfn_gates(self, feature_id, stage_id) -> int:
"""Create all new incubation gates on a PSA stage"""
logging.info('Creating xfn gates')
existing_gates = Gate.query(
Gate.feature_id == feature_id, Gate.stage_id == stage_id).fetch()
existing_gate_types = set([eg.gate_type for eg in existing_gates])
logging.info('Found existing: %r', existing_gate_types)
new_gates = []
for gate_type in self.get_needed_gate_types():
if gate_type not in existing_gate_types:
logging.info(f'Creating gate type {gate_type}')
gate = Gate(
feature_id=feature_id, stage_id=stage_id, gate_type=gate_type,
state=Gate.PREPARING)
new_gates.append(gate)

ndb.put_multi(new_gates)
num_new = len(new_gates)
logging.info(f'Created {num_new} gates')
return num_new
108 changes: 107 additions & 1 deletion api/reviews_api_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,19 @@
from google.cloud import ndb # type: ignore

from api import reviews_api
from internals import core_enums
from internals import approval_defs
from internals.core_enums import *
from internals import core_models
from internals.review_models import Gate, Vote

test_app = flask.Flask(__name__)

NOW = datetime.datetime.now()

ALL_SHIPPING_GATE_TYPES = [
GATE_PRIVACY_SHIP, GATE_SECURITY_SHIP, GATE_ENTERPRISE_SHIP,
GATE_DEBUGGABILITY_SHIP, GATE_TESTING_SHIP, GATE_API_SHIP]


class VotesAPITest(testing_config.CustomTestCase):

Expand Down Expand Up @@ -355,3 +360,104 @@ def test_do_get__empty_gates(self, mock_get_approvers):
'gates': [],
}
self.assertEqual(actual, expected)


class XfnGatesAPITest(testing_config.CustomTestCase):

def setUp(self):
self.feature_1 = core_models.FeatureEntry(
name='feature one', summary='sum', category=1,
owner_emails=['[email protected]'])
self.feature_1.put()
self.feature_id = self.feature_1.key.integer_id()

self.stage_1 = core_models.Stage(
feature_id=self.feature_id, stage_type=STAGE_BLINK_SHIPPING)
self.stage_1.put()
self.stage_id = self.stage_1.key.integer_id()

self.gate_1 = Gate(id=1, feature_id=self.feature_id, stage_id=self.stage_id,
gate_type=GATE_API_SHIP, state=Vote.NA)
self.gate_1.put()
self.gate_1_id = self.gate_1.key.integer_id()

self.handler = reviews_api.XfnGatesAPI()
self.request_path = '/api/v0/features/%d/stages/%d/addXfnGates' % (
self.feature_id, self.stage_id)

def tearDown(self):
self.feature_1.key.delete()
kinds: list[ndb.Model] = [Gate, Vote]
for kind in kinds:
for entity in kind.query():
entity.key.delete()

def test_get(self):
"""We reject all GETs to this endpoint."""
with test_app.test_request_context(self.request_path):
with self.assertRaises(werkzeug.exceptions.MethodNotAllowed):
self.handler.do_get()

def test_do_post__not_found(self):
"""Handler rejects bad requests."""
with test_app.test_request_context(self.request_path):
with self.assertRaises(werkzeug.exceptions.NotFound):
self.handler.do_post(
feature_id=self.feature_id + 1, stage_id=self.stage_id)

with test_app.test_request_context(self.request_path):
with self.assertRaises(werkzeug.exceptions.NotFound):
self.handler.do_post(
feature_id=self.feature_id, stage_id=self.stage_id + 1)

def test_do_post__not_allowed(self):
"""Handler rejects users who lack permission."""
testing_config.sign_out()
with test_app.test_request_context(self.request_path):
with self.assertRaises(werkzeug.exceptions.Forbidden):
self.handler.do_post(
feature_id=self.feature_id, stage_id=self.stage_id)

testing_config.sign_in('[email protected]', 999)
with test_app.test_request_context(self.request_path):
with self.assertRaises(werkzeug.exceptions.Forbidden):
self.handler.do_post(
feature_id=self.feature_id, stage_id=self.stage_id)

@mock.patch('api.reviews_api.XfnGatesAPI.create_xfn_gates')
def test_do_post__editors_allowed(self, mock_create):
"""Handler accepts users who can edit the feature."""
testing_config.sign_in('[email protected]', 123567890)
mock_create.return_value = 111
with test_app.test_request_context(self.request_path):
actual = self.handler.do_post(
feature_id=self.feature_id, stage_id=self.stage_id)

mock_create.assert_called_once_with(self.feature_id, self.stage_id)
self.assertEqual(actual, {'message': 'Created 111 gates'})

@mock.patch('api.reviews_api.XfnGatesAPI.create_xfn_gates')
def test_do_post__reviewers_allowed(self, mock_create):
"""Handler accepts users who can review any gate."""
testing_config.sign_in(approval_defs.ENTERPRISE_APPROVERS[0], 123567890)
mock_create.return_value = 222
with test_app.test_request_context(self.request_path):
actual = self.handler.do_post(
feature_id=self.feature_id, stage_id=self.stage_id)

mock_create.assert_called_once_with(self.feature_id, self.stage_id)
self.assertEqual(actual, {'message': 'Created 222 gates'})

def test_get_needed_gate_types(self):
"""We always assume that we are adding all gates for STAGE_BLINK_SHIPPING."""
actual = self.handler.get_needed_gate_types()
self.assertEqual(actual,ALL_SHIPPING_GATE_TYPES)

def test_create_xfn_gates__normal(self):
"""We can create the missing gates from STAGE_BLINK_SHIPPING."""
actual = self.handler.create_xfn_gates(self.feature_id, self.stage_id)

self.assertEqual(actual, 5)
actual_gates_dict = Gate.get_feature_gates(self.feature_id)
self.assertCountEqual(
actual_gates_dict.keys(), ALL_SHIPPING_GATE_TYPES)
57 changes: 56 additions & 1 deletion client-src/elements/chromedash-feature-detail.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
PLATFORMS_DISPLAYNAME,
STAGE_SPECIFIC_FIELDS,
OT_MILESTONE_END_FIELDS,
STAGE_PSA_SHIPPING,
ENTERPRISE_FEATURE_CATEGORIES_DISPLAYNAME,
ROLLOUT_IMPACT_DISPLAYNAME} from './form-field-enums';
import '@polymer/iron-icon';
Expand Down Expand Up @@ -124,10 +125,14 @@ class ChromedashFeatureDetail extends LitElement {
padding: 8px 16px;
}

sl-details sl-button {
sl-details sl-button,
sl-details sl-dropdown {
float: right;
margin-right: 4px;
}
sl-details sl-dropdown sl-icon-button {
font-size: 1.4rem;
}

sl-details sl-button[variant="default"]::part(base) {
color: var(--sl-color-primary-600);
Expand Down Expand Up @@ -187,6 +192,15 @@ class ChromedashFeatureDetail extends LitElement {
`];
}

_fireEvent(eventName, detail) {
const event = new CustomEvent(eventName, {
bubbles: true,
composed: true,
detail,
});
this.dispatchEvent(event);
}

connectedCallback() {
super.connectedCallback();
this.intializeGateColumn();
Expand Down Expand Up @@ -244,6 +258,18 @@ class ChromedashFeatureDetail extends LitElement {
});
}

handleAddXfnGates(feStage) {
const prompt = (
'Would you like to add gates for Privacy, Security, etc.? \n\n' +
'This is needed if the API Owners ask you to add them, ' +
'or if you send an "Intent to Ship" rather than a PSA.');
if (confirm(prompt)) {
window.csClient.addXfnGates(feStage.feature_id, feStage.id).then(() => {
this._fireEvent('refetch-needed', {});
});
}
}

renderControls() {
const editAllButton = html`
<sl-button variant="text"
Expand Down Expand Up @@ -591,12 +617,14 @@ class ChromedashFeatureDetail extends LitElement {
const isActive = this.feature.active_stage_id === feStage.id;

// Show any buttons that should be displayed at the top of the detail card.
const stageMenu = this.renderStageMenu(feStage);
const addExtensionButton = this.renderExtensionButton(feStage);
const editButton = this.renderEditButton(feStage, processStage);
const trialButton = this.renderOriginTrialButton(feStage);

const content = html`
<p class="description">
${stageMenu}
${trialButton}
${editButton}
${addExtensionButton}
Expand Down Expand Up @@ -707,6 +735,33 @@ class ChromedashFeatureDetail extends LitElement {
return nothing;
}

offerAddXfnGates(feStage) {
const stageGates = this.gates.filter(g => g.stage_id == feStage.id);
return (feStage.stage_type == STAGE_PSA_SHIPPING &&
stageGates.length < 6);
}

renderStageMenu(feStage) {
const items = [];
if (this.offerAddXfnGates(feStage)) {
items.push(html`
<sl-menu-item @click=${() => this.handleAddXfnGates(feStage)}>
Add cross-functional gates
</sl-menu-item>
`);
}

if (items.length === 0) return nothing;

return html`
<sl-dropdown>
<sl-icon-button library="material" name="more_vert_24px" label="Stage menu"
slot="trigger"></sl-icon-button>
<sl-menu>${items}</sl-menu>
</sl-dropdown>
`;
}

renderAddStageButton() {
if (!this.canEdit) {
return nothing;
Expand Down
5 changes: 5 additions & 0 deletions client-src/js-src/cs-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,11 @@ class ChromeStatusClient {
return this.doPatch(`/features/${featureId}/stages/${stageId}`, body);
}

async addXfnGates(featureId, stageId) {
return this.doPost(
`/features/${featureId}/stages/${stageId}/addXfnGates`);
}

// Processes API
async getFeatureProcess(featureId) {
return this.doGet(`/features/${featureId}/process`);
Expand Down
3 changes: 3 additions & 0 deletions main.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,9 @@ class Route:
stages_api.StagesAPI),
Route(f'{API_BASE}/features/<int:feature_id>/stages/<int:stage_id>',
stages_api.StagesAPI),
Route(
f'{API_BASE}/features/<int:feature_id>/stages/<int:stage_id>/addXfnGates',
reviews_api.XfnGatesAPI),

Route(f'{API_BASE}/blinkcomponents',
blink_components_api.BlinkComponentsAPI),
Expand Down
1 change: 1 addition & 0 deletions static/shoelace/assets/material-icons/more_vert_24px.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading