Skip to content

Commit 0082177

Browse files
Rajat-DabadeRajat-Dabade
committed
coderabbit review comments fixed
1 parent 255c728 commit 0082177

File tree

13 files changed

+154
-87
lines changed

13 files changed

+154
-87
lines changed

‎server/api/users.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ func (a *API) handleGetUser(w http.ResponseWriter, r *http.Request) {
269269
return
270270
}
271271

272-
auditRec := a.makeAuditRecord(r, "postBlocks", audit.Fail)
272+
auditRec := a.makeAuditRecord(r, "getUser", audit.Fail)
273273
defer a.audit.LogRecord(audit.LevelRead, auditRec)
274274
auditRec.AddMeta("userID", userID)
275275

‎server/app/import.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,8 +112,6 @@ func (a *App) ImportArchive(r io.Reader, opt model.ImportArchiveOptions) error {
112112

113113
// Update image and attachment blocks.
114114
func (a *App) fixImagesAttachments(boardMap map[string]*model.Board, fileMap map[string]string, teamID string, userID string) {
115-
blockIDs := make([]string, 0)
116-
blockPatches := make([]model.BlockPatch, 0)
117115
for _, board := range boardMap {
118116
if board.IsTemplate {
119117
continue
@@ -128,6 +126,8 @@ func (a *App) fixImagesAttachments(boardMap map[string]*model.Board, fileMap map
128126
return
129127
}
130128

129+
blockIDs := make([]string, 0)
130+
blockPatches := make([]model.BlockPatch, 0)
131131
for _, block := range newBlocks {
132132
if block.Type == "image" || block.Type == "attachment" {
133133
fieldName := "fileId"

‎server/integrationtests/board_test.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2125,6 +2125,8 @@ func TestGetTemplates(t *testing.T) {
21252125
require.NotNil(t, rBoards)
21262126
require.GreaterOrEqual(t, len(rBoards), 6)
21272127

2128+
var skippedTitles []string
2129+
21282130
t.Log("\n\n")
21292131
for _, board := range rBoards {
21302132
t.Logf("Test get template: %s - %s\n", board.Title, board.ID)
@@ -2141,7 +2143,8 @@ func TestGetTemplates(t *testing.T) {
21412143

21422144
rBoardsAndBlock, resp := th.Client.DuplicateBoard(board.ID, false, teamID)
21432145
if resp.StatusCode == 400 {
2144-
t.Logf("Skipping duplicate test for template %s due to file info limitations", board.Title)
2146+
t.Logf("Skipping duplicate test for template %q due to file info limitations (known issue)", board.Title)
2147+
skippedTitles = append(skippedTitles, board.Title)
21452148
continue
21462149
}
21472150
th.CheckOK(resp)
@@ -2164,6 +2167,10 @@ func TestGetTemplates(t *testing.T) {
21642167
require.NotNil(t, rBlocks2)
21652168
require.Equal(t, len(rBoardsAndBlock.Blocks), len(rBlocks2))
21662169
}
2170+
2171+
if len(skippedTitles) > 0 {
2172+
t.Skipf("skipped %d template(s) due to file info limitations: %v", len(skippedTitles), skippedTitles)
2173+
}
21672174
t.Log("\n\n")
21682175
})
21692176
}

‎server/integrationtests/file_test.go‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ package integrationtests
55

66
import (
77
"bytes"
8-
"strings"
98
"testing"
109

1110
"github.com/mattermost/mattermost-plugin-boards/server/client"
@@ -119,13 +118,7 @@ func TestFileInfo(t *testing.T) {
119118
th.CheckOK(resp)
120119
require.NotNil(t, fileInfo)
121120
require.NotNil(t, fileInfo.Id)
122-
fileIDWithoutPrefix := file.FileID
123-
if len(fileIDWithoutPrefix) > 0 && fileIDWithoutPrefix[0] == '7' {
124-
fileIDWithoutPrefix = fileIDWithoutPrefix[1:]
125-
}
126-
if idx := strings.LastIndex(fileIDWithoutPrefix, "."); idx != -1 {
127-
fileIDWithoutPrefix = fileIDWithoutPrefix[:idx]
128-
}
121+
fileIDWithoutPrefix := utils.RetrieveFileIDFromBlockFieldStorage(file.FileID)
129122
require.Equal(t, fileIDWithoutPrefix, fileInfo.Id)
130123
})
131124
}

‎server/model/board.go‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"io"
99
"time"
1010

11-
"github.com/mattermost/mattermost-plugin-boards/server/utils"
1211
mmModel "github.com/mattermost/mattermost/server/public/model"
1312
)
1413

@@ -390,13 +389,11 @@ func (ibe InvalidBoardErr) Error() string {
390389
}
391390

392391
func (b *Board) IsValid() error {
393-
// Only allow GlobalTeamID ("0") in test environments for security
394-
// In production, GlobalTeamID should only be used for templates
392+
// GlobalTeamID ("0") is only valid for template boards.
395393
if b.TeamID == GlobalTeamID {
396-
if !utils.IsRunningUnitTests() {
394+
if !b.IsTemplate {
397395
return InvalidBoardErr{"invalid-team-id"}
398396
}
399-
// Allow in test environments
400397
} else if !mmModel.IsValidId(b.TeamID) {
401398
return InvalidBoardErr{"invalid-team-id"}
402399
}

‎server/model/board_test.go‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
package model
55

66
import (
7-
"os"
87
"testing"
98

109
"github.com/mattermost/mattermost/server/public/model"
@@ -28,28 +27,41 @@ func TestBoardIsValid(t *testing.T) {
2827
require.NoError(t, err)
2928
})
3029

31-
t.Run("Should allow global team ID for valid board in test scenarios", func(t *testing.T) {
32-
// Set test environment variable to allow GlobalTeamID
33-
origEnv := os.Getenv("FOCALBOARD_UNIT_TESTING")
34-
os.Setenv("FOCALBOARD_UNIT_TESTING", "1")
35-
defer os.Setenv("FOCALBOARD_UNIT_TESTING", origEnv)
36-
30+
t.Run("Should allow global team ID for template boards", func(t *testing.T) {
3731
board := &Board{
3832
ID: model.NewId(),
3933
TeamID: GlobalTeamID,
34+
IsTemplate: true,
4035
CreatedBy: model.NewId(),
4136
ModifiedBy: model.NewId(),
4237
Type: BoardTypeOpen,
4338
MinimumRole: BoardRoleViewer,
44-
Title: "Valid Board",
39+
Title: "Valid Template",
4540
CreateAt: 1234567890,
4641
UpdateAt: 1234567890,
4742
}
4843
err := board.IsValid()
49-
// GlobalTeamID ("0") is now allowed in test scenarios for integration tests
5044
require.NoError(t, err)
5145
})
5246

47+
t.Run("Should return error for global team ID on non-template board", func(t *testing.T) {
48+
board := &Board{
49+
ID: model.NewId(),
50+
TeamID: GlobalTeamID,
51+
IsTemplate: false,
52+
CreatedBy: model.NewId(),
53+
ModifiedBy: model.NewId(),
54+
Type: BoardTypeOpen,
55+
MinimumRole: BoardRoleViewer,
56+
Title: "Non-template Board",
57+
CreateAt: 1234567890,
58+
UpdateAt: 1234567890,
59+
}
60+
err := board.IsValid()
61+
require.Error(t, err)
62+
require.EqualError(t, err, "invalid-team-id")
63+
})
64+
5365
t.Run("Should return error for invalid team ID", func(t *testing.T) {
5466
board := &Board{
5567
ID: model.NewId(),

‎server/services/store/sqlstore/cloud.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ func (s *SQLStore) activeCardsQuery(builder sq.StatementBuilderType, selectStr s
2626
Join(s.tablePrefix + "boards bd on b.board_id=bd.id").
2727
Where(sq.Eq{
2828
"b.type": model.TypeCard,
29+
"b.delete_at": 0,
2930
"bd.is_template": false,
3031
})
3132
if !includeDeleted {

‎server/services/store/sqlstore/helpers_test.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,11 @@ func (t *testServicesAPIForUnitTests) GetLicense() *mmModel.License {
231231
func (t *testServicesAPIForUnitTests) GetFileInfo(fileID string) (*mmModel.FileInfo, error) {
232232
// Query the FileInfo table (Mattermost's table) to retrieve saved file info
233233
// This matches what the real Mattermost servicesAPI would do
234-
query := `SELECT Id, CreateAt, UpdateAt, DeleteAt, Path, ThumbnailPath, PreviewPath, Name, Extension, Size, MimeType, Width, Height, HasPreviewImage, MiniPreview, Content, RemoteId, CreatorId, PostId FROM FileInfo WHERE Id = $1`
234+
placeholder := "$1"
235+
if t.dbType == model.MysqlDBType {
236+
placeholder = "?"
237+
}
238+
query := `SELECT Id, CreateAt, UpdateAt, DeleteAt, Path, ThumbnailPath, PreviewPath, Name, Extension, Size, MimeType, Width, Height, HasPreviewImage, MiniPreview, Content, RemoteId, CreatorId, PostId FROM FileInfo WHERE Id = ` + placeholder
235239

236240
var fileInfo mmModel.FileInfo
237241
err := t.db.QueryRow(query, fileID).Scan(

‎server/services/store/sqlstore/mattermost_tables.go‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,8 @@ func setupTeamMembersTableForIntegration(db *sql.DB, dbType string) error {
169169
roles character varying(64),
170170
deleteat bigint,
171171
schemeuser boolean,
172-
schemeadmin boolean
172+
schemeadmin boolean,
173+
PRIMARY KEY (teamid, userid)
173174
);
174175
`
175176
case model.MysqlDBType:
@@ -180,7 +181,8 @@ func setupTeamMembersTableForIntegration(db *sql.DB, dbType string) error {
180181
Roles VARCHAR(64),
181182
DeleteAt BIGINT,
182183
SchemeUser BOOLEAN,
183-
SchemeAdmin BOOLEAN
184+
SchemeAdmin BOOLEAN,
185+
PRIMARY KEY (TeamId, UserId)
184186
) DEFAULT CHARACTER SET utf8mb4;
185187
`
186188
default:
@@ -382,7 +384,7 @@ func setupFileInfoTableForIntegration(db *sql.DB, dbType string) error {
382384
switch dbType {
383385
case model.PostgresDBType:
384386
createTableSQL = `
385-
CREATE TABLE IF NOT EXISTS FileInfo (
387+
CREATE TABLE IF NOT EXISTS fileinfo (
386388
Id VARCHAR(26) NOT NULL,
387389
CreatorId VARCHAR(26),
388390
PostId VARCHAR(26),
@@ -445,17 +447,17 @@ func setupSessionsTableForIntegration(db *sql.DB, dbType string) error {
445447
switch dbType {
446448
case model.PostgresDBType:
447449
createTableSQL = `
448-
CREATE TABLE IF NOT EXISTS Sessions (
449-
Id character varying(26) NOT NULL PRIMARY KEY,
450-
Token character varying(64),
451-
CreateAt bigint,
452-
ExpiresAt bigint,
453-
LastActivityAt bigint,
454-
UserId character varying(26),
455-
Roles character varying(64),
456-
IsOAuth boolean,
457-
Props text,
458-
DeviceId character varying(512)
450+
CREATE TABLE IF NOT EXISTS sessions (
451+
id character varying(26) NOT NULL PRIMARY KEY,
452+
token character varying(64),
453+
createat bigint,
454+
expiresat bigint,
455+
lastactivityat bigint,
456+
userid character varying(26),
457+
roles character varying(64),
458+
isoauth boolean,
459+
props text,
460+
deviceid character varying(512)
459461
);
460462
`
461463
case model.MysqlDBType:

‎server/services/store/sqlstore/support_for_test.go‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,8 @@ func setupTeamMembersTable(t *testing.T, db *sql.DB, dbType string) {
146146
roles character varying(64),
147147
deleteat bigint,
148148
schemeuser boolean,
149-
schemeadmin boolean
149+
schemeadmin boolean,
150+
PRIMARY KEY (teamid, userid)
150151
);
151152
`
152153
case model.MysqlDBType:
@@ -157,7 +158,8 @@ func setupTeamMembersTable(t *testing.T, db *sql.DB, dbType string) {
157158
Roles VARCHAR(64),
158159
DeleteAt BIGINT,
159160
SchemeUser BOOLEAN,
160-
SchemeAdmin BOOLEAN
161+
SchemeAdmin BOOLEAN,
162+
PRIMARY KEY (TeamId, UserId)
161163
) DEFAULT CHARACTER SET utf8mb4;
162164
`
163165
default:
@@ -369,7 +371,7 @@ func setupFileInfoTable(t *testing.T, db *sql.DB, dbType string) {
369371
switch dbType {
370372
case model.PostgresDBType:
371373
createTableSQL = `
372-
CREATE TABLE IF NOT EXISTS FileInfo (
374+
CREATE TABLE IF NOT EXISTS fileinfo (
373375
Id VARCHAR(26) NOT NULL,
374376
CreatorId VARCHAR(26),
375377
PostId VARCHAR(26),
@@ -435,17 +437,17 @@ func setupSessionsTable(t *testing.T, db *sql.DB, dbType string) {
435437
switch dbType {
436438
case model.PostgresDBType:
437439
createTableSQL = `
438-
CREATE TABLE IF NOT EXISTS Sessions (
439-
Id character varying(26) NOT NULL PRIMARY KEY,
440-
Token character varying(64),
441-
CreateAt bigint,
442-
ExpiresAt bigint,
443-
LastActivityAt bigint,
444-
UserId character varying(26),
445-
Roles character varying(64),
446-
IsOAuth boolean,
447-
Props text,
448-
DeviceId character varying(512)
440+
CREATE TABLE IF NOT EXISTS sessions (
441+
id character varying(26) NOT NULL PRIMARY KEY,
442+
token character varying(64),
443+
createat bigint,
444+
expiresat bigint,
445+
lastactivityat bigint,
446+
userid character varying(26),
447+
roles character varying(64),
448+
isoauth boolean,
449+
props text,
450+
deviceid character varying(512)
449451
);
450452
`
451453
case model.MysqlDBType:

0 commit comments

Comments
 (0)