Add optional "comment" field for emergency_shutoff (#2604)

* Create comment column migration. Add comment column to DB model Add comment field on spec; Insert comment on database Add comment information on balrog ui * Apply code review changes
mozilla-releng · Aug 19, 2022 · 3601d03 · 3601d03
1 parent 97e1f40
commit 3601d03
Show file tree

Hide file tree

Showing 10 changed files with 172 additions and 28 deletions.
diff --git a/src/auslib/db.py b/src/auslib/db.py
@@ -2912,6 +2912,7 @@ def __init__(self, db, metadata, dialect):
             metadata,
             Column("product", String(15), nullable=False, primary_key=True),
             Column("channel", String(75), nullable=False, primary_key=True),
+            Column("comment", String(500)),
         )
         AUSTable.__init__(self, db, dialect, scheduled_changes=True, scheduled_changes_kwargs={"conditions": ["time"]}, historyClass=HistoryTable)
 

diff --git a/src/auslib/migrate/versions/036_add_emergency_shutoff_comments.py b/src/auslib/migrate/versions/036_add_emergency_shutoff_comments.py
@@ -0,0 +1,25 @@
+from sqlalchemy import Column, MetaData, String, Table
+
+
+def upgrade(migrate_engine):
+    metadata = MetaData(bind=migrate_engine)
+
+    comment = Column("comment", String(500))
+    comment.create(Table("emergency_shutoffs", metadata, autoload=True))
+
+    history_comment = Column("comment", String(500))
+    history_comment.create(Table("emergency_shutoffs_history", metadata, autoload=True))
+
+    base_comment = Column("base_comment", String(500))
+    base_comment.create(Table("emergency_shutoffs_scheduled_changes", metadata, autoload=True))
+
+    history_base_comment = Column("base_comment", String(500))
+    history_base_comment.create(Table("emergency_shutoffs_scheduled_changes_history", metadata, autoload=True))
+
+
+def downgrade(migrate_engine):
+    metadata = MetaData(bind=migrate_engine)
+    Table("emergency_shutoffs", metadata, autoload=True).c.comment.drop()
+    Table("emergency_shutoffs_history", metadata, autoload=True).c.comment.drop()
+    Table("emergency_shutoffs_scheduled_changes", metadata, autoload=True).c.base_comment.drop()
+    Table("emergency_shutoffs_scheduled_changes_history", metadata, autoload=True).c.base_comment.drop()
diff --git a/src/auslib/web/admin/emergency_shutoff.py b/src/auslib/web/admin/emergency_shutoff.py
@@ -25,7 +25,11 @@ def post(emergency_shutoff, changed_by, transaction):
     if shutoff_exists(emergency_shutoff["product"], emergency_shutoff["channel"]):
         return problem(400, "Bad Request", "Invalid Emergency shutoff data", ext={"exception": "Emergency shutoff for product/channel already exists."})
     inserted_shutoff = dbo.emergencyShutoffs.insert(
-        changed_by=changed_by, transaction=transaction, product=emergency_shutoff["product"], channel=emergency_shutoff["channel"]
+        changed_by=changed_by,
+        transaction=transaction,
+        product=emergency_shutoff["product"],
+        channel=emergency_shutoff["channel"],
+        comment=emergency_shutoff.get("comment"),
     )
     return Response(status=201, content_type="application/json", response=json.dumps(inserted_shutoff))
 

diff --git a/src/auslib/web/common/swagger/definitions.yml b/src/auslib/web/common/swagger/definitions.yml
@@ -440,13 +440,22 @@ definitions:
                 allowlist: null
 
   EmergencyShutOffModel:
-    title: Emergency shut off data.
-    description: Emergency shut off data.
+    title: Emergency shutoff data.
+    description: Emergency shutoff data.
     type: object
     allOf:
       - $ref: '#/definitions/ProductModel'
       - $ref: '#/definitions/ChannelModel'
       - $ref: '#/definitions/DataVersionModel'
