diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go index 203f455122..d393833832 100644 --- a/backend/app/rest/api/rest_private_test.go +++ b/backend/app/rest/api/rest_private_test.go @@ -373,6 +373,109 @@ func TestRest_UpdateDelete(t *testing.T) { {URL: "https://radio-t.com/blah2", Count: 0}}, j) } +func TestRest_DeleteChildThenParent(t *testing.T) { + ts, _, teardown := startupT(t) + defer teardown() + + p := store.Comment{Text: "test test #1", + Locator: store.Locator{SiteID: "remark42", URL: "https://radio-t.com/blah1"}} + idP := addComment(t, p, ts) + + c1 := store.Comment{Text: "test test #1", ParentID: idP, + Locator: store.Locator{SiteID: "remark42", URL: "https://radio-t.com/blah1"}} + idC1 := addComment(t, c1, ts) + + c2 := store.Comment{Text: "test test #1", ParentID: idP, + Locator: store.Locator{SiteID: "remark42", URL: "https://radio-t.com/blah1"}} + idC2 := addComment(t, c2, ts) + + // check multi count equals two + resp, err := post(t, ts.URL+"/api/v1/counts?site=remark42", `["https://radio-t.com/blah1"]`) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + bb, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.NoError(t, resp.Body.Close()) + j := []store.PostInfo{} + err = json.Unmarshal(bb, &j) + assert.NoError(t, err) + assert.Equal(t, []store.PostInfo{{URL: "https://radio-t.com/blah1", Count: 3}}, j) + + // update a parent comment fails after child is created + client := http.Client{} + defer client.CloseIdleConnections() + req, err := http.NewRequest(http.MethodPut, ts.URL+"/api/v1/comment/"+idP+"?site=remark42&url=https://radio-t.com/blah1", + strings.NewReader(`{"text": "updated text", "summary":"updated by user"}`)) + require.NoError(t, err) + req.Header.Add("X-JWT", devToken) + b, err := client.Do(req) + require.NoError(t, err) + body, err := io.ReadAll(b.Body) + require.NoError(t, err) + assert.Equal(t, http.StatusBadRequest, b.StatusCode, string(body)) + assert.NoError(t, b.Body.Close()) + + // delete first child comment + req, err = http.NewRequest(http.MethodPut, ts.URL+"/api/v1/comment/"+idC1+"?site=remark42&url=https://radio-t.com/blah1", + strings.NewReader(`{"delete": true, "summary":"removed by user"}`)) + require.NoError(t, err) + req.Header.Add("X-JWT", devToken) + b, err = client.Do(req) + require.NoError(t, err) + body, err = io.ReadAll(b.Body) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, b.StatusCode, string(body)) + assert.NoError(t, b.Body.Close()) + + // delete a parent comment, fails as one comment child still present + defer client.CloseIdleConnections() + req, err = http.NewRequest(http.MethodPut, ts.URL+"/api/v1/comment/"+idP+"?site=remark42&url=https://radio-t.com/blah1", + strings.NewReader(`{"delete": true, "summary":"removed by user"}`)) + require.NoError(t, err) + req.Header.Add("X-JWT", devToken) + b, err = client.Do(req) + require.NoError(t, err) + body, err = io.ReadAll(b.Body) + require.NoError(t, err) + assert.Equal(t, http.StatusBadRequest, b.StatusCode, string(body)) + assert.NoError(t, b.Body.Close()) + + // delete second child comment, as an admin to check both deletion methods + req, err = http.NewRequest(http.MethodDelete, + fmt.Sprintf("%s/api/v1/admin/comment/%s?site=remark42&url=https://radio-t.com/blah1", ts.URL, idC2), http.NoBody) + require.NoError(t, err) + requireAdminOnly(t, req) + resp, err = sendReq(t, req, adminUmputunToken) + assert.NoError(t, err) + assert.NoError(t, resp.Body.Close()) + assert.Equal(t, http.StatusOK, resp.StatusCode) + + // delete a parent comment, shouldn't fail after children are deleted + defer client.CloseIdleConnections() + req, err = http.NewRequest(http.MethodPut, ts.URL+"/api/v1/comment/"+idP+"?site=remark42&url=https://radio-t.com/blah1", + strings.NewReader(`{"delete": true, "summary":"removed by user"}`)) + require.NoError(t, err) + req.Header.Add("X-JWT", devToken) + b, err = client.Do(req) + require.NoError(t, err) + body, err = io.ReadAll(b.Body) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, b.StatusCode, string(body)) + assert.NoError(t, b.Body.Close()) + + // check multi count decremented to zero + resp, err = post(t, ts.URL+"/api/v1/counts?site=remark42", `["https://radio-t.com/blah1"]`) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + bb, err = io.ReadAll(resp.Body) + require.NoError(t, err) + assert.NoError(t, resp.Body.Close()) + j = []store.PostInfo{} + err = json.Unmarshal(bb, &j) + assert.NoError(t, err) + assert.Equal(t, []store.PostInfo{{URL: "https://radio-t.com/blah1", Count: 0}}, j) +} + func TestRest_UpdateNotOwner(t *testing.T) { ts, srv, teardown := startupT(t) defer teardown() @@ -396,8 +499,6 @@ func TestRest_UpdateNotOwner(t *testing.T) { assert.Equal(t, http.StatusForbidden, b.StatusCode, string(body), "update from non-owner") assert.Equal(t, `{"code":3,"details":"can not edit comments for other users","error":"rejected"}`+"\n", string(body)) - client = http.Client{} - defer client.CloseIdleConnections() req, err = http.NewRequest(http.MethodPut, ts.URL+"/api/v1/comment/"+id1+ "?site=remark42&url=https://radio-t.com/blah1", strings.NewReader(`ERRR "text":"updated text", "summary":"my"}`)) assert.NoError(t, err) diff --git a/backend/app/store/service/service.go b/backend/app/store/service/service.go index b601cfa9b2..8427d4cc99 100644 --- a/backend/app/store/service/service.go +++ b/backend/app/store/service/service.go @@ -496,6 +496,12 @@ func (s *DataStore) EditComment(locator store.Locator, commentID string, req Edi if e := s.AdminStore.OnEvent(comment.Locator.SiteID, admin.EvDelete); e != nil { log.Printf("[WARN] failed to send delete event, %s", e) } + // clean up the comment and it's parent from cache, so that + // after cleaning up the child, parent won't be stuck non-deletable till cache expires + if s.repliesCache.LoadingCache != nil { + s.repliesCache.Delete(commentID) + s.repliesCache.Delete(comment.ParentID) + } comment.Deleted = true delReq := engine.DeleteRequest{Locator: locator, CommentID: commentID, DeleteMode: store.SoftDelete} return comment, s.Engine.Delete(delReq) @@ -739,6 +745,17 @@ func (s *DataStore) Delete(locator store.Locator, commentID string, mode store.D if e := s.AdminStore.OnEvent(locator.SiteID, admin.EvDelete); e != nil { log.Printf("[WARN] failed to send delete event, %s", e) } + // get comment to learn it's parent ID + comment, err := s.Engine.Get(engine.GetRequest{Locator: locator, CommentID: commentID}) + if err != nil { + return err + } + // clean up the comment and it's parent from cache, so that + // after cleaning up the child, parent won't be stuck non-deletable till cache expires + if s.repliesCache.LoadingCache != nil { + s.repliesCache.Delete(commentID) + s.repliesCache.Delete(comment.ParentID) + } req := engine.DeleteRequest{Locator: locator, CommentID: commentID, DeleteMode: mode} return s.Engine.Delete(req) } diff --git a/backend/app/store/service/service_test.go b/backend/app/store/service/service_test.go index 33e9ffe328..80f57d2cab 100644 --- a/backend/app/store/service/service_test.go +++ b/backend/app/store/service/service_test.go @@ -971,7 +971,7 @@ func TestService_HasReplies(t *testing.T) { // two comments for https://radio-t.com, no reply eng, teardown := prepStoreEngine(t) defer teardown() - b := DataStore{Engine: eng, EditDuration: 100 * time.Millisecond, + b := DataStore{Engine: eng, AdminStore: admin.NewStaticStore("secret 123", []string{"radio-t"}, []string{"user2"}, "user@email.com")} defer b.Close() @@ -982,11 +982,10 @@ func TestService_HasReplies(t *testing.T) { Locator: store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, User: store.User{ID: "user1", Name: "user name"}, } - assert.False(t, b.HasReplies(comment)) reply := store.Comment{ - ID: "123456", + ID: "c-1", ParentID: "id-1", Text: "some text", Timestamp: time.Date(2017, 12, 20, 15, 18, 22, 0, time.Local), @@ -995,7 +994,45 @@ func TestService_HasReplies(t *testing.T) { } _, err := b.Create(reply) assert.NoError(t, err) + _, found := b.repliesCache.Peek(comment.ID) + assert.False(t, found, "not yet checked") assert.True(t, b.HasReplies(comment)) + _, found = b.repliesCache.Peek(reply.ParentID) + assert.True(t, found, "checked and has replies") + + // deletion of the parent comment shouldn't work as the comment has replies + _, err = b.EditComment(reply.Locator, comment.ID, EditRequest{Orig: comment.ID, Delete: true, Summary: "user deletes the comment"}) + assert.EqualError(t, err, "parent comment with reply can't be edited, "+comment.ID) + _, found = b.repliesCache.Peek(reply.ParentID) + assert.True(t, found, "checked and has replies") + + // should not report replies after deletion of the child + err = b.Delete(reply.Locator, reply.ID, store.HardDelete) + assert.NoError(t, err) + _, found = b.repliesCache.Peek(reply.ParentID) + assert.False(t, found, "cleaned up from cache by Delete call") + assert.False(t, b.HasReplies(comment)) + _, found = b.repliesCache.Peek(reply.ParentID) + assert.False(t, found, "checked and has no replies") + + // recreate reply with the new ID + reply.ID = "c-2" + _, err = b.Create(reply) + assert.NoError(t, err) + _, found = b.repliesCache.Peek(reply.ParentID) + assert.False(t, found, "not yet checked") + assert.True(t, b.HasReplies(comment)) + _, found = b.repliesCache.Peek(reply.ParentID) + assert.True(t, found, "checked and has replies again") + + // should not report replies after deletion of the child using Edit mechanism + _, err = b.EditComment(reply.Locator, reply.ID, EditRequest{Orig: reply.ID, Delete: true, Summary: "user deletes the comment"}) + assert.NoError(t, err) + _, found = b.repliesCache.Peek(reply.ParentID) + assert.False(t, found, "cleaned up from cache by EditComment call") + assert.False(t, b.HasReplies(comment)) + _, found = b.repliesCache.Peek(reply.ParentID) + assert.False(t, found, "checked and has no replies") } func TestService_UserReplies(t *testing.T) {