Add include_hidden parameter to Spond.get_events()

[Yannis-G]

Description

This PR enhances the functionality of Spond.get_events() by adding a new parameter, include_hidden, which allows fetching hidden events that are visible to users with at least admin permissions, addressing the feature request in issue #164.

Note: The include_hidden parameter is only supported if a group_id is provided. If used without group_id, the Spond API will respond with an error:

'includeHidden' filter is only available inside a group

Therefore, this parameter is included in the request only when both group_id and include_hidden=True are specified. Otherwise, the method behaves as it did previously.

Summary of Changes

• New Parameter: Added include_hidden to the method signature.
• Update Docstring: Tried to match the current style of the docstring.
• API Parameter: The "includeHidden" flag is added to the request parameters only if both group_id is provided and include_hidden is set to True. The behavior remains unchanged if these conditions are not met.

Example Usage

events = await spond_client.get_events(group_id=<GROUP_ID>, include_hidden=True)

[elliot-100]

Hi - many thanks for this. I'll have a go at testing it against live data. Might take a few weeks.

[SimonHenz97]

Hey, due to changes in our organization, I'm now unable to fetch certain events.
Including this parameter would resolve the issue for me.

Since it's been a while since this pull request was created, I wanted to follow up and bring attention to it again.

[elliot-100]

I went ahead and updated your fork (you may already know this ofc, but it's generally a good idea to raise your PRs against a feature branch rather than your main, to reduce risk unexpected behaviour change on your side).

I've tested and and it appears to work as described 👍
[Edit: I thought as group owner I was already able to get hidden events, but I was wrong]

But include_hidden param is ignored if group_id is not supplied, silently hiding the error response from API. I don't think this is a good idea, it should ideally raise an exception and pass through the API response. @Olen what do you think?

[Olen]

I think that if it is documented that it does not do anything unless group_id is present, it's totally fine to just ignore it.
I believe there are situations where you could add the parameter "unintentionally" and expect to get a list of events without specifying the group id.

Unless you believe it might be confusing, because you would get the same result for queries both with and without the parameter set?

[elliot-100]

It's a design decision to silently suppress meaningful errors returned by the API, and increases abstraction with little obvious benefit, and some immediate downsides - getting the same result for queries both with and without the parameter set is an example.

But it also risks hiding future API behaviour changes. Why not raise an exception including the message, and then the end user can handle it or ignore as they wish?

[Olen]

Ok, I accept your arguments.

But do you mean we should try the request (even if we are certain it will fail) or should we raise an exception before we run the request if we see that group_id is missing?

[elliot-100]

From a functional POV, the response does not fail - it provides a response with useful information to an application developer, 'includeHidden' filter is only available inside a group.

I don't think we should hide that and reimplement a layer in the client to effectively do the same thing; we should pass it on via exception, and allow the end user to choose how their application behaves via exception handling.

We can't be certain it will fail in future, and by hiding the actual API behaviour both we and end-users are a lot less likely to find out. Or Spond might usefully internationalise their error messages for the user's language.

[Yannis-G]

I see your point about hiding an API response. My intention was to follow the get_events documentation, which states that it returns a list of events or None. However, when calling the function with only include_hidden, it returns {'message': "'includeHidden' filter is only available inside a group"}, which deviates from the expected event-based response and could lead to unexpected behavior.

[Olen]

I think it should raise an exception instead of returning that.

[Yannis-G]

Updated get_events and improved error handling:

• include_hidden is now always included in the request if specified, even when group_id is not provided.
• Added error handling: If the API response status code is 400 or higher, a RuntimeError is raised with the status code and response message for better debugging.

(I wasn't entirely sure if RuntimeError is the best exception type here - this might need to be adjusted.)

[elliot-100]

This is great, but will have a proper test/think Monday.

[Yannis-G]

Sorry for the formatting issues. Forgot to merge the changes in the actions. Now all the checks should be passed.

[elliot-100]

Sorry for delay.

I checked the behaviour and I got this:

Traceback (most recent call last):
...
  File "/home/elliot/code/my_project/.venv/lib/python3.10/site-packages/spond/base.py", line 33, in wrapper
    return await func(self, *args, **kwargs)
  File "/home/elliot/code/my_project/.venv/lib/python3.10/site-packages/spond/spond.py", line 322, in get_events
    raise RuntimeError(
RuntimeError: Request failed with status 400: {"message":"'includeHidden' filter is only available inside a group"}
ERROR:asyncio:Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x7f27f5e61c30>
ERROR:asyncio:Unclosed connector
connections: ['deque([(<aiohttp.client_proto.ResponseHandler object at 0x7f27f5e44c40>, 151637.232855538)])']
connector: <aiohttp.connector.TCPConnector object at 0x7f27f5e619f0>

• So the exception is raised OK 👍

• But I think it should be a ValueError - see e.g. https://stackoverflow.com/questions/256222/which-exception-should-i-raise-on-bad-illegal-argument-combinations-in-python

• There's also the asyncio:Unclosed client session error. I am sure I have seen this before when passing incorrect params, so I don't think it's a new issue caused by this code change, just provides more obvious way to cause it.

  But I thought the context handler here is supposed to look after session closure - @Olen?

async with self.clientsession.get(
    url, headers=self.auth_headers, params=params
) as r:
    self.events = await r.json()

[Edit: I'll leave that last bit here for now, but I think this is a separate issue]

[Yannis-G]

I encountered the same error, which occurs because the client session isn't properly closed. This issue appears in any scenario where .close() is not explicitly called.
I agree that raising a ValueError would be more appropriate—I also considered it but wasn't entirely sure.
The context manager in the method ensures that the request (self.clientsession.get(...)) is properly closed after execution. However, it does not close the entire aiohttp.ClientSession instance; it only manages the specific request (r).

To ensure the clientsession is properly closed, you could modify _SpondBase as follows:

class _SpondBase(ABC):
    def __init__(self, username: str, password: str, api_url: str) -> None:
        ...
        self.clientsession = None

    async def __aenter__(self):
        """Initialize the aiohttp session when entering the async context."""
        self.clientsession = aiohttp.ClientSession(cookie_jar=aiohttp.CookieJar())
        return self  # Return the instance for usage inside `async with`

    async def __aexit__(self, exc_type, exc_val, exc_tb):
        """Ensure the session is properly closed when exiting the async context."""
        if self.clientsession and not self.clientsession.closed:
            await self.clientsession.close()

However, this change significantly affects usage, as the clientsession is only available within the asynchronous context manager:

async with Spond(SPOND_USERNAME, SPOND_PASSWORD) as s:
    group = await s.get_group(group_id)

[Yannis-G]

Sorry, forgot to change the exception from RuntimeError to ValueError in the doc string. But I don't know why the actions failed in the poetry install.

[elliot-100]

Thanks for explaining the context manager / session vs. request part.

I think this PR should be merged as is, as it's currently up to the end application developer to make sure sessions are closed, and this PR doesn't change that.

(Separately, we should:

• make this clear and amend example scripts
• consider changing code)

Are you OK with the merge please @Olen?

[elliot-100]

Sorry, forgot to change the exception from RuntimeError to ValueError in the doc string. But I don't know why the actions failed in the poetry install.

Looks like a temporary issue with propcache package not being available to the VM that's running actions maybe? Anyway works now.

.. and has made me spot a separate warning with ruff version in actions.

[Olen]

Merging is fine with me.
Do you need to fix anything re your last comment first?

[elliot-100]

No I have raised a separate issue for that.

[elliot-100]

Squashed to simplify as there is only one file changed.