mirror of
https://github.com/go-vikunja/vikunja.git
synced 2026-02-01 22:47:40 +00:00
fix(positions): detect and repair duplicate task positions automatically (#1998)
Relates to: https://community.vikunja.io/t/reordering-not-possible-position-value-the-same-for-different-tasks/4078 Duplicate positions can occur due to race conditions or historical bugs, causing tasks to appear in the wrong order or jump around when the page is refreshed. This change adds a `repair-task-positions` CLI command to detect and resolve task position conflicts, with dry-run preview option. Also implemented automatic conflict detection and resolution to ensure unique task positions. 🐰 Positions once conflicted, clustered tight, But now we nudge them back into the light! MinSpacing guards precision from decay, While conflicts heal and duplicates give way. ✨
This commit is contained in:
@@ -1,21 +1,31 @@
|
||||
// Minimum spacing between positions. Must survive JSON round-trip.
|
||||
// Matches backend MinPositionSpacing constant.
|
||||
const MIN_POSITION_SPACING = 0.01
|
||||
|
||||
export const calculateItemPosition = (
|
||||
positionBefore: number | null = null,
|
||||
positionAfter: number | null = null,
|
||||
): number => {
|
||||
// Both neighbors have the same position (conflict)
|
||||
if (positionBefore !== null && positionAfter !== null && positionBefore === positionAfter) {
|
||||
// Nudge slightly above to maintain ordering intent
|
||||
return positionAfter + MIN_POSITION_SPACING
|
||||
}
|
||||
|
||||
if (positionBefore === null) {
|
||||
if (positionAfter === null) {
|
||||
return 0
|
||||
}
|
||||
|
||||
// If there is no task after it, we just add 2^16 to the last position to have enough room in the future
|
||||
|
||||
// If there is no task before it, place it at half the position of the task after
|
||||
return positionAfter / 2
|
||||
}
|
||||
|
||||
|
||||
// If there is no task after it, we just add 2^16 to the last position to have enough room in the future
|
||||
if (positionAfter === null) {
|
||||
return positionBefore + Math.pow(2, 16)
|
||||
}
|
||||
|
||||
|
||||
// If we have both a task before and after it, we actually calculate the position
|
||||
return positionBefore + (positionAfter - positionBefore) / 2
|
||||
}
|
||||
|
||||
@@ -1,20 +1,44 @@
|
||||
import {it, expect} from 'vitest'
|
||||
import {describe, it, expect} from 'vitest'
|
||||
|
||||
import {calculateItemPosition} from './calculateItemPosition'
|
||||
|
||||
it('should calculate the task position', () => {
|
||||
const result = calculateItemPosition(10, 100)
|
||||
expect(result).toBe(55)
|
||||
})
|
||||
it('should return 0 if no position was provided', () => {
|
||||
const result = calculateItemPosition(null, null)
|
||||
expect(result).toBe(0)
|
||||
})
|
||||
it('should calculate the task position for the first task', () => {
|
||||
const result = calculateItemPosition(null, 100)
|
||||
expect(result).toBe(50)
|
||||
})
|
||||
it('should calculate the task position for the last task', () => {
|
||||
const result = calculateItemPosition(10, null)
|
||||
expect(result).toBe(65546)
|
||||
describe('calculateItemPosition', () => {
|
||||
it('should calculate the task position', () => {
|
||||
const result = calculateItemPosition(10, 100)
|
||||
expect(result).toBe(55)
|
||||
})
|
||||
|
||||
it('should return 0 if no position was provided', () => {
|
||||
const result = calculateItemPosition(null, null)
|
||||
expect(result).toBe(0)
|
||||
})
|
||||
|
||||
it('should calculate the task position for the first task', () => {
|
||||
const result = calculateItemPosition(null, 100)
|
||||
expect(result).toBe(50)
|
||||
})
|
||||
|
||||
it('should calculate the task position for the last task', () => {
|
||||
const result = calculateItemPosition(10, null)
|
||||
expect(result).toBe(65546)
|
||||
})
|
||||
|
||||
it('should handle equal positions (conflict) by nudging above', () => {
|
||||
const result = calculateItemPosition(100, 100)
|
||||
expect(result).toBeGreaterThan(100)
|
||||
expect(result).toBeLessThan(101)
|
||||
})
|
||||
|
||||
it('should handle equal positions at zero', () => {
|
||||
const result = calculateItemPosition(0, 0)
|
||||
expect(result).toBeGreaterThan(0)
|
||||
})
|
||||
|
||||
it('should preserve precision after JSON round-trip', () => {
|
||||
const position = calculateItemPosition(100, 100)
|
||||
const serialized = JSON.stringify(position)
|
||||
const deserialized = JSON.parse(serialized)
|
||||
expect(deserialized).toBe(position)
|
||||
expect(deserialized).toBeGreaterThan(100)
|
||||
})
|
||||
})
|
||||
|
||||
102
pkg/cmd/repair_task_positions.go
Normal file
102
pkg/cmd/repair_task_positions.go
Normal file
@@ -0,0 +1,102 @@
|
||||
// Vikunja is a to-do list application to facilitate your life.
|
||||
// Copyright 2018-present Vikunja and contributors. All rights reserved.
|
||||
//
|
||||
// This program is free software: you can redistribute it and/or modify
|
||||
// it under the terms of the GNU Affero General Public License as published by
|
||||
// the Free Software Foundation, either version 3 of the License, or
|
||||
// (at your option) any later version.
|
||||
//
|
||||
// This program is distributed in the hope that it will be useful,
|
||||
// but WITHOUT ANY WARRANTY; without even the implied warranty of
|
||||
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
|
||||
// GNU Affero General Public License for more details.
|
||||
//
|
||||
// You should have received a copy of the GNU Affero General Public License
|
||||
// along with this program. If not, see <https://www.gnu.org/licenses/>.
|
||||
|
||||
package cmd
|
||||
|
||||
import (
|
||||
"code.vikunja.io/api/pkg/db"
|
||||
"code.vikunja.io/api/pkg/initialize"
|
||||
"code.vikunja.io/api/pkg/log"
|
||||
"code.vikunja.io/api/pkg/models"
|
||||
|
||||
"github.com/spf13/cobra"
|
||||
)
|
||||
|
||||
func init() {
|
||||
repairTaskPositionsCmd.Flags().Bool("dry-run", false, "Preview repairs without making changes")
|
||||
rootCmd.AddCommand(repairTaskPositionsCmd)
|
||||
}
|
||||
|
||||
var repairTaskPositionsCmd = &cobra.Command{
|
||||
Use: "repair-task-positions",
|
||||
Short: "Detect and repair duplicate task positions across all views",
|
||||
Long: `Scans all project views for tasks with duplicate position values and repairs them.
|
||||
|
||||
Duplicate positions can occur due to race conditions or historical bugs, causing
|
||||
tasks to appear in the wrong order or jump around when the page is refreshed.
|
||||
|
||||
This command will:
|
||||
1. Scan all project views for duplicate positions
|
||||
2. Attempt localized repair by redistributing conflicting tasks
|
||||
3. Fall back to full view recalculation if localized repair fails
|
||||
|
||||
Use --dry-run to preview what would be fixed without making changes.`,
|
||||
PreRun: func(_ *cobra.Command, _ []string) {
|
||||
initialize.FullInitWithoutAsync()
|
||||
},
|
||||
Run: func(cmd *cobra.Command, _ []string) {
|
||||
dryRun, _ := cmd.Flags().GetBool("dry-run")
|
||||
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
if dryRun {
|
||||
log.Infof("Running in dry-run mode - no changes will be made")
|
||||
} else {
|
||||
if err := s.Begin(); err != nil {
|
||||
log.Errorf("Failed to start transaction: %s", err)
|
||||
return
|
||||
}
|
||||
defer func() {
|
||||
// Rollback is a no-op if commit already succeeded
|
||||
_ = s.Rollback()
|
||||
}()
|
||||
}
|
||||
|
||||
result, err := models.RepairTaskPositions(s, dryRun)
|
||||
if err != nil {
|
||||
log.Errorf("Failed to repair task positions: %s", err)
|
||||
return
|
||||
}
|
||||
|
||||
if !dryRun {
|
||||
if err := s.Commit(); err != nil {
|
||||
log.Errorf("Failed to commit changes: %s", err)
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
// Print summary
|
||||
log.Infof("Repair complete:")
|
||||
log.Infof(" Views scanned: %d", result.ViewsScanned)
|
||||
log.Infof(" Views repaired: %d", result.ViewsRepaired)
|
||||
log.Infof(" Tasks affected: %d", result.TasksAffected)
|
||||
if result.FullRecalcViews > 0 {
|
||||
log.Infof(" Views requiring full recalculation: %d", result.FullRecalcViews)
|
||||
}
|
||||
|
||||
if len(result.Errors) > 0 {
|
||||
log.Errorf("Errors encountered (%d):", len(result.Errors))
|
||||
for _, e := range result.Errors {
|
||||
log.Errorf(" - %s", e)
|
||||
}
|
||||
}
|
||||
|
||||
if result.ViewsRepaired == 0 && len(result.Errors) == 0 {
|
||||
log.Infof("No position conflicts found - all views are healthy!")
|
||||
}
|
||||
},
|
||||
}
|
||||
@@ -1192,6 +1192,34 @@ func (err ErrInvalidTaskColumn) HTTPError() web.HTTPError {
|
||||
}
|
||||
}
|
||||
|
||||
// ErrNeedsFullRecalculation represents an error where localized position repair cannot proceed
|
||||
// and the entire view must be recalculated.
|
||||
type ErrNeedsFullRecalculation struct {
|
||||
ProjectViewID int64
|
||||
}
|
||||
|
||||
// IsErrNeedsFullRecalculation checks if an error is ErrNeedsFullRecalculation.
|
||||
func IsErrNeedsFullRecalculation(err error) bool {
|
||||
_, ok := err.(*ErrNeedsFullRecalculation)
|
||||
return ok
|
||||
}
|
||||
|
||||
func (err *ErrNeedsFullRecalculation) Error() string {
|
||||
return fmt.Sprintf("Insufficient spacing for localized repair [ProjectViewID: %d]", err.ProjectViewID)
|
||||
}
|
||||
|
||||
// ErrCodeNeedsFullRecalculation holds the unique world-error code of this error
|
||||
const ErrCodeNeedsFullRecalculation = 4028
|
||||
|
||||
// HTTPError holds the http error description
|
||||
func (err *ErrNeedsFullRecalculation) HTTPError() web.HTTPError {
|
||||
return web.HTTPError{
|
||||
HTTPCode: http.StatusInternalServerError,
|
||||
Code: ErrCodeNeedsFullRecalculation,
|
||||
Message: "Position repair requires full view recalculation.",
|
||||
}
|
||||
}
|
||||
|
||||
// ============
|
||||
// Team errors
|
||||
// ============
|
||||
|
||||
@@ -17,7 +17,9 @@
|
||||
package models
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"math"
|
||||
"sort"
|
||||
|
||||
"xorm.io/xorm"
|
||||
|
||||
@@ -26,6 +28,9 @@ import (
|
||||
"code.vikunja.io/api/pkg/web"
|
||||
)
|
||||
|
||||
// MinPositionSpacing is the smallest gap we allow between positions.
|
||||
const MinPositionSpacing = 0.01
|
||||
|
||||
type TaskPosition struct {
|
||||
// The ID of the task this position is for
|
||||
TaskID int64 `xorm:"bigint not null index" json:"task_id" param:"task"`
|
||||
@@ -54,20 +59,20 @@ func (tp *TaskPosition) CanUpdate(s *xorm.Session, a web.Auth) (bool, error) {
|
||||
return t.CanUpdate(s, a)
|
||||
}
|
||||
|
||||
func (tp *TaskPosition) refresh(s *xorm.Session) (err error) {
|
||||
updatedPosition := &TaskPosition{}
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", tp.TaskID, tp.ProjectViewID).Get(updatedPosition)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
tp.Position = updatedPosition.Position
|
||||
return nil
|
||||
}
|
||||
|
||||
// updateTaskPosition is the internal function that performs the task position update logic
|
||||
// without dispatching events. This is used by moveTaskToDoneBuckets to avoid duplicate events.
|
||||
func updateTaskPosition(s *xorm.Session, a web.Auth, tp *TaskPosition) (err error) {
|
||||
// Update all positions if the newly saved position is < 0.1
|
||||
var shouldRecalculate bool
|
||||
var view *ProjectView
|
||||
if tp.Position < 0.1 {
|
||||
shouldRecalculate = true
|
||||
view, err = GetProjectViewByID(s, tp.ProjectViewID)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
exists, err := s.
|
||||
Where("task_id = ? AND project_view_id = ?", tp.TaskID, tp.ProjectViewID).
|
||||
Exist(&TaskPosition{})
|
||||
@@ -80,22 +85,42 @@ func updateTaskPosition(s *xorm.Session, a web.Auth, tp *TaskPosition) (err erro
|
||||
if err != nil {
|
||||
return
|
||||
}
|
||||
if shouldRecalculate {
|
||||
return RecalculateTaskPositions(s, view, a)
|
||||
} else {
|
||||
_, err = s.
|
||||
Where("task_id = ? AND project_view_id = ?", tp.TaskID, tp.ProjectViewID).
|
||||
Cols("project_view_id", "position").
|
||||
Update(tp)
|
||||
if err != nil {
|
||||
return
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
_, err = s.
|
||||
Where("task_id = ? AND project_view_id = ?", tp.TaskID, tp.ProjectViewID).
|
||||
Cols("project_view_id", "position").
|
||||
Update(tp)
|
||||
if tp.Position < MinPositionSpacing {
|
||||
view, err := GetProjectViewByID(s, tp.ProjectViewID)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
err = RecalculateTaskPositions(s, view, a)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
return tp.refresh(s)
|
||||
}
|
||||
|
||||
// Check for and resolve position conflicts
|
||||
conflicts, err := findPositionConflicts(s, tp.ProjectViewID, tp.Position)
|
||||
if err != nil {
|
||||
return
|
||||
return err
|
||||
}
|
||||
|
||||
if shouldRecalculate {
|
||||
return RecalculateTaskPositions(s, view, a)
|
||||
if len(conflicts) > 1 {
|
||||
err = resolveTaskPositionConflicts(s, tp.ProjectViewID, conflicts)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
return tp.refresh(s)
|
||||
}
|
||||
|
||||
return nil
|
||||
@@ -231,6 +256,57 @@ func getPositionsForView(s *xorm.Session, view *ProjectView) (positions []*TaskP
|
||||
return
|
||||
}
|
||||
|
||||
// recalculateTaskPositionsForRepair recalculates positions for all tasks in a view
|
||||
// without requiring auth. Used by CLI repair when localized repair fails.
|
||||
// Unlike RecalculateTaskPositions, this only operates on tasks that already have
|
||||
// positions in the view (doesn't discover new tasks).
|
||||
func recalculateTaskPositionsForRepair(s *xorm.Session, view *ProjectView) error {
|
||||
log.Debugf("Recalculating task positions for view %d (repair mode)", view.ID)
|
||||
|
||||
// Get all existing positions for this view, ordered by current position then task ID
|
||||
var existingPositions []*TaskPosition
|
||||
err := s.Where("project_view_id = ?", view.ID).
|
||||
OrderBy("position ASC, task_id ASC").
|
||||
Find(&existingPositions)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
if len(existingPositions) == 0 {
|
||||
return nil
|
||||
}
|
||||
|
||||
// Delete all existing positions
|
||||
_, err = s.Where("project_view_id = ?", view.ID).Delete(&TaskPosition{})
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Reassign evenly spaced positions
|
||||
maxPosition := math.Pow(2, 32)
|
||||
newPositions := make([]*TaskPosition, 0, len(existingPositions))
|
||||
|
||||
for i, pos := range existingPositions {
|
||||
currentPosition := maxPosition / float64(len(existingPositions)) * float64(i+1)
|
||||
newPositions = append(newPositions, &TaskPosition{
|
||||
TaskID: pos.TaskID,
|
||||
ProjectViewID: view.ID,
|
||||
Position: currentPosition,
|
||||
})
|
||||
}
|
||||
|
||||
count, err := s.Insert(newPositions)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
log.Debugf("Repair: inserted %d new positions for view %d", count, view.ID)
|
||||
|
||||
return events.Dispatch(&TaskPositionsRecalculatedEvent{
|
||||
NewTaskPositions: newPositions,
|
||||
})
|
||||
}
|
||||
|
||||
func calculateNewPositionForTask(s *xorm.Session, a web.Auth, t *Task, view *ProjectView) (*TaskPosition, error) {
|
||||
if t.Position == 0 {
|
||||
lowestPosition := &TaskPosition{}
|
||||
@@ -291,8 +367,7 @@ func createPositionsForTasksInView(s *xorm.Session, tasks []*Task, view *Project
|
||||
}
|
||||
|
||||
var basePosition float64
|
||||
if !has || lowestPosition.Position == 0 {
|
||||
// No existing positions or all are zero - trigger full recalculation
|
||||
if !has || lowestPosition.Position < MinPositionSpacing {
|
||||
return RecalculateTaskPositions(s, view, a)
|
||||
}
|
||||
|
||||
@@ -312,3 +387,207 @@ func createPositionsForTasksInView(s *xorm.Session, tasks []*Task, view *Project
|
||||
_, err = s.Insert(&newPositions)
|
||||
return err
|
||||
}
|
||||
|
||||
// findPositionConflicts returns all task positions that share the same position value
|
||||
// within a given project view. Returns an empty slice if no conflicts exist.
|
||||
func findPositionConflicts(s *xorm.Session, projectViewID int64, position float64) (conflicts []*TaskPosition, err error) {
|
||||
conflicts = []*TaskPosition{}
|
||||
err = s.
|
||||
Where("project_view_id = ? AND position = ?", projectViewID, position).
|
||||
Find(&conflicts)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return conflicts, nil
|
||||
}
|
||||
|
||||
// RepairResult contains the summary of a repair operation.
|
||||
type RepairResult struct {
|
||||
ViewsScanned int
|
||||
ViewsRepaired int
|
||||
TasksAffected int
|
||||
FullRecalcViews int // Views that needed full recalculation
|
||||
Errors []string // Views that couldn't be repaired
|
||||
}
|
||||
|
||||
// RepairTaskPositions scans all project views for duplicate task positions
|
||||
// and repairs them using localized conflict resolution or full recalculation.
|
||||
// If dryRun is true, it reports what would be fixed without making changes.
|
||||
func RepairTaskPositions(s *xorm.Session, dryRun bool) (*RepairResult, error) {
|
||||
result := &RepairResult{}
|
||||
|
||||
// Get all task positions in a single query
|
||||
var allPositions []*TaskPosition
|
||||
err := s.OrderBy("project_view_id ASC, position ASC").Find(&allPositions)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
if len(allPositions) == 0 {
|
||||
return result, nil
|
||||
}
|
||||
|
||||
// Group positions by view ID
|
||||
positionsByView := make(map[int64][]*TaskPosition)
|
||||
for _, pos := range allPositions {
|
||||
positionsByView[pos.ProjectViewID] = append(positionsByView[pos.ProjectViewID], pos)
|
||||
}
|
||||
|
||||
viewIDs := []int64{}
|
||||
for viewID := range positionsByView {
|
||||
viewIDs = append(viewIDs, viewID)
|
||||
}
|
||||
|
||||
viewsByID := make(map[int64]*ProjectView)
|
||||
err = s.In("id", viewIDs).Find(&viewsByID)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// Process each view
|
||||
for viewID, positions := range positionsByView {
|
||||
result.ViewsScanned++
|
||||
|
||||
// Find duplicate positions within this view's positions
|
||||
duplicates := findDuplicatesInPositions(positions)
|
||||
if len(duplicates) == 0 {
|
||||
continue
|
||||
}
|
||||
|
||||
if dryRun {
|
||||
// Count affected tasks without making changes
|
||||
for _, dup := range duplicates {
|
||||
result.TasksAffected += len(dup)
|
||||
}
|
||||
result.ViewsRepaired++
|
||||
log.Infof("[dry-run] Would repair %d position conflicts in view %d", len(duplicates), viewID)
|
||||
continue
|
||||
}
|
||||
|
||||
view, has := viewsByID[viewID]
|
||||
if !has {
|
||||
continue
|
||||
}
|
||||
|
||||
viewRepaired := false
|
||||
for _, conflicts := range duplicates {
|
||||
result.TasksAffected += len(conflicts)
|
||||
|
||||
err = resolveTaskPositionConflicts(s, viewID, conflicts)
|
||||
if IsErrNeedsFullRecalculation(err) {
|
||||
// Fall back to full recalculation for this view
|
||||
err = recalculateTaskPositionsForRepair(s, view)
|
||||
if err != nil {
|
||||
result.Errors = append(result.Errors, fmt.Sprintf("view %d: recalculation failed: %v", viewID, err))
|
||||
continue
|
||||
}
|
||||
result.FullRecalcViews++
|
||||
viewRepaired = true
|
||||
// After full recalculation, no need to process more duplicates in this view
|
||||
break
|
||||
} else if err != nil {
|
||||
result.Errors = append(result.Errors, fmt.Sprintf("view %d: %v", viewID, err))
|
||||
continue
|
||||
}
|
||||
viewRepaired = true
|
||||
}
|
||||
|
||||
if viewRepaired {
|
||||
result.ViewsRepaired++
|
||||
}
|
||||
}
|
||||
|
||||
return result, nil
|
||||
}
|
||||
|
||||
// findDuplicatesInPositions finds groups of positions that share the same position value.
|
||||
func findDuplicatesInPositions(positions []*TaskPosition) [][]*TaskPosition {
|
||||
// Group by position
|
||||
positionGroups := make(map[float64][]*TaskPosition)
|
||||
for _, tp := range positions {
|
||||
positionGroups[tp.Position] = append(positionGroups[tp.Position], tp)
|
||||
}
|
||||
|
||||
// Filter to only duplicates (groups with more than 1 task)
|
||||
var duplicates [][]*TaskPosition
|
||||
for _, group := range positionGroups {
|
||||
if len(group) > 1 {
|
||||
duplicates = append(duplicates, group)
|
||||
}
|
||||
}
|
||||
|
||||
return duplicates
|
||||
}
|
||||
|
||||
// resolveTaskPositionConflicts redistributes conflicting task positions within the
|
||||
// available gap between their neighbors. Returns ErrNeedsFullRecalculation if there
|
||||
// is insufficient spacing to assign unique positions.
|
||||
func resolveTaskPositionConflicts(s *xorm.Session, projectViewID int64, conflicts []*TaskPosition) error {
|
||||
if len(conflicts) <= 1 {
|
||||
return nil // No conflict to resolve
|
||||
}
|
||||
|
||||
conflictPosition := conflicts[0].Position
|
||||
|
||||
// Find the nearest distinct neighbor positions
|
||||
var leftNeighbor, rightNeighbor *TaskPosition
|
||||
var lowerBound, upperBound float64
|
||||
|
||||
// Get the position immediately before the conflict position
|
||||
leftNeighbor = &TaskPosition{}
|
||||
hasLeft, err := s.
|
||||
Where("project_view_id = ? AND position < ?", projectViewID, conflictPosition).
|
||||
OrderBy("position DESC").
|
||||
Get(leftNeighbor)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if hasLeft {
|
||||
lowerBound = leftNeighbor.Position
|
||||
}
|
||||
|
||||
// Get the position immediately after the conflict position
|
||||
rightNeighbor = &TaskPosition{}
|
||||
hasRight, err := s.
|
||||
Where("project_view_id = ? AND position > ?", projectViewID, conflictPosition).
|
||||
OrderBy("position ASC").
|
||||
Get(rightNeighbor)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if hasRight {
|
||||
upperBound = rightNeighbor.Position
|
||||
} else {
|
||||
upperBound = math.Pow(2, 32)
|
||||
}
|
||||
|
||||
// Calculate spacing needed
|
||||
availableGap := upperBound - lowerBound
|
||||
spacing := availableGap / float64(len(conflicts)+1)
|
||||
|
||||
// Check if we have enough spacing
|
||||
if spacing < MinPositionSpacing {
|
||||
return &ErrNeedsFullRecalculation{ProjectViewID: projectViewID}
|
||||
}
|
||||
|
||||
// Sort conflicts by task ID for deterministic ordering
|
||||
sort.Slice(conflicts, func(i, j int) bool {
|
||||
return conflicts[i].TaskID < conflicts[j].TaskID
|
||||
})
|
||||
|
||||
// Assign new positions
|
||||
for i, tp := range conflicts {
|
||||
newPosition := lowerBound + spacing*float64(i+1)
|
||||
_, err = s.
|
||||
Where("task_id = ? AND project_view_id = ?", tp.TaskID, projectViewID).
|
||||
Cols("position").
|
||||
Update(&TaskPosition{Position: newPosition})
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
log.Debugf("Repaired position conflict in view %d: %d tasks respaced from position %.6f", projectViewID, len(conflicts), conflictPosition)
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
435
pkg/models/task_position_test.go
Normal file
435
pkg/models/task_position_test.go
Normal file
@@ -0,0 +1,435 @@
|
||||
// Vikunja is a to-do list application to facilitate your life.
|
||||
// Copyright 2018-present Vikunja and contributors. All rights reserved.
|
||||
//
|
||||
// This program is free software: you can redistribute it and/or modify
|
||||
// it under the terms of the GNU Affero General Public License as published by
|
||||
// the Free Software Foundation, either version 3 of the License, or
|
||||
// (at your option) any later version.
|
||||
//
|
||||
// This program is distributed in the hope that it will be useful,
|
||||
// but WITHOUT ANY WARRANTY; without even the implied warranty of
|
||||
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
|
||||
// GNU Affero General Public License for more details.
|
||||
//
|
||||
// You should have received a copy of the GNU Affero General Public License
|
||||
// along with this program. If not, see <https://www.gnu.org/licenses/>.
|
||||
|
||||
package models
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"code.vikunja.io/api/pkg/db"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestFindPositionConflicts(t *testing.T) {
|
||||
t.Run("no conflicts", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Project view 1 has tasks at positions 2 and 4 - no conflicts
|
||||
conflicts, err := findPositionConflicts(s, 1, 2)
|
||||
require.NoError(t, err)
|
||||
assert.Len(t, conflicts, 1) // Only one task at position 2
|
||||
})
|
||||
|
||||
t.Run("finds conflicts", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Insert two tasks with the same position
|
||||
_, err := s.Insert(&TaskPosition{TaskID: 100, ProjectViewID: 1, Position: 999})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 101, ProjectViewID: 1, Position: 999})
|
||||
require.NoError(t, err)
|
||||
|
||||
conflicts, err := findPositionConflicts(s, 1, 999)
|
||||
require.NoError(t, err)
|
||||
assert.Len(t, conflicts, 2)
|
||||
})
|
||||
|
||||
t.Run("no conflicts at nonexistent position", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
conflicts, err := findPositionConflicts(s, 1, 12345)
|
||||
require.NoError(t, err)
|
||||
assert.Empty(t, conflicts)
|
||||
})
|
||||
}
|
||||
|
||||
func TestResolveTaskPositionConflicts(t *testing.T) {
|
||||
t.Run("no conflict to resolve", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Single task - no conflict
|
||||
conflicts := []*TaskPosition{
|
||||
{TaskID: 1, ProjectViewID: 1, Position: 100},
|
||||
}
|
||||
err := resolveTaskPositionConflicts(s, 1, conflicts)
|
||||
require.NoError(t, err)
|
||||
})
|
||||
|
||||
t.Run("resolves conflicts with neighbors", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Set up: Create positions at 100, 200 (conflict), 200 (conflict), 300
|
||||
_, err := s.Insert(&TaskPosition{TaskID: 100, ProjectViewID: 1, Position: 100})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 101, ProjectViewID: 1, Position: 200})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 102, ProjectViewID: 1, Position: 200})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 103, ProjectViewID: 1, Position: 300})
|
||||
require.NoError(t, err)
|
||||
|
||||
conflicts := []*TaskPosition{
|
||||
{TaskID: 101, ProjectViewID: 1, Position: 200},
|
||||
{TaskID: 102, ProjectViewID: 1, Position: 200},
|
||||
}
|
||||
|
||||
err = resolveTaskPositionConflicts(s, 1, conflicts)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Check that the positions are now different
|
||||
var pos1, pos2 TaskPosition
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", 101, 1).Get(&pos1)
|
||||
require.NoError(t, err)
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", 102, 1).Get(&pos2)
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.NotEqual(t, pos1.Position, pos2.Position)
|
||||
// Both should be between 100 and 300
|
||||
assert.Greater(t, pos1.Position, 100.0)
|
||||
assert.Less(t, pos1.Position, 300.0)
|
||||
assert.Greater(t, pos2.Position, 100.0)
|
||||
assert.Less(t, pos2.Position, 300.0)
|
||||
})
|
||||
|
||||
t.Run("resolves conflicts at start (no left neighbor)", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Clear existing positions for this view to control test data
|
||||
_, err := s.Where("project_view_id = ?", 99).Delete(&TaskPosition{})
|
||||
require.NoError(t, err)
|
||||
|
||||
// Set up: positions at 50 (conflict), 50 (conflict), 100
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 200, ProjectViewID: 99, Position: 50})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 201, ProjectViewID: 99, Position: 50})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 202, ProjectViewID: 99, Position: 100})
|
||||
require.NoError(t, err)
|
||||
|
||||
conflicts := []*TaskPosition{
|
||||
{TaskID: 200, ProjectViewID: 99, Position: 50},
|
||||
{TaskID: 201, ProjectViewID: 99, Position: 50},
|
||||
}
|
||||
|
||||
err = resolveTaskPositionConflicts(s, 99, conflicts)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Check positions are unique and between 0 and 100
|
||||
var pos1, pos2 TaskPosition
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", 200, 99).Get(&pos1)
|
||||
require.NoError(t, err)
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", 201, 99).Get(&pos2)
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.NotEqual(t, pos1.Position, pos2.Position)
|
||||
assert.GreaterOrEqual(t, pos1.Position, 0.0)
|
||||
assert.Less(t, pos1.Position, 100.0)
|
||||
assert.GreaterOrEqual(t, pos2.Position, 0.0)
|
||||
assert.Less(t, pos2.Position, 100.0)
|
||||
})
|
||||
|
||||
t.Run("resolves conflicts at end (no right neighbor)", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Clear existing positions for this view
|
||||
_, err := s.Where("project_view_id = ?", 98).Delete(&TaskPosition{})
|
||||
require.NoError(t, err)
|
||||
|
||||
// Set up: positions at 100, 200 (conflict), 200 (conflict) - no right neighbor
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 300, ProjectViewID: 98, Position: 100})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 301, ProjectViewID: 98, Position: 200})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 302, ProjectViewID: 98, Position: 200})
|
||||
require.NoError(t, err)
|
||||
|
||||
conflicts := []*TaskPosition{
|
||||
{TaskID: 301, ProjectViewID: 98, Position: 200},
|
||||
{TaskID: 302, ProjectViewID: 98, Position: 200},
|
||||
}
|
||||
|
||||
err = resolveTaskPositionConflicts(s, 98, conflicts)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Check positions are unique and > 100
|
||||
var pos1, pos2 TaskPosition
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", 301, 98).Get(&pos1)
|
||||
require.NoError(t, err)
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", 302, 98).Get(&pos2)
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.NotEqual(t, pos1.Position, pos2.Position)
|
||||
assert.Greater(t, pos1.Position, 100.0)
|
||||
assert.Greater(t, pos2.Position, 100.0)
|
||||
})
|
||||
|
||||
t.Run("returns error when spacing exhausted", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Clear existing positions for this view
|
||||
_, err := s.Where("project_view_id = ?", 97).Delete(&TaskPosition{})
|
||||
require.NoError(t, err)
|
||||
|
||||
// Set up: extremely tight spacing that can't accommodate multiple tasks
|
||||
// Gap of 2e-9 with 2 conflicts means spacing of ~6.67e-10 < MinPositionSpacing (1e-9)
|
||||
basePos := 100.0
|
||||
tinyGap := 1e-9
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 400, ProjectViewID: 97, Position: basePos})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 401, ProjectViewID: 97, Position: basePos + tinyGap})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 402, ProjectViewID: 97, Position: basePos + tinyGap})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 403, ProjectViewID: 97, Position: basePos + 2*tinyGap})
|
||||
require.NoError(t, err)
|
||||
|
||||
conflicts := []*TaskPosition{
|
||||
{TaskID: 401, ProjectViewID: 97, Position: basePos + tinyGap},
|
||||
{TaskID: 402, ProjectViewID: 97, Position: basePos + tinyGap},
|
||||
}
|
||||
|
||||
err = resolveTaskPositionConflicts(s, 97, conflicts)
|
||||
assert.True(t, IsErrNeedsFullRecalculation(err))
|
||||
})
|
||||
|
||||
t.Run("handles multiple conflicts deterministically", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Clear existing positions for this view
|
||||
_, err := s.Where("project_view_id = ?", 96).Delete(&TaskPosition{})
|
||||
require.NoError(t, err)
|
||||
|
||||
// Set up: 4 tasks at the same position
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 504, ProjectViewID: 96, Position: 0})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 501, ProjectViewID: 96, Position: 500})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 503, ProjectViewID: 96, Position: 500})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 502, ProjectViewID: 96, Position: 500})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 500, ProjectViewID: 96, Position: 500})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 505, ProjectViewID: 96, Position: 1000})
|
||||
require.NoError(t, err)
|
||||
|
||||
conflicts := []*TaskPosition{
|
||||
{TaskID: 501, ProjectViewID: 96, Position: 500},
|
||||
{TaskID: 503, ProjectViewID: 96, Position: 500},
|
||||
{TaskID: 502, ProjectViewID: 96, Position: 500},
|
||||
{TaskID: 500, ProjectViewID: 96, Position: 500},
|
||||
}
|
||||
|
||||
err = resolveTaskPositionConflicts(s, 96, conflicts)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Fetch all positions and verify they are unique and ordered by task ID
|
||||
var positions []*TaskPosition
|
||||
err = s.Where("project_view_id = ? AND task_id IN (500, 501, 502, 503)", 96).
|
||||
OrderBy("task_id ASC").
|
||||
Find(&positions)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, positions, 4)
|
||||
|
||||
// Positions should be strictly increasing (sorted by task_id)
|
||||
for i := 1; i < len(positions); i++ {
|
||||
assert.Greater(t, positions[i].Position, positions[i-1].Position,
|
||||
"Position for task %d should be greater than task %d",
|
||||
positions[i].TaskID, positions[i-1].TaskID)
|
||||
}
|
||||
|
||||
// All should be between 0 and 1000
|
||||
for _, p := range positions {
|
||||
assert.Greater(t, p.Position, 0.0)
|
||||
assert.Less(t, p.Position, 1000.0)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func TestUpdateTaskPositionWithConflictResolution(t *testing.T) {
|
||||
t.Run("resolves conflict on update", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Clear existing positions for this view
|
||||
_, err := s.Where("project_view_id = ?", 95).Delete(&TaskPosition{})
|
||||
require.NoError(t, err)
|
||||
|
||||
// Set up: two tasks with different positions
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 600, ProjectViewID: 95, Position: 100})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 601, ProjectViewID: 95, Position: 200})
|
||||
require.NoError(t, err)
|
||||
|
||||
// Update task 600 to have the same position as task 601
|
||||
tp := &TaskPosition{
|
||||
TaskID: 600,
|
||||
ProjectViewID: 95,
|
||||
Position: 200,
|
||||
}
|
||||
|
||||
err = updateTaskPosition(s, nil, tp)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Verify both tasks now have unique positions
|
||||
var pos1, pos2 TaskPosition
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", 600, 95).Get(&pos1)
|
||||
require.NoError(t, err)
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", 601, 95).Get(&pos2)
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.NotEqual(t, pos1.Position, pos2.Position)
|
||||
})
|
||||
}
|
||||
|
||||
func TestRepairTaskPositions(t *testing.T) {
|
||||
t.Run("no duplicates to repair", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Clear all positions and set up clean data with no duplicates
|
||||
_, err := s.Where("project_view_id = ?", 94).Delete(&TaskPosition{})
|
||||
require.NoError(t, err)
|
||||
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 700, ProjectViewID: 94, Position: 100})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 701, ProjectViewID: 94, Position: 200})
|
||||
require.NoError(t, err)
|
||||
|
||||
result, err := RepairTaskPositions(s, false)
|
||||
require.NoError(t, err)
|
||||
|
||||
// View 94 should be scanned but not repaired (no duplicates)
|
||||
assert.GreaterOrEqual(t, result.ViewsScanned, 1)
|
||||
assert.Empty(t, result.Errors)
|
||||
})
|
||||
|
||||
t.Run("repairs duplicates in view", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Clear and set up duplicates
|
||||
_, err := s.Where("project_view_id = ?", 93).Delete(&TaskPosition{})
|
||||
require.NoError(t, err)
|
||||
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 800, ProjectViewID: 93, Position: 100})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 801, ProjectViewID: 93, Position: 200})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 802, ProjectViewID: 93, Position: 200}) // Duplicate!
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 803, ProjectViewID: 93, Position: 300})
|
||||
require.NoError(t, err)
|
||||
|
||||
result, err := RepairTaskPositions(s, false)
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.GreaterOrEqual(t, result.ViewsRepaired, 1)
|
||||
assert.GreaterOrEqual(t, result.TasksAffected, 2)
|
||||
assert.Empty(t, result.Errors)
|
||||
|
||||
// Verify positions are now unique
|
||||
var pos1, pos2 TaskPosition
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", 801, 93).Get(&pos1)
|
||||
require.NoError(t, err)
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", 802, 93).Get(&pos2)
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.NotEqual(t, pos1.Position, pos2.Position)
|
||||
})
|
||||
|
||||
t.Run("dry run reports without changes", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Clear and set up duplicates
|
||||
_, err := s.Where("project_view_id = ?", 92).Delete(&TaskPosition{})
|
||||
require.NoError(t, err)
|
||||
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 900, ProjectViewID: 92, Position: 500})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 901, ProjectViewID: 92, Position: 500}) // Duplicate!
|
||||
require.NoError(t, err)
|
||||
|
||||
result, err := RepairTaskPositions(s, true) // dry run
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.GreaterOrEqual(t, result.ViewsRepaired, 1)
|
||||
assert.GreaterOrEqual(t, result.TasksAffected, 2)
|
||||
|
||||
// Verify positions are still duplicates (dry run shouldn't change them)
|
||||
var pos1, pos2 TaskPosition
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", 900, 92).Get(&pos1)
|
||||
require.NoError(t, err)
|
||||
_, err = s.Where("task_id = ? AND project_view_id = ?", 901, 92).Get(&pos2)
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.InDelta(t, pos1.Position, pos2.Position, 0) // Still duplicates
|
||||
})
|
||||
|
||||
t.Run("handles multiple views", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// Set up duplicates in two different views
|
||||
_, err := s.Where("project_view_id IN (90, 91)").Delete(&TaskPosition{})
|
||||
require.NoError(t, err)
|
||||
|
||||
// View 90: duplicates
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 1000, ProjectViewID: 90, Position: 100})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 1001, ProjectViewID: 90, Position: 100})
|
||||
require.NoError(t, err)
|
||||
|
||||
// View 91: duplicates
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 1002, ProjectViewID: 91, Position: 200})
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(&TaskPosition{TaskID: 1003, ProjectViewID: 91, Position: 200})
|
||||
require.NoError(t, err)
|
||||
|
||||
result, err := RepairTaskPositions(s, false)
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.GreaterOrEqual(t, result.ViewsRepaired, 2)
|
||||
assert.GreaterOrEqual(t, result.TasksAffected, 4)
|
||||
assert.Empty(t, result.Errors)
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user