Merge 41cba664c8 into 076cd214fe

pkg/models/saved_filter_positions_test.go

@@ -313,3 +313,61 @@ func TestIssue724_SortingOnFilteredViews(t *testing.T) {
 	assert.Zero(t, zeroCount,
 		"No position=0 records should exist in database for view %d", view.ID)
 }
+
+// A task that starts matching a saved filter has no position row in that filter's
+// view yet. The fetch-time safety net creates one at the top, but it runs after the
+// query has already ordered the page. With NULLS LAST this made the new task appear
+// last on the first load and jump to the top only after a refresh. It must instead
+// stay at the top on the very first fetch and not move on subsequent fetches.
+func TestSavedFilterNewTaskStaysAtTopAcrossFetches(t *testing.T) {
+	db.LoadAndAssertFixtures(t)
+	s := db.NewSession()
+	defer s.Close()
+
+	u := &user.User{ID: 1}
+
+	sf := &SavedFilter{
+		Title:   "open-tasks-position",
+		Filters: &TaskCollection{Filter: "done = false"},
+	}
+	require.NoError(t, sf.Create(s, u))
+
+	listView := &ProjectView{}
+	exists, err := s.Where("project_id = ? AND view_kind = ?",
+		getProjectIDFromSavedFilterID(sf.ID), ProjectViewKindList).Get(listView)
+	require.NoError(t, err)
+	require.True(t, exists)
+
+	// Give every currently-matching task a position so the new task below is the
+	// only one without one (this is the state after the cron / a previous fetch).
+	require.NoError(t, RecalculateTaskPositions(s, listView, u))
+
+	newTask := &Task{Title: "freshly created open task", ProjectID: 1}
+	require.NoError(t, newTask.Create(s, u))
+
+	indexOfNewTask := func() int {
+		tc := &TaskCollection{
+			ProjectID:     getProjectIDFromSavedFilterID(sf.ID),
+			ProjectViewID: listView.ID,
+			SortBy:        []string{"position"},
+			OrderBy:       []string{"asc"},
+		}
+		result, _, _, err := tc.ReadAll(s, u, "", 1, 1000)
+		require.NoError(t, err)
+		tasks, ok := result.([]*Task)
+		require.True(t, ok)
+		for i, task := range tasks {
+			if task.ID == newTask.ID {
+				return i
+			}
+		}
+		t.Fatalf("new task %d not found in filter results", newTask.ID)
+		return -1
+	}
+
+	first := indexOfNewTask()
+	second := indexOfNewTask()
+
+	assert.Equal(t, 0, first, "newly matching task must appear at the top on the first fetch, not only after a refresh")
+	assert.Equal(t, first, second, "task order must be stable across fetches")
+}

pkg/models/task_search.go

@@ -137,11 +137,17 @@ func getOrderByDBStatement(opts *taskSearchOptions) (orderby string, err error)
 			prefix = "tasks."
 		}
 
+		nullsFirst := opts.isSavedFilter && param.sortBy == taskPropertyPosition
+
 		// Mysql sorts columns with null values before ones without null value.
 		// Because it does not have support for NULLS FIRST or NULLS LAST we work around this by
 		// first sorting for null (or not null) values and then the order we actually want to.
 		if db.Type() == schemas.MYSQL {
-			orderby += prefix + "`" + param.sortBy + "` IS NULL, "
+			nullSort := ""
+			if nullsFirst {
+				nullSort = " DESC"
+			}
+			orderby += prefix + "`" + param.sortBy + "` IS NULL" + nullSort + ", "
 		}
 
 		orderby += prefix + "`" + param.sortBy + "` " + param.orderBy.String()
@@ -149,7 +155,11 @@ func getOrderByDBStatement(opts *taskSearchOptions) (orderby string, err error)
 		// Postgres and sqlite allow us to control how columns with null values are sorted.
 		// To make that consistent with the sort order we have and other dbms, we're adding a separate clause here.
 		if db.Type() == schemas.POSTGRES || db.Type() == schemas.SQLITE {
-			orderby += " NULLS LAST"
+			if nullsFirst {
+				orderby += " NULLS FIRST"
+			} else {
+				orderby += " NULLS LAST"
+			}
 		}
 
 		if (i + 1) < len(opts.sortby) {

pkg/models/tasks.go

@@ -720,6 +720,10 @@ func addMoreInfoToTasks(s *xorm.Session, taskMap map[int64]*Task, a web.Auth, vi
 				}
 			}
 
+			sort.Slice(tasksNeedingPositions, func(i, j int) bool {
+				return tasksNeedingPositions[i].ID < tasksNeedingPositions[j].ID
+			})
+
 			if len(tasksNeedingPositions) > 0 {
 				// Create positions for tasks that don't have them
 				if err = createPositionsForTasksInView(s, tasksNeedingPositions, view, a); err != nil {