Fix check for missing parts on incoming SMS (#105068)

* Fix check for missing parts on incoming SMS

* Add tests for get_and_delete_all_sms function

* Fix CI issues

* Install libgammu-dev in CI

* Bust the venv cache

* Include python-gammu in requirements-all.txt

* Adjust install of dependencies

---------

Co-authored-by: Erik <erik@montnemery.com>

.github/workflows/ci.yaml

@@ -33,7 +33,7 @@ on:
         type: boolean
 
 env:
-  CACHE_VERSION: 5
+  CACHE_VERSION: 7
   UV_CACHE_VERSION: 1
   MYPY_CACHE_VERSION: 8
   HA_SHORT_VERSION: "2024.5"
@@ -484,6 +484,7 @@ jobs:
             libavfilter-dev \
             libavformat-dev \
             libavutil-dev \
+            libgammu-dev \
             libswresample-dev \
             libswscale-dev \
             libudev-dev
@@ -496,6 +497,7 @@ jobs:
           pip install "$(grep '^uv' < requirements_test.txt)"
           uv pip install -U "pip>=21.3.1" setuptools wheel
           uv pip install -r requirements_all.txt
+          uv pip install "$(grep 'python-gammu' < requirements_all.txt | sed -e 's|# python-gammu|python-gammu|g')"
           uv pip install -r requirements_test.txt
           uv pip install -e . --config-settings editable_mode=compat
 
@@ -688,7 +690,8 @@ jobs:
           sudo apt-get update
           sudo apt-get -y install \
             bluez \
-            ffmpeg
+            ffmpeg \
+            libgammu-dev
       - name: Check out code from GitHub
         uses: actions/checkout@v4.1.2
       - name: Set up Python ${{ env.DEFAULT_PYTHON }}
@@ -747,7 +750,8 @@ jobs:
           sudo apt-get update
           sudo apt-get -y install \
             bluez \
-            ffmpeg
+            ffmpeg \
+            libgammu-dev
       - name: Check out code from GitHub
         uses: actions/checkout@v4.1.2
       - name: Set up Python ${{ matrix.python-version }}
@@ -1124,7 +1128,8 @@ jobs:
           sudo apt-get update
           sudo apt-get -y install \
             bluez \
-            ffmpeg
+            ffmpeg \
+            libgammu-dev
       - name: Check out code from GitHub
         uses: actions/checkout@v4.1.2
       - name: Set up Python ${{ matrix.python-version }}

CODEOWNERS

@@ -1272,6 +1272,7 @@ build.json @home-assistant/supervisor
 /homeassistant/components/smhi/ @gjohansson-ST
 /tests/components/smhi/ @gjohansson-ST
 /homeassistant/components/sms/ @ocalvo
+/tests/components/sms/ @ocalvo
 /homeassistant/components/snapcast/ @luar123
 /tests/components/snapcast/ @luar123
 /homeassistant/components/snmp/ @nmaggioni

homeassistant/components/sms/gateway.py

@@ -92,7 +92,6 @@ class Gateway:
         start = True
         entries = []
         all_parts = -1
-        all_parts_arrived = False
         _LOGGER.debug("Start remaining:%i", start_remaining)
 
         try:
@@ -101,33 +100,31 @@ class Gateway:
                     entry = state_machine.GetNextSMS(Folder=0, Start=True)
                     all_parts = entry[0]["UDH"]["AllParts"]
                     part_number = entry[0]["UDH"]["PartNumber"]
-                    is_single_part = all_parts == 0
+                    part_is_missing = all_parts > start_remaining
-                    is_multi_part = 0 <= all_parts < start_remaining
                     _LOGGER.debug("All parts:%i", all_parts)
                     _LOGGER.debug("Part Number:%i", part_number)
                     _LOGGER.debug("Remaining:%i", remaining)
-                    all_parts_arrived = is_multi_part or is_single_part
+                    _LOGGER.debug("Start is_part_missing:%s", part_is_missing)
-                    _LOGGER.debug("Start all_parts_arrived:%s", all_parts_arrived)
                     start = False
                 else:
                     entry = state_machine.GetNextSMS(
                         Folder=0, Location=entry[0]["Location"]
                     )
 
-                if all_parts_arrived or force:
+                if part_is_missing and not force:
-                    remaining = remaining - 1
-                    entries.append(entry)
-
-                    # delete retrieved sms
-                    _LOGGER.debug("Deleting message")
-                    try:
-                        state_machine.DeleteSMS(Folder=0, Location=entry[0]["Location"])
-                    except gammu.ERR_MEMORY_NOT_AVAILABLE:
-                        _LOGGER.error("Error deleting SMS, memory not available")
-                else:
                     _LOGGER.debug("Not all parts have arrived")
                     break
 
+                remaining = remaining - 1
+                entries.append(entry)
+
+                # delete retrieved sms
+                _LOGGER.debug("Deleting message")
+                try:
+                    state_machine.DeleteSMS(Folder=0, Location=entry[0]["Location"])
+                except gammu.ERR_MEMORY_NOT_AVAILABLE:
+                    _LOGGER.error("Error deleting SMS, memory not available")
+
         except gammu.ERR_EMPTY:
             # error is raised if memory is empty (this induces wrong reported
             # memory status)

requirements_test_all.txt

@@ -1723,6 +1723,9 @@ python-ecobee-api==0.2.17
 # homeassistant.components.fully_kiosk
 python-fullykiosk==0.0.12
 
+# homeassistant.components.sms
+# python-gammu==3.2.4
+
 # homeassistant.components.analytics_insights
 python-homeassistant-analytics==0.6.0
 

tests/components/sms/__init__.py

@@ -0,0 +1 @@
+"""Tests for SMS integration."""

tests/components/sms/const.py

@@ -0,0 +1,143 @@
+"""Constants for tests of the SMS component."""
+
+import datetime
+
+SMS_STATUS_SINGLE = {
+    "SIMUnRead": 0,
+    "SIMUsed": 1,
+    "SIMSize": 30,
+    "PhoneUnRead": 0,
+    "PhoneUsed": 0,
+    "PhoneSize": 50,
+    "TemplatesUsed": 0,
+}
+
+NEXT_SMS_SINGLE = [
+    {
+        "SMSC": {
+            "Location": 0,
+            "Name": "",
+            "Format": "Text",
+            "Validity": "NA",
+            "Number": "+358444111111",
+            "DefaultNumber": "",
+        },
+        "UDH": {
+            "Type": "NoUDH",
+            "Text": b"",
+            "ID8bit": 0,
+            "ID16bit": 0,
+            "PartNumber": -1,
+            "AllParts": 0,
+        },
+        "Folder": 1,
+        "InboxFolder": 1,
+        "Memory": "SM",
+        "Location": 1,
+        "Name": "",
+        "Number": "+358444222222",
+        "Text": "Short message",
+        "Type": "Deliver",
+        "Coding": "Default_No_Compression",
+        "DateTime": datetime.datetime(2024, 3, 23, 20, 15, 37),
+        "SMSCDateTime": datetime.datetime(2024, 3, 23, 20, 15, 41),
+        "DeliveryStatus": 0,
+        "ReplyViaSameSMSC": 0,
+        "State": "UnRead",
+        "Class": -1,
+        "MessageReference": 0,
+        "ReplaceMessage": 0,
+        "RejectDuplicates": 0,
+        "Length": 7,
+    }
+]
+
+SMS_STATUS_MULTIPLE = {
+    "SIMUnRead": 0,
+    "SIMUsed": 2,
+    "SIMSize": 30,
+    "PhoneUnRead": 0,
+    "PhoneUsed": 0,
+    "PhoneSize": 50,
+    "TemplatesUsed": 0,
+}
+
+NEXT_SMS_MULTIPLE_1 = [
+    {
+        "SMSC": {
+            "Location": 0,
+            "Name": "",
+            "Format": "Text",
+            "Validity": "NA",
+            "Number": "+358444111111",
+            "DefaultNumber": "",
+        },
+        "UDH": {
+            "Type": "ConcatenatedMessages",
+            "Text": b"\x05\x00\x03\x00\x02\x01",
+            "ID8bit": 0,
+            "ID16bit": -1,
+            "PartNumber": 1,
+            "AllParts": 2,
+        },
+        "Folder": 1,
+        "InboxFolder": 1,
+        "Memory": "SM",
+        "Location": 1,
+        "Name": "",
+        "Number": "+358444222222",
+        "Text": "Longer test again: 01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123",
+        "Type": "Deliver",
+        "Coding": "Default_No_Compression",
+        "DateTime": datetime.datetime(2024, 3, 25, 19, 53, 56),
+        "SMSCDateTime": datetime.datetime(2024, 3, 25, 19, 54, 6),
+        "DeliveryStatus": 0,
+        "ReplyViaSameSMSC": 0,
+        "State": "UnRead",
+        "Class": -1,
+        "MessageReference": 0,
+        "ReplaceMessage": 0,
+        "RejectDuplicates": 0,
+        "Length": 153,
+    }
+]
+
+NEXT_SMS_MULTIPLE_2 = [
+    {
+        "SMSC": {
+            "Location": 0,
+            "Name": "",
+            "Format": "Text",
+            "Validity": "NA",
+            "Number": "+358444111111",
+            "DefaultNumber": "",
+        },
+        "UDH": {
+            "Type": "ConcatenatedMessages",
+            "Text": b"\x05\x00\x03\x00\x02\x02",
+            "ID8bit": 0,
+            "ID16bit": -1,
+            "PartNumber": 2,
+            "AllParts": 2,
+        },
+        "Folder": 1,
+        "InboxFolder": 1,
+        "Memory": "SM",
+        "Location": 2,
+        "Name": "",
+        "Number": "+358444222222",
+        "Text": "4567890123456789012345678901",
+        "Type": "Deliver",
+        "Coding": "Default_No_Compression",
+        "DateTime": datetime.datetime(2024, 3, 25, 19, 53, 56),
+        "SMSCDateTime": datetime.datetime(2024, 3, 25, 19, 54, 7),
+        "DeliveryStatus": 0,
+        "ReplyViaSameSMSC": 0,
+        "State": "UnRead",
+        "Class": -1,
+        "MessageReference": 0,
+        "ReplaceMessage": 0,
+        "RejectDuplicates": 0,
+        "Length": 28,
+    }
+]

tests/components/sms/test_gateway.py

@@ -0,0 +1,52 @@
+"""Test the SMS Gateway."""
+
+from unittest.mock import MagicMock
+
+from homeassistant.components.sms.gateway import Gateway
+from homeassistant.core import HomeAssistant
+
+from .const import (
+    NEXT_SMS_MULTIPLE_1,
+    NEXT_SMS_MULTIPLE_2,
+    NEXT_SMS_SINGLE,
+    SMS_STATUS_MULTIPLE,
+    SMS_STATUS_SINGLE,
+)
+
+
+async def test_get_and_delete_all_sms_single_message(hass: HomeAssistant) -> None:
+    """Test that a single message produces a list of entries containing the single message."""
+
+    # Mock the Gammu state_machine
+    state_machine = MagicMock()
+    state_machine.GetSMSStatus = MagicMock(return_value=SMS_STATUS_SINGLE)
+    state_machine.GetNextSMS = MagicMock(return_value=NEXT_SMS_SINGLE)
+    state_machine.DeleteSMS = MagicMock()
+
+    response = Gateway({"Connection": None}, hass).get_and_delete_all_sms(state_machine)
+
+    # Assert the length of the list
+    assert len(response) == 1
+    assert len(response[0]) == 1
+
+    # Assert the content of the message
+    assert response[0][0]["Text"] == "Short message"
+
+
+async def test_get_and_delete_all_sms_two_part_message(hass: HomeAssistant) -> None:
+    """Test that a two-part message produces a list of entries containing one combined message."""
+
+    state_machine = MagicMock()
+    state_machine.GetSMSStatus = MagicMock(return_value=SMS_STATUS_MULTIPLE)
+    state_machine.GetNextSMS = MagicMock(
+        side_effect=iter([NEXT_SMS_MULTIPLE_1, NEXT_SMS_MULTIPLE_2])
+    )
+    state_machine.DeleteSMS = MagicMock()
+
+    response = Gateway({"Connection": None}, hass).get_and_delete_all_sms(state_machine)
+
+    assert len(response) == 1
+    assert len(response[0]) == 2
+
+    assert response[0][0]["Text"] == NEXT_SMS_MULTIPLE_1[0]["Text"]
+    assert response[0][1]["Text"] == NEXT_SMS_MULTIPLE_2[0]["Text"]