Incidents: Add separate factories for full id tags/partial id tags - #545
Conversation
nilmerg
left a comment
There was a problem hiding this comment.
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:
- Showing subscribers and recipients
a. also checks whether an incident is returned first - Showing a quick action to the current user
a. only if an incident exists - 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.
e49c2fb to
74190c0
Compare
c315e7b to
a456c1f
Compare
|
please rebase |
a456c1f to
d088c4a
Compare
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.
2afa695 to
b2ac761
Compare
Replace the
findfactory with 3 separate factories for different use cases.get(): expects the full id tags of an object and return the matching incident ornullgetAll(): expects aniterablethat yields arrays if id tags, which are expected to be complete. the createdIncidentsinstance will yield every matching incident.matchAll(): allows partial matches.In the case of
get()the createdIncidentis instantiated with a query which is only executed when required.If the query does not return an incident an
IncidentNotFoundExceptionis thrown.IncidentsTestwas adjusted with claude to match the new implementation.