populateChecklistIDs consistently

We need to return the playbookRun after populating checklist ids, otherwise they sometimes make it to the client without an id and can't be relied upon.

server/app/playbook_run.go

@@ -683,7 +683,7 @@ type PlaybookRunStore interface {
 	CreatePlaybookRun(playbookRun *PlaybookRun) (*PlaybookRun, error)
 
 	// UpdatePlaybookRun updates a playbook run.
-	UpdatePlaybookRun(playbookRun *PlaybookRun) error
+	UpdatePlaybookRun(playbookRun *PlaybookRun) (*PlaybookRun, error)
 
 	// UpdateStatus updates the status of a playbook run.
 	UpdateStatus(statusPost *SQLStatusPost) error

server/app/playbook_run_service.go

@@ -1091,7 +1091,8 @@ func (s *PlaybookRunServiceImpl) UpdateRunActions(playbookRunID, userID string,
 	playbookRunToModify.WebhookOnStatusUpdateURLs = settings.WebhookOnStatusUpdateURLs
 	playbookRunToModify.StatusUpdateBroadcastWebhooksEnabled = settings.StatusUpdateBroadcastWebhooksEnabled
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1228,7 +1229,9 @@ func (s *PlaybookRunServiceImpl) ChangeOwner(playbookRunID, userID, ownerID stri
 	}
 
 	playbookRunToModify.OwnerUserID = ownerID
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1297,7 +1300,8 @@ func (s *PlaybookRunServiceImpl) ModifyCheckedState(playbookRunID, userID, newSt
 	itemToCheck.StateModified = model.GetMillis()
 	playbookRunToModify.Checklists[checklistNumber].Items[itemNumber] = itemToCheck
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run, is now in inconsistent state")
 	}
 
@@ -1384,7 +1388,8 @@ func (s *PlaybookRunServiceImpl) SetAssignee(playbookRunID, userID, assigneeID s
 	itemToCheck.AssigneeModified = model.GetMillis()
 	playbookRunToModify.Checklists[checklistNumber].Items[itemNumber] = itemToCheck
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run; it is now in an inconsistent state")
 	}
 
@@ -1455,7 +1460,8 @@ func (s *PlaybookRunServiceImpl) SetCommandToChecklistItem(playbookRunID, userID
 
 	playbookRunToModify.Checklists[checklistNumber].Items[itemNumber].Command = newCommand
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1479,7 +1485,8 @@ func (s *PlaybookRunServiceImpl) SetDueDate(playbookRunID, userID string, duedat
 	itemToCheck.DueDate = duedate
 	playbookRunToModify.Checklists[checklistNumber].Items[itemNumber] = itemToCheck
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run; it is now in an inconsistent state")
 	}
 
@@ -1526,7 +1533,9 @@ func (s *PlaybookRunServiceImpl) RunChecklistItemSlashCommand(playbookRunID, use
 
 	// Record the last (successful) run time.
 	playbookRun.Checklists[checklistNumber].Items[itemNumber].CommandLastRun = model.GetMillis()
-	if err = s.store.UpdatePlaybookRun(playbookRun); err != nil {
+
+	playbookRun, err = s.store.UpdatePlaybookRun(playbookRun)
+	if err != nil {
 		return "", errors.Wrapf(err, "failed to update playbook run recording run of slash command")
 	}
 
@@ -1568,7 +1577,8 @@ func (s *PlaybookRunServiceImpl) DuplicateChecklistItem(playbookRunID, userID st
 
 	playbookRunToModify.Checklists[checklistNumber].Items = append(playbookRunToModify.Checklists[checklistNumber].Items, checklistItem)
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1594,7 +1604,9 @@ func (s *PlaybookRunServiceImpl) AddChecklist(playbookRunID, userID string, chec
 	}
 
 	playbookRunToModify.Checklists = append(playbookRunToModify.Checklists, checklist)
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1613,7 +1625,9 @@ func (s *PlaybookRunServiceImpl) DuplicateChecklist(playbookRunID, userID string
 
 	duplicate := playbookRunToModify.Checklists[checklistNumber].Clone()
 	playbookRunToModify.Checklists = append(playbookRunToModify.Checklists, duplicate)
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1633,7 +1647,9 @@ func (s *PlaybookRunServiceImpl) RemoveChecklist(playbookRunID, userID string, c
 	oldChecklist := playbookRunToModify.Checklists[checklistNumber]
 
 	playbookRunToModify.Checklists = append(playbookRunToModify.Checklists[:checklistNumber], playbookRunToModify.Checklists[checklistNumber+1:]...)
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1651,7 +1667,9 @@ func (s *PlaybookRunServiceImpl) RenameChecklist(playbookRunID, userID string, c
 	}
 
 	playbookRunToModify.Checklists[checklistNumber].Title = newTitle
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1670,7 +1688,8 @@ func (s *PlaybookRunServiceImpl) AddChecklistItem(playbookRunID, userID string,
 
 	playbookRunToModify.Checklists[checklistNumber].Items = append(playbookRunToModify.Checklists[checklistNumber].Items, checklistItem)
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1693,7 +1712,8 @@ func (s *PlaybookRunServiceImpl) RemoveChecklistItem(playbookRunID, userID strin
 		playbookRunToModify.Checklists[checklistNumber].Items[itemNumber+1:]...,
 	)
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1717,7 +1737,8 @@ func (s *PlaybookRunServiceImpl) SkipChecklist(playbookRunID, userID string, che
 
 	checklist := playbookRunToModify.Checklists[checklistNumber]
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1740,7 +1761,8 @@ func (s *PlaybookRunServiceImpl) RestoreChecklist(playbookRunID, userID string,
 
 	checklist := playbookRunToModify.Checklists[checklistNumber]
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1762,7 +1784,8 @@ func (s *PlaybookRunServiceImpl) SkipChecklistItem(playbookRunID, userID string,
 
 	checklistItem := playbookRunToModify.Checklists[checklistNumber].Items[itemNumber]
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1783,7 +1806,8 @@ func (s *PlaybookRunServiceImpl) RestoreChecklistItem(playbookRunID, userID stri
 
 	checklistItem := playbookRunToModify.Checklists[checklistNumber].Items[itemNumber]
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1805,7 +1829,8 @@ func (s *PlaybookRunServiceImpl) EditChecklistItem(playbookRunID, userID string,
 
 	checklistItem := playbookRunToModify.Checklists[checklistNumber].Items[itemNumber]
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1837,7 +1862,8 @@ func (s *PlaybookRunServiceImpl) MoveChecklist(playbookRunID, userID string, sou
 	copy(playbookRunToModify.Checklists[destChecklistIdx+1:], playbookRunToModify.Checklists[destChecklistIdx:])
 	playbookRunToModify.Checklists[destChecklistIdx] = checklistMoved
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -1889,7 +1915,8 @@ func (s *PlaybookRunServiceImpl) MoveChecklistItem(playbookRunID, userID string,
 		playbookRunToModify.Checklists[destChecklistIdx].Items = destChecklist
 	}
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run")
 	}
 
@@ -2150,7 +2177,9 @@ func (s *PlaybookRunServiceImpl) UpdateDescription(playbookRunID, description st
 
 	playbookRun.Summary = description
 	playbookRun.SummaryModifiedAt = model.GetMillis()
-	if err = s.store.UpdatePlaybookRun(playbookRun); err != nil {
+
+	playbookRun, err = s.store.UpdatePlaybookRun(playbookRun)
+	if err != nil {
 		return errors.Wrap(err, "failed to update playbook run")
 	}
 
@@ -2526,7 +2555,8 @@ func (s *PlaybookRunServiceImpl) UpdateRetrospective(playbookRunID, updaterID st
 	playbookRunToModify.Retrospective = newRetrospective.Text
 	playbookRunToModify.MetricsData = newRetrospective.Metrics
 
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrap(err, "failed to update playbook run")
 	}
 
@@ -2549,7 +2579,9 @@ func (s *PlaybookRunServiceImpl) PublishRetrospective(playbookRunID, publisherID
 	playbookRunToPublish.MetricsData = retrospective.Metrics
 	playbookRunToPublish.RetrospectivePublishedAt = now
 	playbookRunToPublish.RetrospectiveWasCanceled = false
-	if err = s.store.UpdatePlaybookRun(playbookRunToPublish); err != nil {
+
+	playbookRunToPublish, err = s.store.UpdatePlaybookRun(playbookRunToPublish)
+	if err != nil {
 		return errors.Wrap(err, "failed to update playbook run")
 	}
 
@@ -2641,7 +2673,9 @@ func (s *PlaybookRunServiceImpl) CancelRetrospective(playbookRunID, cancelerID s
 	playbookRunToCancel.Retrospective = "No retrospective for this run."
 	playbookRunToCancel.RetrospectivePublishedAt = now
 	playbookRunToCancel.RetrospectiveWasCanceled = true
-	if err = s.store.UpdatePlaybookRun(playbookRunToCancel); err != nil {
+
+	playbookRunToCancel, err = s.store.UpdatePlaybookRun(playbookRunToCancel)
+	if err != nil {
 		return errors.Wrap(err, "failed to update playbook run")
 	}
 

server/app/reminder.go

@@ -108,7 +108,9 @@ func (s *PlaybookRunServiceImpl) handleStatusUpdateReminder(playbookRunID string
 	}
 
 	playbookRunToModify.ReminderPostID = post.Id
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		s.logger.Errorf(errors.Wrapf(err, "error updating with reminder post id, playbook run id: %s", playbookRunToModify.ID).Error())
 	}
 }
@@ -153,7 +155,9 @@ func (s *PlaybookRunServiceImpl) resetReminderTimer(playbookRunID string) error
 	}
 
 	playbookRunToModify.PreviousReminder = 0
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run after resetting reminder timer")
 	}
 
@@ -184,7 +188,9 @@ func (s *PlaybookRunServiceImpl) SetNewReminder(playbookRunID string, newReminde
 
 	playbookRunToModify.PreviousReminder = newReminder
 	playbookRunToModify.LastStatusUpdateAt = model.GetMillis()
-	if err = s.store.UpdatePlaybookRun(playbookRunToModify); err != nil {
+
+	playbookRunToModify, err = s.store.UpdatePlaybookRun(playbookRunToModify)
+	if err != nil {
 		return errors.Wrapf(err, "failed to update playbook run after resetting reminder timer")
 	}
 

server/sqlstore/playbook_run.go

@@ -412,12 +412,15 @@ func (s *playbookRunStore) CreatePlaybookRun(playbookRun *app.PlaybookRun) (*app
 	if playbookRun == nil {
 		return nil, errors.New("playbook run is nil")
 	}
+
 	playbookRun = playbookRun.Clone()
 
 	if playbookRun.ID == "" {
 		playbookRun.ID = model.NewId()
 	}
 
+	playbookRun.Checklists = populateChecklistIDs(playbookRun.Checklists)
+
 	rawPlaybookRun, err := toSQLPlaybookRun(*playbookRun)
 	if err != nil {
 		return nil, err
@@ -477,21 +480,24 @@ func (s *playbookRunStore) CreatePlaybookRun(playbookRun *app.PlaybookRun) (*app
 }
 
 // UpdatePlaybookRun updates a playbook run.
-func (s *playbookRunStore) UpdatePlaybookRun(playbookRun *app.PlaybookRun) error {
+func (s *playbookRunStore) UpdatePlaybookRun(playbookRun *app.PlaybookRun) (*app.PlaybookRun, error) {
 	if playbookRun == nil {
-		return errors.New("playbook run is nil")
+		return nil, errors.New("playbook run is nil")
 	}
 	if playbookRun.ID == "" {
-		return errors.New("ID should not be empty")
+		return nil, errors.New("ID should not be empty")
 	}
 
+	playbookRun = playbookRun.Clone()
+	playbookRun.Checklists = populateChecklistIDs(playbookRun.Checklists)
+
 	rawPlaybookRun, err := toSQLPlaybookRun(*playbookRun)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	tx, err := s.store.db.Beginx()
 	if err != nil {
-		return errors.Wrap(err, "could not begin transaction")
+		return nil, errors.Wrap(err, "could not begin transaction")
 	}
 	defer s.store.finalizeTransaction(tx)
 
@@ -524,18 +530,18 @@ func (s *playbookRunStore) UpdatePlaybookRun(playbookRun *app.PlaybookRun) error
 		Where(sq.Eq{"ID": rawPlaybookRun.ID}))
 
 	if err != nil {
-		return errors.Wrapf(err, "failed to update playbook run with id '%s'", rawPlaybookRun.ID)
+		return nil, errors.Wrapf(err, "failed to update playbook run with id '%s'", rawPlaybookRun.ID)
 	}
 
 	if err = s.updateRunMetrics(tx, rawPlaybookRun.PlaybookRun); err != nil {
-		return errors.Wrapf(err, "failed to update playbook run metrics for run with id '%s'", rawPlaybookRun.PlaybookRun.ID)
+		return nil, errors.Wrapf(err, "failed to update playbook run metrics for run with id '%s'", rawPlaybookRun.PlaybookRun.ID)
 	}
 
 	if err = tx.Commit(); err != nil {
-		return errors.Wrap(err, "could not commit transaction")
+		return nil, errors.Wrap(err, "could not commit transaction")
 	}
 
-	return nil
+	return playbookRun, nil
 }
 
 func (s *playbookRunStore) UpdateStatus(statusPost *app.SQLStatusPost) error {
@@ -1334,8 +1340,7 @@ func (s *playbookRunStore) updateRunMetrics(q queryExecer, playbookRun app.Playb
 }
 
 func toSQLPlaybookRun(playbookRun app.PlaybookRun) (*sqlPlaybookRun, error) {
-	newChecklists := populateChecklistIDs(playbookRun.Checklists)
-	checklistsJSON, err := checklistsToJSON(newChecklists)
+	checklistsJSON, err := checklistsToJSON(playbookRun.Checklists)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to marshal checklist json for playbook run id '%s'", playbookRun.ID)
 	}