Add subcomands for notifications #386

Closed
6543 wants to merge 18 commits from 6543:add-subcomands-for-notifications_rebase283 into main
Owner

Fix #243

use #283 and resolved conflicts + some code dedub

Fix #243 use #283 and resolved conflicts + some code dedub
6543 added 9 commits 2021-08-16 13:14:45 +00:00
add subcomands for notifications
Fix #243

Signed-off-by: Karl Heinz Marbaise <kama@soebes.de>
74047bee0d
Fixed exported function comments.
Signed-off-by: Karl Heinz Marbaise <kama@soebes.de>
f978713928
Integrated review comments.
Signed-off-by: Karl Heinz Marbaise <kama@soebes.de>
91b5db3e42
Moved notifications_list from task into cmd/notifications
Signed-off-by: Karl Heinz Marbaise <kama@soebes.de>
481132ffdb
WIP - Improved.
Some checks failed
continuous-integration/drone/pr Build is failing
abd32a7074
code dedub
All checks were successful
continuous-integration/drone/pr Build is passing
be0e857273
6543 added 1 commit 2021-08-16 13:22:23 +00:00
Next dedub
All checks were successful
continuous-integration/drone/pr Build is passing
0f12c0a77f
6543 added the
kind
feature
status/wip
labels 2021-08-16 13:22:45 +00:00
6543 added 1 commit 2021-08-16 13:23:43 +00:00
Merge branch 'master' into add-subcomands-for-notifications_rebase283
All checks were successful
continuous-integration/drone/pr Build is passing
e52b2678a9
noerw requested changes 2021-08-16 13:41:07 +00:00
noerw left a comment
Member

Initial review, still quite some work to do (I might pick it up this or next week, no promises though)

Initial review, still quite some work to do (I might pick it up this or next week, no promises though)
@ -104,0 +104,4 @@
// NotificationFlags defines flags that should be available on notifications.
var NotificationFlags = append([]cli.Flag{
&cli.BoolFlag{
Name: "all",
Member

can we rename this to user, account or any-scope?
all is ambiguous as it could also mean --state=all #usability

can we rename this to `user`, `account` or `any-scope`? `all` is ambiguous as it could also mean `--state=all` #usability
@ -26,3 +20,1 @@
Name: "all",
Aliases: []string{"a"},
Usage: "show all notifications of related gitea instance",
Description: "Show notifications, by default based of the current repo",
Member

Description: "Show notifications, by default based on the current repo if available",

` Description: "Show notifications, by default based on the current repo if available",`
@ -29,0 +28,4 @@
Flags: append(flags.NotificationFlags,
&cli.StringFlag{
Name: "state",
Usage: "set notification state (default is all), pinned,read,unread",
Member
- set notification...
+ filter by notification...
```diff - set notification... + filter by notification... ```
@ -0,0 +30,4 @@
// notif ls --state all
// notif ls --state pinned
// notif ls --state read
// notif ls --state unread
Member

?

?
@ -0,0 +21,4 @@
ctx := context.InitCommand(cmd)
client := ctx.Login.Client()
all := ctx.Bool("all")
Member

see above

see [above](https://gitea.com/gitea/tea/pulls/386#issuecomment-443162)
@ -0,0 +26,4 @@
//This enforces pagination.
listOpts := ctx.GetListOptions()
if listOpts.Page == 0 {
listOpts.Page = 1
Member

do we really want this here? a reusable helper would be better. or even move this logic into the sdk?

do we really want this here? a reusable helper would be better. or even move this logic into the sdk?
@ -0,0 +35,4 @@
Status: status,
})
} else {
ctx.Ensure(context.CtxRequirement{RemoteRepo: true})
Member

this is a breaking change, previously tea n ran fine without a remote repo, and just listed the user notifications.
I think the new behaviour is better, so we just need to make sure to note that breaking change in the PR description/labels

this is a breaking change, previously `tea n` ran fine without a remote repo, and just listed the user notifications. I think the new behaviour is better, so we just need to make sure to note that breaking change in the PR description/labels
@ -0,0 +12,4 @@
// CmdNotificationsPinned represents a sub command of notifications to list pinned notifications
var CmdNotificationsPinned = cli.Command{
Name: "pinned",
Member

These subcommands are unneeded/wrong (was a misunderstanding with the original author i think):
We want subcommands that pin/unpin + mark as read/unread.

These subcommands are unneeded/wrong (was a misunderstanding with the original author i think): We want subcommands that pin/unpin + mark as read/unread.
noerw added this to the v0.8.0 milestone 2021-08-16 13:44:12 +00:00
noerw added 7 commits 2021-08-24 02:22:26 +00:00
refactor FieldsFlag as CsvFlag for reusability
to be used in --state + --type of notifications listing
5ee01374bf
rename --for-user flag to --mine for consistency
with `tea time --mine`. also move the flag to the list subcommand only,
as it's not needed for the other subcmds
4acda8d111
print notification id
to be able to mark single notifications
Some checks failed
continuous-integration/drone/pr Build is failing
8414c23ed5
Member

superseded by #389

superseded by #389
noerw closed this pull request 2021-08-24 02:31:11 +00:00
noerw deleted branch add-subcomands-for-notifications_rebase283 2021-08-24 02:31:23 +00:00
Some checks are pending
continuous-integration/drone/pr Build is failing
check-and-test / check-and-test (pull_request)
Required

Pull request closed

Sign in to join this conversation.
No description provided.