gitea/tea
gitea/tea
56
129
Fork 64
Code Issues 97 Pull Requests 19 Actions Releases 16 Activity

add subcomands for notifications #283

Closed
khmarbaise wants to merge 7 commits from khmarbaise/tea:issue-243-add_subcomands_for_notifications into main
pull from: khmarbaise/tea:issue-243-add_subcomands_for_notifications
merge into: gitea:main
Conversation 3 Commits 7 Files Changed 7 +227 -14
khmarbaise commented 2020-12-08 19:45:01 +00:00
Member
Copy Link

Fix #243

Signed-off-by: Karl Heinz Marbaise kama@soebes.de

Fix #243 Signed-off-by: Karl Heinz Marbaise <kama@soebes.de>
khmarbaise added 1 commit 2020-12-08 19:45:02 +00:00
add subcomands for notifications 
Fix #243

Signed-off-by: Karl Heinz Marbaise <kama@soebes.de>
Some checks failed
continuous-integration/drone/pr Build is failing
Details
ce4d69ece9
khmarbaise reviewed 2020-12-08 19:45:49 +00:00
Dismissed
modules/task/notifications_list.go Outdated
@ -0,0 +1,49 @@
// Copyright 2020 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.
khmarbaise commented 2020-12-08 19:45:49 +00:00
Author
Member
Copy Link

Is this the way to go with common code from cmd/... ? Or should that put into cmd/notifications/ ?

Is this the way to go with common code from cmd/... ? Or should that put into cmd/notifications/ ?
noerw marked this conversation as resolved
noerw added the
kind
feature
 label 2020-12-08 19:55:53 +00:00
