-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(server): request approval should publish the exact version #1291
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -297,3 +297,129 @@ func TestCloseRequest(t *testing.T) { | |
_, itm_closed := getItem(e, iid1) | ||
itm_closed.Path("$.data.node").Object().Value("version").IsEqual(ver1) | ||
} | ||
|
||
func TestRequestFlow(t *testing.T) { | ||
e := StartServer(t, &app.Config{}, true, baseSeederUser) | ||
|
||
// 1- create public project | ||
pId, _ := createProject(e, wId.String(), "test", "test", "test-1") | ||
updateProject(e, pId, "test", "test", "test-1", "PUBLIC", true) | ||
|
||
// 2- create public model | ||
mId, _ := createModel(e, pId, "test", "test", "test-1") | ||
updateModel(e, mId, lo.ToPtr("test"), lo.ToPtr("test"), lo.ToPtr("test-1"), true) | ||
|
||
fid, _ := createField(e, mId, "text", "text", "text", | ||
false, false, false, false, "Text", | ||
map[string]any{ | ||
"text": map[string]any{}, | ||
}) | ||
sId, _, _ := getModel(e, mId) | ||
|
||
// 3- create item with version 1 | ||
iid1, i1 := createItem(e, mId, sId, nil, []map[string]any{ | ||
{"schemaFieldId": fid, "value": "v1", "type": "Text"}, | ||
}) | ||
ver1 := i1.Path("$.data.createItem.item.version").Raw().(string) | ||
|
||
// 4- update item to version 2 | ||
iid1, i1 = updateItem(e, iid1, ver1, []map[string]any{ | ||
{"schemaFieldId": fid, "value": "v2", "type": "Text"}, | ||
}) | ||
ver2 := i1.Path("$.data.updateItem.item.version").Raw().(string) | ||
|
||
// check public item: should return no results | ||
res := e.GET("/api/p/{project}/{model}", "test-1", "test-1"). | ||
Expect(). | ||
Status(http.StatusOK). | ||
JSON() | ||
res.IsEqual(map[string]any{ | ||
"results": []map[string]any{}, | ||
"totalCount": 0, | ||
"hasMore": false, | ||
"limit": 50, | ||
"offset": 0, | ||
"page": 1, | ||
}) | ||
|
||
// 5- create request with version 2 | ||
res = createRequest(e, pId, "test", lo.ToPtr("test"), lo.ToPtr("WAITING"), []string{uId1.String()}, []any{map[string]any{"itemId": iid1, "version": ver2}}) | ||
req := res.Path("$.data.createRequest.request").Object() | ||
rid := req.Value("id").String().Raw() | ||
|
||
// 6- update item to version 3 | ||
iid1, i1 = updateItem(e, iid1, ver2, []map[string]any{ | ||
{"schemaFieldId": fid, "value": "v3", "type": "Text"}, | ||
}) | ||
ver3 := i1.Path("$.data.updateItem.item.version").Raw().(string) | ||
|
||
// 7- approve request | ||
res = approveRequest(e, rid) | ||
req = res.Path("$.data.approveRequest.request").Object() | ||
req.Value("id").IsEqual(rid) | ||
req.Value("state").IsEqual("APPROVED") | ||
|
||
// check item and its status | ||
_, itm := getItem(e, iid1) | ||
itm.Path("$.data.node").Object().Value("version").IsEqual(ver3) | ||
itm.Path("$.data.node").Object().Value("status").IsEqual("PUBLIC_DRAFT") | ||
|
||
// check public item: should return version 2 | ||
res = e.GET("/api/p/{project}/{model}", "test-1", "test-1"). | ||
Expect(). | ||
Status(http.StatusOK). | ||
JSON() | ||
res.IsEqual(map[string]any{ | ||
"results": []map[string]any{ | ||
{ | ||
"id": iid1, | ||
"text": "v2", | ||
}, | ||
}, | ||
"totalCount": 1, | ||
"hasMore": false, | ||
"limit": 50, | ||
"offset": 0, | ||
"page": 1, | ||
}) | ||
|
||
// 8- create request with version 3 | ||
res = createRequest(e, pId, "test", lo.ToPtr("test"), lo.ToPtr("WAITING"), []string{uId1.String()}, []any{map[string]any{"itemId": iid1, "version": ver3}}) | ||
req = res.Path("$.data.createRequest.request").Object() | ||
rid = req.Value("id").String().Raw() | ||
|
||
// check item and its status | ||
_, itm = getItem(e, iid1) | ||
itm.Path("$.data.node").Object().Value("version").IsEqual(ver3) | ||
itm.Path("$.data.node").Object().Value("status").IsEqual("PUBLIC_REVIEW") | ||
|
||
// 9- approve request | ||
res = approveRequest(e, rid) | ||
req = res.Path("$.data.approveRequest.request").Object() | ||
req.Value("id").IsEqual(rid) | ||
req.Value("state").IsEqual("APPROVED") | ||
|
||
// check item and its status | ||
_, itm = getItem(e, iid1) | ||
itm.Path("$.data.node").Object().Value("version").IsEqual(ver3) | ||
itm.Path("$.data.node").Object().Value("status").IsEqual("PUBLIC") | ||
|
||
// check public item: should return version 3 | ||
res = e.GET("/api/p/{project}/{model}", "test-1", "test-1"). | ||
Expect(). | ||
Status(http.StatusOK). | ||
JSON() | ||
res.IsEqual(map[string]any{ | ||
"results": []map[string]any{ | ||
{ | ||
"id": iid1, | ||
"text": "v3", | ||
}, | ||
}, | ||
"totalCount": 1, | ||
"hasMore": false, | ||
"limit": 50, | ||
"offset": 0, | ||
"page": 1, | ||
}) | ||
} | ||
Comment on lines
+396
to
+425
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification 🛠️ Refactor suggestion Based on the gathered information, I can now generate the final response. Add test cases for request approval error scenarios The codebase already has error handling for invalid request states, but test coverage is missing. Add test cases for:
Example: t.Run("approve_non_waiting_request", func(t *testing.T) {
req := createRequest(e, pId, "test", lo.ToPtr("test"), lo.ToPtr("DRAFT"),
[]string{uId1.String()},
[]any{map[string]any{"itemId": iid1, "version": "1.0"}})
res := approveRequest(e, req.Path("$.data.createRequest.request.id").String().Raw())
res.Path("$.errors").Array().NotEmpty()
}) 🔗 Analysis chainAdd error case testing for request approval flow. While the happy path is well tested, consider adding test cases for error scenarios: Consider adding test cases for:
Example: t.Run("approve_invalid_version", func(t *testing.T) {
// Setup request with invalid version
invalidVer := "invalid-version"
res := createRequest(e, pId, "test", lo.ToPtr("test"), lo.ToPtr("WAITING"),
[]string{uId1.String()},
[]any{map[string]any{"itemId": iid1, "version": invalidVer}})
// Verify error response
res.Path("$.errors").Array().NotEmpty()
}) Let me gather more information about the request approval flow to better understand the error cases. Let me search for more specific information about request approval handling. Let me search for request state definitions and error handling in the usecase layer. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Find potential error cases in the codebase
rg -A 3 "errors\.New|fmt\.Errorf" server/internal/usecase/interactor/
Length of output: 7822 Script: #!/bin/bash
# Find request approval related code and error handling
ast-grep --pattern 'func $_ approveRequest($$$) {
$$$
}'
# Also check for request state related code
rg -A 5 "request.*state" server/internal/usecase/interactor/request.go
Length of output: 335 Script: #!/bin/bash
# Search for request approval related code in the repository
rg -A 10 "ApproveRequest|approveRequest" server/
# Look for request state constants or enums
rg -A 5 "type.*State.*=.*string|iota" server/
Length of output: 23433 Script: #!/bin/bash
# Search for request state definitions
rg -A 5 "const.*State|type.*State|iota.*State" server/internal/
# Search for request approval related error handling in usecase
rg -A 5 "func.*Approve|error.*request" server/internal/usecase/
Length of output: 2523 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -991,8 +991,22 @@ func TestWorkFlow(t *testing.T) { | |
s := schema.New().NewID().Workspace(accountdomain.NewWorkspaceID()).Project(prj.ID()).MustBuild() | ||
m := model.New().NewID().Project(prj.ID()).Schema(s.ID()).RandomKey().MustBuild() | ||
i := item.New().NewID().Schema(s.ID()).Model(m.ID()).Project(prj.ID()).Thread(id.NewThreadID()).MustBuild() | ||
ri, _ := request.NewItem(i.ID(), lo.ToPtr(version.New().String())) | ||
u := user.New().Name("aaa").NewID().Email("[email protected]").Workspace(wid).MustBuild() | ||
|
||
ctx := context.Background() | ||
db := memory.New() | ||
err := db.Project.Save(ctx, prj) | ||
assert.NoError(t, err) | ||
err = db.Schema.Save(ctx, s) | ||
assert.NoError(t, err) | ||
err = db.Model.Save(ctx, m) | ||
assert.NoError(t, err) | ||
err = db.Item.Save(ctx, i) | ||
assert.NoError(t, err) | ||
|
||
vi, err := db.Item.FindByID(ctx, i.ID(), nil) | ||
assert.NoError(t, err) | ||
ri, _ := request.NewItem(i.ID(), lo.ToPtr(vi.Version().String())) | ||
Comment on lines
+995
to
+1009
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider refactoring test setup for better maintainability. The test setup could be improved by:
+func setupTestData(t *testing.T, ctx context.Context, db *memory.Container) (*project.Project, *schema.Schema, *model.Model, *item.Item) {
+ prj := project.New().NewID().Workspace(wid).MustBuild()
+ s := schema.New().NewID().Workspace(accountdomain.NewWorkspaceID()).Project(prj.ID()).MustBuild()
+ m := model.New().NewID().Project(prj.ID()).Schema(s.ID()).RandomKey().MustBuild()
+ i := item.New().NewID().Schema(s.ID()).Model(m.ID()).Project(prj.ID()).Thread(id.NewThreadID()).MustBuild()
+
+ assert.NoError(t, db.Project.Save(ctx, prj))
+ assert.NoError(t, db.Schema.Save(ctx, s))
+ assert.NoError(t, db.Model.Save(ctx, m))
+ assert.NoError(t, db.Item.Save(ctx, i))
+
+ return prj, s, m, i
+}
|
||
req1 := request.New(). | ||
NewID(). | ||
Workspace(wid). | ||
|
@@ -1009,17 +1023,6 @@ func TestWorkFlow(t *testing.T) { | |
OwningWorkspaces: id.WorkspaceIDList{wid}, | ||
}, | ||
} | ||
ctx := context.Background() | ||
|
||
db := memory.New() | ||
err := db.Project.Save(ctx, prj) | ||
assert.NoError(t, err) | ||
err = db.Schema.Save(ctx, s) | ||
assert.NoError(t, err) | ||
err = db.Model.Save(ctx, m) | ||
assert.NoError(t, err) | ||
err = db.Item.Save(ctx, i) | ||
assert.NoError(t, err) | ||
|
||
itemUC := NewItem(db, nil) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -353,21 +353,38 @@ func TestRequest_Approve(t *testing.T) { | |
defer util.MockNow(now)() | ||
|
||
// TODO: add error cases | ||
wid := accountdomain.NewWorkspaceID() | ||
prj := project.New().NewID().MustBuild() | ||
s := schema.New().NewID().Workspace(accountdomain.NewWorkspaceID()).Project(prj.ID()).MustBuild() | ||
m := model.New().NewID().Schema(s.ID()).RandomKey().MustBuild() | ||
i := item.New().NewID().Schema(s.ID()).Model(m.ID()).Project(prj.ID()).Thread(id.NewThreadID()).MustBuild() | ||
item, _ := request.NewItem(i.ID(), lo.ToPtr(version.New().String())) | ||
wid := accountdomain.NewWorkspaceID() | ||
u := user.New().Name("aaa").NewID().Email("[email protected]").Workspace(wid).MustBuild() | ||
|
||
ctx := context.Background() | ||
db := memory.New() | ||
// if tc.mockRequestErr { | ||
// memory.SetRequestError(db.Request, tc.wantErr) | ||
// } | ||
err := db.Project.Save(ctx, prj) | ||
assert.NoError(t, err) | ||
err = db.Schema.Save(ctx, s) | ||
assert.NoError(t, err) | ||
err = db.Model.Save(ctx, m) | ||
assert.NoError(t, err) | ||
err = db.Item.Save(ctx, i) | ||
assert.NoError(t, err) | ||
|
||
vi, err := db.Item.FindByID(ctx, i.ID(), nil) | ||
assert.NoError(t, err) | ||
ri, _ := request.NewItem(i.ID(), lo.ToPtr(vi.Version().String())) | ||
req1 := request.New(). | ||
NewID(). | ||
Workspace(wid). | ||
Project(prj.ID()). | ||
Reviewers(accountdomain.UserIDList{u.ID()}). | ||
CreatedBy(accountdomain.NewUserID()). | ||
Thread(id.NewThreadID()). | ||
Items(request.ItemList{item}). | ||
Items(request.ItemList{ri}). | ||
Title("foo"). | ||
MustBuild() | ||
op := &usecase.Operator{ | ||
|
@@ -376,22 +393,9 @@ func TestRequest_Approve(t *testing.T) { | |
OwningWorkspaces: accountdomain.WorkspaceIDList{wid}, | ||
}, | ||
} | ||
ctx := context.Background() | ||
|
||
db := memory.New() | ||
// if tc.mockRequestErr { | ||
// memory.SetRequestError(db.Request, tc.wantErr) | ||
// } | ||
err := db.Project.Save(ctx, prj) | ||
assert.NoError(t, err) | ||
err = db.Request.Save(ctx, req1) | ||
assert.NoError(t, err) | ||
err = db.Schema.Save(ctx, s) | ||
assert.NoError(t, err) | ||
err = db.Model.Save(ctx, m) | ||
assert.NoError(t, err) | ||
err = db.Item.Save(ctx, i) | ||
assert.NoError(t, err) | ||
|
||
requestUC := NewRequest(db, nil) | ||
_, err = requestUC.Approve(ctx, req1.ID(), op) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider using table-driven tests for status transitions.
The status transition testing could be more comprehensive using a table-driven approach to test all possible state transitions: