Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New notification system: implementation #10922
New notification system: implementation #10922
Changes from all commits
92447e3
a7cb5ad
fb79264
780e28b
749622c
7777960
105e7c8
cbf0e54
584c055
3118eea
03ca9a4
dd8fb31
514c8f9
e9d0c02
0c01d76
36de5f8
e05592a
9f9fcfb
b7600ce
fa27fb1
1dc0f6f
27425fd
05e7e31
fa11b1a
10d5c9c
4e00365
4e8d63d
ff43348
15c8517
82637ec
18c9067
7af4e87
98447fc
130cff9
da3c503
e214eb9
fef0e01
b27c038
8bc8eca
5137152
e619b96
ab09850
482a415
2be5731
10199b9
75bcc86
6d0f90e
87c1e03
51435e1
65212a1
23105c8
41b9993
3c1dcb5
17953c6
33806ee
ff844c9
201584b
ead8ab1
3e9c293
8d50fc5
f46ea00
2617d02
6f3c13d
ecd270a
6549a3f
78c3bd7
9a22ba6
4562969
3bcde18
8712ffd
f55a4f7
d869a71
ee657b3
47762ed
1472ab0
aa96583
1cb8128
db8dd38
738b3bf
3a6e8ea
212bbdb
2c4b99d
05285e4
8b925b4
0a765de
57c26e1
623190d
30b0830
7d1daee
a930ce5
6f16258
bf85137
ac624b0
d532c53
35b64cc
d4951a8
e711d8f
5f15af7
2047a57
4c71772
36db237
8828a4b
e7b83e0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I'm following things the same way you are, but what is this going to look like when we add notification look up by project, for example? Are you planning on a separate view set class, or can we use independent query params to distinguish a lookup for build/project/etc notifications?
If the plan is a separate class, perhaps this class should be renamed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can be flexible here. We can have 1) one API endpoint per resource or 2) only one global endpoint with a filter per resource.
I started doing 1) as a test because is what DRF extensions allows to do and it's a pattern we have been following for other endpoints too. Also, I think it's easier to understand what notifications you are getting back. An example of the request would be:
/api/v3/project/docs/builds/123456/notifications/
/api/v3/project/docs/notifications/
The particular view where you commented (
NotificationsViewSet
) would cover 2) --it's not finished, and can probably be commented out/removed for now-- and would require the user sending the?attached_to=build
and also?attached_to_id=123456
so the endpoint knows what's the exact object to retrieve notifications for:/api/v3/notifications/?attached_to=build&attached_to_id=123456
I didn't finish this because I wanted to discuss a little more with you before making the final decision. I personally prefer 1) since it's a cleaner UX and it's already working 😄 . Do you have any preference here?
However, the view
NotificationsViewSet
may also be required for a different usage: mark a specific notification object as read via the URL:PATCH /api/v3/notifications/718273/
. So, we may keep it for that purpose.We don't have to decide this right now, since it's outside the scope of this PR, tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened an issue for this at #10984
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll follow along that issue, but to note here:
I think I prefer this too. It doesn't make much difference to the front end as we need to resolve the API URL and pass that into the JS view either way.
Good point. On the front end, the API should give a URL for dismissing the notification in the API response, so the JS doesn't have to reconstruct this. So either way works here too, and it wouldn't be any more/less work to PATCH a deeper URL like
/api/v3/project/docs/builds/123456/notifications/789