Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write schedule/group_id for contact notified histories #64

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yhabteab
Copy link
Member

When a contact is notified because of being member of a Schedule or a ContactGroup, it may not be clear why a particular contact was notified as it has no direct reference to the current incident. With this PR, noma-web can easily identify that the contact was notified due to a matching ContactGroup or Schedule and rendere a proper message accordingly.

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label May 10, 2023
@julianbrost
Copy link
Collaborator

Another thing I just noticed which would be nice: If a schedule with no entry at the current time or an empty group is notified (i.e. no notification actually happened because it resolved to no contact), writing a row with schedule_id=42, contact_id=NULL nice, meaning "a notification would have been sent to that schedule, but there was no contact in it at that time, so nothing was sent to it".

@@ -33,6 +33,24 @@ func (r Key) MarshalText() (text []byte, err error) {
}
}

// CopyNonNil copies non-nil fields from the provided recipient.Key to the current one and
// returns the modified recipient key.
func (r Key) CopyNonNil(recipientKey Key) Key {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function and how you use recipient.Key in incident.HistoryRow contradicts the current assumption in the code that there's a 1:1 relation between recipient.Recipient and recipient.Key, as it can be seen in multiple places:

type Key struct {
// Only one of the members is allowed to be a non-zero value.
ContactID types.Int `db:"contact_id"`
GroupID types.Int `db:"contactgroup_id"`
ScheduleID types.Int `db:"schedule_id"`
}

// MarshalText implements the encoding.TextMarshaler interface to allow JSON marshaling of map[Key]T.
func (r Key) MarshalText() (text []byte, err error) {
if r.ContactID.Valid {
return []byte(fmt.Sprintf("contact_id=%d", r.ContactID.Int64)), nil
} else if r.GroupID.Valid {
return []byte(fmt.Sprintf("group_id=%d", r.GroupID.Int64)), nil
} else if r.ScheduleID.Valid {
return []byte(fmt.Sprintf("schedule_id=%d", r.ScheduleID.Int64)), nil
} else {
return nil, nil
}
}

func (r *RuntimeConfig) GetRecipient(k recipient.Key) recipient.Recipient {
// Note: be careful to return nil for non-existent IDs instead of (*T)(nil) as (*T)(nil) != nil.
if k.ContactID.Valid {
c := r.Contacts[k.ContactID.Int64]
if c != nil {
return c
}
} else if k.GroupID.Valid {
g := r.Groups[k.GroupID.Int64]
if g != nil {
return g
}
} else if k.ScheduleID.Valid {
s := r.Schedules[k.ScheduleID.Int64]
if s != nil {
return s
}
}
return nil
}

I'd want to keep that assumption in place as recipient.Key is used as a key in maps for example and that would result in nasty bugs if recipient.Key values with more than one member set would ever be used in a map key.

So I think it would be better to add the three columns as explicit members in incident.HistoryRow if it's now intended that more than one is set to a non-null value.

Comment on lines +457 to +459
if !isContact {
groupsOrSchedules[c] = recipient.ToKey(r)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's supposed to happen if the same contact if references multiple times, i.e. directly, via a group and via a schedule? This would just arbitrarily pick a group or schedule. We could write all of that information into the database, but that would probably mean writing multiple rows for the same notification, i.e. the web interface would have to handle and display this probably, so I don't want to decide this on my own.

No matter how we do it, a comment in the schema file how these rows are to be interpreted would be great.

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