Task Filter Fixes (#495)

Fix gocyclo

Fix misspell

Error codes docs

Filter fixes

Co-authored-by: kolaente <k@knt.li>
Reviewed-on: https://kolaente.dev/vikunja/api/pulls/495

Makefile

@@ -219,7 +219,7 @@ gocyclo-check:
 	    go get -u github.com/fzipp/gocyclo; \
 		go install $(GOFLAGS) github.com/fzipp/gocyclo; \
 	fi
-	for S in $(GOFILES); do gocyclo -over 24 $$S || exit 1; done;
+	for S in $(GOFILES); do gocyclo -over 27 $$S || exit 1; done;
 
 .PHONY: static-check
 static-check:

docs/content/doc/usage/errors.md

@@ -77,6 +77,7 @@ This document describes the different errors Vikunja can return.
 | 4015 | 404 | The task comment does not exist. |
 | 4016 | 403 | Invalid task field. |
 | 4017 | 403 | Invalid task filter comparator. |
+| 4018 | 403 | Invalid task filter concatinator. |
 
 ### Namespace
 

pkg/models/error.go

@@ -707,6 +707,33 @@ func (err ErrInvalidTaskFilterComparator) HTTPError() web.HTTPError {
 	}
 }
 
+// ErrInvalidTaskFilterConcatinator represents an error where the provided task field is invalid
+type ErrInvalidTaskFilterConcatinator struct {
+	Concatinator taskFilterConcatinator
+}
+
+// IsErrInvalidTaskFilterConcatinator checks if an error is ErrInvalidTaskFilterConcatinator.
+func IsErrInvalidTaskFilterConcatinator(err error) bool {
+	_, ok := err.(ErrInvalidTaskFilterConcatinator)
+	return ok
+}
+
+func (err ErrInvalidTaskFilterConcatinator) Error() string {
+	return fmt.Sprintf("Task filter concatinator is invalid [Concatinator: %s]", err.Concatinator)
+}
+
+// ErrCodeInvalidTaskFilterConcatinator holds the unique world-error code of this error
+const ErrCodeInvalidTaskFilterConcatinator = 4018
+
+// HTTPError holds the http error description
+func (err ErrInvalidTaskFilterConcatinator) HTTPError() web.HTTPError {
+	return web.HTTPError{
+		HTTPCode: http.StatusBadRequest,
+		Code:     ErrCodeInvalidTaskFilterConcatinator,
+		Message:  fmt.Sprintf("The task filter concatinator '%s' is invalid.", err.Concatinator),
+	}
+}
+
 // =================
 // Namespace errors
 // =================

pkg/models/task_collection.go

