Let clients reorder the pins

docs/dashboard.md

@@ -43,7 +43,8 @@ nothing. All of them answer the body of `GET /pins/`. An unknown document id ans
 }
 ```
 
-Both lists are newest first. `pinnedBy` is the account as it is now, see [Authors](#authors).
+Both lists are in their order: a new pin goes to the end, and the order can be changed, see
+[Order](#order). `pinnedBy` is the account as it is now, see [Authors](#authors).
 
 The backend stores pins by path, so:
 
@@ -54,6 +55,24 @@ The backend stores pins by path, so:
   is left out of the lists, but its pin stays. When the file comes back, the pin
   shows again.
 
+### Order
+
+`PUT /pins/family/order/` and `PUT /pins/personal/order/` set the order of a list, with the
+same rights as pinning in it:
+
+```json
+{ "documentIds": ["17683270065320168437", "4120567934781623001"] }
+```
+
+The listed documents come first, in this order. Ids that are not pinned in that list are
+ignored, and the pins that are not listed follow in their order. The answer is the body of
+`GET /pins/`. `features` of `GET /server-info/` contains `pin-order` on a server with these
+endpoints. Pins keep their place when their document moves.
+
+Pins from before the order existed keep the order they were listed in, newest first.
+
+### Events
+
 After a change the events websocket sends `{"type": "pins-changed"}`. A change of the
 family pins goes to every connection, a change of personal pins only to the connections of
 that account. That includes changes caused by moves and deletes.

internal/activity/activity.go

@@ -76,6 +76,12 @@ CREATE TABLE push_subscriptions (
 	last_push_at INTEGER NOT NULL DEFAULT 0
 );
 CREATE INDEX push_subscriptions_account ON push_subscriptions(account_id);
+`,
+	// 3: the order of the pins, newest first as they were listed before
+	`
+ALTER TABLE pins ADD COLUMN position INTEGER NOT NULL DEFAULT 0;
+UPDATE pins SET position = (SELECT COUNT(*) FROM pins p2 WHERE p2.account_id = pins.account_id
+	AND (p2.pinned_at > pins.pinned_at OR (p2.pinned_at = pins.pinned_at AND p2.path < pins.path)));
 `,
 }
 

internal/activity/activity_test.go

@@ -1,7 +1,10 @@
 package activity
 
 import (
+	"database/sql"
+	"path/filepath"
 	"reflect"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -172,7 +175,8 @@ func TestPins(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(family) != 2 || family[0].Path != "index.md" || len(personal) != 1 || personal[0].Path != "guide/sub/deep.md" {
+	// new pins go to the end
+	if len(family) != 2 || family[0].Path != "guide/setup.md" || len(personal) != 1 || personal[0].Path != "guide/sub/deep.md" {
 		t.Fatalf("family %+v personal %+v", family, personal)
 	}
 
@@ -204,3 +208,76 @@ func TestPins(t *testing.T) {
 		t.Error("removed twice")
 	}
 }
+
+func TestReorderPins(t *testing.T) {
+	s, c := newStore(t)
+	for _, p := range []string{"a.md", "b.md", "c.md", "d.md"} {
+		if _, err := s.AddPin(Family, p, "alice"); err != nil {
+			t.Fatal(err)
+		}
+		c.advance(time.Second)
+	}
+	order := func() string {
+		family, _, err := s.Pins("bob")
+		if err != nil {
+			t.Fatal(err)
+		}
+		var out []string
+		for _, p := range family {
+			out = append(out, p.Path)
+		}
+		return strings.Join(out, ",")
+	}
+	// unknown paths are ignored, the missing pins follow in their order
+	changed, err := s.ReorderPins(Family, []string{"c.md", "nope.md", "a.md", "c.md"})
+	if err != nil || !changed || order() != "c.md,a.md,b.md,d.md" {
+		t.Fatalf("changed %v, %v, order %s", changed, err, order())
+	}
+	if changed, _ := s.ReorderPins(Family, []string{"c.md"}); changed {
+		t.Error("the same order changed")
+	}
+	if _, err := s.AddPin(Family, "e.md", "alice"); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := s.MovePins("a.md", "z.md"); err != nil {
+		t.Fatal(err)
+	}
+	if order() != "c.md,z.md,b.md,d.md,e.md" {
+		t.Errorf("order %s", order())
+	}
+	// personal pins are separate
+	if changed, _ := s.ReorderPins("bob", []string{"c.md"}); changed {
+		t.Error("bob has no pins")
+	}
+}
+
+// The pins of a database from before the order keep the order they were listed in, newest first
+func TestPinOrderMigration(t *testing.T) {
+	dir := t.TempDir()
+	db, err := sql.Open("sqlite", "file:"+filepath.Join(dir, DatabaseFileName))
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, m := range migrations[:2] {
+		if _, err = db.Exec(m); err != nil {
+			t.Fatal(err)
+		}
+	}
+	if _, err = db.Exec(`PRAGMA user_version = 2;
+		INSERT INTO pins (account_id, path, pinned_by, pinned_at) VALUES ('', 'old.md', 'a', 1), ('', 'new.md', 'a', 3), ('', 'mid.md', 'a', 2), ('bob', 'x.md', 'b', 5)`); err != nil {
+		t.Fatal(err)
+	}
+	_ = db.Close()
+	s, err := Open(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = s.Close() }()
+	family, personal, err := s.Pins("bob")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(family) != 3 || family[0].Path != "new.md" || family[1].Path != "mid.md" || family[2].Path != "old.md" || len(personal) != 1 {
+		t.Errorf("family %+v personal %+v", family, personal)
+	}
+}

internal/activity/pins.go

@@ -2,6 +2,7 @@ package activity
 
 import (
 	"database/sql"
+	"slices"
 	"time"
 
 	"github.com/MkDocsEditor/MkDocsEditor-Backend/internal/wiki"
@@ -43,10 +44,10 @@ func (c *PinChanges) add(accountID string) {
 	c.Accounts = append(c.Accounts, accountID)
 }
 
-// Pins returns the family pins and the personal pins of an account, newest first
+// Pins returns the family pins and the personal pins of an account, in their order
 func (s *Store) Pins(accountID string) (family []Pin, personal []Pin, err error) {
 	rows, err := s.db.Query(`SELECT account_id, path, pinned_by, pinned_at FROM pins WHERE account_id IN (?, ?)
-		ORDER BY pinned_at DESC, path`, Family, accountID)
+		ORDER BY position, pinned_at DESC, path`, Family, accountID)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -70,11 +71,12 @@ func (s *Store) Pins(accountID string) (family []Pin, personal []Pin, err error)
 	return family, personal, rows.Err()
 }
 
-// AddPin pins a document for an account, or for the family with Family. It returns false if
-// the document was pinned already.
+// AddPin pins a document for an account, or for the family with Family, at the end of the
+// list. It returns false if the document was pinned already.
 func (s *Store) AddPin(accountID string, path string, by string) (bool, error) {
-	res, err := s.db.Exec(`INSERT INTO pins (account_id, path, pinned_by, pinned_at) VALUES (?, ?, ?, ?)
-		ON CONFLICT DO NOTHING`, accountID, path, by, s.now().UnixMilli())
+	res, err := s.db.Exec(`INSERT INTO pins (account_id, path, pinned_by, pinned_at, position)
+		VALUES (?, ?, ?, ?, (SELECT COALESCE(MAX(position) + 1, 0) FROM pins WHERE account_id = ?))
+		ON CONFLICT DO NOTHING`, accountID, path, by, s.now().UnixMilli(), accountID)
 	if err != nil {
 		return false, err
 	}
@@ -92,6 +94,60 @@ func (s *Store) RemovePin(accountID string, path string) (bool, error) {
 	return n > 0, err
 }
 
+// ReorderPins puts the pins of an account (or Family) at the paths first, in that order. Paths
+// that are not pinned are ignored, the other pins follow in their order. It returns false if
+// the order stayed the same.
+func (s *Store) ReorderPins(accountID string, paths []string) (bool, error) {
+	tx, err := s.db.Begin()
+	if err != nil {
+		return false, err
+	}
+	defer func() { _ = tx.Rollback() }()
+	rows, err := tx.Query(`SELECT path FROM pins WHERE account_id = ? ORDER BY position, pinned_at DESC, path`, accountID)
+	if err != nil {
+		return false, err
+	}
+	var current []string
+	for rows.Next() {
+		var p string
+		if err = rows.Scan(&p); err != nil {
+			_ = rows.Close()
+			return false, err
+		}
+		current = append(current, p)
+	}
+	_ = rows.Close()
+	if err = rows.Err(); err != nil {
+		return false, err
+	}
+	pinned := map[string]bool{}
+	for _, p := range current {
+		pinned[p] = true
+	}
+	order := make([]string, 0, len(current))
+	placed := map[string]bool{}
+	for _, p := range paths {
+		if pinned[p] && !placed[p] {
+			placed[p] = true
+			order = append(order, p)
+		}
+	}
+	for _, p := range current {
+		if !placed[p] {
+			order = append(order, p)
+		}
+	}
+	if slices.Equal(order, current) {
+		return false, nil
+	}
+	for i, p := range order {
+		if _, err = tx.Exec(`UPDATE pins SET position = ? WHERE account_id = ? AND path = ?`, i, accountID, p); err != nil {
+			return false, err
+		}
+	}
+	return true, tx.Commit()
+}
+
 // MovePins changes the paths of the pins of a moved item: the item itself and, for a section,
 // everything below it. A pin that already exists at the new path stays.
 func (s *Store) MovePins(oldPath string, newPath string) (PinChanges, error) {

internal/api/dashboard_test.go

@@ -201,3 +201,46 @@ func TestFeedLeavesOutReferenceUpdates(t *testing.T) {
 		t.Error("the reference was not updated")
 	}
 }
+
+func TestPinOrder(t *testing.T) {
+	e := newSyncEnv(t)
+	setup, index := e.docID("guide/setup.md"), e.docID("index.md")
+	ev := dialEvents(t, e, "carol", false)
+	e.pins(http.MethodPut, "/pins/family/"+setup+"/", "bob", http.StatusOK)
+	pins := e.pins(http.MethodPut, "/pins/family/"+index+"/", "bob", http.StatusOK)
+	if pinPaths(pins.Family) != "guide/setup.md,index.md" {
+		t.Fatalf("new pins not at the end: %s", pinPaths(pins.Family))
+	}
+	order := func(scope string, user string, ids []string, want int) *PinsResponse {
+		t.Helper()
+		body, _ := json.Marshal(PinOrderRequest{DocumentIDs: ids})
+		rec := e.doBody(http.MethodPut, "/pins/"+scope+"/order/", user, string(body))
+		if rec.Code != want {
+			t.Fatalf("order %s as %s: %d %s", scope, user, rec.Code, rec.Body.String())
+		}
+		var out PinsResponse
+		_ = json.Unmarshal(rec.bytes(), &out)
+		return &out
+	}
+	before := ev.count(collab.MsgPinsChanged)
+	pins = order("family", "bob", []string{index, "unknown"}, http.StatusOK)
+	if pinPaths(pins.Family) != "index.md,guide/setup.md" {
+		t.Errorf("order = %s", pinPaths(pins.Family))
+	}
+	eventually(t, "pins-changed", func() bool { return ev.count(collab.MsgPinsChanged) == before+1 })
+	if got := pinPaths(e.pins(http.MethodGet, "/pins/", "alice", http.StatusOK).Family); got != "index.md,guide/setup.md" {
+		t.Errorf("stored order = %s", got)
+	}
+
+	// readers order their own pins only
+	order("family", "carol", []string{setup}, http.StatusForbidden)
+	e.pins(http.MethodPut, "/pins/personal/"+index+"/", "carol", http.StatusOK)
+	e.pins(http.MethodPut, "/pins/personal/"+setup+"/", "carol", http.StatusOK)
+	pins = order("personal", "carol", []string{setup}, http.StatusOK)
+	if pinPaths(pins.Personal) != "guide/setup.md,index.md" || pinPaths(pins.Family) != "index.md,guide/setup.md" {
+		t.Errorf("carol = %+v", pins)
+	}
+	if rec := e.doBody(http.MethodPut, "/pins/personal/order/", "carol", `{"documentIds": "x"}`); rec.Code != http.StatusBadRequest {
+		t.Errorf("bad body: %d", rec.Code)
+	}
+}

internal/api/dashboardapi.go

@@ -20,6 +20,8 @@ const (
 	FeaturePins = "pins"
 	// FeatureFeed means the server has the change feed of docs/dashboard.md
 	FeatureFeed = "feed"
+	// FeaturePinOrder means the pins can be reordered, see docs/dashboard.md
+	FeaturePinOrder = "pin-order"
 
 	feedDefaultLimit = 50
 	feedMaxLimit     = 200
@@ -47,6 +49,11 @@ type (
 		PinnedAt   string `json:"pinnedAt"`
 	}
 
+	// PinOrderRequest is the body of PUT /pins/{family|personal}/order/
+	PinOrderRequest struct {
+		DocumentIDs []string `json:"documentIds"`
+	}
+
 	// PinsResponse is the answer of GET /pins/ and of the pin changes
 	PinsResponse struct {
 		Family   []PinResponse `json:"family"`
@@ -169,6 +176,38 @@ func (rs *RestService) changePin(family bool, add bool) echo.HandlerFunc {
 	}
 }
 
+// changePinOrder answers PUT /pins/family/order/ and /pins/personal/order/
+func (rs *RestService) changePinOrder(family bool) echo.HandlerFunc {
+	return func(c *echo.Context) error {
+		user := UserFromContext(c)
+		if family && !user.CanWrite() {
+			return apiError(c, http.StatusForbidden, CodeForbidden, "only editors and admins may change the family pins", nil)
+		}
+		r := new(PinOrderRequest)
+		if err := c.Bind(r); err != nil {
+			return apiError(c, http.StatusBadRequest, CodeBadRequest, "the body must be {\"documentIds\": [\"…\"]}", nil)
+		}
+		paths := make([]string, 0, len(r.DocumentIDs))
+		for _, id := range r.DocumentIDs {
+			if d := rs.treeManager.GetDocument(id); d != nil {
+				paths = append(paths, d.RelPath)
+			}
+		}
+		owner := user.ID
+		if family {
+			owner = activity.Family
+		}
+		changed, err := rs.sync.Activity().ReorderPins(owner, paths)
+		if err != nil {
+			return rs.ReturnError(c, err)
+		}
+		if changed {
+			rs.sync.PinsChanged(family, user.ID)
+		}
+		return rs.getPins(c)
+	}
+}
+
 // GET /feed/
 func (rs *RestService) getFeed(c *echo.Context) error {
 	limit := feedDefaultLimit

internal/api/devicelinkapi.go

@@ -115,7 +115,7 @@ func (rs *RestService) serverInfo(c *echo.Context) error {
 		}
 		methods = append(methods, "device-link")
 	}
-	features := []string{FeatureMovedLookup, FeatureJoinPresence, FeatureInstancePresence, FeatureAvatars, FeaturePins, FeatureFeed}
+	features := []string{FeatureMovedLookup, FeatureJoinPresence, FeatureInstancePresence, FeatureAvatars, FeaturePins, FeatureFeed, FeaturePinOrder}
 	if rs.authenticator.AccessRequestsAvailable() {
 		features = append(features, FeatureAccessRequests)
 	}

internal/api/devicelinkapi_test.go

@@ -29,7 +29,7 @@ func TestServerInfo(t *testing.T) {
 		info.WebClientURL == nil || *info.WebClientURL != testWebOrigin ||
 		!info.SetupRequired || info.AuthDisabled || !info.Passkeys.Enabled || info.Passkeys.RPID != "localhost" ||
 		strings.Join(info.LoginMethods, ",") != "password,passkey,device-link" || info.Require2FA ||
-		strings.Join(info.Features, ",") != FeatureMovedLookup+","+FeatureJoinPresence+","+FeatureInstancePresence+","+FeatureAvatars+","+FeaturePins+","+FeatureFeed {
+		strings.Join(info.Features, ",") != FeatureMovedLookup+","+FeatureJoinPresence+","+FeatureInstancePresence+","+FeatureAvatars+","+FeaturePins+","+FeatureFeed+","+FeaturePinOrder {
 		t.Errorf("server info = %s", r.body)
 	}
 	// nothing about accounts

internal/api/rest.go

@@ -301,6 +301,8 @@ func (rs *RestService) registerContentRoutes(echoRest *echo.Echo) {
 	echoRest.GET("/moved/", rs.getMovedList)
 	echoRest.GET("/moved/:"+urlParamID+"/", rs.getMoved)
 	echoRest.GET("/pins/", rs.getPins)
+	echoRest.PUT("/pins/family/order/", rs.changePinOrder(true))
+	echoRest.PUT("/pins/personal/order/", rs.changePinOrder(false))
 	echoRest.PUT("/pins/family/:"+urlParamID+"/", rs.changePin(true, true))
 	echoRest.DELETE("/pins/family/:"+urlParamID+"/", rs.changePin(true, false))
 	echoRest.PUT("/pins/personal/:"+urlParamID+"/", rs.changePin(false, true))