Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(server): request approval should publish the exact version #1291

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+218 −32
Open

fix(server): request approval should publish the exact version #1291

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
83c86ac
fix(server): request approval should publish the exact version
yk-eukarya Oct 29, 2024
f5e18af
fix test
yk-eukarya Oct 29, 2024
c6b1600
fix test
yk-eukarya Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions server/e2e/gql_item_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ func updateItem(e *httpexpect.Expect, iID, version string, fields []map[string]a
item {
id
schemaId
version
fields {
value
type
Expand Down
6 changes: 3 additions & 3 deletions server/e2e/gql_model_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func createModel(e *httpexpect.Expect, pID, name, desc, key string) (string, *ht
return res.Path("$.data.createModel.model.id").Raw().(string), res
}

func updateModel(e *httpexpect.Expect, mId string, name, desc, key *string) *httpexpect.Value {
func updateModel(e *httpexpect.Expect, mId string, name, desc, key *string, public bool) *httpexpect.Value {
requestBody := GraphQLRequest{
Query: `mutation UpdateModel($modelId: ID!, $name: String, $description: String, $key: String, $public: Boolean!) {
updateModel(input: {modelId: $modelId, name: $name, description: $description, key: $key, public: $public}) {
Expand All @@ -64,7 +64,7 @@ func updateModel(e *httpexpect.Expect, mId string, name, desc, key *string) *htt
"name": name,
"description": desc,
"key": key,
"public": false,
"public": public,
},
}

Expand Down Expand Up @@ -336,7 +336,7 @@ func TestUpdateModel(t *testing.T) {
pId, _ := createProject(e, wId.String(), "test", "test", "test-2")

mId, _ := createModel(e, pId, "test", "test", "test-2")
res := updateModel(e, mId, lo.ToPtr("updated name"), lo.ToPtr("updated desc"), lo.ToPtr("updated_key"))
res := updateModel(e, mId, lo.ToPtr("updated name"), lo.ToPtr("updated desc"), lo.ToPtr("updated_key"), false)
res.Object().
Value("data").Object().
Value("updateModel").Object().
Expand Down
41 changes: 41 additions & 0 deletions server/e2e/gql_porject_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,47 @@ func createProject(e *httpexpect.Expect, wID, name, desc, alias string) (string,
return res.Path("$.data.createProject.project.id").Raw().(string), res
}

func updateProject(e *httpexpect.Expect, pID, name, desc, alias, publicationScope string, publicAssets bool) (string, *httpexpect.Value) {
requestBody := GraphQLRequest{
Query: `mutation UpdateProject($projectId: ID!, $name: String!, $description: String!, $alias: String!, $publicationScope: ProjectPublicationScope!, $publicAssets: Boolean!) {
updateProject(input: {projectId: $projectId, name: $name, description: $description, alias: $alias, publication: {scope: $publicationScope, assetPublic: $publicAssets}}) {
project {
id
name
description
alias
publication {
scope
assetPublic
__typename
}
__typename
}
__typename
}
}`,
Variables: map[string]any{
"projectId": pID,
"name": name,
"description": desc,
"alias": alias,
"publicationScope": publicationScope,
"publicAssets": publicAssets,
},
}

res := e.POST("/api/graphql").
WithHeader("Origin", "https://example.com").
WithHeader("X-Reearth-Debug-User", uId1.String()).
WithHeader("Content-Type", "application/json").
WithJSON(requestBody).
Expect().
Status(http.StatusOK).
JSON()

return res.Path("$.data.updateProject.project.id").Raw().(string), res
}

func TestCreateProject(t *testing.T) {
e := StartServer(t, &app.Config{}, true, baseSeederUser)

Expand Down
126 changes: 126 additions & 0 deletions server/e2e/gql_request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Comment on lines +356 to +395
Copy link

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:

// Add before the test function
var statusTransitionTests = []struct {
    initialStatus string
    action        string
    expectedStatus string
}{
    {"DRAFT", "approve", "PUBLIC_DRAFT"},
    {"PUBLIC_DRAFT", "request", "PUBLIC_REVIEW"},
    {"PUBLIC_REVIEW", "approve", "PUBLIC"},
}

// Then use in test:
for _, tt := range statusTransitionTests {
    t.Run(fmt.Sprintf("%s->%s", tt.initialStatus, tt.expectedStatus), func(t *testing.T) {
        // Test status transition
    })
}

// 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
Copy link

Choose a reason for hiding this comment

The 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:

  • Approving a request with "DRAFT" or "CLOSED" state (only "WAITING" state can be approved)
  • Approving a request when the operator is not a user
  • Approving a request that fails during save operation

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 chain

Add 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:

  1. Approving an already approved request
  2. Approving a request with an invalid version
  3. Approving a request for a non-existent item
  4. Concurrent request approvals for the same item

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 executed

The 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

6 changes: 6 additions & 0 deletions server/internal/usecase/interactor/item.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,12 @@ func (i Item) Update(ctx context.Context, param interfaces.UpdateItemParam, oper
return nil, err
}

// re-fetch item so the new version is returned
itm, err = i.repos.Item.FindByID(ctx, param.ItemID, nil)
if err != nil {
return nil, err
}

if err = i.handleReferenceFields(ctx, *s, itm.Value(), oldFields); err != nil {
return nil, err
}
Expand Down
27 changes: 15 additions & 12 deletions server/internal/usecase/interactor/item_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The 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:

  1. Extracting the database setup into a helper function to reduce boilerplate.
  2. Using table-driven tests to cover different workflow scenarios.
+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
+}

Committable suggestion was skipped due to low confidence.

req1 := request.New().
NewID().
Workspace(wid).
Expand All @@ -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)

Expand Down
7 changes: 6 additions & 1 deletion server/internal/usecase/interactor/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,12 @@ func (r Request) Approve(ctx context.Context, requestID id.RequestID, operator *
// apply changes to items (publish items)
for _, itm := range req.Items() {
// publish the approved version
if err := r.repos.Item.UpdateRef(ctx, itm.Item(), version.Public, version.Latest.OrVersion().Ref()); err != nil {
dist := itm.Pointer().Ref()
// this should not happen, used for backward compatibility (will set the latest version as published)
if dist == nil {
dist = version.Latest.OrVersion().Ref()
}
if err := r.repos.Item.UpdateRef(ctx, itm.Item(), version.Public, dist); err != nil {
return nil, err
}
}
Expand Down
Loading