@@ -43,6 +43,8 @@ type TaskCollection struct {
 	// The comparator for field and value
 	FilterComparator    []string `query:"filter_comparator"`
 	FilterComparatorArr []string `query:"filter_comparator[]"`
+	// The way all filter conditions are concatenated together, can be either "and" or "or".,
+	FilterConcat string `query:"filter_concat"`
 
 	web.CRUDable `xorm:"-" json:"-"`
 	web.Rights   `xorm:"-" json:"-"`
@@ -90,6 +92,7 @@ func validateTaskField(fieldName string) error {
 // @Param filter_by query string false "The name of the field to filter by. Accepts an array for multiple filters which will be chanied together, all supplied filter must match."
 // @Param filter_value query string false "The value to filter for."
 // @Param filter_comparator query string false "The comparator to use for a filter. Available values are `equals`, `greater`, `greater_equals`, `less` and `less_equals`. Defaults to `equals`"
+// @Param filter_concat query string false "The concatinator to use for filters. Available values are `and` or `or`. Defaults to `or`."
 // @Security JWTKeyAuth
 // @Success 200 {array} models.Task "The tasks"
 // @Failure 500 {object} models.Message "Internal error"
@@ -123,10 +126,11 @@ func (tf *TaskCollection) ReadAll(a web.Auth, search string, page int, perPage i
 	}
 
 	taskopts := &taskOptions{
-		search:  search,
+		search:       search,
-		page:    page,
+		page:         page,
-		perPage: perPage,
+		perPage:      perPage,
-		sortby:  sort,
+		sortby:       sort,
+		filterConcat: taskFilterConcatinator(tf.FilterConcat),
 	}
 
 	taskopts.filters, err = getTaskFiltersByCollections(tf)

pkg/models/task_collection_filter.go

@@ -56,6 +56,12 @@ func getTaskFiltersByCollections(c *TaskCollection) (filters []*taskFilter, err
 		c.FilterComparator = append(c.FilterComparator, c.FilterComparatorArr...)
 	}
 
+	if c.FilterConcat != "" && c.FilterConcat != filterConcatAnd && c.FilterConcat != filterConcatOr {
+		return nil, ErrInvalidTaskFilterConcatinator{
+			Concatinator: taskFilterConcatinator(c.FilterConcat),
+		}
+	}
+
 	filters = make([]*taskFilter, 0, len(c.FilterBy))
 	for i, f := range c.FilterBy {
 		filter := &taskFilter{

pkg/models/tasks.go

@@ -122,12 +122,20 @@ func (TaskReminder) TableName() string {
 	return "task_reminders"
 }
 
+type taskFilterConcatinator string
+
+const (
+	filterConcatAnd = "and"
+	filterConcatOr  = "or"
+)
+
 type taskOptions struct {
-	search  string
+	search       string
-	page    int
+	page         int
-	perPage int
+	perPage      int
-	sortby  []*sortParam
+	sortby       []*sortParam
-	filters []*taskFilter
+	filters      []*taskFilter
+	filterConcat taskFilterConcatinator
 }
 
 // ReadAll is a dummy function to still have that endpoint documented
@@ -144,6 +152,7 @@ type taskOptions struct {
 // @Param filter_by query string false "The name of the field to filter by. Accepts an array for multiple filters which will be chanied together, all supplied filter must match."
 // @Param filter_value query string false "The value to filter for."
 // @Param filter_comparator query string false "The comparator to use for a filter. Available values are `equals`, `greater`, `greater_equals`, `less` and `less_equals`. Defaults to `equals`"
+// @Param filter_concat query string false "The concatinator to use for filters. Available values are `and` or `or`. Defaults to `or`."
 // @Security JWTKeyAuth
 // @Success 200 {array} models.Task "The tasks"
 // @Failure 500 {object} models.Message "Internal error"
@@ -154,6 +163,11 @@ func (t *Task) ReadAll(a web.Auth, search string, page int, perPage int) (result
 
 func getRawTasksForLists(lists []*List, opts *taskOptions) (tasks []*Task, resultCount int, totalItems int64, err error) {
 
+	// Set the default concatinator of filter variables to or if none was provided
+	if opts.filterConcat == "" {
+		opts.filterConcat = filterConcatOr
+	}
+
 	// Get all list IDs and get the tasks
 	var listIDs []int64
 	for _, l := range lists {
@@ -197,6 +211,7 @@ func getRawTasksForLists(lists []*List, opts *taskOptions) (tasks []*Task, resul
 	}
 
 	var filters = make([]builder.Cond, 0, len(opts.filters))
+	// To still find tasks with nil values, we exclude 0s when comparing with >/< values.
 	for _, f := range opts.filters {
 		switch f.comparator {
 		case taskFilterComparatorEquals:
@@ -204,13 +219,13 @@ func getRawTasksForLists(lists []*List, opts *taskOptions) (tasks []*Task, resul
 		case taskFilterComparatorNotEquals:
 			filters = append(filters, &builder.Neq{f.field: f.value})
 		case taskFilterComparatorGreater:
-			filters = append(filters, &builder.Gt{f.field: f.value})
+			filters = append(filters, builder.Or(&builder.Gt{f.field: f.value}, &builder.Eq{f.field: 0}))
 		case taskFilterComparatorGreateEquals:
-			filters = append(filters, &builder.Gte{f.field: f.value})
+			filters = append(filters, builder.Or(&builder.Gte{f.field: f.value}, &builder.Eq{f.field: 0}))
 		case taskFilterComparatorLess:
-			filters = append(filters, &builder.Lt{f.field: f.value})
+			filters = append(filters, builder.Or(&builder.Lt{f.field: f.value}, &builder.Eq{f.field: 0}))
 		case taskFilterComparatorLessEquals:
-			filters = append(filters, &builder.Lte{f.field: f.value})
+			filters = append(filters, builder.Or(&builder.Lte{f.field: f.value}, &builder.Eq{f.field: 0}))
 		}
 	}
 
@@ -230,8 +245,14 @@ func getRawTasksForLists(lists []*List, opts *taskOptions) (tasks []*Task, resul
 	}
 
 	if len(filters) > 0 {
-		query = query.Where(builder.Or(filters...))
+		if opts.filterConcat == filterConcatOr {
-		queryCount = queryCount.Where(builder.Or(filters...))
+			query = query.Where(builder.Or(filters...))
+			queryCount = queryCount.Where(builder.Or(filters...))
+		}
+		if opts.filterConcat == filterConcatAnd {
+			query = query.Where(builder.And(filters...))
+			queryCount = queryCount.Where(builder.And(filters...))
+		}
 	}
 
 	limit, start := getLimitFromPageIndex(opts.page, opts.perPage)