diff --git a/core/handlers/ticket_opened_test.go b/core/handlers/ticket_opened_test.go index 652552356..51fa7c50d 100644 --- a/core/handlers/ticket_opened_test.go +++ b/core/handlers/ticket_opened_test.go @@ -120,12 +120,12 @@ func TestTicketOpened(t *testing.T) { Count: 1, }, { // admin will have a ticket assigned notification for the ticket directly assigned to them - SQL: "select count(*) from notifications_notification n inner join notifications_log l on l.id = n.log_id where n.user_id = $1 and l.log_type = 'ticket:assigned'", + SQL: "select count(*) from notifications_notification where user_id = $1 and notification_type = 'tickets:activity'", Args: []interface{}{testdata.Admin.ID}, Count: 1, }, { // all assignable users will have a ticket opened notification for the unassigned ticket - SQL: "select count(*) from notifications_notification n inner join notifications_log l on l.id = n.log_id where l.log_type = 'ticket:opened'", + SQL: "select count(*) from notifications_notification where notification_type = 'tickets:opened'", Args: nil, Count: 3, }, diff --git a/core/hooks/insert_tickets.go b/core/hooks/insert_tickets.go index 16820a446..4860aeaf4 100644 --- a/core/hooks/insert_tickets.go +++ b/core/hooks/insert_tickets.go @@ -34,8 +34,11 @@ func (h *insertTicketsHook) Apply(ctx context.Context, tx *sqlx.Tx, rp *redis.Po // generate opened events for each ticket openEvents := make([]*models.TicketEvent, len(tickets)) + eventsByTicket := make(map[*models.Ticket]*models.TicketEvent, len(tickets)) for i, ticket := range tickets { - openEvents[i] = models.NewTicketOpenedEvent(ticket, models.NilUserID, ticket.AssigneeID()) + evt := models.NewTicketOpenedEvent(ticket, models.NilUserID, ticket.AssigneeID()) + openEvents[i] = evt + eventsByTicket[ticket] = evt } // and insert those too @@ -45,9 +48,9 @@ func (h *insertTicketsHook) Apply(ctx context.Context, tx *sqlx.Tx, rp *redis.Po } // and insert logs/notifications for those - err = models.LogTicketsOpened(ctx, tx, oa, openEvents) + err = models.NotificationsFromTicketEvents(ctx, tx, oa, eventsByTicket) if err != nil { - return errors.Wrapf(err, "error inserting logs and notifications") + return errors.Wrapf(err, "error inserting notifications") } return nil diff --git a/core/models/imports.go b/core/models/imports.go index bd91d3a66..0f6aab119 100644 --- a/core/models/imports.go +++ b/core/models/imports.go @@ -2,6 +2,7 @@ package models import ( "context" + "database/sql/driver" "encoding/json" "fmt" "strings" @@ -14,13 +15,19 @@ import ( "github.com/nyaruka/goflow/envs" "github.com/nyaruka/goflow/flows" "github.com/nyaruka/goflow/flows/modifiers" + "github.com/nyaruka/null" "github.com/pkg/errors" "github.com/jmoiron/sqlx" ) // ContactImportID is the type for contact import IDs -type ContactImportID int64 +type ContactImportID null.Int + +func (i ContactImportID) MarshalJSON() ([]byte, error) { return null.Int(i).MarshalJSON() } +func (i *ContactImportID) UnmarshalJSON(b []byte) error { return null.UnmarshalInt(b, (*null.Int)(i)) } +func (i ContactImportID) Value() (driver.Value, error) { return null.Int(i).Value() } +func (i *ContactImportID) Scan(value interface{}) error { return null.ScanInt(value, (*null.Int)(i)) } // ContactImportBatchID is the type for contact import batch IDs type ContactImportBatchID int64 diff --git a/core/models/notifications.go b/core/models/notifications.go index fb011ba02..3491d8786 100644 --- a/core/models/notifications.go +++ b/core/models/notifications.go @@ -2,182 +2,116 @@ package models import ( "context" + "time" "github.com/nyaruka/mailroom/utils/dbutil" "github.com/pkg/errors" ) -// LogID is our type for log ids -type LogID int +// NotificationID is our type for notification ids +type NotificationID int -type LogType string +type NotificationType string const ( - LogTypeBroadcastStarted LogType = "bcast:started" - LogTypeBroadcastCompleted LogType = "bcast:completed" - LogTypeChannelAlert LogType = "channel:alert" - LogTypeExportStarted LogType = "export:started" - LogTypeExportCompleted LogType = "export:completed" - LogTypeFlowStartStarted LogType = "start:started" - LogTypeFlowStartCompleted LogType = "start:completed" - LogTypeImportStarted LogType = "import:started" - LogTypeImportCompleted LogType = "import:completed" - LogTypeTicketOpened LogType = "ticket:opened" - LogTypeTicketNewMsgs LogType = "ticket:msgs" - LogTypeTicketAssigned LogType = "ticket:assigned" - LogTypeTicketNote LogType = "ticket:note" + NotificationTypeChannelAlert NotificationType = "channel:alert" + NotificationTypeExportFinished NotificationType = "export:finished" + NotificationTypeImportFinished NotificationType = "import:finished" + NotificationTypeTicketsOpened NotificationType = "tickets:opened" + NotificationTypeTicketsActivity NotificationType = "tickets:activity" ) -type Log struct { - ID LogID `db:"id"` - OrgID OrgID `db:"org_id"` - LogType LogType `db:"log_type"` - CreatedByID UserID `db:"created_by_id"` - - BroadcastID BroadcastID `db:"broadcast_id"` - FlowStartID StartID `db:"flow_start_id"` - TicketID TicketID `db:"ticket_id"` - TicketEventID TicketEventID `db:"ticket_event_id"` -} - type Notification struct { - ID LogID `db:"id"` - OrgID OrgID `db:"org_id"` - UserID UserID `db:"user_id"` - LogID LogID `db:"log_id"` -} - -type notifyWhoFunc func(l *Log) ([]UserRole, []UserID) - -/* -func notifyUser(id UserID) notifyWhoFunc { - return func(l *Log) ([]UserRole, []UserID) { - return nil, []UserID{id} - } -} - -func notifyRoles(roles ...UserRole) notifyWhoFunc { - return func(l *Log) ([]UserRole, []UserID) { - return roles, nil - } + ID NotificationID `db:"id"` + OrgID OrgID `db:"org_id"` + Type NotificationType `db:"notification_type"` + Scope string `db:"scope"` + UserID UserID `db:"user_id"` + IsSeen bool `db:"is_seen"` + CreatedOn time.Time `db:"created_on"` + + ChannelID ChannelID `db:"channel_id"` + ContactImportID ContactImportID `db:"contact_import_id"` } -*/ - -// LogTicketsOpened logs the opening of new tickets and notifies all assignable users if tickets is not already assigned -func LogTicketsOpened(ctx context.Context, db Queryer, oa *OrgAssets, events []*TicketEvent) error { - // create log for each ticket event and record which users are assigned - logs := make([]*Log, len(events)) - assignees := make(map[*Log]UserID, len(events)) - - for i, evt := range events { - logType := LogTypeTicketOpened - if evt.AssigneeID() != NilUserID { - logType = LogTypeTicketAssigned - } - log := &Log{ - OrgID: evt.OrgID(), - LogType: logType, - CreatedByID: evt.CreatedByID(), - TicketID: evt.TicketID(), - TicketEventID: evt.ID(), +var ticketAssignableToles = []UserRole{UserRoleAdministrator, UserRoleEditor, UserRoleAgent} + +// NotificationsFromTicketEvents logs the opening of new tickets and notifies all assignable users if tickets is not already assigned +func NotificationsFromTicketEvents(ctx context.Context, db Queryer, oa *OrgAssets, events map[*Ticket]*TicketEvent) error { + notifyTicketsOpened := make(map[UserID]bool) + notifyTicketsActivity := make(map[UserID]bool) + + for ticket, evt := range events { + switch evt.EventType() { + case TicketEventTypeOpened: + // if ticket is unassigned notify all possible assignees + if evt.AssigneeID() == NilUserID { + for _, u := range oa.users { + user := u.(*User) + + if hasAnyRole(user, ticketAssignableToles) && evt.CreatedByID() != user.ID() { + notifyTicketsOpened[user.ID()] = true + } + } + } else if evt.AssigneeID() != evt.CreatedByID() { + notifyTicketsActivity[evt.AssigneeID()] = true + } + case TicketEventTypeAssigned: + // notify new ticket assignee if they didn't self-assign + if evt.AssigneeID() != NilUserID && evt.AssigneeID() != evt.CreatedByID() { + notifyTicketsActivity[evt.AssigneeID()] = true + } + case TicketEventTypeNoteAdded: + // notify ticket assignee if they didn't add note themselves + if ticket.AssigneeID() != evt.CreatedByID() { + notifyTicketsActivity[ticket.AssigneeID()] = true + } } - logs[i] = log - assignees[log] = evt.AssigneeID() } - return insertLogsAndNotifications(ctx, db, oa, logs, func(l *Log) ([]UserRole, []UserID) { - // if this log is actually an assignment then only notify the assignee - if l.LogType == LogTypeTicketAssigned { - return nil, []UserID{assignees[l]} - } - - // otherwise notify all possible assignees - return []UserRole{UserRoleAdministrator, UserRoleEditor, UserRoleAgent}, nil - }) -} + notifications := make([]*Notification, 0, len(events)) -// LogTicketsAssigned logs the assignment of tickets and notifies the assignees -func LogTicketsAssigned(ctx context.Context, db Queryer, oa *OrgAssets, events []*TicketEvent) error { - // create log for each ticket event and record which users are assigned - logs := make([]*Log, len(events)) - assignees := make(map[*Log]UserID, len(events)) - - for i, evt := range events { - log := &Log{ - OrgID: evt.OrgID(), - LogType: LogTypeTicketAssigned, - CreatedByID: evt.CreatedByID(), - TicketID: evt.TicketID(), - TicketEventID: evt.ID(), - } - logs[i] = log - assignees[log] = evt.AssigneeID() + for userID := range notifyTicketsOpened { + notifications = append(notifications, &Notification{ + OrgID: oa.OrgID(), + Type: NotificationTypeTicketsOpened, + Scope: "", + UserID: userID, + }) } - return insertLogsAndNotifications(ctx, db, oa, logs, func(l *Log) ([]UserRole, []UserID) { - assigneeID := assignees[l] - - // if ticket has been assigned, notify assignee - if assigneeID != NilUserID { - return nil, []UserID{assigneeID} - } + for userID := range notifyTicketsActivity { + notifications = append(notifications, &Notification{ + OrgID: oa.OrgID(), + Type: NotificationTypeTicketsActivity, + Scope: "", + UserID: userID, + }) + } - // otherwise don't notify anyone if it's being unassigned - return nil, nil - }) + return insertNotifications(ctx, db, notifications) } -const insertLogSQL = ` -INSERT INTO notifications_log(org_id, log_type, created_on, created_by_id, broadcast_id, flow_start_id, ticket_id, ticket_event_id) - VALUES(:org_id, :log_type, NOW(), :created_by_id, :broadcast_id, :flow_start_id, :ticket_id, :ticket_event_id) -RETURNING id` - const insertNotificationSQL = ` -INSERT INTO notifications_notification(org_id, user_id, log_id, is_seen) - VALUES(:org_id, :user_id, :log_id, FALSE)` - -func insertLogsAndNotifications(ctx context.Context, db Queryer, oa *OrgAssets, logs []*Log, notifyWho notifyWhoFunc) error { - is := make([]interface{}, len(logs)) - for i := range logs { - is[i] = logs[i] - } - - err := dbutil.BulkQuery(ctx, db, insertLogSQL, is) - if err != nil { - return errors.Wrap(err, "error inserting logs") - } - - var notifications []interface{} - - for _, log := range logs { - notifyRoles, notifyUsers := notifyWho(log) - - for _, u := range oa.users { - user := u.(*User) - - // don't create a notification for the user that did the logged thing - if log.CreatedByID != user.ID() && userMatchesRoleOrID(user, notifyRoles, notifyUsers) { - notifications = append(notifications, &Notification{OrgID: oa.OrgID(), UserID: user.ID(), LogID: log.ID}) - } - } +INSERT INTO notifications_notification(org_id, notification_type, scope, user_id, is_seen, created_on, channel_id, contact_import_id) + VALUES(:org_id, :notification_type, :scope, :user_id, FALSE, NOW(), :channel_id, :contact_import_id) + ON CONFLICT DO NOTHING` + +func insertNotifications(ctx context.Context, db Queryer, notifications []*Notification) error { + is := make([]interface{}, len(notifications)) + for i := range notifications { + is[i] = notifications[i] } - err = dbutil.BulkQuery(ctx, db, insertNotificationSQL, notifications) - + err := dbutil.BulkQuery(ctx, db, insertNotificationSQL, is) return errors.Wrap(err, "error inserting notifications") } -func userMatchesRoleOrID(user *User, roles []UserRole, ids []UserID) bool { +func hasAnyRole(user *User, roles []UserRole) bool { for _, r := range roles { if user.Role() == r { return true } } - for _, id := range ids { - if user.ID() == id { - return true - } - } return false } diff --git a/core/models/notifications_test.go b/core/models/notifications_test.go index 15e43d248..fe333c0f8 100644 --- a/core/models/notifications_test.go +++ b/core/models/notifications_test.go @@ -3,6 +3,7 @@ package models_test import ( "context" "testing" + "time" "github.com/jmoiron/sqlx" "github.com/nyaruka/mailroom/core/models" @@ -12,90 +13,115 @@ import ( "github.com/stretchr/testify/require" ) -func TestTicketOpenedNotifications(t *testing.T) { +func TestTicketNotifications(t *testing.T) { ctx, _, db, _ := testsuite.Get() + defer deleteTickets(db) + oa, err := models.GetOrgAssets(ctx, db, testdata.Org1.ID) require.NoError(t, err) - // open an unassigned ticket by a flow (i.e. no user) - _, openedEvent := openTicket(t, ctx, db, nil, nil) - err = models.LogTicketsOpened(ctx, db, oa, []*models.TicketEvent{openedEvent}) + t0 := time.Now() + + // open unassigned tickets by a flow (i.e. no user) + ticket1, openedEvent1 := openTicket(t, ctx, db, nil, nil) + ticket2, openedEvent2 := openTicket(t, ctx, db, nil, nil) + err = models.NotificationsFromTicketEvents(ctx, db, oa, map[*models.Ticket]*models.TicketEvent{ticket1: openedEvent1, ticket2: openedEvent2}) require.NoError(t, err) - // check that all assignable users are notified - log := assertNotifications(t, ctx, db, models.LogTypeTicketOpened, nil, []*testdata.User{testdata.Admin, testdata.Editor, testdata.Agent}) - assert.Equal(t, openedEvent.TicketID(), log.TicketID) - assert.Equal(t, openedEvent.ID(), log.TicketEventID) + // check that all assignable users are notified once + assertNotifications(t, ctx, db, t0, map[*testdata.User][]models.NotificationType{ + testdata.Admin: {models.NotificationTypeTicketsOpened}, + testdata.Editor: {models.NotificationTypeTicketsOpened}, + testdata.Agent: {models.NotificationTypeTicketsOpened}, + }) + + t1 := time.Now() + + // another ticket opened won't create new notifications + ticket3, openedEvent3 := openTicket(t, ctx, db, nil, nil) + err = models.NotificationsFromTicketEvents(ctx, db, oa, map[*models.Ticket]*models.TicketEvent{ticket3: openedEvent3}) + require.NoError(t, err) + + assertNotifications(t, ctx, db, t1, map[*testdata.User][]models.NotificationType{}) + + // mark all notifications as seen + db.MustExec(`UPDATE notifications_notification SET is_seen = TRUE`) // open an unassigned ticket by a user - _, openedEvent = openTicket(t, ctx, db, testdata.Editor, nil) - err = models.LogTicketsOpened(ctx, db, oa, []*models.TicketEvent{openedEvent}) + ticket4, openedEvent4 := openTicket(t, ctx, db, testdata.Editor, nil) + err = models.NotificationsFromTicketEvents(ctx, db, oa, map[*models.Ticket]*models.TicketEvent{ticket4: openedEvent4}) require.NoError(t, err) - // check that all assignable users are notified except the user who opened the ticket - assertNotifications(t, ctx, db, models.LogTypeTicketOpened, testdata.Editor, []*testdata.User{testdata.Admin, testdata.Agent}) + // check that all assignable users are notified except the user that opened the ticket + assertNotifications(t, ctx, db, t1, map[*testdata.User][]models.NotificationType{ + testdata.Admin: {models.NotificationTypeTicketsOpened}, + testdata.Agent: {models.NotificationTypeTicketsOpened}, + }) + + t2 := time.Now() + db.MustExec(`UPDATE notifications_notification SET is_seen = TRUE`) // open an already assigned ticket - _, openedEvent = openTicket(t, ctx, db, nil, testdata.Agent) - err = models.LogTicketsOpened(ctx, db, oa, []*models.TicketEvent{openedEvent}) + ticket5, openedEvent5 := openTicket(t, ctx, db, nil, testdata.Agent) + err = models.NotificationsFromTicketEvents(ctx, db, oa, map[*models.Ticket]*models.TicketEvent{ticket5: openedEvent5}) require.NoError(t, err) - // check that the log type is actually assigned and that we notify the assigned user only - assertNotifications(t, ctx, db, models.LogTypeTicketAssigned, nil, []*testdata.User{testdata.Agent}) + // check that the assigned user gets a ticket activity notification + assertNotifications(t, ctx, db, t2, map[*testdata.User][]models.NotificationType{ + testdata.Agent: {models.NotificationTypeTicketsActivity}, + }) + + t3 := time.Now() - // unless they self-assigned.. - _, openedEvent = openTicket(t, ctx, db, testdata.Agent, testdata.Agent) - err = models.LogTicketsOpened(ctx, db, oa, []*models.TicketEvent{openedEvent}) + // however if a user opens a ticket which is assigned to themselves, no notification + ticket6, openedEvent6 := openTicket(t, ctx, db, testdata.Admin, testdata.Admin) + err = models.NotificationsFromTicketEvents(ctx, db, oa, map[*models.Ticket]*models.TicketEvent{ticket6: openedEvent6}) require.NoError(t, err) - // in which case there is nobody to notify - assertNotifications(t, ctx, db, models.LogTypeTicketAssigned, testdata.Agent, nil) -} + // check that the assigned user gets a ticket activity notification + assertNotifications(t, ctx, db, t3, map[*testdata.User][]models.NotificationType{}) -func TestTicketAssignedNotifications(t *testing.T) { - ctx, _, db, _ := testsuite.Get() + t4 := time.Now() + db.MustExec(`UPDATE notifications_notification SET is_seen = TRUE`) - oa, err := models.GetOrgAssets(ctx, db, testdata.Org1.ID) + // now have a user assign existing tickets to another user + _, err = models.TicketsAssign(ctx, db, oa, testdata.Admin.ID, []*models.Ticket{ticket1, ticket2}, testdata.Agent.ID, "") require.NoError(t, err) - // open an unassigned ticket - ticket, _ := openTicket(t, ctx, db, nil, nil) - require.NoError(t, err) + // check that the assigned user gets a ticket activity notification + assertNotifications(t, ctx, db, t4, map[*testdata.User][]models.NotificationType{ + testdata.Agent: {models.NotificationTypeTicketsActivity}, + }) - assignedEvent := models.NewTicketAssignedEvent(ticket, testdata.Admin.ID, testdata.Agent.ID, "please") - err = models.InsertTicketEvents(ctx, db, []*models.TicketEvent{assignedEvent}) - require.NoError(t, err) + t5 := time.Now() + db.MustExec(`UPDATE notifications_notification SET is_seen = TRUE`) - err = models.LogTicketsAssigned(ctx, db, oa, []*models.TicketEvent{assignedEvent}) + // and finally a user assigning a ticket to themselves + _, err = models.TicketsAssign(ctx, db, oa, testdata.Editor.ID, []*models.Ticket{ticket3}, testdata.Editor.ID, "") require.NoError(t, err) - // check that assignee is notified - assertNotifications(t, ctx, db, models.LogTypeTicketAssigned, testdata.Admin, []*testdata.User{testdata.Agent}) + // no notifications for self-assignment + assertNotifications(t, ctx, db, t5, map[*testdata.User][]models.NotificationType{}) } -func assertNotifications(t *testing.T, ctx context.Context, db *sqlx.DB, expectedType models.LogType, expectedCreatedBy *testdata.User, expectedNotified []*testdata.User) *models.Log { +func assertNotifications(t *testing.T, ctx context.Context, db *sqlx.DB, after time.Time, expected map[*testdata.User][]models.NotificationType) { // check last log - log := &models.Log{} - err := db.GetContext(ctx, log, `SELECT id, org_id, log_type, created_by_id, ticket_id, ticket_event_id FROM notifications_log ORDER BY id DESC LIMIT 1`) + var notifications []*models.Notification + err := db.SelectContext(ctx, ¬ifications, `SELECT id, org_id, notification_type, scope, user_id, is_seen, created_on FROM notifications_notification WHERE created_on > $1 ORDER BY id`, after) require.NoError(t, err) - assert.Equal(t, expectedType, log.LogType, "log type mismatch") - assert.Equal(t, expectedCreatedBy.SafeID(), log.CreatedByID, "log created by mismatch") - - // check who was notified - var actualNotifiedIDs []models.UserID - err = db.SelectContext(ctx, &actualNotifiedIDs, `SELECT user_id FROM notifications_notification WHERE log_id = $1`, log.ID) - require.NoError(t, err) - - expectedNotifiedIDs := make([]models.UserID, len(expectedNotified)) - for i := range expectedNotified { - expectedNotifiedIDs[i] = expectedNotified[i].ID + expectedByID := map[models.UserID][]models.NotificationType{} + for user, notificationTypes := range expected { + expectedByID[user.ID] = notificationTypes } - assert.ElementsMatch(t, expectedNotifiedIDs, actualNotifiedIDs, "notified users mismatch") + actual := map[models.UserID][]models.NotificationType{} + for _, notification := range notifications { + actual[notification.UserID] = append(actual[notification.UserID], notification.Type) + } - return log + assert.Equal(t, expectedByID, actual) } func openTicket(t *testing.T, ctx context.Context, db *sqlx.DB, openedBy *testdata.User, assignee *testdata.User) (*models.Ticket, *models.TicketEvent) { diff --git a/core/models/tickets.go b/core/models/tickets.go index 1c96ee34f..e6f01dcc3 100644 --- a/core/models/tickets.go +++ b/core/models/tickets.go @@ -428,9 +428,9 @@ func TicketsAssign(ctx context.Context, db Queryer, oa *OrgAssets, userID UserID return nil, errors.Wrap(err, "error inserting ticket events") } - err = LogTicketsAssigned(ctx, db, oa, events) + err = NotificationsFromTicketEvents(ctx, db, oa, eventsByTicket) if err != nil { - return nil, errors.Wrap(err, "error inserting logs and notifications") + return nil, errors.Wrap(err, "error inserting notifications") } return eventsByTicket, nil diff --git a/core/models/tickets_test.go b/core/models/tickets_test.go index 9b80f69e7..17da6cb33 100644 --- a/core/models/tickets_test.go +++ b/core/models/tickets_test.go @@ -210,8 +210,7 @@ func TestTicketsAssign(t *testing.T) { // and there are new assigned events testsuite.AssertQuery(t, db, `SELECT count(*) FROM tickets_ticketevent WHERE event_type = 'A' AND note = 'please handle these'`).Returns(2) - testsuite.AssertQuery(t, db, `SELECT count(*) FROM notifications_log WHERE log_type = 'ticket:assigned' AND created_by_id = $1`, testdata.Admin.ID).Returns(2) - testsuite.AssertQuery(t, db, `SELECT count(*) FROM notifications_notification WHERE user_id = $1`, testdata.Agent.ID).Returns(2) + testsuite.AssertQuery(t, db, `SELECT count(*) FROM notifications_notification WHERE user_id = $1 AND notification_type = 'tickets:activity'`, testdata.Agent.ID).Returns(1) } func TestTicketsAddNote(t *testing.T) { @@ -374,7 +373,6 @@ func TestReopenTickets(t *testing.T) { func deleteTickets(db *sqlx.DB) { db.MustExec(`DELETE FROM notifications_notification`) - db.MustExec(`DELETE FROM notifications_log`) db.MustExec(`DELETE FROM request_logs_httplog`) db.MustExec(`DELETE FROM tickets_ticketevent`) db.MustExec(`DELETE FROM tickets_ticket`) diff --git a/mailroom_test.dump b/mailroom_test.dump index fa09ffa4f..0b203ddbc 100644 Binary files a/mailroom_test.dump and b/mailroom_test.dump differ