Links in MS Teams notifications were incorrect. Note: the fix only wo…

…rks for new notification destinations. Please recreate existing notification destinations in *Quality-time* to get correct links in MS Teams notifications. Fixes #7614.

components/api_server/src/routes/notification.py

@@ -23,6 +23,7 @@ def post_new_notification_destination(database: Database, report: Report, report
         "webhook": "",
         "name": "Microsoft Teams webhook",
         "sleep_duration": 0,
+        "report_url": dict(bottle.request.json)["report_url"],
     }
     delta_description = f"{{user}} created a new destination for notifications in report '{report.name}'."
     uuids = [report_uuid, notification_destination_uuid]

components/api_server/tests/routes/test_notification.py

@@ -1,6 +1,6 @@
 """Unit tests for the notification routes."""
 
-from unittest.mock import patch
+from unittest.mock import Mock, patch
 
 from shared.model.report import Report
 from shared.utils.functions import first
@@ -89,6 +89,7 @@ def test_non_existing_report(self, request):
 class NotificationDestinationTest(NotificationTestCase):
     """Unit tests for adding and deleting notification destinations."""
 
+    @patch("bottle.request", Mock(json={"report_url": "https://report"}))
     def test_add_new_notification_destination(self):
         """Test that a notification destination can be added."""
         self.assertTrue(post_new_notification_destination(self.database, REPORT_ID)["ok"])
@@ -100,6 +101,7 @@ def test_add_new_notification_destination(self):
             report=updated_report,
         )
 
+    @patch("bottle.request", Mock(json={"report_url": "https://report"}))
     def test_add_first_new_notification_destination(self):
         """Test that a notification destination can be added."""
         del self.report["notification_destinations"]

components/frontend/src/api/notification.js

