Allow for leaving desired reaction times empty.

Closes #9135.

components/api_server/src/model/report.py

@@ -1,6 +1,9 @@
 """Report model class."""
 
+from datetime import timedelta
+
 from shared.model.report import Report as SharedReport
+from shared.utils.date_time import now
 
 from .issue_tracker import IssueParameters, IssueTracker, IssueTrackerCredentials
 
@@ -26,25 +29,27 @@ def issue_tracker(self) -> IssueTracker:
         )
         return IssueTracker(url, issue_parameters, credentials)
 
-    def desired_response_time(self, status: str) -> int:
-        """Return the desired response time for the metric status."""
-        # Note that the frontend also has these constant, in src/defaults.js.
+    def deadline(self, status: str) -> str | None:
+        """Return the deadline for metrics or measurement entities with the given status."""
+        desired_response_time = self._desired_response_time(status)
+        return None if desired_response_time is None else str((now() + timedelta(days=desired_response_time)).date())
+
+    def _desired_response_time(self, status: str) -> int | None:
+        """Return the desired response time for the given status."""
+        # Note that the frontend also has these constants, in src/defaults.js.
         defaults = {
             "debt_target_met": 60,
             "near_target_met": 21,
             "target_not_met": 7,
             "unknown": 3,
-        }
-        default = defaults.get(status, defaults["unknown"])
-        return int(self.get("desired_response_times", {}).get(status, default))
-
-    def desired_measurement_entity_response_time(self, status: str) -> int | None:
-        """Return the desired response time for the measurement entity status."""
-        defaults = {
             "confirmed": 180,
             "false_positive": 180,
             "wont_fix": 180,
             "fixed": 7,
         }
-        default = defaults.get(status)
-        return int(self.get("desired_response_times", {}).get(status, default)) if status in defaults else None
+        if status not in self.get("desired_response_times", {}):
+            return defaults.get(status)
+        try:
+            return int(self["desired_response_times"][status])
+        except (ValueError, TypeError):  # The desired response time can be empty, treat any non-integer as None
+            return None

components/api_server/src/routes/measurement.py

@@ -3,15 +3,13 @@
 import logging
 import time
 from collections.abc import Iterator
-from datetime import timedelta
 from typing import cast
 
 import bottle
 from pymongo.database import Database
 
 from shared.database.measurements import insert_new_measurement, latest_measurement
 from shared.model.measurement import Measurement
-from shared.utils.date_time import now
 from shared.utils.functions import first
 from shared.utils.type import MetricId, SourceId
 
