Address Google Sheets PR feedback (#78889)

homeassistant/components/google_sheets/__init__.py

@@ -2,7 +2,6 @@
 from __future__ import annotations
 
 from datetime import datetime
-from typing import cast
 
 import aiohttp
 from google.auth.exceptions import RefreshError
@@ -105,12 +104,13 @@ async def async_setup_service(hass: HomeAssistant) -> None:
 
     async def append_to_sheet(call: ServiceCall) -> None:
         """Append new line of data to a Google Sheets document."""
-
+        entry: ConfigEntry | None = hass.config_entries.async_get_entry(
-        entry = cast(
+            call.data[DATA_CONFIG_ENTRY]
-            ConfigEntry,
-            hass.config_entries.async_get_entry(call.data[DATA_CONFIG_ENTRY]),
         )
-        session: OAuth2Session = hass.data[DOMAIN][entry.entry_id]
+        if not entry:
+            raise ValueError(f"Invalid config entry: {call.data[DATA_CONFIG_ENTRY]}")
+        if not (session := hass.data[DOMAIN].get(entry.entry_id)):
+            raise ValueError(f"Config entry not loaded: {call.data[DATA_CONFIG_ENTRY]}")
         await session.async_ensure_token_valid()
         await hass.async_add_executor_job(_append_to_sheet, call, entry)
 

homeassistant/components/google_sheets/config_flow.py

@@ -8,7 +8,7 @@ from typing import Any
 from google.oauth2.credentials import Credentials
 from gspread import Client, GSpreadException
 
-from homeassistant.config_entries import SOURCE_REAUTH, ConfigEntry
+from homeassistant.config_entries import ConfigEntry
 from homeassistant.const import CONF_ACCESS_TOKEN, CONF_TOKEN
 from homeassistant.data_entry_flow import FlowResult
 from homeassistant.helpers import config_entry_oauth2_flow
@@ -25,6 +25,8 @@ class OAuth2FlowHandler(
 
     DOMAIN = DOMAIN
 
+    reauth_entry: ConfigEntry | None = None
+
     @property
     def logger(self) -> logging.Logger:
         """Return logger."""
@@ -42,6 +44,9 @@ class OAuth2FlowHandler(
 
     async def async_step_reauth(self, entry_data: Mapping[str, Any]) -> FlowResult:
         """Perform reauth upon an API authentication error."""
+        self.reauth_entry = self.hass.config_entries.async_get_entry(
+            self.context["entry_id"]
+        )
         return await self.async_step_reauth_confirm()
 
     async def async_step_reauth_confirm(
@@ -52,40 +57,27 @@ class OAuth2FlowHandler(
             return self.async_show_form(step_id="reauth_confirm")
         return await self.async_step_user()
 
-    def _async_reauth_entry(self) -> ConfigEntry | None:
-        """Return existing entry for reauth."""
-        if self.source != SOURCE_REAUTH or not (
-            entry_id := self.context.get("entry_id")
-        ):
-            return None
-        return next(
-            (
-                entry
-                for entry in self._async_current_entries()
-                if entry.entry_id == entry_id
-            ),
-            None,
-        )
-
     async def async_oauth_create_entry(self, data: dict[str, Any]) -> FlowResult:
         """Create an entry for the flow, or update existing entry."""
         service = Client(Credentials(data[CONF_TOKEN][CONF_ACCESS_TOKEN]))
 
-        if entry := self._async_reauth_entry():
+        if self.reauth_entry:
             _LOGGER.debug("service.open_by_key")
             try:
                 await self.hass.async_add_executor_job(
                     service.open_by_key,
-                    entry.unique_id,
+                    self.reauth_entry.unique_id,
                 )
             except GSpreadException as err:
                 _LOGGER.error(
-                    "Could not find spreadsheet '%s': %s", entry.unique_id, str(err)
+                    "Could not find spreadsheet '%s': %s",
+                    self.reauth_entry.unique_id,
+                    str(err),
                 )
                 return self.async_abort(reason="open_spreadsheet_failure")
 
-            self.hass.config_entries.async_update_entry(entry, data=data)
+            self.hass.config_entries.async_update_entry(self.reauth_entry, data=data)
-            await self.hass.config_entries.async_reload(entry.entry_id)
+            await self.hass.config_entries.async_reload(self.reauth_entry.entry_id)
             return self.async_abort(reason="reauth_successful")
 
         try:
@@ -97,6 +89,7 @@ class OAuth2FlowHandler(
             return self.async_abort(reason="create_spreadsheet_failure")
 
         await self.async_set_unique_id(doc.id)
+        self._abort_if_unique_id_configured()
         return self.async_create_entry(
             title=DEFAULT_NAME, data=data, description_placeholders={"url": doc.url}
         )

homeassistant/components/google_sheets/strings.json

@@ -10,6 +10,10 @@
       },
       "auth": {
         "title": "Link Google Account"
+      },
+      "reauth_confirm": {
+        "title": "[%key:common::config_flow::title::reauth%]",
+        "description": "The Google Sheets integration needs to re-authenticate your account"
       }
     },
     "abort": {

tests/components/google_sheets/test_config_flow.py

@@ -312,3 +312,66 @@ async def test_reauth_abort(
     result = await hass.config_entries.flow.async_configure(result["flow_id"])
     assert result.get("type") == "abort"
     assert result.get("reason") == "open_spreadsheet_failure"
+
+
+async def test_already_configured(
+    hass: HomeAssistant,
+    hass_client_no_auth,
+    aioclient_mock,
+    current_request_with_host,
+    setup_credentials,
+    mock_client,
+) -> None:
+    """Test case where config flow discovers unique id was already configured."""
+    config_entry = MockConfigEntry(
+        domain=DOMAIN,
+        unique_id=SHEET_ID,
+        data={
+            "token": {
+                "access_token": "mock-access-token",
+            },
+        },
+    )
+    config_entry.add_to_hass(hass)
+
+    result = await hass.config_entries.flow.async_init(
+        "google_sheets", context={"source": config_entries.SOURCE_USER}
+    )
+    state = config_entry_oauth2_flow._encode_jwt(
+        hass,
+        {
+            "flow_id": result["flow_id"],
+            "redirect_uri": "https://example.com/auth/external/callback",
+        },
+    )
+
+    assert result["url"] == (
+        f"{oauth2client.GOOGLE_AUTH_URI}?response_type=code&client_id={CLIENT_ID}"
+        "&redirect_uri=https://example.com/auth/external/callback"
+        f"&state={state}&scope=https://www.googleapis.com/auth/drive.file"
+        "&access_type=offline&prompt=consent"
+    )
+
+    client = await hass_client_no_auth()
+    resp = await client.get(f"/auth/external/callback?code=abcd&state={state}")
+    assert resp.status == 200
+    assert resp.headers["content-type"] == "text/html; charset=utf-8"
+
+    # Prepare fake client library response when creating the sheet
+    mock_create = Mock()
+    mock_create.return_value.id = SHEET_ID
+    mock_client.return_value.create = mock_create
+
+    aioclient_mock.post(
+        oauth2client.GOOGLE_TOKEN_URI,
+        json={
+            "refresh_token": "mock-refresh-token",
+            "access_token": "mock-access-token",
+            "type": "Bearer",
+            "expires_in": 60,
+        },
+    )
+
+    result = await hass.config_entries.flow.async_configure(result["flow_id"])
+    assert result.get("type") == "abort"
+    assert result.get("reason") == "already_configured"

tests/components/google_sheets/test_init.py

@@ -14,6 +14,7 @@ from homeassistant.components.application_credentials import (
 from homeassistant.components.google_sheets import DOMAIN
 from homeassistant.config_entries import ConfigEntryState
 from homeassistant.core import HomeAssistant
+from homeassistant.exceptions import ServiceNotFound
 from homeassistant.setup import async_setup_component
 
 from tests.common import MockConfigEntry
@@ -75,16 +76,6 @@ async def mock_setup_integration(
 
     yield func
 
-    # Verify clean unload
-    entries = hass.config_entries.async_entries(DOMAIN)
-    assert len(entries) == 1
-    await hass.config_entries.async_unload(entries[0].entry_id)
-    await hass.async_block_till_done()
-    assert not len(hass.services.async_services().get(DOMAIN, {}))
-
-    assert not hass.data.get(DOMAIN)
-    assert entries[0].state is ConfigEntryState.NOT_LOADED
-
 
 async def test_setup_success(
     hass: HomeAssistant, setup_integration: ComponentSetup
@@ -96,6 +87,13 @@ async def test_setup_success(
     assert len(entries) == 1
     assert entries[0].state is ConfigEntryState.LOADED
 
+    await hass.config_entries.async_unload(entries[0].entry_id)
+    await hass.async_block_till_done()
+
+    assert not hass.data.get(DOMAIN)
+    assert entries[0].state is ConfigEntryState.NOT_LOADED
+    assert not len(hass.services.async_services().get(DOMAIN, {}))
+
 
 @pytest.mark.parametrize(
     "scopes",
@@ -194,7 +192,7 @@ async def test_append_sheet(
     setup_integration: ComponentSetup,
     config_entry: MockConfigEntry,
 ) -> None:
-    """Test successful setup and unload."""
+    """Test service call appending to a sheet."""
     await setup_integration()
 
     entries = hass.config_entries.async_entries(DOMAIN)
@@ -213,3 +211,79 @@ async def test_append_sheet(
             blocking=True,
         )
     assert len(mock_client.mock_calls) == 8
+
+
+async def test_append_sheet_invalid_config_entry(
+    hass: HomeAssistant,
+    setup_integration: ComponentSetup,
+    config_entry: MockConfigEntry,
+    expires_at: int,
+    scopes: list[str],
+) -> None:
+    """Test service call with invalid config entries."""
+    config_entry2 = MockConfigEntry(
+        domain=DOMAIN,
+        unique_id=TEST_SHEET_ID + "2",
+        data={
+            "auth_implementation": DOMAIN,
+            "token": {
+                "access_token": "mock-access-token",
+                "refresh_token": "mock-refresh-token",
+                "expires_at": expires_at,
+                "scope": " ".join(scopes),
+            },
+        },
+    )
+    config_entry2.add_to_hass(hass)
+
+    await setup_integration()
+
+    assert config_entry.state is ConfigEntryState.LOADED
+    assert config_entry2.state is ConfigEntryState.LOADED
+
+    # Exercise service call on a config entry that does not exist
+    with pytest.raises(ValueError, match="Invalid config entry"):
+        await hass.services.async_call(
+            DOMAIN,
+            "append_sheet",
+            {
+                "config_entry": config_entry.entry_id + "XXX",
+                "worksheet": "Sheet1",
+                "data": {"foo": "bar"},
+            },
+            blocking=True,
+        )
+
+    # Unload the config entry invoke the service on the unloaded entry id
+    await hass.config_entries.async_unload(config_entry2.entry_id)
+    await hass.async_block_till_done()
+    assert config_entry2.state is ConfigEntryState.NOT_LOADED
+
+    with pytest.raises(ValueError, match="Config entry not loaded"):
+        await hass.services.async_call(
+            DOMAIN,
+            "append_sheet",
+            {
+                "config_entry": config_entry2.entry_id,
+                "worksheet": "Sheet1",
+                "data": {"foo": "bar"},
+            },
+            blocking=True,
+        )
+
+    # Unloading the other config entry will de-register the service
+    await hass.config_entries.async_unload(config_entry.entry_id)
+    await hass.async_block_till_done()
+    assert config_entry.state is ConfigEntryState.NOT_LOADED
+
+    with pytest.raises(ServiceNotFound):
+        await hass.services.async_call(
+            DOMAIN,
+            "append_sheet",
+            {
+                "config_entry": config_entry.entry_id,
+                "worksheet": "Sheet1",
+                "data": {"foo": "bar"},
+            },
+            blocking=True,
+        )