Skip to content

Incidents: Add separate factories for full id tags/partial id tags - #545

Merged
BastianLedererIcinga merged 1 commit into
mainfrom
refactor-incidents
Sep 2, 2026
Merged

Incidents: Add separate factories for full id tags/partial id tags#545
BastianLedererIcinga merged 1 commit into
mainfrom
refactor-incidents

Conversation

@BastianLedererIcinga

@BastianLedererIcinga BastianLedererIcinga commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Replace the find factory with 3 separate factories for different use cases.

get(): expects the full id tags of an object and return the matching incident or null
getAll(): expects an iterable that yields arrays if id tags, which are expected to be complete. the created Incidents instance will yield every matching incident.
matchAll(): allows partial matches.

In the case of get() the created Incident is instantiated with a query which is only executed when required.
If the query does not return an incident an IncidentNotFoundException is thrown.

IncidentsTest was adjusted with claude to match the new implementation.

@cla-bot cla-bot Bot added the cla/signed CLA is signed by all contributors of a PR label Aug 19, 2026
Comment thread library/Notifications/Integrations/Incidents.php Outdated
Comment thread library/Notifications/Integrations/Incidents.php Outdated
Comment thread library/Notifications/Integrations/Incident.php

@nilmerg nilmerg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so far. But I feel there's more room for improvement 😉

Looking at how this is used in Icinga DB Web shows that hasIncident and getRoles are now obsolete. That's rather a sign of the reason to only support a single incident, though.

But what Icinga DB Web, as an integration for a single incident, actually does are three things:

  1. Showing subscribers and recipients
    a. also checks whether an incident is returned first
  2. Showing a quick action to the current user
    a. only if an incident exists
  3. Processing said quick action
    a. for a list of incidents, due to bulk actions

There's no reason to fetch the incident in any case, except for the reason to know it exists. But that aside, it is not needed to fetch any incident details at all. And the existence can be verified differently on the use case. Incident::getRole for example, may just throw an error, or return null and none for no role, to indicate a missing incident. Incidents::get must be lazy for this and thus may only be an alias for a factory of Incident.

And this approach I'd like to see also for getAll and matchAll. It must be mandatory to iterate the result to know something matched. hasIncident has really no use in this case and can safely be removed. getRoles may still have a use, but only in case of other integrations, but no known one and thus I'd object it's existence as well.

Comment thread library/Notifications/Integrations/Incident.php
Comment thread library/Notifications/Integrations/Incident.php
Comment thread library/Notifications/Integrations/Incident.php Outdated
Comment thread library/Notifications/Integrations/Incidents.php Outdated
Comment thread library/Notifications/Integrations/Incident.php Outdated
Comment thread library/Notifications/Integrations/Incident.php Outdated
nilmerg
nilmerg previously approved these changes Aug 28, 2026
@BastianLedererIcinga
BastianLedererIcinga force-pushed the refactor-incidents branch 2 times, most recently from c315e7b to a456c1f Compare August 31, 2026 07:34
@nilmerg

nilmerg commented Sep 2, 2026

Copy link
Copy Markdown
Member

please rebase

Comment thread library/Notifications/Integrations/Incidents.php
Add 3 separate factories to `Incidents`.

`matchAll()` and `getAll()` aaccept an iterable where each element
is a set of tags, all incidents matching any of the tag sets are yielded.

In the case of `getAll()` exact matches are required, `matchAll()` allows
partial matches as well.

`get()` expects the full tags of a single object, and creates an `Incident`
from a query instead of a `Model`. The query is run lazily, if it does not return
an `Inciednt` an `IncidentNotFoundException` is thrown.
@BastianLedererIcinga
BastianLedererIcinga merged commit 16ed23d into main Sep 2, 2026
13 checks passed
@BastianLedererIcinga
BastianLedererIcinga deleted the refactor-incidents branch September 2, 2026 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla/signed CLA is signed by all contributors of a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants