Notification: support new notifications through API

[agjohnson]

Edit: in discussion below we narrowed the focus of this for right now. Currently, everything is template driven, but the build detail page needs to be API driven still. We'll punt on the rest of the items as there isn't a huge amount of value in API driven user/organization notifications yet.

• Remove page reload to view errors on build detail view and render errors from the API response
• Ensure cold storage builds show errors as well, these should come from cold storage API data. (This is assuming the modeling has these new errors too, I'm not sure if we touched on this one at the modeling level yet)

---

Decisions

• Will project/organization/user notifications be rendered by templates? Is there a need to make these API driven?

Work

• Create a common Notification listing include template
  • Include this template on project base template, organization base template, and profile base template
  • If API driven, pass the notification API URL to the relevant Knockout view. There will be distinct URLs for project notifications, organization notifications, and user notifications (?).
  • Iterate over Notification instances and render. Reuse and tune current messages template for this
• Create a Notification listing for build detail page
  • Most of the logic above is still applicable, but this will be rendered slightly differently and in a different location than other notifications
• We did talk about how to use the bell icon menu and what notifications might go there. I feel we still aren't settled here, and maybe need to try out some options like news messages and high priority messages.

cc @humitos, you might have some input on what is needed here

[humitos]

Will project/organization/user notifications be rendered by templates? Is there a need to make these API driven?

Regarding template rendering vs. API driven, there are a few things to consider IMO:

• 👍🏼 template rendering is a lot easier to me
• 😐 API driven is useful if we want more dynamic UX (similar to build detail's page), but I think it's not required for the notifications in the other resources at this point
• 👍🏼 using API driven will give us parity with the build detail's page
• 👎🏼 API driven will require writing and maintaining extra endpoints only for this use case

With that in mind, I would start with template rendering because it's easier to implement. We can keep thinking about this and change the approach if we find a more dynamic UX is required.

We need to create the PATCH endpoint to mark them as read / dismissed either way (see readthedocs/readthedocs.org#10983)

Create a Notification listing for build detail page

This is already implemented and it's working.

• Endpoint lives at http://devthedocs.org/api/v3/projects/test-builds/builds/3357/notifications/
• URL registered in code at https://github.com/readthedocs/readthedocs.org/blob/e778db59ce79e642cb26aafe483d0d16e2eabb0f/readthedocs/api/v3/urls.py#L73-L81

Since we will mark the notification as read dynamically here, we need to add the _links field in the notification response for this use case (see readthedocs/readthedocs.org#10982)

We did talk about how to use the bell icon menu and what notifications might go there. I feel we still aren't settled here, and maybe need to try out some options like news messages and high priority messages.

I still don't have a great answer. I'm happy to discuss more these approaches and figure it out while working on this or after implementing the initial version of the front-end. I think that will give us some valuable input to make a better decision here.

[agjohnson]

With that in mind, I would start with template rendering because it's easier to implement.

Cool, those are all my exact thoughts too, we're on the same page 👍

Create a Notification listing for build detail page

This is already implemented and it's working.

Sorry, I wasn't super clear here. I was trying to refer to the notification listing UI elements in the build detail page, created from the Notification API. Your first pass in #254 on that UI will be close, but we need to add styling for the message levels, custom icons, etc, and feed it API data.

I still don't have a great answer.

Same. I think I need to see it in aciton. When I get to theme pieces, I'll try to mock something up -- I've had a hard time making time to do this so far

[agjohnson]

@humitos Can this be closed?

[agjohnson]

Nevermind, this is still a valid issue. I thought we got rid of this hack but we do need to return to it:

ext-theme/src/js/build/detail.js

Lines 447 to 455 in e063afa

	// HACK: this is a small hack to avoid implementing the new notification system
	// on ext-theme at this point. This will come in a future PR.
	// The notifications are rendered properly via Django template in a static way.
	// So, we refresh the page once the build has finished to make Django render the notifications.
	// We use a check of 1 API poll for those builds that are already finished when opened.
	// The new dashboard will implement the new notification system in a nicer way using APIv3.
	if (this.poll_api_counts !== 1) {
	location.reload();
	}

The other notifications are fine to drive with template logic however, I don't feel we need to adjust those yet. We can focus this issue on the build detail notifications instead.

So the question instead is: can we finish this work now, or are we still blocked?

[humitos]

This work is about "rendering all the notifications via APIv3". This requires creating all the KO structure/pattern to consume the APIv3 endpoints --which is not implemented already and think you are the best one to do this work here.

Besides, there are some other work here missing as well:

• design the UI notification element to expose the header + body
• add the ability to dismiss the notification
• remove the hack that refreshes the page

Note that rending the notifications by using template logic was only just a temporal workaround to allow us to move forward with the implementation of the new notification system.

[agjohnson]

From our chat on this:

• I can take a look into this to start, this might be a good issue to start with too however
• This is probably starting with build detail errors first, as these definitely should be API driven
• Separate but related work would be using the API pattern for all notifications, so some of the build detail view notifications work might be reusable or might extend a general pattern
• And as we are implementing the KO view/etc for general API driven notifications, we should implement the dismiss URL. Or rather, it probably doesn't make sense to implement this for template driven notifications as that work might be replaced.

[agjohnson]

I started poking this on the build detail view and found that we can't use the APIv3 build endpoint still. This view uses the build v2 API for access to BuildCommands. I opened up

• API: BuildCommands and missing fields to build v3 API readthedocs.org#11179

I didn't realize we implemented the Notifications in v2 API, so can at least start there.

[agjohnson]

I started poking this last night when I got back to my desk, it did go in fairly easy on the build detail view. Still some rough edges but solid improvement:

[humitos]

This can be closed since it was implemented in #300