+      - type: object
+        properties:
+          comment:
+            description: A string describing the emergency shutoff reason.
+            type: ["string", "null"]
+            format: ascii
+            minLength: 0
+            maxLength: 500
+            example: lorem ipso facto
     required:
       - product
       - channel

diff --git a/tests/admin/views/test_emergency_shutoff.py b/tests/admin/views/test_emergency_shutoff.py
@@ -9,7 +9,7 @@ class TestEmergencyShutoff(ViewTest):
     def setUp(self):
         super(TestEmergencyShutoff, self).setUp()
         dbo.emergencyShutoffs.t.insert().execute(product="Firefox", channel="nightly", data_version=1)
-        dbo.emergencyShutoffs.t.insert().execute(product="Fennec", channel="beta", data_version=1)
+        dbo.emergencyShutoffs.t.insert().execute(product="Fennec", channel="beta", comment="Fennec panic!()", data_version=1)
         dbo.emergencyShutoffs.t.insert().execute(product="Thunderbird", channel="nightly", data_version=1)
         dbo.emergencyShutoffs.scheduled_changes.t.insert().execute(
             sc_id=1, scheduled_by="bill", change_type="delete", data_version=1, base_product="Firefox", base_channel="nightly", base_data_version=1
@@ -38,6 +38,8 @@ def test_get_by_id(self):
         self.assertEqual(data["product"], "Fennec")
         self.assertIn("channel", data)
         self.assertEqual(data["channel"], "beta")
+        self.assertIn("comment", data)
+        self.assertEqual(data["comment"], "Fennec panic!()")
 
     def test_get_notfound(self):
         resp = self._get("/emergency_shutoff/foo/bar")
@@ -47,10 +49,23 @@ def test_create(self):
         data = {"product": "Thunderbird", "channel": "release"}
         resp = self._post("/emergency_shutoff", data=data.copy())
         self.assertStatusCode(resp, 201)
-        shutoffs = dbo.emergencyShutoffs.select(where=data)
-        self.assertTrue(shutoffs)
-        for key in data.keys():
-            self.assertEqual(data[key], shutoffs[0][key])
+        emergency_shutoff_tables = [dbo.emergencyShutoffs, dbo.emergencyShutoffs.history]
+        for shutoff_table in emergency_shutoff_tables:
+            shutoffs = shutoff_table.select(where=data)
+            self.assertTrue(shutoffs)
+            for key in data.keys():
+                self.assertEqual(data[key], shutoffs[0][key])
+
+    def test_create_with_comment(self):
+        data = {"product": "Fennec", "channel": "release", "comment": "Fennec panic!()"}
+        resp = self._post("/emergency_shutoff", data=data.copy())
+        self.assertStatusCode(resp, 201)
+        emergency_shutoff_tables = [dbo.emergencyShutoffs, dbo.emergencyShutoffs.history]
+        for shutoff_table in emergency_shutoff_tables:
+            shutoffs = shutoff_table.select(where=data)
+            self.assertTrue(shutoffs)
+            for key in data.keys():
+                self.assertEqual(data[key], shutoffs[0][key])
 
     def test_create_no_permission(self):
         data = {"product": "Thunderbird", "channel": "release"}

diff --git a/tests/test_db.py b/tests/test_db.py
@@ -6154,6 +6154,29 @@ def _add_emergency_shutoff_tables(self, db, upgrade=True):
             for table in shutoff_tables:
                 self.assertNotIn(table, metadata.tables)
 
+    def _add_emergency_shutoff_comments(self, db, upgrade=True):
+        metadata = self._get_reflected_metadata(db)
+        emergency_shutoff_tables = [
+            "emergency_shutoffs",
+            "emergency_shutoffs_history",
+        ]
+
+        emergency_shutoff_sc_tables = [
+            "emergency_shutoffs_scheduled_changes",
+            "emergency_shutoffs_scheduled_changes_history",
+        ]
+
+        if upgrade:
+            for table_name in emergency_shutoff_tables:
+                self.assertIn("comment", metadata.tables[table_name].c)
+            for table_name in emergency_shutoff_sc_tables:
+                self.assertIn("base_comment", metadata.tables[table_name].c)
+        else:
+            for table_name in emergency_shutoff_tables:
+                self.assertNotIn("comment", metadata.tables[table_name].c)
+            for table_name in emergency_shutoff_sc_tables:
+                self.assertNotIn("base_comment", metadata.tables[table_name].c)
+
     def _add_release_json_tables(self, db, upgrade=True):
         metadata = self._get_reflected_metadata(db)
         releases_tables = [
@@ -6263,6 +6286,7 @@ def _noop(*args, **kwargs):
             pass
 
         versions_migrate_tests_dict = {
+            35: self._add_emergency_shutoff_comments,
             34: self._add_pinnable_releases_tables,
             33: self._add_release_json_tables,
             # This version removes the releases_history table, which is verified by other tests

diff --git a/ui/src/components/DialogAction/index.jsx b/ui/src/components/DialogAction/index.jsx
@@ -95,10 +95,7 @@ function DialogAction(props) {
         {body && <DialogContentText component="div">{body}</DialogContentText>}
       </DialogContent>
       <DialogActions>
-        <Button
-          disabled={actionExecuting}
-          onClick={onClose}
-          action={actions => actions && actions.focusVisible()}>
+        <Button disabled={actionExecuting} onClick={onClose}>
           Cancel
         </Button>
         <div className={classes.executingActionWrapper}>

diff --git a/ui/src/components/EmergencyShutoffCard/index.jsx b/ui/src/components/EmergencyShutoffCard/index.jsx
@@ -30,6 +30,14 @@ const useStyles = makeStyles(theme => ({
   cardActions: {
     justifyContent: 'flex-end',
   },
+  reason: {
+    alignSelf: 'center',
+    width: '100%',
+    color: theme.palette.text.secondary,
+  },
+  actionButton: {
+    minWidth: 'max-content',
+  },
 }));
 
 function EmergencyShutoffCard({
@@ -75,15 +83,23 @@ function EmergencyShutoffCard({
         )}
       </CardContent>
       <CardActions className={classes.cardActions}>
+        {emergencyShutoff.comment && (
+          <Typography component="p" className={classes.reason}>
+            Reason: {emergencyShutoff.comment}
+          </Typography>
+        )}
+
         {emergencyShutoff.scheduledChange ? (
           <Button
+            className={classes.actionButton}
             color="secondary"
             disabled={!user}
             onClick={() => onCancelEnable(emergencyShutoff)}>
             Keep Updates Disabled
           </Button>
         ) : (
           <Button
+            className={classes.actionButton}
             color="secondary"
             disabled={!user}
             onClick={() => onEnableUpdates(emergencyShutoff)}>
@@ -92,11 +108,19 @@ function EmergencyShutoffCard({
         )}
         {requiresSignoff &&
           (user && user.email in emergencyShutoff.scheduledChange.signoffs ? (
-            <Button color="secondary" disabled={!user} onClick={onRevoke}>
+            <Button
+              color="secondary"
+              disabled={!user}
+              onClick={onRevoke}
+              className={classes.actionButton}>
               Revoke Signoff
             </Button>
           ) : (
-            <Button color="secondary" disabled={!user} onClick={onSignoff}>
+            <Button
+              color="secondary"
+              disabled={!user}
+              onClick={onSignoff}
+              className={classes.actionButton}>
               Signoff
             </Button>
           ))}

diff --git a/ui/src/services/emergency_shutoff.js b/ui/src/services/emergency_shutoff.js
@@ -1,8 +1,8 @@
 import axios from 'axios';
 
 const getEmergencyShutoffs = () => axios.get('/emergency_shutoff');
-const createEmergencyShutoff = (product, channel) =>
-  axios.post('/emergency_shutoff', { product, channel });
+const createEmergencyShutoff = (product, channel, comment) =>
+  axios.post('/emergency_shutoff', { product, channel, comment });
 const deleteEmergencyShutoff = (product, channel, dataVersion) =>
   axios.delete(`/emergency_shutoff/${product}/${channel}`, {
     params: { data_version: dataVersion },

diff --git a/ui/src/views/Rules/ListRules/index.jsx b/ui/src/views/Rules/ListRules/index.jsx
@@ -150,6 +150,10 @@ function ListRules(props) {
   const [scrollToRow, setScrollToRow] = useState(null);
   const [roles, setRoles] = useState([]);
   const [emergencyShutoffs, setEmergencyShutoffs] = useState([]);
+  const [
+    emergencyShutoffReasonComment,
+    setEmergencyShutoffReasonComment,
+  ] = useState('');
   const [signoffRole, setSignoffRole] = useState('');
   const [drawerState, setDrawerState] = useState({ open: false, item: {} });
   const [drawerReleaseName, setDrawerReleaseName] = useState(null);
@@ -724,9 +728,26 @@ function ListRules(props) {
     });
   };
 
-  const handleDisableUpdates = async () => {
+  const disableUpdatesBody = (
+    <FormControl component="fieldset">
+      <TextField
+        fullWidth
+        multiline
+        label="Reason comment"
+        onChange={({ target: { value } }) => {
+          setEmergencyShutoffReasonComment(value);
+        }}
+        value={emergencyShutoffReasonComment}
+      />
+    </FormControl>
+  );
+  const handleDisableUpdatesSubmit = async () => {
     const [product, channel] = productChannelQueries;
-    const { error, data } = await disableUpdates(product, channel);
+    const { error, data } = await disableUpdates(
+      product,
+      channel,
+      emergencyShutoffReasonComment
+    );
 
     if (!error) {
       const result = clone(emergencyShutoffs);
@@ -736,7 +757,24 @@ function ListRules(props) {
       handleSnackbarOpen({
         message: `Updates for the ${product} ${channel} channel have been disabled`,
       });
+      setEmergencyShutoffReasonComment('');
     }
+
+    handleDialogClose();
+  };
+
+  const handleDisableUpdates = async () => {
+    const [product, channel] = productChannelQueries;
+
+    setDialogState({
+      ...dialogState,
+      open: true,
+      title: `Disable updates for ${product} : ${channel} ?`,
+      confirmText: 'Yes',
+      destructive: false,
+      mode: 'disableUpdates',
+      handleSubmit: handleDisableUpdatesSubmit,
+    });
   };
 
   const handleEnableUpdates = async () => {
@@ -1001,15 +1039,24 @@ function ListRules(props) {
   };
 
   const getDialogSubmit = () => {
-    if (dialogState.mode === 'delete') {
-      return handleDeleteDialogSubmit;
-    }
+    const dialogSubmits = {
+      delete: handleDeleteDialogSubmit,
+      signoff: handleSignoffDialogSubmit,
+      disableUpdates: handleDisableUpdatesSubmit,
+      signoffEnableUpdates: handleSignoffEnableUpdatesDialogSubmit,
+    };
 
-    if (dialogState.mode === 'signoff') {
-      return handleSignoffDialogSubmit;
-    }
+    return dialogSubmits[dialogState.mode];
+  };
 
-    return handleSignoffEnableUpdatesDialogSubmit;
+  const getDialogBody = () => {
+    const dialogStates = {
+      delete: deleteDialogBody,
+      signoff: signoffDialogBody,
+      disableUpdates: disableUpdatesBody,
+    };
+
+    return dialogStates[dialogState.mode];
   };
 
   const getRowHeight = ({ index }) => {
@@ -1278,9 +1325,7 @@ function ListRules(props) {
         open={dialogState.open}
         title={dialogState.title}
         destructive={dialogState.destructive}
-        body={
-          dialogState.mode === 'delete' ? deleteDialogBody : signoffDialogBody
-        }
+        body={getDialogBody()}
         confirmText={dialogState.confirmText}
         onSubmit={getDialogSubmit()}
         onError={handleDialogError}