From c1c94e9794fb27bbd45c9e340880d9d9f9c07601 Mon Sep 17 00:00:00 2001 From: Doug Lauder Date: Fri, 3 Feb 2023 05:54:14 -0500 Subject: [PATCH] Fix notifications for deleted card content blocks (#4508) * comment only * add getBlockHistoryNewestChildren store method * fixed delete comment notify * fix notification for content block deletion * fix linter errors * address review comments --------- Co-authored-by: Mattermost Build --- .../server/boards/notifications.go | 4 +- server/model/block.go | 8 ++ server/model/blocktype.go | 27 ++-- .../notify/notifysubscriptions/app_api.go | 2 +- .../notify/notifysubscriptions/diff.go | 4 +- .../diff2slackattachments.go | 85 ++++++++---- .../notify/notifysubscriptions/notifier.go | 2 +- .../subscriptions_backend.go | 12 ++ server/services/store/mockstore/mockstore.go | 16 +++ server/services/store/sqlstore/blocks.go | 122 +++++++++++++++--- server/services/store/sqlstore/board.go | 54 ++++---- .../services/store/sqlstore/public_methods.go | 5 + server/services/store/store.go | 1 + server/services/store/storetests/blocks.go | 103 ++++++++++++++- server/utils/utils.go | 19 +-- 15 files changed, 363 insertions(+), 101 deletions(-) diff --git a/mattermost-plugin/server/boards/notifications.go b/mattermost-plugin/server/boards/notifications.go index 3df9c6f07..5a860c966 100644 --- a/mattermost-plugin/server/boards/notifications.go +++ b/mattermost-plugin/server/boards/notifications.go @@ -95,8 +95,8 @@ func (a *appAPI) GetBlockHistory(blockID string, opts model.QueryBlockHistoryOpt return a.store.GetBlockHistory(blockID, opts) } -func (a *appAPI) GetSubTree2(boardID, blockID string, opts model.QuerySubtreeOptions) ([]*model.Block, error) { - return a.store.GetSubTree2(boardID, blockID, opts) +func (a *appAPI) GetBlockHistoryNewestChildren(parentID string, opts model.QueryBlockHistoryChildOptions) ([]*model.Block, bool, error) { + return a.store.GetBlockHistoryNewestChildren(parentID, opts) } func (a *appAPI) GetBoardAndCardByID(blockID string) (board *model.Board, card *model.Block, err error) { diff --git a/server/model/block.go b/server/model/block.go index 341e9b67a..9e1658ec6 100644 --- a/server/model/block.go +++ b/server/model/block.go @@ -199,6 +199,14 @@ type QueryBoardHistoryOptions struct { Descending bool // if true then the records are sorted by insert_at in descending order } +// QueryBlockHistoryOptions are query options that can be passed to GetBlockHistory. +type QueryBlockHistoryChildOptions struct { + BeforeUpdateAt int64 // if non-zero then filter for records with update_at less than BeforeUpdateAt + AfterUpdateAt int64 // if non-zero then filter for records with update_at greater than AfterUpdateAt + Page int // page number to select when paginating + PerPage int // number of blocks per page (default=-1, meaning unlimited) +} + func StampModificationMetadata(userID string, blocks []*Block, auditRec *audit.Record) { if userID == SingleUser { userID = "" diff --git a/server/model/blocktype.go b/server/model/blocktype.go index 6c6c35444..0c42349a9 100644 --- a/server/model/blocktype.go +++ b/server/model/blocktype.go @@ -14,13 +14,16 @@ import ( type BlockType string const ( - TypeUnknown = "unknown" - TypeBoard = "board" - TypeCard = "card" - TypeView = "view" - TypeText = "text" - TypeComment = "comment" - TypeImage = "image" + TypeUnknown = "unknown" + TypeBoard = "board" + TypeCard = "card" + TypeView = "view" + TypeText = "text" + TypeCheckbox = "checkbox" + TypeComment = "comment" + TypeImage = "image" + TypeAttachment = "attachment" + TypeDivider = "divider" ) func (bt BlockType) String() string { @@ -38,10 +41,16 @@ func BlockTypeFromString(s string) (BlockType, error) { return TypeView, nil case "text": return TypeText, nil + case "checkbox": + return TypeCheckbox, nil case "comment": return TypeComment, nil case "image": return TypeImage, nil + case "attachment": + return TypeAttachment, nil + case "divider": + return TypeDivider, nil } return TypeUnknown, ErrInvalidBlockType{s} } @@ -55,8 +64,10 @@ func BlockType2IDType(blockType BlockType) utils.IDType { return utils.IDTypeCard case TypeView: return utils.IDTypeView - case TypeText, TypeComment: + case TypeText, TypeCheckbox, TypeComment, TypeDivider: return utils.IDTypeBlock + case TypeImage, TypeAttachment: + return utils.IDTypeAttachment } return utils.IDTypeNone } diff --git a/server/services/notify/notifysubscriptions/app_api.go b/server/services/notify/notifysubscriptions/app_api.go index a822e9c3d..9f2a25b50 100644 --- a/server/services/notify/notifysubscriptions/app_api.go +++ b/server/services/notify/notifysubscriptions/app_api.go @@ -11,7 +11,7 @@ import ( type AppAPI interface { GetBlockHistory(blockID string, opts model.QueryBlockHistoryOptions) ([]*model.Block, error) - GetSubTree2(boardID, blockID string, opts model.QuerySubtreeOptions) ([]*model.Block, error) + GetBlockHistoryNewestChildren(parentID string, opts model.QueryBlockHistoryChildOptions) ([]*model.Block, bool, error) GetBoardAndCardByID(blockID string) (board *model.Board, card *model.Block, err error) GetUserByID(userID string) (*model.User, error) diff --git a/server/services/notify/notifysubscriptions/diff.go b/server/services/notify/notifysubscriptions/diff.go index ecabe23e5..e96b98525 100644 --- a/server/services/notify/notifysubscriptions/diff.go +++ b/server/services/notify/notifysubscriptions/diff.go @@ -149,10 +149,10 @@ func (dg *diffGenerator) generateDiffsForCard(card *model.Block, schema model.Pr } // fetch all card content blocks that were updated after last notify - opts := model.QuerySubtreeOptions{ + opts := model.QueryBlockHistoryChildOptions{ AfterUpdateAt: dg.lastNotifyAt, } - blocks, err := dg.store.GetSubTree2(card.BoardID, card.ID, opts) + blocks, _, err := dg.store.GetBlockHistoryNewestChildren(card.ID, opts) if err != nil { return nil, fmt.Errorf("could not get subtree for card %s: %w", card.ID, err) } diff --git a/server/services/notify/notifysubscriptions/diff2slackattachments.go b/server/services/notify/notifysubscriptions/diff2slackattachments.go index 2afd23d09..31c66768b 100644 --- a/server/services/notify/notifysubscriptions/diff2slackattachments.go +++ b/server/services/notify/notifysubscriptions/diff2slackattachments.go @@ -246,7 +246,7 @@ func appendCommentChanges(fields []*mm_model.SlackAttachmentField, cardDiff *Dif msg = child.NewBlock.Title } - if child.NewBlock == nil && child.OldBlock != nil { + if (child.NewBlock == nil || child.NewBlock.DeleteAt != 0) && child.OldBlock != nil { // deleted comment format = "~~`%s`~~" msg = stripNewlines(child.OldBlock.Title) @@ -266,36 +266,73 @@ func appendCommentChanges(fields []*mm_model.SlackAttachmentField, cardDiff *Dif func appendContentChanges(fields []*mm_model.SlackAttachmentField, cardDiff *Diff, logger mlog.LoggerIFace) []*mm_model.SlackAttachmentField { for _, child := range cardDiff.Diffs { - if child.BlockType != model.TypeComment { - var newTitle, oldTitle string - if child.OldBlock != nil { - oldTitle = child.OldBlock.Title - } - if child.NewBlock != nil { - newTitle = child.NewBlock.Title - } + var opAdd, opDelete bool + var opString string - // only strip newlines when modifying or deleting - if child.OldBlock != nil && child.NewBlock == nil { - newTitle = stripNewlines(newTitle) + switch { + case child.OldBlock == nil && child.NewBlock != nil: + opAdd = true + opString = "added" // TODO: localize when i18n added to server + case child.NewBlock == nil || child.NewBlock.DeleteAt != 0: + opDelete = true + opString = "deleted" + default: + opString = "modified" + } + + var newTitle, oldTitle string + if child.OldBlock != nil { + oldTitle = child.OldBlock.Title + } + if child.NewBlock != nil { + newTitle = child.NewBlock.Title + } + + switch child.BlockType { + case model.TypeDivider, model.TypeComment: + // do nothing + continue + case model.TypeImage: + if newTitle == "" { + newTitle = "An image was " + opString + "." // TODO: localize when i18n added to server + } + oldTitle = "" + case model.TypeAttachment: + if newTitle == "" { + newTitle = "A file attachment was " + opString + "." // TODO: localize when i18n added to server + } + oldTitle = "" + default: + if !opAdd { + if opDelete { + newTitle = "" + } + // only strip newlines when modifying or deleting oldTitle = stripNewlines(oldTitle) + newTitle = stripNewlines(newTitle) } - if newTitle == oldTitle { continue } - - markdown := generateMarkdownDiff(oldTitle, newTitle, logger) - if markdown == "" { - continue - } - - fields = append(fields, &mm_model.SlackAttachmentField{ - Short: false, - Title: "Description", - Value: markdown, - }) } + + logger.Debug("appendContentChanges", + mlog.String("type", string(child.BlockType)), + mlog.String("opString", opString), + mlog.String("oldTitle", oldTitle), + mlog.String("newTitle", newTitle), + ) + + markdown := generateMarkdownDiff(oldTitle, newTitle, logger) + if markdown == "" { + continue + } + + fields = append(fields, &mm_model.SlackAttachmentField{ + Short: false, + Title: "Description", + Value: markdown, + }) } return fields } diff --git a/server/services/notify/notifysubscriptions/notifier.go b/server/services/notify/notifysubscriptions/notifier.go index 6b0d26761..a8deae7f5 100644 --- a/server/services/notify/notifysubscriptions/notifier.go +++ b/server/services/notify/notifysubscriptions/notifier.go @@ -200,7 +200,7 @@ func (n *notifier) notifySubscribers(hint *model.NotificationHint) error { } opts := DiffConvOpts{ - Language: "en", // TODO: use correct language with i18n available on server. + Language: "en", // TODO: use correct language when i18n is available on server. MakeCardLink: func(block *model.Block, board *model.Board, card *model.Block) string { return fmt.Sprintf("[%s](%s)", block.Title, utils.MakeCardLink(n.serverRoot, board.TeamID, board.ID, card.ID)) }, diff --git a/server/services/notify/notifysubscriptions/subscriptions_backend.go b/server/services/notify/notifysubscriptions/subscriptions_backend.go index baf66e728..1323c5c3c 100644 --- a/server/services/notify/notifysubscriptions/subscriptions_backend.go +++ b/server/services/notify/notifysubscriptions/subscriptions_backend.go @@ -5,6 +5,8 @@ package notifysubscriptions import ( "fmt" + "os" + "strconv" "time" "github.com/mattermost/focalboard/server/model" @@ -73,6 +75,16 @@ func (b *Backend) Name() string { } func (b *Backend) getBlockUpdateFreq(blockType model.BlockType) time.Duration { + // check for env variable override + sFreq := os.Getenv("MM_BOARDS_NOTIFY_FREQ_SECONDS") + if sFreq != "" && sFreq != "0" { + if freq, err := strconv.ParseInt(sFreq, 10, 64); err != nil { + b.logger.Error("Environment variable MM_BOARDS_NOTIFY_FREQ_SECONDS invalid (ignoring)", mlog.Err(err)) + } else { + return time.Second * time.Duration(freq) + } + } + switch blockType { case model.TypeCard: return time.Second * time.Duration(b.notifyFreqCardSeconds) diff --git a/server/services/store/mockstore/mockstore.go b/server/services/store/mockstore/mockstore.go index 75f3aa6dc..d21cc4bff 100644 --- a/server/services/store/mockstore/mockstore.go +++ b/server/services/store/mockstore/mockstore.go @@ -457,6 +457,22 @@ func (mr *MockStoreMockRecorder) GetBlockHistoryDescendants(arg0, arg1 interface return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetBlockHistoryDescendants", reflect.TypeOf((*MockStore)(nil).GetBlockHistoryDescendants), arg0, arg1) } +// GetBlockHistoryNewestChildren mocks base method. +func (m *MockStore) GetBlockHistoryNewestChildren(arg0 string, arg1 model.QueryBlockHistoryChildOptions) ([]*model.Block, bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetBlockHistoryNewestChildren", arg0, arg1) + ret0, _ := ret[0].([]*model.Block) + ret1, _ := ret[1].(bool) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// GetBlockHistoryNewestChildren indicates an expected call of GetBlockHistoryNewestChildren. +func (mr *MockStoreMockRecorder) GetBlockHistoryNewestChildren(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetBlockHistoryNewestChildren", reflect.TypeOf((*MockStore)(nil).GetBlockHistoryNewestChildren), arg0, arg1) +} + // GetBlocks mocks base method. func (m *MockStore) GetBlocks(arg0 model.QueryBlocksOptions) ([]*model.Block, error) { m.ctrl.T.Helper() diff --git a/server/services/store/sqlstore/blocks.go b/server/services/store/sqlstore/blocks.go index 361128eca..69b650af6 100644 --- a/server/services/store/sqlstore/blocks.go +++ b/server/services/store/sqlstore/blocks.go @@ -4,6 +4,7 @@ import ( "database/sql" "encoding/json" "fmt" + "strings" "github.com/mattermost/focalboard/server/utils" @@ -42,27 +43,31 @@ func (s *SQLStore) timestampToCharField(name string, as string) string { } } -func (s *SQLStore) blockFields() []string { +func (s *SQLStore) blockFields(tableAlias string) []string { + if tableAlias != "" && !strings.HasSuffix(tableAlias, ".") { + tableAlias += "." + } + return []string{ - "id", - "parent_id", - "created_by", - "modified_by", - s.escapeField("schema"), - "type", - "title", - "COALESCE(fields, '{}')", - s.timestampToCharField("insert_at", "insertAt"), - "create_at", - "update_at", - "delete_at", - "COALESCE(board_id, '0')", + tableAlias + "id", + tableAlias + "parent_id", + tableAlias + "created_by", + tableAlias + "modified_by", + tableAlias + s.escapeField("schema"), + tableAlias + "type", + tableAlias + "title", + "COALESCE(" + tableAlias + "fields, '{}')", + s.timestampToCharField(tableAlias+"insert_at", "insertAt"), + tableAlias + "create_at", + tableAlias + "update_at", + tableAlias + "delete_at", + "COALESCE(" + tableAlias + "board_id, '0')", } } func (s *SQLStore) getBlocks(db sq.BaseRunner, opts model.QueryBlocksOptions) ([]*model.Block, error) { query := s.getQueryBuilder(db). - Select(s.blockFields()...). + Select(s.blockFields("")...). From(s.tablePrefix + "blocks") if opts.BoardID != "" { @@ -115,7 +120,7 @@ func (s *SQLStore) getBlocksWithParent(db sq.BaseRunner, boardID, parentID strin func (s *SQLStore) getBlocksByIDs(db sq.BaseRunner, ids []string) ([]*model.Block, error) { query := s.getQueryBuilder(db). - Select(s.blockFields()...). + Select(s.blockFields("")...). From(s.tablePrefix + "blocks"). Where(sq.Eq{"id": ids}) @@ -150,7 +155,7 @@ func (s *SQLStore) getBlocksWithType(db sq.BaseRunner, boardID, blockType string // getSubTree2 returns blocks within 2 levels of the given blockID. func (s *SQLStore) getSubTree2(db sq.BaseRunner, boardID string, blockID string, opts model.QuerySubtreeOptions) ([]*model.Block, error) { query := s.getQueryBuilder(db). - Select(s.blockFields()...). + Select(s.blockFields("")...). From(s.tablePrefix + "blocks"). Where(sq.Or{sq.Eq{"id": blockID}, sq.Eq{"parent_id": blockID}}). Where(sq.Eq{"board_id": boardID}). @@ -550,7 +555,7 @@ func (s *SQLStore) getBoardCount(db sq.BaseRunner) (int64, error) { func (s *SQLStore) getBlock(db sq.BaseRunner, blockID string) (*model.Block, error) { query := s.getQueryBuilder(db). - Select(s.blockFields()...). + Select(s.blockFields("")...). From(s.tablePrefix + "blocks"). Where(sq.Eq{"id": blockID}) @@ -580,7 +585,7 @@ func (s *SQLStore) getBlockHistory(db sq.BaseRunner, blockID string, opts model. } query := s.getQueryBuilder(db). - Select(s.blockFields()...). + Select(s.blockFields("")...). From(s.tablePrefix + "blocks_history"). Where(sq.Eq{"id": blockID}). OrderBy("insert_at " + order + ", update_at" + order) @@ -614,7 +619,7 @@ func (s *SQLStore) getBlockHistoryDescendants(db sq.BaseRunner, boardID string, } query := s.getQueryBuilder(db). - Select(s.blockFields()...). + Select(s.blockFields("")...). From(s.tablePrefix + "blocks_history"). Where(sq.Eq{"board_id": boardID}). OrderBy("insert_at " + order + ", update_at" + order) @@ -641,6 +646,83 @@ func (s *SQLStore) getBlockHistoryDescendants(db sq.BaseRunner, boardID string, return s.blocksFromRows(rows) } +// getBlockHistoryNewestChildren returns the newest (latest) version child blocks for the +// specified parent from the blocks_history table. This includes any deleted children. +func (s *SQLStore) getBlockHistoryNewestChildren(db sq.BaseRunner, parentID string, opts model.QueryBlockHistoryChildOptions) ([]*model.Block, bool, error) { + // as we're joining 2 queries, we need to avoid numbered + // placeholders until the join is done, so we use the default + // question mark placeholder here + builder := s.getQueryBuilder(db).PlaceholderFormat(sq.Question) + + sub := builder. + Select("bh2.id", "MAX(bh2.insert_at) AS max_insert_at"). + From(s.tablePrefix + "blocks_history AS bh2"). + Where(sq.Eq{"bh2.parent_id": parentID}). + GroupBy("bh2.id") + + if opts.AfterUpdateAt != 0 { + sub = sub.Where(sq.Gt{"bh2.update_at": opts.AfterUpdateAt}) + } + + if opts.BeforeUpdateAt != 0 { + sub = sub.Where(sq.Lt{"bh2.update_at": opts.BeforeUpdateAt}) + } + + subQuery, subArgs, err := sub.ToSql() + if err != nil { + return nil, false, fmt.Errorf("getBlockHistoryNewestChildren unable to generate subquery: %w", err) + } + + query := s.getQueryBuilder(db). + Select(s.blockFields("bh")...). + From(s.tablePrefix+"blocks_history AS bh"). + InnerJoin("("+subQuery+") AS sub ON bh.id=sub.id AND bh.insert_at=sub.max_insert_at", subArgs...) + + if opts.Page != 0 { + query = query.Offset(uint64(opts.Page * opts.PerPage)) + } + + if opts.PerPage > 0 { + // limit+1 to detect if more records available + query = query.Limit(uint64(opts.PerPage + 1)) + } + + sql, args, err := query.ToSql() + if err != nil { + return nil, false, fmt.Errorf("getBlockHistoryNewestChildren unable to generate sql: %w", err) + } + + // if we're using postgres or sqlite, we need to replace the + // question mark placeholder with the numbered dollar one, now + // that the full query is built + if s.dbType == model.PostgresDBType || s.dbType == model.SqliteDBType { + var rErr error + sql, rErr = sq.Dollar.ReplacePlaceholders(sql) + if rErr != nil { + return nil, false, fmt.Errorf("getBlockHistoryNewestChildren unable to replace sql placeholders: %w", rErr) + } + } + + rows, err := db.Query(sql, args...) + if err != nil { + s.logger.Error(`getBlockHistoryNewestChildren ERROR`, mlog.Err(err)) + return nil, false, err + } + defer s.CloseRows(rows) + + blocks, err := s.blocksFromRows(rows) + if err != nil { + return nil, false, err + } + + hasMore := false + if opts.PerPage > 0 && len(blocks) > opts.PerPage { + blocks = blocks[:opts.PerPage] + hasMore = true + } + return blocks, hasMore, nil +} + // getBoardAndCardByID returns the first parent of type `card` and first parent of type `board` for the block specified by ID. // `board` and/or `card` may return nil without error if the block does not belong to a board or card. func (s *SQLStore) getBoardAndCardByID(db sq.BaseRunner, blockID string) (board *model.Board, card *model.Block, err error) { diff --git a/server/services/store/sqlstore/board.go b/server/services/store/sqlstore/board.go index c81ceb411..48faa76e2 100644 --- a/server/services/store/sqlstore/board.go +++ b/server/services/store/sqlstore/board.go @@ -17,41 +17,31 @@ import ( "github.com/mattermost/mattermost-server/v6/shared/mlog" ) -func boardFields(prefix string) []string { - fields := []string{ - "id", - "team_id", - "COALESCE(channel_id, '')", - "COALESCE(created_by, '')", - "modified_by", - "type", - "minimum_role", - "title", - "description", - "icon", - "show_description", - "is_template", - "template_version", - "COALESCE(properties, '{}')", - "COALESCE(card_properties, '[]')", - "create_at", - "update_at", - "delete_at", +func boardFields(tableAlias string) []string { + if tableAlias != "" && !strings.HasSuffix(tableAlias, ".") { + tableAlias += "." } - if prefix == "" { - return fields + return []string{ + tableAlias + "id", + tableAlias + "team_id", + "COALESCE(" + tableAlias + "channel_id, '')", + "COALESCE(" + tableAlias + "created_by, '')", + tableAlias + "modified_by", + tableAlias + "type", + tableAlias + "minimum_role", + tableAlias + "title", + tableAlias + "description", + tableAlias + "icon", + tableAlias + "show_description", + tableAlias + "is_template", + tableAlias + "template_version", + "COALESCE(" + tableAlias + "properties, '{}')", + "COALESCE(" + tableAlias + "card_properties, '[]')", + tableAlias + "create_at", + tableAlias + "update_at", + tableAlias + "delete_at", } - - prefixedFields := make([]string, len(fields)) - for i, field := range fields { - if strings.HasPrefix(field, "COALESCE(") { - prefixedFields[i] = strings.Replace(field, "COALESCE(", "COALESCE("+prefix, 1) - } else { - prefixedFields[i] = prefix + field - } - } - return prefixedFields } func boardHistoryFields() []string { diff --git a/server/services/store/sqlstore/public_methods.go b/server/services/store/sqlstore/public_methods.go index 885df152f..58a129b54 100644 --- a/server/services/store/sqlstore/public_methods.go +++ b/server/services/store/sqlstore/public_methods.go @@ -328,6 +328,11 @@ func (s *SQLStore) GetBlockHistoryDescendants(boardID string, opts model.QueryBl } +func (s *SQLStore) GetBlockHistoryNewestChildren(parentID string, opts model.QueryBlockHistoryChildOptions) ([]*model.Block, bool, error) { + return s.getBlockHistoryNewestChildren(s.db, parentID, opts) + +} + func (s *SQLStore) GetBlocks(opts model.QueryBlocksOptions) ([]*model.Block, error) { return s.getBlocks(s.db, opts) diff --git a/server/services/store/store.go b/server/services/store/store.go index 7edd3e19c..7cacc8298 100644 --- a/server/services/store/store.go +++ b/server/services/store/store.go @@ -38,6 +38,7 @@ type Store interface { PatchBlock(blockID string, blockPatch *model.BlockPatch, userID string) error GetBlockHistory(blockID string, opts model.QueryBlockHistoryOptions) ([]*model.Block, error) GetBlockHistoryDescendants(boardID string, opts model.QueryBlockHistoryOptions) ([]*model.Block, error) + GetBlockHistoryNewestChildren(parentID string, opts model.QueryBlockHistoryChildOptions) ([]*model.Block, bool, error) GetBoardHistory(boardID string, opts model.QueryBoardHistoryOptions) ([]*model.Board, error) GetBoardAndCardByID(blockID string) (board *model.Board, card *model.Block, err error) GetBoardAndCard(block *model.Block) (board *model.Board, card *model.Block, err error) diff --git a/server/services/store/storetests/blocks.go b/server/services/store/storetests/blocks.go index 6da1ffbee..bbd01ef72 100644 --- a/server/services/store/storetests/blocks.go +++ b/server/services/store/storetests/blocks.go @@ -1,6 +1,8 @@ package storetests import ( + "math" + "strconv" "testing" "time" @@ -79,6 +81,11 @@ func StoreTestBlocksStore(t *testing.T, setup func(t *testing.T) (store.Store, f defer tearDown() testUndeleteBlockChildren(t, store) }) + t.Run("GetBlockHistoryNewestChildren", func(t *testing.T) { + store, tearDown := setup(t) + defer tearDown() + testGetBlockHistoryNewestChildren(t, store) + }) } func testInsertBlock(t *testing.T, store store.Store) { @@ -1069,14 +1076,15 @@ func testUndeleteBlockChildren(t *testing.T, store store.Store) { boards := createTestBoards(t, store, testTeamID, testUserID, 2) boardDelete := boards[0] boardKeep := boards[1] + userID := testUserID // create some blocks to be deleted - cardsDelete := createTestCards(t, store, testUserID, boardDelete.ID, 3) + cardsDelete := createTestCards(t, store, userID, boardDelete.ID, 3) blocksDelete := createTestBlocksForCard(t, store, cardsDelete[0].ID, 5) require.Len(t, blocksDelete, 5) // create some blocks to keep - cardsKeep := createTestCards(t, store, testUserID, boardKeep.ID, 3) + cardsKeep := createTestCards(t, store, userID, boardKeep.ID, 3) blocksKeep := createTestBlocksForCard(t, store, cardsKeep[0].ID, 4) require.Len(t, blocksKeep, 4) @@ -1153,3 +1161,94 @@ func testUndeleteBlockChildren(t *testing.T, store store.Store) { assert.Len(t, blocks, len(blocksDelete)+len(cardsDelete)) }) } + +func testGetBlockHistoryNewestChildren(t *testing.T, store store.Store) { + boards := createTestBoards(t, store, testTeamID, testUserID, 2) + board := boards[0] + + const cardCount = 10 + const patchCount = 5 + + // create a card and some content blocks + cards := createTestCards(t, store, testUserID, board.ID, 1) + card := cards[0] + content := createTestBlocksForCard(t, store, card.ID, cardCount) + + // patch the content blocks to create some history records + for i := 1; i <= patchCount; i++ { + for _, block := range content { + title := strconv.FormatInt(int64(i), 10) + patch := &model.BlockPatch{ + Title: &title, + } + err := store.PatchBlock(block.ID, patch, testUserID) + require.NoError(t, err, "error patching content blocks") + } + } + + // delete some of the content blocks + err := store.DeleteBlock(content[0].ID, testUserID) + require.NoError(t, err, "error deleting content block") + err = store.DeleteBlock(content[3].ID, testUserID) + require.NoError(t, err, "error deleting content block") + err = store.DeleteBlock(content[7].ID, testUserID) + require.NoError(t, err, "error deleting content block") + + t.Run("invalid card", func(t *testing.T) { + opts := model.QueryBlockHistoryChildOptions{} + blocks, hasMore, err := store.GetBlockHistoryNewestChildren(utils.NewID(utils.IDTypeCard), opts) + require.NoError(t, err) + require.False(t, hasMore) + require.Empty(t, blocks) + }) + + t.Run("valid card with no children", func(t *testing.T) { + opts := model.QueryBlockHistoryChildOptions{} + emptyCard := createTestCards(t, store, testUserID, board.ID, 1)[0] + blocks, hasMore, err := store.GetBlockHistoryNewestChildren(emptyCard.ID, opts) + require.NoError(t, err) + require.False(t, hasMore) + require.Empty(t, blocks) + }) + + t.Run("valid card with children", func(t *testing.T) { + opts := model.QueryBlockHistoryChildOptions{} + blocks, hasMore, err := store.GetBlockHistoryNewestChildren(card.ID, opts) + require.NoError(t, err) + require.False(t, hasMore) + require.Len(t, blocks, cardCount) + require.ElementsMatch(t, extractIDs(t, blocks), extractIDs(t, content)) + + expected := strconv.FormatInt(patchCount, 10) + for _, b := range blocks { + require.Equal(t, expected, b.Title) + } + }) + + t.Run("pagination", func(t *testing.T) { + opts := model.QueryBlockHistoryChildOptions{ + PerPage: 3, + } + + collected := make([]*model.Block, 0) + reps := 0 + for { + reps++ + blocks, hasMore, err := store.GetBlockHistoryNewestChildren(card.ID, opts) + require.NoError(t, err) + collected = append(collected, blocks...) + if !hasMore { + break + } + opts.Page++ + } + + assert.Len(t, collected, cardCount) + assert.Equal(t, math.Floor(float64(cardCount/opts.PerPage)+1), float64(reps)) + + expected := strconv.FormatInt(patchCount, 10) + for _, b := range collected { + require.Equal(t, expected, b.Title) + } + }) +} diff --git a/server/utils/utils.go b/server/utils/utils.go index 4a93e1a67..7a1ad9763 100644 --- a/server/utils/utils.go +++ b/server/utils/utils.go @@ -11,15 +11,16 @@ import ( type IDType byte const ( - IDTypeNone IDType = '7' - IDTypeTeam IDType = 't' - IDTypeBoard IDType = 'b' - IDTypeCard IDType = 'c' - IDTypeView IDType = 'v' - IDTypeSession IDType = 's' - IDTypeUser IDType = 'u' - IDTypeToken IDType = 'k' - IDTypeBlock IDType = 'a' + IDTypeNone IDType = '7' + IDTypeTeam IDType = 't' + IDTypeBoard IDType = 'b' + IDTypeCard IDType = 'c' + IDTypeView IDType = 'v' + IDTypeSession IDType = 's' + IDTypeUser IDType = 'u' + IDTypeToken IDType = 'k' + IDTypeBlock IDType = 'a' + IDTypeAttachment IDType = 'i' ) // NewId is a globally unique identifier. It is a [A-Z0-9] string 27