@@ -47,11 +45,11 @@ def set_entity_attribute(
     description = f"{user.name()} changed the {attribute} of '{entity_description}' from '{old_value}' to '{new_value}'"
     entity_user_data = source.setdefault("entity_user_data", {}).setdefault(entity_key, {})
     entity_user_data[attribute] = new_value
-    if attribute == "status":
-        desired_reponse_time = report.desired_measurement_entity_response_time(new_value)
-        end_date = str((now() + timedelta(days=desired_reponse_time)).date()) if desired_reponse_time else None
-        entity_user_data["status_end_date"] = end_date
-        description += f" and changed the status end date to '{end_date}'"
+    if attribute == "status" and (end_date := report.deadline(new_value)):
+        old_end_date = entity_user_data.get("status_end_date")
+        if end_date != old_end_date:
+            entity_user_data["status_end_date"] = end_date
+            description += f" and changed the status end date from '{old_end_date}' to '{end_date}'"
     new_measurement["delta"] = {
         "uuids": [report.uuid, metric.subject_uuid, metric_uuid, source_uuid],
         "description": description + ".",

components/api_server/src/routes/metric.py

@@ -1,6 +1,5 @@
 """Metric routes."""
 
-from datetime import UTC, datetime, timedelta
 from typing import Any, cast
 
 import bottle
@@ -157,8 +156,7 @@ def post_metric_debt(metric_uuid: MetricId, database: Database):
         latest = latest_measurement(database, Metric(DATA_MODEL.model_dump(), metric, metric_uuid))
         # Only if the metric has at least one measurement can a technical debt target be set:
         new_debt_target = latest.value() if latest else None
-        today = datetime.now(tz=UTC).date()
-        new_end_date = (today + timedelta(days=report.desired_response_time("debt_target_met"))).isoformat()
+        new_end_date = report.deadline("debt_target_met")
     else:
         new_debt_target = None
         new_end_date = None

components/api_server/tests/routes/test_measurement.py

@@ -144,22 +144,53 @@ def test_set_status_also_sets_status_end_date_if_status_has_a_desired_response_t
         self.assertEqual(
             {
                 "description": "John Doe changed the status of 'entity title/foo/None' from '' to 'false_positive' "
-                f"and changed the status end date to '{deadline}'.",
+                f"and changed the status end date from 'None' to '{deadline}'.",
                 "email": JOHN["email"],
                 "uuids": [REPORT_ID, SUBJECT_ID, METRIC_ID, SOURCE_ID],
             },
             measurement["delta"],
         )
 
-    def test_set_status_resets_status_end_date_if_status_is_unconfirmed(self):
-        """Test that setting the status to unconfirmed also resets the end date."""
+    def test_set_status_does_not_set_the_status_end_date_if_it_is_unchanged(self):
+        """Test that setting the status does not set the end date if it hasn't changed."""
+        deadline = (now() + timedelta(days=10)).date()
+        self.measurement["sources"][0]["entity_user_data"] = {"entity_key": {"status_end_date": str(deadline)}}
+        self.report["desired_response_times"] = {"false_positive": 10}
+        measurement = self.set_entity_attribute("status", "false_positive")
+        entity = measurement["sources"][0]["entity_user_data"]["entity_key"]
+        self.assertEqual({"status": "false_positive", "status_end_date": str(deadline)}, entity)
+        self.assertEqual(
+            {
+                "description": "John Doe changed the status of 'entity title/foo/None' from '' to 'false_positive'.",
+                "email": JOHN["email"],
+                "uuids": [REPORT_ID, SUBJECT_ID, METRIC_ID, SOURCE_ID],
+            },
+            measurement["delta"],
+        )
+
+    def test_set_status_does_not_set_status_end_date_if_desired_response_time_turned_off(self):
+        """Test that setting the status does not set the end date if the desired response time has been turned off."""
+        self.report["desired_response_times"] = {"false_positive": None}
+        measurement = self.set_entity_attribute("status", "false_positive")
+        entity = measurement["sources"][0]["entity_user_data"]["entity_key"]
+        self.assertEqual({"status": "false_positive"}, entity)
+        self.assertEqual(
+            {
+                "description": "John Doe changed the status of 'entity title/foo/None' from '' to 'false_positive'.",
+                "email": JOHN["email"],
+                "uuids": [REPORT_ID, SUBJECT_ID, METRIC_ID, SOURCE_ID],
+            },
+            measurement["delta"],
+        )
+
+    def test_set_status_does_not_reset_status_end_date_if_status_is_unconfirmed(self):
+        """Test that setting the status to unconfirmed does not reset the end date."""
         measurement = self.set_entity_attribute("status", "unconfirmed")
         entity = measurement["sources"][0]["entity_user_data"]["entity_key"]
-        self.assertEqual({"status": "unconfirmed", "status_end_date": None}, entity)
+        self.assertEqual({"status": "unconfirmed"}, entity)
         self.assertEqual(
             {
-                "description": "John Doe changed the status of 'entity title/foo/None' from '' to 'unconfirmed' "
-                "and changed the status end date to 'None'.",
+                "description": "John Doe changed the status of 'entity title/foo/None' from '' to 'unconfirmed'.",
                 "email": JOHN["email"],
                 "uuids": [REPORT_ID, SUBJECT_ID, METRIC_ID, SOURCE_ID],
             },

components/api_server/tests/routes/test_metric.py

@@ -1,6 +1,5 @@
 """Unit tests for the metric routes."""
 
-from datetime import UTC, datetime, timedelta
 from unittest.mock import Mock, patch
 
 import requests
@@ -355,11 +354,33 @@ def test_turn_metric_technical_debt_on_with_existing_measurement(self, request):
             post_metric_debt(METRIC_ID, self.database),
         )
         updated_report = self.updated_report()
-        desired_response_time = Report(self.DATA_MODEL, updated_report).desired_response_time("debt_target_met")
-        expected_date = datetime.now(tz=UTC).date() + timedelta(days=desired_response_time)
+        expected_date = Report(self.DATA_MODEL, updated_report).deadline("debt_target_met")
         self.assert_delta(
             "accepted debt from 'False' to 'True' and the debt target from 'None' to '100' and the debt end date from "
-            f"'None' to '{expected_date.isoformat()}' of metric 'name' of subject 'Subject' in report 'Report'",
+            f"'None' to '{expected_date}' of metric 'name' of subject 'Subject' in report 'Report'",
+            report=updated_report,
+        )
+        self.assertTrue(updated_report["subjects"][SUBJECT_ID]["metrics"][METRIC_ID]["accept_debt"])
+
+    @patch("shared.model.measurement.iso_timestamp", new=Mock(return_value="2019-01-01"))
+    def test_turn_metric_technical_debt_on_with_existing_measurement_but_without_desired_response_time(self, request):
+        """Test that accepting technical debt also sets the technical debt value, but not the deadline."""
+        self.report["desired_response_times"] = {"debt_target_met": None}
+        self.database.measurements.find_one.return_value = {
+            "_id": "id",
+            "metric_uuid": METRIC_ID,
+            "count": {"value": "100", "status_start": "2018-01-01"},
+            "sources": [],
+        }
+        request.json = {"accept_debt": True}
+        self.assertDictEqual(
+            self.create_measurement(debt_target="100", status="debt_target_met", target="0"),
+            post_metric_debt(METRIC_ID, self.database),
+        )
+        updated_report = self.updated_report()
+        self.assert_delta(
+            "accepted debt from 'False' to 'True' and the debt target from 'None' to '100' "
+            "of metric 'name' of subject 'Subject' in report 'Report'",
             report=updated_report,
         )
         self.assertTrue(updated_report["subjects"][SUBJECT_ID]["metrics"][METRIC_ID]["accept_debt"])
@@ -369,8 +390,7 @@ def test_turn_metric_technical_debt_on_without_existing_measurement(self, reques
         request.json = {"accept_debt": True}
         self.assertDictEqual({"ok": True}, post_metric_debt(METRIC_ID, self.database))
         updated_report = self.updated_report()
-        desired_response_time = Report(self.DATA_MODEL, updated_report).desired_response_time("debt_target_met")
-        expected_date = (datetime.now(tz=UTC).date() + timedelta(days=desired_response_time)).isoformat()
+        expected_date = Report(self.DATA_MODEL, updated_report).deadline("debt_target_met")
         self.assert_delta(
             f"accepted debt from 'False' to 'True' and the debt end date from 'None' to '{expected_date}' "
             "of metric 'name' of subject 'Subject' in report 'Report'",

components/frontend/src/fields/IntegerInput.js

@@ -1,4 +1,4 @@
-import { func, number, oneOfType, string } from "prop-types"
+import { bool, func, number, oneOfType, string } from "prop-types"
 import { useState } from "react"
 
 import { ReadOnlyOrEditable } from "../context/Permissions"
@@ -7,12 +7,15 @@ import { labelPropType, permissionsPropType } from "../sharedPropTypes"
 import { ReadOnlyInput } from "./ReadOnlyInput"
 
 function EditableIntegerInput(props) {
-    let { editableLabel, label, min, prefix, set_value, unit, ...otherProps } = props
-    const initialValue = props.value || 0
+    let { allowEmpty, editableLabel, label, min, prefix, set_value, unit, ...otherProps } = props
+    const initialValue = props.value || (allowEmpty ? "" : 0)
     const [value, setValue] = useState(initialValue)
     const minValue = min || 0
 
     function isValid(aValue) {
+        if (aValue === "") {
+            return allowEmpty
+        }
         if (Number.isNaN(parseInt(aValue))) {
             return false
         }
@@ -69,6 +72,7 @@ function EditableIntegerInput(props) {
     )
 }
 EditableIntegerInput.propTypes = {
+    allowEmpty: bool,
     editableLabel: labelPropType,
     label: labelPropType,
     max: oneOfType([number, string]),

components/frontend/src/fields/IntegerInput.test.js

@@ -75,6 +75,13 @@ it("does not accept an empty value", async () => {
     expect(setValue).toHaveBeenCalledWith("4")
 })
 
+it("accepts an empty value if an empty value is allowed", async () => {
+    let setValue = jest.fn()
+    render(<IntegerInput allowEmpty value="42" set_value={setValue} />)
+    await userEvent.type(screen.getByDisplayValue(/42/), "{selectall}{backspace}{backspace}{enter}")
+    expect(setValue).toHaveBeenCalledWith("")
+})
+
 it("undoes the change on escape", async () => {
     let setValue = jest.fn()
     render(<IntegerInput value="42" set_value={setValue} />)
@@ -107,3 +114,8 @@ it("renders missing value as 0", () => {
     render(<IntegerInput />)
     expect(screen.queryAllByDisplayValue(/0/).length).toBe(1)
 })
+
+it("renders missing value as empty if empty allowed", () => {
+    render(<IntegerInput allowEmpty />)
+    expect(screen.queryAllByDisplayValue(/0/).length).toBe(0)
+})

components/frontend/src/report/ReportTitle.js

@@ -68,6 +68,7 @@ function DesiredResponseTimeInput({ hoverableLabel, reload, report, status }) {
     const help = STATUS_DESCRIPTION[status] || SOURCE_ENTITY_STATUS_DESCRIPTION[status]
     return (
         <IntegerInput
+            allowEmpty
             id={inputId}
             label={<LabelWithHelp hoverable={hoverableLabel} labelFor={inputId} label={label} help={help} />}
             requiredPermissions={[EDIT_REPORT_PERMISSION]}

components/frontend/src/source/SourceEntityDetails.js

@@ -1,4 +1,4 @@
-import { func, node, string } from "prop-types"
+import { func, node, oneOf, string } from "prop-types"
 import { Grid, Header } from "semantic-ui-react"
 
 import { set_source_entity_attribute } from "../api/source"
@@ -9,34 +9,55 @@ import { TextInput } from "../fields/TextInput"
 import { entityPropType, entityStatusPropType, reportPropType } from "../sharedPropTypes"
 import { capitalize, getDesiredResponseTime } from "../utils"
 import { LabelWithDate } from "../widgets/LabelWithDate"
-import { SOURCE_ENTITY_STATUS_NAME } from "./source_entity_status"
+import { SOURCE_ENTITY_STATUS_ACTION, SOURCE_ENTITY_STATUS_NAME } from "./source_entity_status"
 
-function entityStatusOption(status, content, subheader) {
+function entityStatusOption(status, subheader) {
     return {
         key: status,
         text: SOURCE_ENTITY_STATUS_NAME[status],
         value: status,
-        content: <Header as="h5" content={content} subheader={subheader} />,
+        content: <Header as="h5" content={SOURCE_ENTITY_STATUS_ACTION[status]} subheader={subheader} />,
     }
 }
 entityStatusOption.propTypes = {
     status: entityStatusPropType,
-    content: node,
     subheader: node,
 }
 
+function responseTimeClause(report, status, prefix = "for") {
+    const responseTime = getDesiredResponseTime(report, status)
+    return responseTime === null ? "" : ` ${prefix} ${responseTime} days`
+}
+responseTimeClause.propTypes = {
+    status: entityStatusPropType,
+    report: reportPropType,
+    prefix: oneOf(["for", "within"]),
+}
+
 function entityStatusOptions(entityType, report) {
-    const unconfirmedSubheader = `This ${entityType} should be reviewed to decide what to do with it.`
-    const confirmedSubheader = `This ${entityType} has been reviewed and should be addressed within ${getDesiredResponseTime(report, "confirmed")} days.`
-    const fixedSubheader = `Ignore this ${entityType} for ${getDesiredResponseTime(report, "fixed")} days because it has been fixed or will be fixed shortly.`
-    const falsePositiveSubheader = `Ignore this "${entityType}" for ${getDesiredResponseTime(report, "false_positive")} days because it has been incorrectly identified as ${entityType}.`
-    const wontFixSubheader = `Ignore this ${entityType} for ${getDesiredResponseTime(report, "wont_fix")} days because it will not be fixed.`
     return [
-        entityStatusOption("unconfirmed", "Unconfirm", unconfirmedSubheader),
-        entityStatusOption("confirmed", "Confirm", confirmedSubheader),
-        entityStatusOption("fixed", "Resolve as fixed", fixedSubheader),
-        entityStatusOption("false_positive", "Resolve as false positive", falsePositiveSubheader),
-        entityStatusOption("wont_fix", "Resolve as won't fix", wontFixSubheader),
+        entityStatusOption(
+            "unconfirmed",
+            `This ${entityType} should be reviewed in order to decide what to do with it.`,
+        ),
+        entityStatusOption(
+            "confirmed",
+            `This ${entityType} has been reviewed and should be addressed${responseTimeClause(report, "confirmed", "within")}.`,
+        ),
+        entityStatusOption(
+            "fixed",
+            `Ignore this ${entityType}${responseTimeClause(report, "fixed")} because it has been fixed or will be fixed shortly.`,
+        ),
+        entityStatusOption(
+            "false_positive",
+            // If the user marks then entity status as false positive, apparently the entity type is incorrect,
+            // hence the false positive option has quotes around the entity type:
+            `Ignore this "${entityType}"${responseTimeClause(report, "false_positive")} because it has been incorrectly identified as ${entityType}.`,
+        ),
+        entityStatusOption(
+            "wont_fix",
+            `Ignore this ${entityType}${responseTimeClause(report, "wont_fix")} because it will not be fixed.`,
+        ),
     ]
 }
 entityStatusOptions.propTypes = {