vikunja/vikunja
5
48
Fork 86
Code Issues 150 Pull Requests 37 Packages Releases 37 Activity

User setting to sort comments #2307

Open
andileni wants to merge 3 commits from andileni/vikunja:main into main
pull from: andileni/vikunja:main
merge into: vikunja:main
Conversation 0 Commits 3 Files Changed 7 +33 -3
andileni commented 2024-04-27 12:03:35 +00:00
First-time contributor
Copy Link

Following the discussion here https://community.vikunja.io/t/add-option-to-sort-comments/2286/8 this implements the needed functionality.
I tested it on my end as good as I could, but no guarantee.
Please have a look and point out any issues which must be fixed before merging.

Thanks and kind regards
Andreas

Resolves https://github.com/go-vikunja/vikunja/issues/269
Resolves https://community.vikunja.io/t/add-option-to-sort-comments/2286

Following the discussion here https://community.vikunja.io/t/add-option-to-sort-comments/2286/8 this implements the needed functionality. I tested it on my end as good as I could, but no guarantee. Please have a look and point out any issues which must be fixed before merging. Thanks and kind regards Andreas Resolves https://github.com/go-vikunja/vikunja/issues/269 Resolves https://community.vikunja.io/t/add-option-to-sort-comments/2286
andileni added 3 commits 2024-04-27 12:03:38 +00:00
pass orderBy as parameter to taskCommentService in comments.vue
Some checks failed
continuous-integration/drone/pr Build is failing
Details
918c49648b
konrad requested changes 2024-05-05 13:46:12 +00:00
konrad left a comment
Owner
Copy Link

Please also check why the CI fails and make it pass.

Please also check why the CI fails and make it pass.
pkg/models/task_comments.go
@ -38,2 +39,4 @@
TaskID int64 `xorm:"not null" json:"-" param:"task"`
// The query parameter to order the items by. This can be either asc or desc, with asc being the default.
OrderBy []string `query:"order_by" json:"order_by"`
konrad commented 2024-05-05 13:43:22 +00:00
Owner
Copy Link

There should be a SortBy as well for the property to sort comments by. "Order asc / desc" does not tell you which property the sorting is using. Similar to how sorting is done in the TaskCollection.

No need to implement any other property than created in this PR but it should be extendable without breaking changes in the future.

Please exclude this from json as it's not used in the actual comment and document this as query parameter in the swagger annotation comment.

There should be a `SortBy` as well for the property to sort comments by. "Order asc / desc" does not tell you which property the sorting is using. Similar to how sorting is done in the `TaskCollection`. No need to implement any other property than `created` in this PR but it should be extendable without breaking changes in the future. Please exclude this from `json` as it's not used in the actual comment and document this as query parameter in the swagger annotation comment.
pkg/models/task_comments.go
@ -271,3 +275,3 @@
Where(builder.And(where...)).
Join("LEFT", "users", "users.id = task_comments.author_id").
OrderBy("task_comments.created asc")
OrderBy(fmt.Sprintf("task_comments.created %s", tc.OrderBy[0]))
konrad commented 2024-05-05 13:45:57 +00:00
Owner
Copy Link

Please use "task_comments.created " + orderBy and make sure the property to order is defined. With this approach, the api will crash when you do not pass an order parameter.

Please use `"task_comments.created " + orderBy` and make sure the property to order is defined. With this approach, the api will crash when you do not pass an order parameter.
Some checks failed
continuous-integration/drone/pr Build is failing
Required
Details
This pull request has changes conflicting with the target branch.
  • frontend/src/components/tasks/partials/comments.vue
  • frontend/src/modelTypes/IUserSettings.ts
  • frontend/src/models/userSettings.ts
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u main:andileni-main
git checkout andileni-main
Sign in to join this conversation.
Reviewers
No Reviewers
konrad
Labels
Clear labels
dependencies
duplicate
This issue or pull request already exists
help wanted
Need some help
invalid
Something is wrong
kind/bug
Something is not working
kind/feature
New feature
needs reproduction
question
More information is needed
security
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vikunja/vikunja#2307
No description provided.
Delete Branch "andileni/vikunja:main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?