diff --git a/pkg/models/task_relation_authz_test.go b/pkg/models/task_relation_authz_test.go index 720b8bcf2..9e37db0f4 100644 --- a/pkg/models/task_relation_authz_test.go +++ b/pkg/models/task_relation_authz_test.go @@ -48,7 +48,7 @@ func TestAddRelatedTasksToTasks_InheritedProjectAccess(t *testing.T) { } taskIDs := []int64{49} - err := addRelatedTasksToTasks(s, taskIDs, taskMap, u) + err := addRelatedTasksToTasks(s, taskIDs, taskMap, u, nil) require.NoError(t, err) foundTask50 := false @@ -81,7 +81,7 @@ func TestAddRelatedTasksToTasks_NoAccessToHierarchy(t *testing.T) { } taskIDs := []int64{49} - err := addRelatedTasksToTasks(s, taskIDs, taskMap, u) + err := addRelatedTasksToTasks(s, taskIDs, taskMap, u, nil) require.NoError(t, err) foundTask50 := false @@ -117,7 +117,7 @@ func TestAddRelatedTasksToTasks_FiltersInaccessibleProjects(t *testing.T) { } taskIDs := []int64{1} - err := addRelatedTasksToTasks(s, taskIDs, taskMap, u) + err := addRelatedTasksToTasks(s, taskIDs, taskMap, u, nil) require.NoError(t, err) // Task 29 is in project 1 (same project, user 1 has access) — should be present diff --git a/pkg/models/task_relation_test.go b/pkg/models/task_relation_test.go index 06c1ca65f..7862be13a 100644 --- a/pkg/models/task_relation_test.go +++ b/pkg/models/task_relation_test.go @@ -24,6 +24,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "xorm.io/xorm" ) func TestTaskRelation_Create(t *testing.T) { @@ -422,32 +423,84 @@ func TestTaskRelation_CanCreate(t *testing.T) { } func TestTaskRelation_SubtaskOrder(t *testing.T) { - // Clients render subtasks in the order the api returns them, so the order must - // follow relation creation and not whatever the database hands back. - // SQLite returns insertion order even without ORDER BY, so this only catches regressions on MySQL/Postgres. - db.LoadAndAssertFixtures(t) - s := db.NewSession() - defer s.Close() - u := &user.User{ID: 1} subtaskIDs := []int64{12, 11, 10, 9, 8} - for _, subtaskID := range subtaskIDs { - rel := TaskRelation{ - TaskID: 2, - OtherTaskID: subtaskID, - RelationKind: RelationKindSubtask, + + // Relations are created in an order that does not match the subtask ids, so an + // accidental sort by task id can't pass these. + seed := func(t *testing.T, s *xorm.Session) { + for _, subtaskID := range subtaskIDs { + rel := TaskRelation{ + TaskID: 2, + OtherTaskID: subtaskID, + RelationKind: RelationKindSubtask, + } + require.NoError(t, rel.Create(s, u)) } - require.NoError(t, rel.Create(s, u)) } - taskMap := map[int64]*Task{ - 2: {ID: 2, RelatedTasks: map[RelationKind][]*Task{}}, + readSubtaskIDs := func(t *testing.T, s *xorm.Session, view *ProjectView) (ids []int64) { + taskMap := map[int64]*Task{ + 2: {ID: 2, RelatedTasks: map[RelationKind][]*Task{}}, + } + require.NoError(t, addRelatedTasksToTasks(s, []int64{2}, taskMap, u, view)) + for _, subtask := range taskMap[2].RelatedTasks[RelationKindSubtask] { + ids = append(ids, subtask.ID) + } + return ids } - require.NoError(t, addRelatedTasksToTasks(s, []int64{2}, taskMap, u)) - var got []int64 - for _, subtask := range taskMap[2].RelatedTasks[RelationKindSubtask] { - got = append(got, subtask.ID) - } - assert.Equal(t, subtaskIDs, got) + t.Run("sorted by their position in the view", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + seed(t, s) + + // Deliberately the reverse of the relation creation order + for i, subtaskID := range subtaskIDs { + _, err := s.Insert(&TaskPosition{ + TaskID: subtaskID, + ProjectViewID: 1, + Position: float64(len(subtaskIDs)-i) * 100, + }) + require.NoError(t, err) + } + + view, err := GetProjectViewByID(s, 1) + require.NoError(t, err) + + reversed := make([]int64, 0, len(subtaskIDs)) + for i := len(subtaskIDs) - 1; i >= 0; i-- { + reversed = append(reversed, subtaskIDs[i]) + } + assert.Equal(t, reversed, readSubtaskIDs(t, s, view)) + }) + + t.Run("falls back to relation order without a view", func(t *testing.T) { + // The task detail page, CalDAV and exports read tasks without a view, so there + // are no positions to sort by. Relation id keeps that stable. + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + seed(t, s) + + assert.Equal(t, subtaskIDs, readSubtaskIDs(t, s, nil)) + }) + + t.Run("sorts subtasks without a position in the view last", func(t *testing.T) { + // Subtasks living in another project have no position row for this view. + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + seed(t, s) + + _, err := s.Insert(&TaskPosition{TaskID: 9, ProjectViewID: 1, Position: 100}) + require.NoError(t, err) + + view, err := GetProjectViewByID(s, 1) + require.NoError(t, err) + + // 9 is positioned so it comes first, the rest keep their relation order + assert.Equal(t, []int64{9, 12, 11, 10, 8}, readSubtaskIDs(t, s, view)) + }) } diff --git a/pkg/models/tasks.go b/pkg/models/tasks.go index 7af2328ba..b623c6764 100644 --- a/pkg/models/tasks.go +++ b/pkg/models/tasks.go @@ -572,10 +572,10 @@ func getTaskReminderMap(s *xorm.Session, taskIDs []int64) (taskReminders map[int return } -func addRelatedTasksToTasks(s *xorm.Session, taskIDs []int64, taskMap map[int64]*Task, a web.Auth) (err error) { +func addRelatedTasksToTasks(s *xorm.Session, taskIDs []int64, taskMap map[int64]*Task, a web.Auth, view *ProjectView) (err error) { relatedTasks := []*TaskRelation{} - // Ordered by id so clients rendering subtasks get them in the order the relations - // were created in, instead of whatever order the database happens to return. + // Ordered by id so the relations have a stable order to fall back to when they + // cannot be sorted by position - see sortRelatedTasksByPosition. err = s.In("task_id", taskIDs).OrderBy("id ASC").Find(&relatedTasks) if err != nil { return @@ -627,7 +627,53 @@ func addRelatedTasksToTasks(s *xorm.Session, taskIDs []int64, taskMap map[int64] taskMap[rt.TaskID].RelatedTasks[rt.RelationKind] = append(taskMap[rt.TaskID].RelatedTasks[rt.RelationKind], otherTask) } - return + return sortRelatedTasksByPosition(s, taskMap, relatedTaskIDs, view) +} + +// sortRelatedTasksByPosition orders every task's related tasks by the position they +// have in the view being read, so clients rendering subtasks under their parent show +// them in the same order as the view itself. +// +// Not every related task has a position to sort by: there is no view at all on the +// task detail page, in CalDAV and in exports, and a related task living in another +// project has no row for this view. Those keep the relation order they were fetched +// in (by relation id) and sort after the ones that do have a position. +func sortRelatedTasksByPosition(s *xorm.Session, taskMap map[int64]*Task, relatedTaskIDs []int64, view *ProjectView) (err error) { + if view == nil { + return nil + } + + positions := []*TaskPosition{} + err = s.In("task_id", relatedTaskIDs). + And("project_view_id = ?", view.ID). + Find(&positions) + if err != nil { + return err + } + if len(positions) == 0 { + return nil + } + + positionByTask := make(map[int64]float64, len(positions)) + for _, p := range positions { + positionByTask[p.TaskID] = p.Position + } + + for _, task := range taskMap { + for _, related := range task.RelatedTasks { + // Stable so tasks without a position keep their relation id order + sort.SliceStable(related, func(i, j int) bool { + a, hasA := positionByTask[related[i].ID] + b, hasB := positionByTask[related[j].ID] + if hasA != hasB { + return hasA + } + return hasA && a < b + }) + } + } + + return nil } func addBucketsToTasks(s *xorm.Session, a web.Auth, taskIDs []int64, taskMap map[int64]*Task) (err error) { @@ -817,7 +863,7 @@ func addMoreInfoToTasks(s *xorm.Session, taskMap map[int64]*Task, a web.Auth, vi } // Get all related tasks - err = addRelatedTasksToTasks(s, taskIDs, taskMap, a) + err = addRelatedTasksToTasks(s, taskIDs, taskMap, a, view) return }