From 07ab1f3acdd2e7fb11c12676132efeb7a0a57eeb Mon Sep 17 00:00:00 2001 From: Umputun Date: Thu, 28 Jun 2018 19:51:47 -0500 Subject: [PATCH] feature/subtree (#116) * hide deleted subtree from response #114 * hide deleted sub comments with no reply #114 * lint: tree json err check --- backend/app/rest/testdata/tree.json | 252 +++++++++++++ .../tree_bench.json} | 0 backend/app/rest/testdata/tree_del.json | 186 ++++++++++ backend/app/rest/tree.go | 9 +- backend/app/rest/tree_test.go | 333 +++--------------- 5 files changed, 498 insertions(+), 282 deletions(-) create mode 100644 backend/app/rest/testdata/tree.json rename backend/app/rest/{testfile.json => testdata/tree_bench.json} (100%) create mode 100644 backend/app/rest/testdata/tree_del.json diff --git a/backend/app/rest/testdata/tree.json b/backend/app/rest/testdata/tree.json new file mode 100644 index 00000000..b06eb1d2 --- /dev/null +++ b/backend/app/rest/testdata/tree.json @@ -0,0 +1,252 @@ + { + "comments": [{ + "comment": { + "id": "1", + "pid": "", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:46:01Z" + }, + "replies": [{ + "comment": { + "id": "11", + "pid": "1", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:46:11Z" + } + }, + { + "comment": { + "id": "12", + "pid": "1", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:46:12Z" + } + }, + { + "comment": { + "id": "13", + "pid": "1", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:46:13Z" + }, + "replies": [{ + "comment": { + "id": "131", + "pid": "13", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:46:31Z" + } + }, + { + "comment": { + "id": "132", + "pid": "13", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:46:32Z" + } + } + ] + }, + { + "comment": { + "id": "14", + "pid": "1", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:46:14Z" + } + } + ] + }, + { + "comment": { + "id": "2", + "pid": "", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:47:02Z" + }, + "replies": [{ + "comment": { + "id": "21", + "pid": "2", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:47:21Z" + } + }, + { + "comment": { + "id": "22", + "pid": "2", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:47:22Z" + } + } + ] + }, + { + "comment": { + "id": "4", + "pid": "", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:47:22Z" + } + }, + { + "comment": { + "id": "3", + "pid": "", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:47:22Z" + } + } + ], + "info": { + "url": "url", + "count": 12, + "first_time": "2017-12-25T19:46:01Z", + "last_time": "2017-12-25T19:47:22Z" + } + } \ No newline at end of file diff --git a/backend/app/rest/testfile.json b/backend/app/rest/testdata/tree_bench.json similarity index 100% rename from backend/app/rest/testfile.json rename to backend/app/rest/testdata/tree_bench.json diff --git a/backend/app/rest/testdata/tree_del.json b/backend/app/rest/testdata/tree_del.json new file mode 100644 index 00000000..e8a2ce91 --- /dev/null +++ b/backend/app/rest/testdata/tree_del.json @@ -0,0 +1,186 @@ + { + "comments": [{ + "comment": { + "id": "1", + "pid": "", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:46:01Z" + }, + "replies": [{ + "comment": { + "id": "11", + "pid": "1", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:46:11Z" + }, + "replies": [{ + "comment": { + "id": "111", + "pid": "11", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:46:12Z" + } + }] + }] + }, { + "comment": { + "id": "2", + "pid": "", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:47:01Z" + }, + "replies": [{ + "comment": { + "id": "22", + "pid": "2", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:47:02Z" + }, + "replies": [{ + "comment": { + "id": "223", + "pid": "22", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "0001-01-01T00:00:00Z", + "delete": true + }, + "replies": [{ + "comment": { + "id": "2241", + "pid": "223", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:47:05Z" + } + }] + }, { + "comment": { + "id": "221", + "pid": "22", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:47:03Z" + } + }, { + "comment": { + "id": "222", + "pid": "22", + "text": "", + "user": { + "name": "", + "id": "", + "picture": "", + "admin": false + }, + "locator": { + "site": "site", + "url": "url" + }, + "score": 0, + "votes": null, + "time": "2017-12-25T19:47:04Z" + } + }] + }] + }], + "info": { + "url": "url", + "count": 8, + "first_time": "2017-12-25T19:46:01Z", + "last_time": "2017-12-25T19:47:05Z" + } + } \ No newline at end of file diff --git a/backend/app/rest/tree.go b/backend/app/rest/tree.go index 48757db0..bccf0a21 100644 --- a/backend/app/rest/tree.go +++ b/backend/app/rest/tree.go @@ -87,18 +87,21 @@ func (t *Tree) proc(comments []store.Comment, node *Node, rd *recurData, parentI repComments := t.filter(comments, func(comment store.Comment) bool { return comment.ParentID == parentID }) for _, rc := range repComments { - if rc.Timestamp.After(rd.tsModified) { + if !rc.Timestamp.IsZero() && rc.Timestamp.After(rd.tsModified) { rd.tsModified = rc.Timestamp } - if rc.Timestamp.Before(rd.tsCreated) { + if !rc.Timestamp.IsZero() && rc.Timestamp.Before(rd.tsCreated) { rd.tsCreated = rc.Timestamp } if !rc.Deleted { - rd.visible = true + rd.visible = true // indicates top-level should be visible } rnode := &Node{Comment: rc, Replies: []*Node{}} node.Replies = append(node.Replies, rnode) t.proc(comments, rnode, rd, rc.ID) + if !rd.visible || (len(rnode.Replies) == 0 && rc.Deleted) { // clean all-deleted subtree + node.Replies = node.Replies[:len(node.Replies)-1] + } } // replies always sorted by time sort.Slice(node.Replies, func(i, j int) bool { diff --git a/backend/app/rest/tree_test.go b/backend/app/rest/tree_test.go index c6ff7ae7..65892168 100644 --- a/backend/app/rest/tree_test.go +++ b/backend/app/rest/tree_test.go @@ -1,15 +1,14 @@ package rest import ( - "bytes" "encoding/json" "io/ioutil" - "regexp" - "strings" + "log" "testing" "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/umputun/remark/backend/app/store" ) @@ -41,14 +40,11 @@ func TestMakeTree(t *testing.T) { } res := MakeTree(comments, "time", 0) + resJSON, err := json.Marshal(&res) + require.Nil(t, err) - buf := &bytes.Buffer{} - enc := json.NewEncoder(buf) - enc.SetIndent("", " ") - err := enc.Encode(res) - assert.Nil(t, err) - expected, actual := cleanFormatting(expJSON, buf.String()) - assert.Equal(t, expected, actual) + expJSON := mustLoadJSONFile(t, "testdata/tree.json") + assert.Equal(t, expJSON, resJSON) assert.Equal(t, store.PostInfo{URL: "url", Count: 12, FirstTS: ts(46, 1), LastTS: ts(47, 22)}, res.Info) res = MakeTree([]store.Comment{}, "time", 0) @@ -58,6 +54,43 @@ func TestMakeTree(t *testing.T) { assert.Equal(t, store.PostInfo{URL: "url", Count: 12, FirstTS: ts(46, 1), LastTS: ts(47, 22), ReadOnly: true}, res.Info) } +func TestMakeEmptySubtree(t *testing.T) { + loc := store.Locator{URL: "url", SiteID: "site"} + ts := func(min int, sec int) time.Time { return time.Date(2017, 12, 25, 19, min, sec, 0, time.UTC) } + + // unsorted by purpose + comments := []store.Comment{ + {Locator: loc, ID: "1", Timestamp: ts(46, 1)}, + {Locator: loc, ID: "11", ParentID: "1", Timestamp: ts(46, 11)}, + {Locator: loc, ID: "111", ParentID: "11", Timestamp: ts(46, 12)}, + {Locator: loc, ID: "112", ParentID: "11", Deleted: true}, // subtree deleted + {Locator: loc, ID: "1121", ParentID: "112", Deleted: true}, + {Locator: loc, ID: "1122", ParentID: "112", Deleted: true}, + {Locator: loc, ID: "12", ParentID: "12", Deleted: true}, // subcomment deleted + + {Locator: loc, ID: "2", Timestamp: ts(47, 1)}, + {Locator: loc, ID: "21", ParentID: "2", Deleted: true}, // subtree deleted + {Locator: loc, ID: "211", ParentID: "21", Deleted: true}, + {Locator: loc, ID: "212", ParentID: "21", Deleted: true}, + {Locator: loc, ID: "22", ParentID: "2", Timestamp: ts(47, 2)}, + {Locator: loc, ID: "221", ParentID: "22", Timestamp: ts(47, 3)}, + {Locator: loc, ID: "222", ParentID: "22", Timestamp: ts(47, 4)}, + {Locator: loc, ID: "223", ParentID: "22", Deleted: true}, + {Locator: loc, ID: "224", ParentID: "22", Deleted: true}, + {Locator: loc, ID: "2241", ParentID: "223", Timestamp: ts(47, 5)}, + {Locator: loc, ID: "3", Timestamp: ts(48, 1), Deleted: true}, // deleted top level + } + + res := MakeTree(comments, "time", 0) + resJSON, err := json.Marshal(&res) + require.Nil(t, err) + log.Print(string(resJSON)) + + expJSON := mustLoadJSONFile(t, "testdata/tree_del.json") + assert.Equal(t, string(expJSON), string(resJSON)) + +} + func TestTreeSortNodes(t *testing.T) { // unsorted by purpose comments := []store.Comment{ @@ -114,7 +147,7 @@ func TestTreeSortNodes(t *testing.T) { func BenchmarkTree(b *testing.B) { comments := []store.Comment{} - data, err := ioutil.ReadFile("testfile.json") + data, err := ioutil.ReadFile("testdata/tree_bench.json") assert.Nil(b, err) err = json.Unmarshal(data, &comments) assert.Nil(b, err) @@ -125,272 +158,14 @@ func BenchmarkTree(b *testing.B) { } } -const expJSON = `{ - "comments": [ - { - "comment": { - "id": "1", - "pid": "", - "text": "", - "user": { - "name": "", - "id": "", - "picture": "", - "admin": false - }, - "locator": { - "site": "site", - "url": "url" - }, - "score": 0, - "votes": null, - "time": "2017-12-25T19:46:01Z" - }, - "replies": [ - { - "comment": { - "id": "11", - "pid": "1", - "text": "", - "user": { - "name": "", - "id": "", - "picture": "", - "admin": false - }, - "locator": { - "site": "site", - "url": "url" - }, - "score": 0, - "votes": null, - "time": "2017-12-25T19:46:11Z" - } - }, - { - "comment": { - "id": "12", - "pid": "1", - "text": "", - "user": { - "name": "", - "id": "", - "picture": "", - "admin": false - }, - "locator": { - "site": "site", - "url": "url" - }, - "score": 0, - "votes": null, - "time": "2017-12-25T19:46:12Z" - } - }, - { - "comment": { - "id": "13", - "pid": "1", - "text": "", - "user": { - "name": "", - "id": "", - "picture": "", - "admin": false - }, - "locator": { - "site": "site", - "url": "url" - }, - "score": 0, - "votes": null, - "time": "2017-12-25T19:46:13Z" - }, - "replies": [ - { - "comment": { - "id": "131", - "pid": "13", - "text": "", - "user": { - "name": "", - "id": "", - "picture": "", - "admin": false - }, - "locator": { - "site": "site", - "url": "url" - }, - "score": 0, - "votes": null, - "time": "2017-12-25T19:46:31Z" - } - }, - { - "comment": { - "id": "132", - "pid": "13", - "text": "", - "user": { - "name": "", - "id": "", - "picture": "", - "admin": false - }, - "locator": { - "site": "site", - "url": "url" - }, - "score": 0, - "votes": null, - "time": "2017-12-25T19:46:32Z" - } - } - ] - }, - { - "comment": { - "id": "14", - "pid": "1", - "text": "", - "user": { - "name": "", - "id": "", - "picture": "", - "admin": false - }, - "locator": { - "site": "site", - "url": "url" - }, - "score": 0, - "votes": null, - "time": "2017-12-25T19:46:14Z" - } - } - ] - }, - { - "comment": { - "id": "2", - "pid": "", - "text": "", - "user": { - "name": "", - "id": "", - "picture": "", - "admin": false - }, - "locator": { - "site": "site", - "url": "url" - }, - "score": 0, - "votes": null, - "time": "2017-12-25T19:47:02Z" - }, - "replies": [ - { - "comment": { - "id": "21", - "pid": "2", - "text": "", - "user": { - "name": "", - "id": "", - "picture": "", - "admin": false - }, - "locator": { - "site": "site", - "url": "url" - }, - "score": 0, - "votes": null, - "time": "2017-12-25T19:47:21Z" - } - }, - { - "comment": { - "id": "22", - "pid": "2", - "text": "", - "user": { - "name": "", - "id": "", - "picture": "", - "admin": false - }, - "locator": { - "site": "site", - "url": "url" - }, - "score": 0, - "votes": null, - "time": "2017-12-25T19:47:22Z" - } - } - ] - }, - { - "comment": { - "id": "4", - "pid": "", - "text": "", - "user": { - "name": "", - "id": "", - "picture": "", - "admin": false - }, - "locator": { - "site": "site", - "url": "url" - }, - "score": 0, - "votes": null, - "time": "2017-12-25T19:47:22Z" - } - }, - { - "comment": { - "id": "3", - "pid": "", - "text": "", - "user": { - "name": "", - "id": "", - "picture": "", - "admin": false - }, - "locator": { - "site": "site", - "url": "url" - }, - "score": 0, - "votes": null, - "time": "2017-12-25T19:47:22Z" - } - } - ], - "info": { - "url": "url", - "count": 12, - "first_time": "2017-12-25T19:46:01Z", - "last_time": "2017-12-25T19:47:22Z" - } -} -` - -func cleanFormatting(expected, actual string) (string, string) { - reSpaces := regexp.MustCompile(`[\s\p{Zs}]{2,}`) - - expected = strings.Replace(expected, "\n", " ", -1) - expected = strings.Replace(expected, "\t", " ", -1) - expected = reSpaces.ReplaceAllString(expected, " ") - - actual = strings.Replace(actual, "\n", " ", -1) - actual = reSpaces.ReplaceAllString(actual, " ") - return expected, actual +// loadJsonFile read fixtrue file and clear any custom json formatting +func mustLoadJSONFile(t *testing.T, file string) []byte { + expJSON, err := ioutil.ReadFile(file) + require.Nil(t, err) + expTree := Tree{} + err = json.Unmarshal(expJSON, &expTree) + require.Nil(t, err) + expJSON, err = json.Marshal(expTree) + require.Nil(t, err) + return expJSON }