diff --git a/pkg/models/saved_filter_positions_test.go b/pkg/models/saved_filter_positions_test.go index 3fe719b16..d233460d2 100644 --- a/pkg/models/saved_filter_positions_test.go +++ b/pkg/models/saved_filter_positions_test.go @@ -91,3 +91,216 @@ func TestCronInsertsNonZeroPosition(t *testing.T) { require.True(t, exists) assert.NotZero(t, tp.Position) } + +func TestCronCreatesNonZeroPositions(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + // Create a saved filter + sf := &SavedFilter{ + Title: "cron-test-filter", + Filters: &TaskCollection{Filter: "done = false"}, + } + u := &user.User{ID: 1} + err := sf.Create(s, u) + require.NoError(t, err) + + // Get the kanban view for this filter + view := &ProjectView{} + exists, err := s.Where("project_id = ? AND view_kind = ?", + getProjectIDFromSavedFilterID(sf.ID), ProjectViewKindKanban).Get(view) + require.NoError(t, err) + require.True(t, exists) + + // Simulate what the cron does: call RecalculateTaskPositions + err = RecalculateTaskPositions(s, view, u) + require.NoError(t, err) + + // Verify no positions are 0 + zeroCount, err := s.Where("project_view_id = ? AND position = 0", view.ID).Count(&TaskPosition{}) + require.NoError(t, err) + assert.Zero(t, zeroCount, "No positions should be 0") + + // Verify all tasks have positions + positionCount, err := s.Where("project_view_id = ?", view.ID).Count(&TaskPosition{}) + require.NoError(t, err) + assert.NotZero(t, positionCount, "Should have positions") +} + +func TestFilterUpdateCreatesNonZeroPositions(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + // Create a saved filter + sf := &SavedFilter{ + Title: "update-test-filter", + Filters: &TaskCollection{Filter: "done = false"}, + } + u := &user.User{ID: 1} + err := sf.Create(s, u) + require.NoError(t, err) + + // Update the filter (this triggers position creation) + err = sf.Update(s, u) + require.NoError(t, err) + + // Get the kanban view + view := &ProjectView{} + exists, err := s.Where("project_id = ? AND view_kind = ?", + getProjectIDFromSavedFilterID(sf.ID), ProjectViewKindKanban).Get(view) + require.NoError(t, err) + require.True(t, exists) + + // Verify no positions are 0 + zeroCount, err := s.Where("project_view_id = ? AND position = 0", view.ID).Count(&TaskPosition{}) + require.NoError(t, err) + assert.Zero(t, zeroCount, "No positions should be 0 after filter update") +} + +func TestMultipleNewTasksGetDistinctPositions(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + // Create a saved filter that matches multiple tasks + sf := &SavedFilter{ + Title: "multi-task-filter", + Filters: &TaskCollection{Filter: "done = false"}, + } + u := &user.User{ID: 1} + err := sf.Create(s, u) + require.NoError(t, err) + + err = sf.Update(s, u) + require.NoError(t, err) + + // Get the kanban view + view := &ProjectView{} + exists, err := s.Where("project_id = ? AND view_kind = ?", + getProjectIDFromSavedFilterID(sf.ID), ProjectViewKindKanban).Get(view) + require.NoError(t, err) + require.True(t, exists) + + // Get all positions + var positions []*TaskPosition + err = s.Where("project_view_id = ?", view.ID).Find(&positions) + require.NoError(t, err) + + // Verify all positions are unique + seen := make(map[float64]int64) + for _, p := range positions { + if existingTaskID, exists := seen[p.Position]; exists { + t.Errorf("Position %f is duplicated between tasks %d and %d", + p.Position, existingTaskID, p.TaskID) + } + seen[p.Position] = p.TaskID + } +} + +func TestTaskFetchCreatesPositionsOnDemand(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + u := &user.User{ID: 1} + + // Create a saved filter + sf := &SavedFilter{ + Title: "on-demand-position-filter", + Filters: &TaskCollection{Filter: "done = false"}, + } + err := sf.Create(s, u) + require.NoError(t, err) + + // Get the list view for this filter + view := &ProjectView{} + exists, err := s.Where("project_id = ? AND view_kind = ?", + getProjectIDFromSavedFilterID(sf.ID), ProjectViewKindList).Get(view) + require.NoError(t, err) + require.True(t, exists) + + // Delete any existing positions to simulate a fresh state (before cron runs) + _, err = s.Where("project_view_id = ?", view.ID).Delete(&TaskPosition{}) + require.NoError(t, err) + + // Verify NO positions exist now + existingCount, err := s.Where("project_view_id = ?", view.ID).Count(&TaskPosition{}) + require.NoError(t, err) + assert.Zero(t, existingCount, "No positions should exist after deletion") + + // Fetch tasks for the view - this should trigger on-demand position creation + tc := &TaskCollection{ + ProjectID: view.ProjectID, + ProjectViewID: view.ID, + } + result, _, _, err := tc.ReadAll(s, u, "", 1, 50) + require.NoError(t, err) + + tasks := result.([]*Task) + require.NotEmpty(t, tasks, "Should have tasks matching the filter") + + // Verify all returned tasks have non-zero positions + for _, task := range tasks { + assert.NotZero(t, task.Position, + "Task %d (%s) should have non-zero position", task.ID, task.Title) + } + + // Verify positions were created in database + createdCount, err := s.Where("project_view_id = ?", view.ID).Count(&TaskPosition{}) + require.NoError(t, err) + assert.NotZero(t, createdCount, "Positions should have been created") + + // Verify no zero positions + zeroCount, err := s.Where("project_view_id = ? AND position = 0", view.ID).Count(&TaskPosition{}) + require.NoError(t, err) + assert.Zero(t, zeroCount, "No positions should be zero") +} + +func TestIssue724_SortingOnFilteredViews(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + u := &user.User{ID: 1} + + // Create a saved filter + sf := &SavedFilter{ + Title: "issue-724-filter", + Filters: &TaskCollection{Filter: "done = false"}, + } + err := sf.Create(s, u) + require.NoError(t, err) + + // Get the list view for this filter + view := &ProjectView{} + exists, err := s.Where("project_id = ? AND view_kind = ?", + getProjectIDFromSavedFilterID(sf.ID), ProjectViewKindList).Get(view) + require.NoError(t, err) + require.True(t, exists) + + // Fetch tasks for the view (simulating what the API does) + tc := &TaskCollection{ + ProjectID: view.ProjectID, + ProjectViewID: view.ID, + } + + // This should trigger position creation + result, _, _, err := tc.ReadAll(s, u, "", 1, 50) + require.NoError(t, err) + + tasks := result.([]*Task) + + // Verify all returned tasks have non-zero positions + for _, task := range tasks { + assert.NotZero(t, task.Position, + "Task %d (%s) should have non-zero position", task.ID, task.Title) + } + + // Verify positions in database are all non-zero + zeroCount, err := s.Where("project_view_id = ? AND position = 0", view.ID).Count(&TaskPosition{}) + require.NoError(t, err) + assert.Zero(t, zeroCount, + "No position=0 records should exist in database for view %d", view.ID) +} diff --git a/pkg/models/saved_filters.go b/pkg/models/saved_filters.go index f88efa863..ef38068be 100644 --- a/pkg/models/saved_filters.go +++ b/pkg/models/saved_filters.go @@ -263,19 +263,12 @@ func (sf *SavedFilter) Update(s *xorm.Session, _ web.Auth) error { } taskBuckets := make([]*TaskBucket, 0, len(tasksToAdd)) - taskPositions := make([]*TaskPosition, 0, len(tasksToAdd)) for _, task := range tasksToAdd { taskBuckets = append(taskBuckets, &TaskBucket{ TaskID: task.ID, BucketID: bucketID, ProjectViewID: view.ID, }) - - taskPositions = append(taskPositions, &TaskPosition{ - TaskID: task.ID, - ProjectViewID: view.ID, - Position: 0, - }) } if len(taskBuckets) > 0 { @@ -284,11 +277,9 @@ func (sf *SavedFilter) Update(s *xorm.Session, _ web.Auth) error { } } - if len(taskPositions) > 0 { - if _, err = s.Insert(taskPositions); err != nil { - return err - } - + // Recalculate positions for all tasks - this will create positions for + // new tasks that don't have them yet + if len(tasksToAdd) > 0 { if err = RecalculateTaskPositions(s, view, &user.User{ID: sf.OwnerID}); err != nil { return err } @@ -504,13 +495,8 @@ func RegisterAddTaskToFilterViewCron() { newTaskBuckets = append(newTaskBuckets, tb) } if _, exists := savedTaskPositionMap[task.ID]; !exists { - tp := &TaskPosition{ - TaskID: task.ID, - ProjectViewID: view.ID, - Position: 0, - } - newTaskPositions = append(newTaskPositions, tp) - + // Mark view for recalculation - RecalculateTaskPositions will create + // positions for all tasks including new ones if _, ok := viewsToRecalc[view.ID]; !ok { viewsToRecalc[view.ID] = struct { view *ProjectView diff --git a/pkg/models/task_position.go b/pkg/models/task_position.go index aa7e82f03..c6e291eb3 100644 --- a/pkg/models/task_position.go +++ b/pkg/models/task_position.go @@ -272,3 +272,43 @@ func DeleteOrphanedTaskPositions(s *xorm.Session) (count int64, err error) { Where("task_id not in (select id from tasks) OR project_view_id not in (select id from project_views)"). Delete(&TaskPosition{}) } + +// createPositionsForTasksInView creates position records for tasks that don't have them. +// Used as a safety net during task fetching for saved filter views. +func createPositionsForTasksInView(s *xorm.Session, tasks []*Task, view *ProjectView, a web.Auth) error { + if len(tasks) == 0 { + return nil + } + + // Get the current lowest position to place new tasks at the top + lowestPosition := &TaskPosition{} + has, err := s. + Where("project_view_id = ?", view.ID). + OrderBy("position asc"). + Get(lowestPosition) + if err != nil { + return err + } + + var basePosition float64 + if !has || lowestPosition.Position == 0 { + // No existing positions or all are zero - trigger full recalculation + return RecalculateTaskPositions(s, view, a) + } + + // Place new tasks before the lowest position, evenly spaced + basePosition = lowestPosition.Position + spacing := basePosition / float64(len(tasks)+1) + + newPositions := make([]*TaskPosition, 0, len(tasks)) + for i, task := range tasks { + newPositions = append(newPositions, &TaskPosition{ + TaskID: task.ID, + ProjectViewID: view.ID, + Position: spacing * float64(i+1), + }) + } + + _, err = s.Insert(&newPositions) + return err +} diff --git a/pkg/models/tasks.go b/pkg/models/tasks.go index e72b43e6d..39d157c63 100644 --- a/pkg/models/tasks.go +++ b/pkg/models/tasks.go @@ -681,6 +681,35 @@ func addMoreInfoToTasks(s *xorm.Session, taskMap map[int64]*Task, a web.Auth, vi for _, position := range positions { positionsMap[position.TaskID] = position } + + // For saved filter views, ensure all tasks have positions + // This is a safety net - the cron job handles bulk position creation, + // but we need immediate positions for newly matching tasks + if GetSavedFilterIDFromProjectID(view.ProjectID) > 0 { + tasksNeedingPositions := make([]*Task, 0) + for _, task := range taskMap { + if _, hasPosition := positionsMap[task.ID]; !hasPosition { + tasksNeedingPositions = append(tasksNeedingPositions, task) + } + } + + if len(tasksNeedingPositions) > 0 { + // Create positions for tasks that don't have them + if err = createPositionsForTasksInView(s, tasksNeedingPositions, view, a); err != nil { + return err + } + + // Reload positions after creation + positions, err = getPositionsForView(s, view) + if err != nil { + return err + } + positionsMap = make(map[int64]*TaskPosition, len(positions)) + for _, p := range positions { + positionsMap[p.TaskID] = p + } + } + } } var reactions map[int64]ReactionMap