collect /find Info for tree and plain types consistently

MakeTree calculated Info locally for historical reasons,
and the results were consistent with the dataService.Info call
but calculated differently.

That change fixes that, ensuring that Info is requested
in the same manner.

Author: paskal

backend/app/rest/api/admin_test.go

@@ -530,7 +530,7 @@ func TestAdmin_ReadOnlyNoComments(t *testing.T) {
 	err = json.Unmarshal([]byte(res), &comments)
 	assert.NoError(t, err)
 	assert.Equal(t, 0, len(comments.Comments), "should have 0 comments")
-	assert.False(t, comments.Info.ReadOnly, "different from the plain format, should be fixed")
+	assert.True(t, comments.Info.ReadOnly)
 	t.Logf("%+v", comments)
 }
 

backend/app/rest/api/rest.go

@@ -98,6 +98,11 @@ type commentsWithInfo struct {
 	Info     store.PostInfo  `json:"info,omitempty"`
 }
 
+type treeWithInfo struct {
+	*service.Tree
+	Info store.PostInfo `json:"info,omitempty"`
+}
+
 // Run the lister and request's router, activate rest server
 func (s *Rest) Run(address string, port int) {
 	if address == "*" {

backend/app/rest/api/rest_public.go

@@ -50,6 +50,8 @@ type pubStore interface {
 
 // GET /find?site=siteID&url=post-url&format=[tree|plain]&sort=[+/-time|+/-score|+/-controversy]&view=[user|all]&since=unix_ts_msec
 // find comments for given post. Returns in tree or plain formats, sorted
+//
+// When `url` parameter is not set (e.g. request is for site-wide comments), does not return deleted comments.
 func (s *public) findCommentsCtrl(w http.ResponseWriter, r *http.Request) {
 	locator := store.Locator{SiteID: r.URL.Query().Get("site"), URL: r.URL.Query().Get("url")}
 	sort := r.URL.Query().Get("sort")
@@ -77,22 +79,36 @@ func (s *public) findCommentsCtrl(w http.ResponseWriter, r *http.Request) {
 			comments = []store.Comment{} // error should clear comments and continue for post info
 		}
 		comments = s.applyView(comments, view)
+
+		var commentsInfo store.PostInfo
+		if info, ee := s.dataService.Info(locator, s.readOnlyAge); ee == nil {
+			commentsInfo = info
+		}
+
+		if !since.IsZero() { // if since is set, number of comments can be different from total in the DB
+			commentsInfo.Count = 0
+			for _, c := range comments {
+				if !c.Deleted {
+					commentsInfo.Count++
+				}
+			}
+		}
+
+		// post might be readonly without any comments, Info call will fail then and ReadOnly flag should be checked separately
+		if !commentsInfo.ReadOnly && locator.URL != "" && s.dataService.IsReadOnly(locator) {
+			commentsInfo.ReadOnly = true
+		}
+
 		var b []byte
 		switch format {
 		case "tree":
-			tree := service.MakeTree(comments, sort, s.readOnlyAge)
-			if tree.Nodes == nil { // eliminate json nil serialization
-				tree.Nodes = []*service.Node{}
-			}
-			if s.dataService.IsReadOnly(locator) {
-				tree.Info.ReadOnly = true
+			withInfo := treeWithInfo{Tree: service.MakeTree(comments, sort), Info: commentsInfo}
+			if withInfo.Nodes == nil { // eliminate json nil serialization
+				withInfo.Nodes = []*service.Node{}
 			}
-			b, e = encodeJSONWithHTML(tree)
+			b, e = encodeJSONWithHTML(withInfo)
 		default:
-			withInfo := commentsWithInfo{Comments: comments}
-			if info, ee := s.dataService.Info(locator, s.readOnlyAge); ee == nil {
-				withInfo.Info = info
-			}
+			withInfo := commentsWithInfo{Comments: comments, Info: commentsInfo}
 			b, e = encodeJSONWithHTML(withInfo)
 		}
 		return b, e

backend/app/rest/api/rest_public_test.go

@@ -229,7 +229,7 @@ func TestRest_Find(t *testing.T) {
 	assert.Equal(t, id2, comments.Comments[0].ID)
 
 	// get in tree mode
-	tree := service.Tree{}
+	tree := treeWithInfo{}
 	res, code = get(t, ts.URL+"/api/v1/find?site=remark42&url=https://radio-t.com/blah1&format=tree")
 	assert.Equal(t, http.StatusOK, code)
 	err = json.Unmarshal([]byte(res), &tree)
@@ -255,7 +255,7 @@ func TestRest_FindAge(t *testing.T) {
 	_, err = srv.DataService.Create(c2)
 	require.NoError(t, err)
 
-	tree := service.Tree{}
+	tree := treeWithInfo{}
 
 	res, code := get(t, ts.URL+"/api/v1/find?site=remark42&url=https://radio-t.com/blah1&format=tree")
 	assert.Equal(t, http.StatusOK, code)
@@ -298,15 +298,15 @@ func TestRest_FindReadOnly(t *testing.T) {
 	require.NoError(t, err)
 	require.NoError(t, resp.Body.Close())
 
-	tree := service.Tree{}
+	tree := treeWithInfo{}
 	res, code := get(t, ts.URL+"/api/v1/find?site=remark42&url=https://radio-t.com/blah1&format=tree")
 	assert.Equal(t, http.StatusOK, code)
 	err = json.Unmarshal([]byte(res), &tree)
 	require.NoError(t, err)
 	assert.Equal(t, "https://radio-t.com/blah1", tree.Info.URL)
 	assert.True(t, tree.Info.ReadOnly, "post is ro")
 
-	tree = service.Tree{}
+	tree = treeWithInfo{}
 	res, code = get(t, ts.URL+"/api/v1/find?site=remark42&url=https://radio-t.com/blah2&format=tree")
 	assert.Equal(t, http.StatusOK, code)
 	err = json.Unmarshal([]byte(res), &tree)
@@ -650,28 +650,28 @@ func TestPublic_FindCommentsCtrl_ConsistentCount(t *testing.T) {
 		{"url=test-url&since=" + sinceTenSecondsAgo, fmt.Sprintf(`"info":{"url":"test-url","count":6,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[7])},
 		{"since=" + sinceTS[0], fmt.Sprintf(`"info":{"count":7,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[8])},
 		{"url=test-url&since=" + sinceTS[0], fmt.Sprintf(`"info":{"url":"test-url","count":6,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[7])},
-		{"since=" + sinceTS[1], fmt.Sprintf(`"info":{"count":7,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[8])},
-		{"url=test-url&since=" + sinceTS[1], fmt.Sprintf(`"info":{"url":"test-url","count":6,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[7])},
-		{"since=" + sinceTS[4], fmt.Sprintf(`"info":{"count":7,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[8])},
-		{"url=test-url&since=" + sinceTS[4], fmt.Sprintf(`"info":{"url":"test-url","count":6,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[7])},
-		{"format=tree", `"info":{"url":"test-url","count":7`},
+		{"since=" + sinceTS[1], fmt.Sprintf(`"info":{"count":6,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[8])},
+		{"url=test-url&since=" + sinceTS[1], fmt.Sprintf(`"info":{"url":"test-url","count":5,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[7])},
+		{"since=" + sinceTS[4], fmt.Sprintf(`"info":{"count":3,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[8])},
+		{"url=test-url&since=" + sinceTS[4], fmt.Sprintf(`"info":{"url":"test-url","count":2,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[7])},
+		{"format=tree", `"info":{"count":7`},
 		{"format=tree&url=test-url", `"info":{"url":"test-url","count":6`},
-		{"format=tree&sort=+time", `"info":{"url":"test-url","count":7`},
+		{"format=tree&sort=+time", `"info":{"count":7`},
 		{"format=tree&url=test-url&sort=+time", `"info":{"url":"test-url","count":6`},
-		{"format=tree&sort=-score", `"info":{"url":"test-url","count":7`},
+		{"format=tree&sort=-score", `"info":{"count":7`},
 		{"format=tree&url=test-url&sort=-score", `"info":{"url":"test-url","count":6`},
 		{"sort=+time", fmt.Sprintf(`"score":-25,"vote":0,"time":%q}],"info":{"count":7`, formattedTS[8])},
 		{"sort=-time", fmt.Sprintf(`"score":1,"vote":0,"time":%q}],"info":{"count":7`, formattedTS[0])},
 		{"sort=+score", fmt.Sprintf(`"score":10,"vote":0,"time":%q}],"info":{"count":7`, formattedTS[2])},
 		{"sort=+score&url=test-url", fmt.Sprintf(`"score":10,"vote":0,"time":%q}],"info":{"url":"test-url","count":6`, formattedTS[2])},
 		{"sort=-score", fmt.Sprintf(`"score":-25,"vote":0,"time":%q}],"info":{"count":7`, formattedTS[8])},
 		{"sort=-score&url=test-url", fmt.Sprintf(`"score":-2,"vote":0,"controversy":1.5874010519681994,"time":%q}],"info":{"url":"test-url","count":6`, formattedTS[6])},
-		{"sort=-time&since=" + sinceTS[4], fmt.Sprintf(`"score":-1,"vote":0,"controversy":2.924017738212866,"time":%q}],"info":{"count":7`, formattedTS[4])},
-		{"sort=-score&since=" + sinceTS[3], fmt.Sprintf(`"score":-25,"vote":0,"time":%q}],"info":{"count":7`, formattedTS[8])},
-		{"sort=-score&url=test-url&since=" + sinceTS[3], fmt.Sprintf(`"score":-2,"vote":0,"controversy":1.5874010519681994,"time":%q}],"info":{"url":"test-url","count":6`, formattedTS[6])},
-		{"sort=+controversy&url=test-url&since=" + sinceTS[5], fmt.Sprintf(`"score":-2,"vote":0,"controversy":1.5874010519681994,"time":%q}],"info":{"url":"test-url","count":6`, formattedTS[6])},
+		{"sort=-time&since=" + sinceTS[4], fmt.Sprintf(`"score":-1,"vote":0,"controversy":2.924017738212866,"time":%q}],"info":{"count":3`, formattedTS[4])},
+		{"sort=-score&since=" + sinceTS[3], fmt.Sprintf(`"score":-25,"vote":0,"time":%q}],"info":{"count":4`, formattedTS[8])},
+		{"sort=-score&url=test-url&since=" + sinceTS[3], fmt.Sprintf(`"score":-2,"vote":0,"controversy":1.5874010519681994,"time":%q}],"info":{"url":"test-url","count":3`, formattedTS[6])},
+		{"sort=+controversy&url=test-url&since=" + sinceTS[5], fmt.Sprintf(`"score":-2,"vote":0,"controversy":1.5874010519681994,"time":%q}],"info":{"url":"test-url","count":1`, formattedTS[6])},
 		// three comments of which last one deleted and doesn't have controversy so returned last
-		{"sort=-controversy&url=test-url&since=" + sinceTS[5], fmt.Sprintf(`"score":0,"vote":0,"time":%q,"delete":true}],"info":{"url":"test-url","count":6`, formattedTS[7])},
+		{"sort=-controversy&url=test-url&since=" + sinceTS[5], fmt.Sprintf(`"score":0,"vote":0,"time":%q,"delete":true}],"info":{"url":"test-url","count":1`, formattedTS[7])},
 	}
 
 	for _, tc := range testCases {

backend/app/store/service/testdata/tree.json

@@ -242,11 +242,5 @@
                   "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"
-      }
-  }
+      ]
+  }

backend/app/store/service/testdata/tree_del.json

@@ -176,11 +176,5 @@
                  }
              }]
          }]
-     }],
-     "info": {
-         "url": "url",
-         "count": 8,
-         "first_time": "2017-12-25T19:46:01Z",
-         "last_time": "2017-12-25T19:47:05Z"
-     }
- }
+     }]
+ }

backend/app/store/service/tree.go

@@ -10,8 +10,7 @@ import (
 
 // Tree is formatter making tree from the list of comments
 type Tree struct {
-	Nodes []*Node        `json:"comments"`
-	Info  store.PostInfo `json:"info,omitempty"`
+	Nodes []*Node `json:"comments"`
 }
 
 // Node is a comment with optional replies
@@ -30,22 +29,12 @@ type recurData struct {
 }
 
 // MakeTree gets unsorted list of comments and produces Tree
-// It will make store.PostInfo by itself and will mark Info.ReadOnly based on passed readOnlyAge
-// Tree maker is local and has no access to the data store. By this reason it has to make Info and won't be able
-// to handle store's read-only status. This status should be set by caller.
-func MakeTree(comments []store.Comment, sortType string, readOnlyAge int) *Tree {
+func MakeTree(comments []store.Comment, sortType string) *Tree {
 	if len(comments) == 0 {
 		return &Tree{}
 	}
 
-	res := Tree{
-		Info: store.PostInfo{
-			URL:     comments[0].Locator.URL,
-			FirstTS: comments[0].Timestamp,
-			LastTS:  comments[0].Timestamp,
-		},
-	}
-	res.Info.Count = len(res.filter(comments, func(c store.Comment) bool { return !c.Deleted }))
+	res := Tree{}
 
 	topComments := res.filter(comments, func(c store.Comment) bool { return c.ParentID == "" })
 
@@ -54,23 +43,12 @@ func MakeTree(comments []store.Comment, sortType string, readOnlyAge int) *Tree
 		node := Node{Comment: rootComment}
 
 		rd := recurData{}
-		commentsTree, tsModified, tsCreated := res.proc(comments, &node, &rd, rootComment.ID)
-		// skip deleted with no sub-comments ar all sub-comments deleted
+		commentsTree := res.proc(comments, &node, &rd, rootComment.ID)
+		// skip deleted with no sub-comments and all sub-comments deleted
 		if rootComment.Deleted && (len(commentsTree.Replies) == 0 || !rd.visible) {
 			continue
 		}
 
-		commentsTree.tsModified, commentsTree.tsCreated = tsModified, tsCreated
-		if commentsTree.tsCreated.Before(res.Info.FirstTS) {
-			res.Info.FirstTS = commentsTree.tsCreated
-		}
-		if commentsTree.tsModified.After(res.Info.LastTS) {
-			res.Info.LastTS = commentsTree.tsModified
-		}
-
-		res.Info.ReadOnly = readOnlyAge > 0 && !res.Info.FirstTS.IsZero() &&
-			res.Info.FirstTS.AddDate(0, 0, readOnlyAge).Before(time.Now())
-
 		res.Nodes = append(res.Nodes, commentsTree)
 	}
 
@@ -79,7 +57,7 @@ func MakeTree(comments []store.Comment, sortType string, readOnlyAge int) *Tree
 }
 
 // proc makes tree for one top-level comment recursively
-func (t *Tree) proc(comments []store.Comment, node *Node, rd *recurData, parentID string) (result *Node, modified, created time.Time) {
+func (t *Tree) proc(comments []store.Comment, node *Node, rd *recurData, parentID string) (result *Node) {
 	if rd.tsModified.IsZero() || rd.tsCreated.IsZero() {
 		rd.tsModified, rd.tsCreated = node.Comment.Timestamp, node.Comment.Timestamp
 	}
@@ -106,7 +84,8 @@ func (t *Tree) proc(comments []store.Comment, node *Node, rd *recurData, parentI
 	sort.Slice(node.Replies, func(i, j int) bool {
 		return node.Replies[i].Comment.Timestamp.Before(node.Replies[j].Comment.Timestamp)
 	})
-	return node, rd.tsModified, rd.tsCreated
+	node.tsModified, node.tsCreated = rd.tsModified, rd.tsCreated
+	return node
 }
 
 // filter returns comments for parentID

backend/app/store/service/tree_test.go

@@ -37,19 +37,15 @@ func TestMakeTree(t *testing.T) {
 		{Locator: loc, ID: "611", ParentID: "61", Deleted: true},
 	}
 
-	res := MakeTree(comments, "time", 0)
+	res := MakeTree(comments, "time")
 	resJSON, err := json.Marshal(&res)
 	require.NoError(t, err)
 
 	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)
+	res = MakeTree([]store.Comment{}, "time")
 	assert.Equal(t, &Tree{}, res)
-
-	res = MakeTree(comments, "time", 10)
-	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) {
@@ -79,7 +75,7 @@ func TestMakeEmptySubtree(t *testing.T) {
 		{Locator: loc, ID: "3", Timestamp: ts(48, 1), Deleted: true}, // deleted top level
 	}
 
-	res := MakeTree(comments, "time", 0)
+	res := MakeTree(comments, "time")
 	resJSON, err := json.Marshal(&res)
 	require.NoError(t, err)
 	t.Log(string(resJSON))
@@ -108,50 +104,50 @@ func TestTreeSortNodes(t *testing.T) {
 		{ID: "5", Deleted: true, Timestamp: time.Date(2017, 12, 25, 19, 47, 22, 150, time.UTC)},
 	}
 
-	res := MakeTree(comments, "+active", 0)
+	res := MakeTree(comments, "+active")
 	assert.Equal(t, "2", res.Nodes[0].Comment.ID)
 	t.Log(res.Nodes[0].Comment.ID, res.Nodes[0].tsModified)
 
-	res = MakeTree(comments, "-active", 0)
+	res = MakeTree(comments, "-active")
 	t.Log(res.Nodes[0].Comment.ID, res.Nodes[0].tsModified)
 	assert.Equal(t, "1", res.Nodes[0].Comment.ID)
 
-	res = MakeTree(comments, "+time", 0)
+	res = MakeTree(comments, "+time")
 	t.Log(res.Nodes[0].Comment.ID, res.Nodes[0].tsModified)
 	assert.Equal(t, "1", res.Nodes[0].Comment.ID)
 
-	res = MakeTree(comments, "-time", 0)
+	res = MakeTree(comments, "-time")
 	assert.Equal(t, "6", res.Nodes[0].Comment.ID)
 
-	res = MakeTree(comments, "score", 0)
+	res = MakeTree(comments, "score")
 	assert.Equal(t, "4", res.Nodes[0].Comment.ID)
 	assert.Equal(t, "3", res.Nodes[1].Comment.ID)
 	assert.Equal(t, "6", res.Nodes[2].Comment.ID)
 	assert.Equal(t, "1", res.Nodes[3].Comment.ID)
 
-	res = MakeTree(comments, "+score", 0)
+	res = MakeTree(comments, "+score")
 	assert.Equal(t, "4", res.Nodes[0].Comment.ID)
 
-	res = MakeTree(comments, "-score", 0)
+	res = MakeTree(comments, "-score")
 	assert.Equal(t, "2", res.Nodes[0].Comment.ID)
 	assert.Equal(t, "1", res.Nodes[1].Comment.ID)
 	assert.Equal(t, "3", res.Nodes[2].Comment.ID)
 	assert.Equal(t, "6", res.Nodes[3].Comment.ID)
 
-	res = MakeTree(comments, "+controversy", 0)
+	res = MakeTree(comments, "+controversy")
 	assert.Equal(t, "3", res.Nodes[0].Comment.ID)
 	assert.Equal(t, "6", res.Nodes[1].Comment.ID)
 	assert.Equal(t, "2", res.Nodes[2].Comment.ID)
 	assert.Equal(t, "4", res.Nodes[3].Comment.ID)
 	assert.Equal(t, "1", res.Nodes[4].Comment.ID)
 
-	res = MakeTree(comments, "-controversy", 0)
+	res = MakeTree(comments, "-controversy")
 	assert.Equal(t, "1", res.Nodes[0].Comment.ID)
 	assert.Equal(t, "4", res.Nodes[1].Comment.ID)
 	assert.Equal(t, "2", res.Nodes[2].Comment.ID)
 	assert.Equal(t, "3", res.Nodes[3].Comment.ID)
 
-	res = MakeTree(comments, "undefined", 0)
+	res = MakeTree(comments, "undefined")
 	t.Log(res.Nodes[0].Comment.ID, res.Nodes[0].tsModified)
 	assert.Equal(t, "1", res.Nodes[0].Comment.ID)
 }
@@ -164,7 +160,7 @@ func BenchmarkTree(b *testing.B) {
 	assert.NoError(b, err)
 
 	for i := 0; i < b.N; i++ {
-		res := MakeTree(comments, "time", 0)
+		res := MakeTree(comments, "time")
 		assert.NotNil(b, res)
 	}
 }