Fix template websocket (#3747)

* fix websockets with templates * add more unit tests * add synthetic, update tests * Update mattermost-plugin/server/manifest.go * clean up lint, reduce cyclomatic complexity * update tests that call patchBoard with patch.Type set * cleanup messages * cleanup code more readable * fixes from code review * fix null check * remove log lines
2022-09-04 16:51:31 -06:00 · 2022-09-04 16:51:31 -06:00 · af2727085d
commit af2727085d
parent 956675aa07
5 changed files with 361 additions and 34 deletions
--- a/mattermost-plugin/server/manifest.go
+++ b/mattermost-plugin/server/manifest.go
@ -45,7 +45,8 @@ const manifestStr = `
        "type": "bool",
        "help_text": "This allows board editors to share boards that can be accessed by anyone with the link.",
        "placeholder": "",
-        "default": false
+        "default": false,
+        "hosting": ""
      }
    ]
  }
--- a/server/app/boards.go
+++ b/server/app/boards.go
@ -304,14 +304,19 @@ func (a *App) CreateBoard(board *model.Board, userID string, addMember bool) (*m
 }

 func (a *App) PatchBoard(patch *model.BoardPatch, boardID, userID string) (*model.Board, error) {
-	var oldMembers []*model.BoardMember
 	var oldChannelID string
-	if patch.ChannelID != nil && *patch.ChannelID == "" {
-		var err error
-		oldMembers, err = a.GetMembersForBoard(boardID)
-		if err != nil {
-			a.logger.Error("Unable to get the board members", mlog.Err(err))
+	var isTemplate bool
+	var oldMembers []*model.BoardMember
+
+	if patch.Type != nil || patch.ChannelID != nil {
+		if patch.ChannelID != nil && *patch.ChannelID == "" {
+			var err error
+			oldMembers, err = a.GetMembersForBoard(boardID)
+			if err != nil {
+				a.logger.Error("Unable to get the board members", mlog.Err(err))
+			}
 		}
+
 		board, err := a.store.GetBoard(boardID)
 		if model.IsErrNotFound(err) {
 			return nil, model.NewErrNotFound(boardID)
@ -320,14 +325,17 @@ func (a *App) PatchBoard(patch *model.BoardPatch, boardID, userID string) (*mode
 			return nil, err
 		}
 		oldChannelID = board.ChannelID
+		isTemplate = board.IsTemplate
 	}
 	updatedBoard, err := a.store.PatchBoard(boardID, patch, userID)
 	if err != nil {
 		return nil, err
 	}

+	// Post message to channel if linked/unlinked
 	if patch.ChannelID != nil {
 		var username string
+
 		user, err := a.store.GetUserByID(userID)
 		if err != nil {
 			a.logger.Error("Unable to get the board updater", mlog.Err(err))
@ -338,39 +346,42 @@ func (a *App) PatchBoard(patch *model.BoardPatch, boardID, userID string) (*mode

 		boardLink := utils.MakeBoardLink(a.config.ServerRoot, updatedBoard.TeamID, updatedBoard.ID)
 		if *patch.ChannelID != "" {
-			// TODO: this needs translated when available on the server
-			message := fmt.Sprintf(linkBoardMessage, username, updatedBoard.Title, boardLink)
-			err := a.store.PostMessage(message, "", *patch.ChannelID)
-			if err != nil {
-				a.logger.Error("Unable to post the link message to channel", mlog.Err(err))
-			}
+			a.postChannelMessage(fmt.Sprintf(linkBoardMessage, username, updatedBoard.Title, boardLink), updatedBoard.ChannelID)
 		} else if *patch.ChannelID == "" {
-			message := fmt.Sprintf(unlinkBoardMessage, username, updatedBoard.Title, boardLink)
-			err := a.store.PostMessage(message, "", oldChannelID)
-			if err != nil {
-				a.logger.Error("Unable to post the link message to channel", mlog.Err(err))
-			}
+			a.postChannelMessage(fmt.Sprintf(unlinkBoardMessage, username, updatedBoard.Title, boardLink), oldChannelID)
 		}
 	}

+	// Broadcast Messages to affected users
 	a.blockChangeNotifier.Enqueue(func() error {
 		a.wsAdapter.BroadcastBoardChange(updatedBoard.TeamID, updatedBoard)
-		if patch.ChannelID != nil && *patch.ChannelID != "" {
+
+		if patch.ChannelID != nil {
+			if *patch.ChannelID != "" {
+				members, err := a.GetMembersForBoard(updatedBoard.ID)
+				if err != nil {
+					a.logger.Error("Unable to get the board members", mlog.Err(err))
+				}
+				for _, member := range members {
+					if member.Synthetic {
+						a.wsAdapter.BroadcastMemberChange(updatedBoard.TeamID, member.BoardID, member)
+					}
+				}
+			} else {
+				for _, oldMember := range oldMembers {
+					if oldMember.Synthetic {
+						a.wsAdapter.BroadcastMemberDelete(updatedBoard.TeamID, boardID, oldMember.UserID)
+					}
+				}
+			}
+		}
+
+		if patch.Type != nil && isTemplate {
 			members, err := a.GetMembersForBoard(updatedBoard.ID)
 			if err != nil {
 				a.logger.Error("Unable to get the board members", mlog.Err(err))
 			}
-			for _, member := range members {
-				if member.Synthetic {
-					a.wsAdapter.BroadcastMemberChange(updatedBoard.TeamID, member.BoardID, member)
-				}
-			}
-		} else if patch.ChannelID != nil && *patch.ChannelID == "" {
-			for _, oldMember := range oldMembers {
-				if oldMember.Synthetic {
-					a.wsAdapter.BroadcastMemberDelete(updatedBoard.TeamID, boardID, oldMember.UserID)
-				}
-			}
+			a.broadcastTeamUsers(updatedBoard.TeamID, updatedBoard.ID, *patch.Type, members)
 		}
 		return nil
 	})
@ -378,6 +389,38 @@ func (a *App) PatchBoard(patch *model.BoardPatch, boardID, userID string) (*mode
 	return updatedBoard, nil
 }

+func (a *App) postChannelMessage(message, channelID string) {
+	err := a.store.PostMessage(message, "", channelID)
+	if err != nil {
+		a.logger.Error("Unable to post the link message to channel", mlog.Err(err))
+	}
+}
+
+// broadcastTeamUsers notifies the members of a team when a template changes its type
+// from public to private or viceversa.
+func (a *App) broadcastTeamUsers(teamID, boardID string, boardType model.BoardType, members []*model.BoardMember) {
+	users, err := a.GetTeamUsers(teamID, "")
+	if err != nil {
+		a.logger.Error("Unable to get the team users", mlog.Err(err))
+	}
+	for _, user := range users {
+		isMember := false
+		for _, member := range members {
+			if member.UserID == user.ID {
+				isMember = true
+				break
+			}
+		}
+		if !isMember {
+			if boardType == model.BoardTypePrivate {
+				a.wsAdapter.BroadcastMemberDelete(teamID, boardID, user.ID)
+			} else if boardType == model.BoardTypeOpen {
+				a.wsAdapter.BroadcastMemberChange(teamID, boardID, &model.BoardMember{UserID: user.ID, BoardID: boardID, SchemeViewer: true, Synthetic: true})
+			}
+		}
+	}
+}
+
 func (a *App) DeleteBoard(boardID, userID string) error {
 	board, err := a.store.GetBoard(boardID)
 	if model.IsErrNotFound(err) {
@ -541,8 +584,8 @@ func (a *App) DeleteBoardMember(boardID, userID string) error {
 	}

 	a.blockChangeNotifier.Enqueue(func() error {
-		if synteticMember, _ := a.store.GetMemberForBoard(boardID, userID); synteticMember != nil {
-			a.wsAdapter.BroadcastMemberChange(board.TeamID, boardID, synteticMember)
+		if syntheticMember, _ := a.GetMemberForBoard(boardID, userID); syntheticMember != nil {
+			a.wsAdapter.BroadcastMemberChange(board.TeamID, boardID, syntheticMember)
 		} else {
 			a.wsAdapter.BroadcastMemberDelete(board.TeamID, boardID, userID)
 		}
--- a/server/app/boards_test.go
+++ b/server/app/boards_test.go
@ -105,3 +105,262 @@ func TestAddMemberToBoard(t *testing.T) {
 		require.Equal(t, boardID, addedBoardMember.BoardID)
 	})
 }
+
+func TestPatchBoard(t *testing.T) {
+	th, tearDown := SetupTestHelper(t)
+	defer tearDown()
+
+	t.Run("base case, title patch", func(t *testing.T) {
+		const boardID = "board_id_1"
+		const userID = "user_id_1"
+		const teamID = "team_id_1"
+
+		patchTitle := "Patched Title"
+		patch := &model.BoardPatch{
+			Title: &patchTitle,
+		}
+
+		th.Store.EXPECT().PatchBoard(boardID, patch, userID).Return(
+			&model.Board{
+				ID:     boardID,
+				TeamID: teamID,
+				Title:  patchTitle,
+			},
+			nil)
+
+		// for WS BroadcastBoardChange
+		th.Store.EXPECT().GetMembersForBoard(boardID).Return([]*model.BoardMember{}, nil).Times(1)
+
+		patchedBoard, err := th.App.PatchBoard(patch, boardID, userID)
+		require.NoError(t, err)
+		require.Equal(t, patchTitle, patchedBoard.Title)
+	})
+
+	t.Run("patch type open, no users", func(t *testing.T) {
+		const boardID = "board_id_1"
+		const userID = "user_id_2"
+		const teamID = "team_id_1"
+
+		patchType := model.BoardTypeOpen
+		patch := &model.BoardPatch{
+			Type: &patchType,
+		}
+
+		// Type not nil, will cause board to be reteived
+		// to check isTemplate
+		th.Store.EXPECT().GetBoard(boardID).Return(&model.Board{
+			ID:         boardID,
+			TeamID:     teamID,
+			IsTemplate: true,
+		}, nil)
+
+		// Type not null will retrieve team members
+		th.Store.EXPECT().GetUsersByTeam(teamID, "").Return([]*model.User{}, nil)
+
+		th.Store.EXPECT().PatchBoard(boardID, patch, userID).Return(
+			&model.Board{
+				ID:     boardID,
+				TeamID: teamID,
+			},
+			nil)
+
+		// Should call GetMembersForBoard 2 times
+		// - for WS BroadcastBoardChange
+		// - for AddTeamMembers check
+		th.Store.EXPECT().GetMembersForBoard(boardID).Return([]*model.BoardMember{}, nil).Times(2)
+
+		patchedBoard, err := th.App.PatchBoard(patch, boardID, userID)
+		require.NoError(t, err)
+		require.Equal(t, boardID, patchedBoard.ID)
+	})
+
+	t.Run("patch type private, no users", func(t *testing.T) {
+		const boardID = "board_id_1"
+		const userID = "user_id_2"
+		const teamID = "team_id_1"
+
+		patchType := model.BoardTypePrivate
+		patch := &model.BoardPatch{
+			Type: &patchType,
+		}
+
+		// Type not nil, will cause board to be reteived
+		// to check isTemplate
+		th.Store.EXPECT().GetBoard(boardID).Return(&model.Board{
+			ID:         boardID,
+			TeamID:     teamID,
+			IsTemplate: true,
+		}, nil)
+
+		// Type not null will retrieve team members
+		th.Store.EXPECT().GetUsersByTeam(teamID, "").Return([]*model.User{}, nil)
+
+		th.Store.EXPECT().PatchBoard(boardID, patch, userID).Return(
+			&model.Board{
+				ID:     boardID,
+				TeamID: teamID,
+			},
+			nil)
+
+		// Should call GetMembersForBoard 2 times
+		// - for WS BroadcastBoardChange
+		// - for AddTeamMembers check
+		th.Store.EXPECT().GetMembersForBoard(boardID).Return([]*model.BoardMember{}, nil).Times(2)
+
+		patchedBoard, err := th.App.PatchBoard(patch, boardID, userID)
+		require.NoError(t, err)
+		require.Equal(t, boardID, patchedBoard.ID)
+	})
+
+	t.Run("patch type open, single user", func(t *testing.T) {
+		const boardID = "board_id_1"
+		const userID = "user_id_2"
+		const teamID = "team_id_1"
+
+		patchType := model.BoardTypeOpen
+		patch := &model.BoardPatch{
+			Type: &patchType,
+		}
+
+		// Type not nil, will cause board to be reteived
+		// to check isTemplate
+		th.Store.EXPECT().GetBoard(boardID).Return(&model.Board{
+			ID:         boardID,
+			TeamID:     teamID,
+			IsTemplate: true,
+		}, nil)
+		// Type not null will retrieve team members
+		th.Store.EXPECT().GetUsersByTeam(teamID, "").Return([]*model.User{{ID: userID}}, nil)
+
+		th.Store.EXPECT().PatchBoard(boardID, patch, userID).Return(
+			&model.Board{
+				ID:     boardID,
+				TeamID: teamID,
+			},
+			nil)
+
+		// Should call GetMembersForBoard 3 times
+		// for WS BroadcastBoardChange
+		// for AddTeamMembers check
+		// for WS BroadcastMemberChange
+		th.Store.EXPECT().GetMembersForBoard(boardID).Return([]*model.BoardMember{}, nil).Times(3)
+
+		patchedBoard, err := th.App.PatchBoard(patch, boardID, userID)
+		require.NoError(t, err)
+		require.Equal(t, boardID, patchedBoard.ID)
+	})
+
+	t.Run("patch type private, single user", func(t *testing.T) {
+		const boardID = "board_id_1"
+		const userID = "user_id_2"
+		const teamID = "team_id_1"
+
+		patchType := model.BoardTypePrivate
+		patch := &model.BoardPatch{
+			Type: &patchType,
+		}
+
+		// Type not nil, will cause board to be reteived
+		// to check isTemplate
+		th.Store.EXPECT().GetBoard(boardID).Return(&model.Board{
+			ID:         boardID,
+			TeamID:     teamID,
+			IsTemplate: true,
+		}, nil)
+		// Type not null will retrieve team members
+		th.Store.EXPECT().GetUsersByTeam(teamID, "").Return([]*model.User{{ID: userID}}, nil)
+
+		th.Store.EXPECT().PatchBoard(boardID, patch, userID).Return(
+			&model.Board{
+				ID:     boardID,
+				TeamID: teamID,
+			},
+			nil)
+
+		// Should call GetMembersForBoard 3 times
+		// for WS BroadcastBoardChange
+		// for AddTeamMembers check
+		// for WS BroadcastMemberChange
+		th.Store.EXPECT().GetMembersForBoard(boardID).Return([]*model.BoardMember{}, nil).Times(3)
+
+		patchedBoard, err := th.App.PatchBoard(patch, boardID, userID)
+		require.NoError(t, err)
+		require.Equal(t, boardID, patchedBoard.ID)
+	})
+
+	t.Run("patch type open, user with member", func(t *testing.T) {
+		const boardID = "board_id_1"
+		const userID = "user_id_2"
+		const teamID = "team_id_1"
+
+		patchType := model.BoardTypeOpen
+		patch := &model.BoardPatch{
+			Type: &patchType,
+		}
+
+		// Type not nil, will cause board to be reteived
+		// to check isTemplate
+		th.Store.EXPECT().GetBoard(boardID).Return(&model.Board{
+			ID:         boardID,
+			TeamID:     teamID,
+			IsTemplate: true,
+		}, nil)
+		// Type not null will retrieve team members
+		th.Store.EXPECT().GetUsersByTeam(teamID, "").Return([]*model.User{{ID: userID}}, nil)
+
+		th.Store.EXPECT().PatchBoard(boardID, patch, userID).Return(
+			&model.Board{
+				ID:     boardID,
+				TeamID: teamID,
+			},
+			nil)
+
+		// Should call GetMembersForBoard 2 times
+		// for WS BroadcastBoardChange
+		// for AddTeamMembers check
+		// We are returning the user as a direct Board Member, so BroadcastMemberDelete won't be called
+		th.Store.EXPECT().GetMembersForBoard(boardID).Return([]*model.BoardMember{{BoardID: boardID, UserID: userID, SchemeEditor: true}}, nil).Times(2)
+
+		patchedBoard, err := th.App.PatchBoard(patch, boardID, userID)
+		require.NoError(t, err)
+		require.Equal(t, boardID, patchedBoard.ID)
+	})
+
+	t.Run("patch type private, user with member", func(t *testing.T) {
+		const boardID = "board_id_1"
+		const userID = "user_id_2"
+		const teamID = "team_id_1"
+
+		patchType := model.BoardTypePrivate
+		patch := &model.BoardPatch{
+			Type: &patchType,
+		}
+
+		// Type not nil, will cause board to be reteived
+		// to check isTemplate
+		th.Store.EXPECT().GetBoard(boardID).Return(&model.Board{
+			ID:         boardID,
+			TeamID:     teamID,
+			IsTemplate: true,
+		}, nil)
+		// Type not null will retrieve team members
+		th.Store.EXPECT().GetUsersByTeam(teamID, "").Return([]*model.User{{ID: userID}}, nil)
+
+		th.Store.EXPECT().PatchBoard(boardID, patch, userID).Return(
+			&model.Board{
+				ID:     boardID,
+				TeamID: teamID,
+			},
+			nil)
+
+		// Should call GetMembersForBoard 2 times
+		// for WS BroadcastBoardChange
+		// for AddTeamMembers check
+		// We are returning the user as a direct Board Member, so BroadcastMemberDelete won't be called
+		th.Store.EXPECT().GetMembersForBoard(boardID).Return([]*model.BoardMember{{BoardID: boardID, UserID: userID, SchemeEditor: true}}, nil).Times(2)
+
+		patchedBoard, err := th.App.PatchBoard(patch, boardID, userID)
+		require.NoError(t, err)
+		require.Equal(t, boardID, patchedBoard.ID)
+	})
+}
--- a/server/app/onboarding_test.go
+++ b/server/app/onboarding_test.go
@ -28,8 +28,9 @@ func TestPrepareOnboardingTour(t *testing.T) {
 		th.Store.EXPECT().GetTemplateBoards("0", "").Return([]*model.Board{&welcomeBoard}, nil)
 		th.Store.EXPECT().DuplicateBoard(welcomeBoard.ID, userID, teamID, false).Return(&model.BoardsAndBlocks{Boards: []*model.Board{&welcomeBoard}},
 			nil, nil)
-		th.Store.EXPECT().GetMembersForBoard(welcomeBoard.ID).Return([]*model.BoardMember{}, nil).Times(2)
+		th.Store.EXPECT().GetMembersForBoard(welcomeBoard.ID).Return([]*model.BoardMember{}, nil).Times(3)
 		th.Store.EXPECT().GetBoard(welcomeBoard.ID).Return(&welcomeBoard, nil).AnyTimes()
+		th.Store.EXPECT().GetUsersByTeam("0", "").Return([]*model.User{}, nil)

 		privateWelcomeBoard := model.Board{
 			ID:         "board_id_1",
@ -75,8 +76,9 @@ func TestCreateWelcomeBoard(t *testing.T) {
 		th.Store.EXPECT().GetTemplateBoards("0", "").Return([]*model.Board{&welcomeBoard}, nil)
 		th.Store.EXPECT().DuplicateBoard(welcomeBoard.ID, userID, teamID, false).
 			Return(&model.BoardsAndBlocks{Boards: []*model.Board{&welcomeBoard}}, nil, nil)
-		th.Store.EXPECT().GetMembersForBoard(welcomeBoard.ID).Return([]*model.BoardMember{}, nil).Times(2)
+		th.Store.EXPECT().GetMembersForBoard(welcomeBoard.ID).Return([]*model.BoardMember{}, nil).Times(3)
 		th.Store.EXPECT().GetBoard(welcomeBoard.ID).Return(&welcomeBoard, nil).AnyTimes()
+		th.Store.EXPECT().GetUsersByTeam("0", "").Return([]*model.User{}, nil)

 		privateWelcomeBoard := model.Board{
 			ID:         "board_id_1",
--- a/server/services/store/mattermostauthlayer/mattermostauthlayer.go
+++ b/server/services/store/mattermostauthlayer/mattermostauthlayer.go
@ -36,6 +36,7 @@ type servicesAPI interface {
 	GetCloudLimits() (*mmModel.ProductLimits, error)
 	EnsureBot(bot *mmModel.Bot) (string, error)
 	CreatePost(post *mmModel.Post) (*mmModel.Post, error)
+	GetTeamMember(teamID string, userID string) (*mmModel.TeamMember, error)
 	GetPreferencesForUser(userID string) (mmModel.Preferences, error)
 	DeletePreferencesForUser(userID string, preferences mmModel.Preferences) error
 	UpdatePreferencesForUser(userID string, preferences mmModel.Preferences) error
@ -768,6 +769,27 @@ func (s *MattermostAuthLayer) GetMemberForBoard(boardID, userID string) (*model.
 				Synthetic:       true,
 			}, nil
 		}
+		if b.Type == model.BoardTypeOpen && b.IsTemplate {
+			_, memberErr := s.servicesAPI.GetTeamMember(b.TeamID, userID)
+			if memberErr != nil {
+				var appErr *mmModel.AppError
+				if errors.As(memberErr, &appErr) && appErr.StatusCode == http.StatusNotFound {
+					return nil, model.NewErrNotFound(userID)
+				}
+				return nil, memberErr
+			}
+
+			return &model.BoardMember{
+				BoardID:         boardID,
+				UserID:          userID,
+				Roles:           "viewer",
+				SchemeAdmin:     false,
+				SchemeEditor:    false,
+				SchemeCommenter: false,
+				SchemeViewer:    true,
+				Synthetic:       true,
+			}, nil
+		}
 	}
 	if err != nil {
 		return nil, err