khmarbaise added 1 commit 2020-12-08 19:59:46 +00:00
Added missing comment on exported function.
Some checks failed
continuous-integration/drone/pr Build is failing
Details
8ce46fe569
khmarbaise added 1 commit 2020-12-08 20:06:37 +00:00
Fixed exported function comments. 
Signed-off-by: Karl Heinz Marbaise <kama@soebes.de>
All checks were successful
continuous-integration/drone/pr Build is passing
Details
eb8fd52023
noerw requested changes 2020-12-08 20:08:44 +00:00
Dismissed
cmd/notifications/list.go Outdated
@ -0,0 +16,4 @@
Name: "ls",
Aliases: []string{"list"},
Usage: "List notifications....",
Description: `List notification....`,
noerw commented 2020-12-08 20:01:33 +00:00
Member
Copy Link
- 	Usage:       "List notifications....",
+ 	Usage:       "List notifications",
```diff - Usage: "List notifications....", + Usage: "List notifications", ```
noerw marked this conversation as resolved
cmd/notifications/list.go Outdated
@ -0,0 +19,4 @@
Description: `List notification....`,
Action: RunNotificationsList,
Flags: append([]cli.Flag{
&cli.BoolFlag{
noerw commented 2020-12-08 20:00:52 +00:00
Member
Copy Link

Please move these common flags into cmd/flags/flags.go as NotificationFlags

Please move these common flags into `cmd/flags/flags.go` as `NotificationFlags`
khmarbaise commented 2020-12-08 20:11:50 +00:00
Author
Member
Copy Link

Good idea not even realized that. Thanks.

Good idea not even realized that. Thanks.
noerw marked this conversation as resolved
cmd/notifications/list.go Outdated
@ -0,0 +22,4 @@
&cli.BoolFlag{
Name: "all",
Aliases: []string{"a"},
Usage: "show all notifications of related gitea instance",
noerw commented 2020-12-08 20:03:09 +00:00
Member
Copy Link
- 			Usage:   "show all notifications of related gitea instance",
+ 			Usage:   "Show notifications across all your repos instead of the current repo only",
```diff - Usage: "show all notifications of related gitea instance", + Usage: "Show notifications across all your repos instead of the current repo only", ```
noerw marked this conversation as resolved
cmd/notifications/list.go Outdated
@ -0,0 +26,4 @@
},
&cli.StringFlag{
Name: "state",
Usage: "Filter by milestone state (all|open|closed)",
noerw commented 2020-12-08 20:05:49 +00:00
Member
Copy Link
  • Are valid states really called all/open/closed? all/read/unread?
  • Doesn't this flag duplicate functionality from the subcommands read/unread?
  • s/milestone/notification
- Are valid states really called all/open/closed? all/read/unread? - Doesn't this flag duplicate functionality from the subcommands read/unread? - s/milestone/notification
khmarbaise commented 2020-12-08 20:31:00 +00:00
Author
Member
Copy Link

Yes of course.

Yes of course.
noerw marked this conversation as resolved
cmd/notifications/pinned.go Outdated
@ -0,0 +14,4 @@
// CmdNotificationsPinned represents a sub command of notifications to list pinned notifications
var CmdNotificationsPinned = cli.Command{
Name: "pinned",
Aliases: []string{"pin"},
noerw commented 2020-12-08 20:08:35 +00:00
Member
Copy Link

"pin" makes it sound like a verb, not a read list-command.
I'd drop this alias

"pin" makes it sound like a verb, not a read list-command. I'd drop this alias
khmarbaise commented 2020-12-08 20:13:20 +00:00
Author
Member
Copy Link

What about with an alias pd instead?

What about with an alias `pd` instead?
noerw marked this conversation as resolved
modules/task/notifications_list.go Outdated
@ -0,0 +21,4 @@
listOpts := flags.GetListOptions(ctx)
if listOpts.Page == 0 {
listOpts.Page = 1
}
noerw commented 2020-12-08 20:06:37 +00:00
Member
Copy Link

@6453 ?

@6453 ?
khmarbaise commented 2020-12-08 20:52:27 +00:00
Author
Member
Copy Link

If you take a look into flags.GetListOptions(ctx)

func GetListOptions(ctx *cli.Context) gitea.ListOptions {
	page := ctx.Int("page")
	limit := ctx.Int("limit")
	if limit != 0 && page == 0 {
		page = 1
	}
	return gitea.ListOptions{
		Page:     page,
		PageSize: limit,
	}
}
If you take a look into `flags.GetListOptions(ctx)` ``` func GetListOptions(ctx *cli.Context) gitea.ListOptions { page := ctx.Int("page") limit := ctx.Int("limit") if limit != 0 && page == 0 { page = 1 } return gitea.ListOptions{ Page: page, PageSize: limit, } } ```
khmarbaise commented 2020-12-09 15:52:30 +00:00
Author
Member
Copy Link

After I've played a little bit with it which means removed the lines from notification_list.go I've not seen a difference. May I have missed a thing... Can someone enlighten me ?

After I've played a little bit with it which means removed the lines from notification_list.go I've not seen a difference. May I have missed a thing... Can someone enlighten me ?
noerw commented 2020-12-09 21:44:30 +00:00
Member
Copy Link

I don't understand why, but these lines are needed still, without I get no results for tea notif read

I don't understand why, but these lines are needed still, without I get no results for `tea notif read`
6543 commented 2020-12-09 22:05:41 +00:00
Owner
Copy Link

in short: it enforces paggination

in short: it enforces paggination
6543 marked this conversation as resolved
khmarbaise added 1 commit 2020-12-08 20:31:13 +00:00
Integrated review comments. 
Signed-off-by: Karl Heinz Marbaise <kama@soebes.de>
All checks were successful
continuous-integration/drone/pr Build is passing
Details
29212a5d3e
khmarbaise requested review from noerw 2020-12-09 19:44:07 +00:00
noerw approved these changes 2020-12-09 21:45:41 +00:00
Dismissed
6543 reviewed 2020-12-09 22:09:30 +00:00
Dismissed
6543 requested changes 2020-12-09 22:10:14 +00:00
Dismissed
cmd/notifications/list.go Outdated
@ -0,0 +23,4 @@
Name: "all",
Aliases: []string{"a"},
Usage: "show all notifications of related gitea instance",
},
6543 commented 2020-12-09 22:10:07 +00:00
Owner
Copy Link

@khmarbaise can you move task.ListNotifications back to cmd/notification module

I dont see a reason to add cli & flag dependency into task module

@khmarbaise can you move task.ListNotifications back to cmd/notification module I dont see a reason to add **cli** & **flag** dependency into task module
khmarbaise commented 2020-12-10 06:57:11 +00:00
Author
Member
Copy Link

So making a file notifications_list.go which contains that. I will do and change it accordingly.

So making a file `notifications_list.go` which contains that. I will do and change it accordingly.
khmarbaise commented 2020-12-10 07:02:43 +00:00
Author
Member
Copy Link

Done as suggested.

Done as suggested.
khmarbaise marked this conversation as resolved
khmarbaise added 1 commit 2020-12-10 07:01:56 +00:00
Moved notifications_list from task into cmd/notifications 
Signed-off-by: Karl Heinz Marbaise <kama@soebes.de>
All checks were successful
continuous-integration/drone/pr Build is passing
Details
d350600d81
khmarbaise requested review from 6543 2020-12-10 07:02:54 +00:00
khmarbaise force-pushed issue-243-add_subcomands_for_notifications from d350600d81 to 85c3001e49 2020-12-10 07:06:29 +00:00 Compare
6543 reviewed 2020-12-10 09:53:11 +00:00
Dismissed
cmd/notifications.go Outdated
@ -79,2 +43,3 @@
print.NotificationsList(news, flags.GlobalOutputValue, ctx.Bool("all"))
func runNotificationsDetails(ctx *cli.Context) error {
log.Fatal("Not yet implemented.")
6543 commented 2020-12-10 09:53:11 +00:00
Owner
Copy Link

I would print use --help to see all options

I would print `use --help to see all options`
6543 reviewed 2020-12-10 10:09:18 +00:00
Dismissed
6543 commented 2020-12-10 10:19:48 +00:00
Owner
Copy Link

I dont get the usage of pin, unpin, read subcommand

they only list stuff as list does?

I expect pin to actualy pin a notification ...

same vor unpin & read

the list should print out the notif id to this actions specific or with or,

  • --all flag for read/pin mark all unread as read
  • --all flag for unpin mark all pined as read

unread should only be used to mark a specific one as "unread"

I dont get the usage of pin, unpin, read subcommand they only list stuff as `list` does? I expect pin to actualy pin a notification ... same vor unpin & read the list should print out the notif id to this actions specific or with or, - `--all` flag for read/pin mark all unread as read - `--all` flag for unpin mark all pined as read unread should only be used to mark a specific one as "unread"
6543 commented 2020-12-11 14:09:04 +00:00
Owner
Copy Link

@khmarbaise can you resolve conflicts?

If you need help just shout out on discord to me

@khmarbaise can you resolve conflicts? If you need help just shout out on discord to me
6543 added this to the v0.7.0 milestone 2020-12-14 11:39:43 +00:00
noerw added the
status/needs-feedback
 label 2020-12-15 11:03:33 +00:00
khmarbaise force-pushed issue-243-add_subcomands_for_notifications from 85c3001e49 to abd32a7074 2020-12-16 22:02:00 +00:00 Compare
6543 modified the milestone from v0.7.0 to v0.8.0 2021-03-10 20:29:55 +00:00
6543 commented 2021-08-16 13:22:59 +00:00
Owner
Copy Link

-> #386

-> #386
6543 closed this pull request 2021-08-16 13:22:59 +00:00
Some checks are pending
continuous-integration/drone/pr Build is failing
Details
check-and-test / check-and-test (pull_request)
Required

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
kind/breaking

   
kind/bug

   
kind/build

   
kind/dependency

   
kind/deployment

   
kind/docs
  
kind
enhancement

  
kind
feature

  
kind/proposal
  
kind
question

  
kind
refactor

  
kind/security

   
kind/testing

   
kind/translation

   
priority/critical

   
priority/high

   
priority/low

   
priority/medium

   
reviewed/duplicate

   
reviewed/invalid

   
reviewed/wontfix

   
skip-changelog

   
status/blocked

   
status/has-backport

   
status/has-pull

   
status/needs-backport

   
status/needs-feedback

   
status/needs-reviews

   
status/wip

   
upstream/gitea

   
upstream/sdk
No Label
kind/breaking
kind/bug
kind/build
kind/dependency
kind/deployment
kind/docs
kind
enhancement
kind
feature
kind/proposal
kind
question
kind
refactor
kind/security
kind/testing
kind/translation
priority/critical
priority/high
priority/low
priority/medium
reviewed/duplicate
reviewed/invalid
reviewed/wontfix
skip-changelog
status/blocked
status/has-backport
status/has-pull
status/needs-backport
status/needs-feedback
status/needs-reviews
status/wip
upstream/gitea
upstream/sdk
Milestone
Clear milestone
No items
No Milestone
v0.8.0
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: gitea/tea#283
No description provided.
Delete Branch "khmarbaise/tea:issue-243-add_subcomands_for_notifications"

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?