@@ -1,7 +1,8 @@
 import { fetch_server_api } from "./fetch_server_api";
 
 export function add_notification_destination(report_uuid, reload) {
-    return fetch_server_api('post', `report/${report_uuid}/notification_destination/new`, {}).then(reload)
+    const reportUrl = window.location.href.split("?")[0]
+    return fetch_server_api('post', `report/${report_uuid}/notification_destination/new`, {report_url: reportUrl}).then(reload)
 }
 
 export function delete_notification_destination(report_uuid, destination_uuid, reload) {

components/frontend/src/notification/NotificationDestinations.test.js

@@ -1,77 +1,74 @@
-import React from 'react';
-import { act, fireEvent, render, screen } from '@testing-library/react';
-import userEvent from '@testing-library/user-event';
-import { EDIT_REPORT_PERMISSION, Permissions } from '../context/Permissions';
-import { NotificationDestinations } from './NotificationDestinations';
-import * as fetch_server_api from '../api/fetch_server_api';
+import React from "react";
+import { fireEvent, render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+import { EDIT_REPORT_PERMISSION, Permissions } from "../context/Permissions";
+import { NotificationDestinations } from "./NotificationDestinations";
+import * as fetch_server_api from "../api/fetch_server_api";
 
 jest.mock("../api/fetch_server_api.js")
 
 const notification_destinations = {
     destination_uuid1: {
         webhook: "",
         name: "new",
-        url: ""
     }
 };
 
 function renderNotificationDestinations(destinations) {
     render(
         <Permissions.Provider value={[EDIT_REPORT_PERMISSION]}>
-            <NotificationDestinations destinations={destinations} report_uuid={"report_uuid"} reload={() => {/* No need to reload during tests */ }} />
+            <NotificationDestinations
+                destinations={destinations}
+                report_uuid={"report_uuid"}
+                reload={() => {/* No need to reload during tests */ }}
+            />
         </Permissions.Provider>
     )
 }
 
-it('creates the first notification destination when the add notification destination button is clicked', async () => {
+it("creates the first notification destination when the add notification destination button is clicked", async () => {
     fetch_server_api.fetch_server_api = jest.fn().mockResolvedValue({ ok: true });
-    await act(async () => {
-        renderNotificationDestinations({})
-    });
-    await act(async () => {
-        fireEvent.click(screen.getByText(/Add notification destination/));
-    });
-    expect(fetch_server_api.fetch_server_api).toHaveBeenCalledWith('post', `report/report_uuid/notification_destination/new`, {});
+    renderNotificationDestinations({});
+    fireEvent.click(screen.getByText(/Add notification destination/));
+    expect(fetch_server_api.fetch_server_api).toHaveBeenCalledWith(
+        "post", "report/report_uuid/notification_destination/new", { report_url: "http://localhost/" }
+    );
 });
 
-it('creates a new notification destination when the add notification destination button is clicked', async () => {
+it("creates a new notification destination when the add notification destination button is clicked", async () => {
     fetch_server_api.fetch_server_api = jest.fn().mockResolvedValue({ ok: true });
-    await act(async () => {
-        renderNotificationDestinations(notification_destinations)
-    });
-    await act(async () => {
-        fireEvent.click(screen.getByText(/Add notification destination/));
-    });
-    expect(fetch_server_api.fetch_server_api).toHaveBeenCalledWith('post', `report/report_uuid/notification_destination/new`, {});
+    renderNotificationDestinations(notification_destinations)
+    fireEvent.click(screen.getByText(/Add notification destination/));
+    expect(fetch_server_api.fetch_server_api).toHaveBeenCalledWith(
+        "post", "report/report_uuid/notification_destination/new", { report_url: "http://localhost/" }
+    );
 });
 
-it('edits notification destination name attribute when it is changed in the input field', async () => {
+it("edits notification destination name attribute when it is changed in the input field", async () => {
     fetch_server_api.fetch_server_api = jest.fn().mockResolvedValue({ ok: true });
-    await act(async () => {
-        renderNotificationDestinations(notification_destinations)
-    });
-    await userEvent.type(screen.getByLabelText(/Name/), ' changed{Enter}');
-
-    expect(fetch_server_api.fetch_server_api).toHaveBeenCalledWith('post', `report/report_uuid/notification_destination/destination_uuid1/attributes`, { name: "new changed" });
+    renderNotificationDestinations(notification_destinations)
+    await userEvent.type(screen.getByLabelText(/Name/), " changed{Enter}");
+    expect(fetch_server_api.fetch_server_api).toHaveBeenCalledWith(
+        "post", "report/report_uuid/notification_destination/destination_uuid1/attributes", { name: "new changed" }
+    );
 });
 
-it('edits multiple notification destination attributes when they are changed in the input fields', async () => {
+it("edits multiple notification destination attributes when they are changed in the input fields", async () => {
     fetch_server_api.fetch_server_api = jest.fn().mockResolvedValue({ ok: true });
-    await act(async () => {
-        renderNotificationDestinations(notification_destinations)
-    });
-    await userEvent.type(screen.getByPlaceholderText(/https:\/\/example/), 'new.webhook.com{Enter}');
-
-    expect(fetch_server_api.fetch_server_api).toHaveBeenCalledWith('post', `report/report_uuid/notification_destination/destination_uuid1/attributes`, { webhook: "new.webhook.com", url: "http://localhost/" });
+    renderNotificationDestinations(notification_destinations)
+    await userEvent.type(screen.getByPlaceholderText(/https:\/\/example/), "new.webhook.com{Enter}");
+    expect(fetch_server_api.fetch_server_api).toHaveBeenCalledWith(
+        "post",
+        "report/report_uuid/notification_destination/destination_uuid1/attributes",
+        { webhook: "new.webhook.com", url: "http://localhost/" }
+    );
 });
 
-it('removes the notification destination when the delete notification destination button is clicked', async () => {
+it("removes the notification destination when the delete notification destination button is clicked", async () => {
     fetch_server_api.fetch_server_api = jest.fn().mockResolvedValue({ ok: true });
-    await act(async () => {
-        renderNotificationDestinations(notification_destinations)
-    });
-    await act(async () => {
-        fireEvent.click(screen.getByText(/Delete notification destination/));
-    });
-    expect(fetch_server_api.fetch_server_api).toHaveBeenCalledWith('delete', `report/report_uuid/notification_destination/destination_uuid1`, {});
+    renderNotificationDestinations(notification_destinations)
+    fireEvent.click(screen.getByText(/Delete notification destination/));
+    expect(fetch_server_api.fetch_server_api).toHaveBeenCalledWith(
+        "delete", "report/report_uuid/notification_destination/destination_uuid1", {}
+    );
 });

components/notifier/src/destinations/ms_teams.py

@@ -11,8 +11,7 @@ def notification_text(notification: Notification) -> str:
     """Create and format the contents of the notification."""
     nr_changed = len(notification.metrics)
     plural_s = "s" if nr_changed > 1 else ""
-    report_link = f"[{notification.report_title}]({notification.url})"
-    markdown = f"{report_link} has {nr_changed} metric{plural_s} that changed status:\n\n"
+    markdown = f"{notification.report_title} has {nr_changed} metric{plural_s} that changed status:\n\n"
     for subject_name in sorted({metric.subject_name for metric in notification.metrics}):
         markdown += _subject_notification_text(notification, subject_name)
     return markdown
@@ -39,12 +38,14 @@ def _metric_notification_text(metric: MetricNotificationData) -> str:
     )
 
 
-def send_notification(destination: str, text: str) -> None:
+def send_notification(destination: str, notification: Notification) -> None:
     """Send notification to Microsoft Teams using a Webhook."""
     logging.info("Sending notification to configured teams webhook")
-    my_teams_message = pymsteams.connectorcard(destination)
-    my_teams_message.text(text)
+    teams_message = pymsteams.connectorcard(destination)
+    teams_message.title("Quality-time notification")
+    teams_message.addLinkButton(f"Open the '{notification.report_title}' report", notification.report_url)
+    teams_message.text(notification_text(notification))
     try:
-        my_teams_message.send()
+        teams_message.send()
     except Exception:
         logging.exception("Could not deliver notification")

components/notifier/src/models/notification.py

@@ -11,15 +11,11 @@ class Notification:
     """Handle notification contents and status."""
 
     report: JSON
+    report_url: str  # The landing URL to be included in the notification so users can navigate back to Quality-time
     metrics: list[MetricNotificationData]
     destination: dict[str, str]
 
     @property
     def report_title(self) -> str:
         """Return the title of the report."""
         return str(self.report["title"])
-
-    @property
-    def url(self) -> str | None:
-        """Return the URL of the report."""
-        return self.report.get("url")

components/notifier/src/notifier/notifier.py

@@ -12,7 +12,7 @@
 from shared.model.measurement import Measurement
 
 from database.reports import get_reports_and_measurements
-from destinations.ms_teams import notification_text, send_notification
+from destinations.ms_teams import send_notification
 from strategies.notification_strategy import NotificationFinder
 
 
@@ -31,7 +31,7 @@ async def notify(database: Database, sleep_duration: int = 60) -> NoReturn:
             measurements = []
 
         for notification in notification_finder.get_notifications(reports, measurements, most_recent_measurement_seen):
-            send_notification(str(notification.destination["webhook"]), notification_text(notification))
+            send_notification(str(notification.destination["webhook"]), notification)
         most_recent_measurement_seen = most_recent_measurement_timestamp(measurements)
         logging.info("Sleeping %.1f seconds...", sleep_duration)
         await asyncio.sleep(sleep_duration)

components/notifier/src/strategies/notification_strategy.py

@@ -32,7 +32,10 @@ def get_notifications(
             if notable_metrics:
                 destinations = report.get("notification_destinations", {}).values()
                 notifications.extend(
-                    [Notification(report, notable_metrics, destination) for destination in destinations],
+                    [
+                        Notification(report, destination.get("report_url", ""), notable_metrics, destination)
+                        for destination in destinations
+                    ],
                 )
         return notifications
 

components/notifier/tests/destinations/test_teams.py

@@ -8,49 +8,52 @@
 from models.notification import Notification
 
 
-@mock.patch("pymsteams.connectorcard.send")
-class SendNotificationToTeamsTests(TestCase):
-    """Unit tests for the Teams destination for notifications."""
+class MsTeamsTestCase(TestCase):
+    """Base class for MsTeams unit tests."""
 
     def setUp(self):
-        """Provide the text contents to the rest of the class."""
-        self.message = "notification message"
+        """Provide a default report for the rest of the class."""
+        self.report_url = "https://report1"
+        self.report = {"title": "Report 1"}
+        self.subject = {"type": "software", "name": "Subject"}
+        metric = {
+            "type": "security_warnings",
+            "name": "Metric",
+            "unit": "my security warnings",
+            "scale": "count",
+        }
+        measurements = [
+            {"count": {"value": 0, "status": "near_target_met"}},
+            {"count": {"value": 42, "status": "target_not_met"}},
+        ]
+        self.metric_notification_data = MetricNotificationData(metric, measurements, self.subject)
+        self.notification = Notification(self.report, self.report_url, [self.metric_notification_data], {})
+
+
+@mock.patch("pymsteams.connectorcard.send")
+class SendNotificationToTeamsTests(MsTeamsTestCase):
+    """Unit tests for the Teams destination for notifications."""
 
     def test_invalid_webhook(self, mock_send: mock.Mock):
         """Test that exceptions are caught."""
         logging.disable(logging.CRITICAL)
         mock_send.side_effect = OSError("Some error")
-        send_notification("invalid_webhook", self.message)
+        send_notification("invalid_webhook", self.notification)
         mock_send.assert_called()
         logging.disable(logging.NOTSET)
 
     def test_valid_webhook(self, mock_send: mock.Mock):
         """Test that a valid message is sent to a valid webhook."""
-        send_notification("valid_webhook", self.message)
+        send_notification("valid_webhook", self.notification)
         mock_send.assert_called()
 
 
-class BuildNotificationTextTests(TestCase):
+class BuildNotificationTextTests(MsTeamsTestCase):
     """Unit tests for the message builder."""
 
-    def setUp(self):
-        """Provide a default report for the rest of the class."""
-        self.report = {"title": "Report 1", "url": "https://report1"}
-        self.subject = {"type": "software", "name": "Subject"}
-
     def test_changed_status_text(self):
         """Test that the text is correct."""
         scale = "count"
-        metric1 = {
-            "type": "security_warnings",
-            "name": "Metric",
-            "unit": "my security warnings",
-            "scale": scale,
-        }
-        measurements1 = [
-            {"count": {"value": 0, "status": "near_target_met"}},
-            {"count": {"value": 42, "status": "target_not_met"}},
-        ]
         metric2 = {
             "type": "security_warnings",
             "name": None,
@@ -61,11 +64,12 @@ def test_changed_status_text(self):
             {"count": {"value": 5, "status": "target_met"}},
             {"count": {"value": 10, "status": "target_not_met"}},
         ]
-        metric_notification_data1 = MetricNotificationData(metric1, measurements1, self.subject)
         metric_notification_data2 = MetricNotificationData(metric2, measurements2, self.subject)
-        notification = Notification(self.report, [metric_notification_data1, metric_notification_data2], {})
+        notification = Notification(
+            self.report, self.report_url, [self.metric_notification_data, metric_notification_data2], {}
+        )
         self.assertEqual(
-            "[Report 1](https://report1) has 2 metrics that changed status:\n\n"
+            "Report 1 has 2 metrics that changed status:\n\n"
             "* Subject:\n"
             "  * *Metric* status is red (target not met), was yellow (near target met). "
             "Value is 42 my security warnings, was 0 my security warnings.\n"
@@ -87,9 +91,9 @@ def test_unknown_text(self):
             {"count": {"value": None, "status": "unknown"}},
         ]
         metric_notification_data1 = MetricNotificationData(metric1, measurements, self.subject)
-        notification = Notification(self.report, [metric_notification_data1], {})
+        notification = Notification(self.report, self.report_url, [metric_notification_data1], {})
         self.assertEqual(
-            "[Report 1](https://report1) has 1 metric that changed status:\n\n"
+            "Report 1 has 1 metric that changed status:\n\n"
             "* Subject:\n"
             "  * *Metric* status is white (unknown), was yellow (near target met). "
             "Value is ? units, was 0 units.\n",

components/notifier/tests/notifier/test_notifier.py

@@ -130,7 +130,13 @@ async def test_one_new_red_metric(self, mocked_get: Mock, mocked_sleep: Mock, mo
             "report_uuid": "report1",
             "title": self.title,
             "url": self.url,
-            "notification_destinations": {"destination1": {"name": "destination name", "webhook": "www.webhook.com"}},
+            "notification_destinations": {
+                "destination1": {
+                    "name": "destination name",
+                    "webhook": "www.webhook.com",
+                    "report_url": "https://report",
+                },
+            },
             "subjects": self.subjects,
         }
         now = datetime.now(tz=UTC).replace(microsecond=0).isoformat()

components/notifier/tests/strategies/test_notification_strategy.py

@@ -39,7 +39,9 @@ def setUp(self):
                 "report_uuid": "report1",
                 "title": "Title",
                 "subjects": {"subject1": {"metrics": {"red_metric": self.red_metric}, "type": "software"}},
-                "notification_destinations": {"destination_uuid": {"name": "destination"}},
+                "notification_destinations": {
+                    "destination_uuid": {"name": "destination", "report_url": "https://report"}
+                },
             },
         ]
 
@@ -163,7 +165,7 @@ def test_multiple_reports_with_same_destination(self):
             "report_uuid": "report2",
             "webhook": "webhook",
             "subjects": {"subject": subject2},
-            "notification_destinations": {"uuid1": {"url": "https://report2", "name": "destination2"}},
+            "notification_destinations": {"uuid1": {"report_url": "https://report2", "name": "destination2"}},
         }
         self.reports.append(report2)
         red_metric2_measurements = [

docs/src/changelog.md

@@ -16,6 +16,7 @@ If your currently installed *Quality-time* version is v4.10.0 or older, please r
 
 ### Fixed
 
+- Links in MS Teams notifications were incorrect. Note: the fix only works for new notification destinations. Please recreate existing notification destinations in *Quality-time* to get correct links in MS Teams notifications. Fixes [#7614](https://github.com/ICTU/quality-time/issues/7614).
 - If a URL with a hashtag was used to navigate to *Quality-time*, shared URLs would get an additional hashtag. Fixes [#7761](https://github.com/ICTU/quality-time/issues/7761).
 
 ### Added