Add the controller for what's new notification for feature announcements

[NiranjanaBinoy]

The controller adds the notifications to the state and keeps and updates the status of whether it is seen by the user or not.
Associated to issue #23

[Gudahtt]

I'd be interested to hear why an array was chosen for the Notification state. It seems that it's almost always being accessed by id, so would it be more appropriate to store them in a Record keyed by id?

One potential downside would be losing the sort order, but we can already derive the sort order here because we have the date property. I wouldn't be concerned about performance here, especially given that Objects will preserve their insert order by default, so we can make the "sort" step really cheap by having them configured in the order we view them in.

[danjm]

I just realized a requirement of the designs that we have not thought of a good solution for yet: the button or link associated with each notification is expected to have a unique set of methods called / actions taken on click.

For instance, the current action taken on click of the button in the current swaps pop up is:

onClick={() => {
    onClose()
    enteredSwapsEvent()
    dispatch(setSwapsFromToken(swapsEthToken))
    history.push(BUILD_QUOTE_ROUTE)
}}

But on click of the mobile link, the user is supposed to see another popup, or maybe be taken to an external link. This controller, or at least the notification data structure, will need to support the passing of some information about this to the front end.

First I'll get clarity on what the product/design requirements are.

[danjm]

Regarding my previous comment: I spoke with Jacob and it seems that the action taken on each link/button click can just be to direct the user to a link. I think we can handle metrics for the clicks of these on the front end without any need for configuration via the notification object. And we can stop supporting custom actions like setSwapsFromToken for now.

However, we will need to add one more property to the notification type, which is a linkUrl or actionUrl. A string that contains the url the user should be taken to. Although, some of these will be routes within the extension, and some external links. We need a way to distinguish those.

[Gudahtt]

Using a route for each action seems like overkill. These actions may not have any associated UI after all. We don't need to contort our routes to perform arbitrary actions.

Note that we don't need the action for each notification to be some serializable value - it can be a function. We don't need this controller to know what the action is. It can just be given an onClick for any notification with an associated action.

[danjm]

We don't need this controller to know what the action is.

Yes, agreed. I think that as long as notifications can include a url, then we are good. So current implementation should work fine.

[Gudahtt]

as long as notifications can include a url, then we are good

Just to be clear - I was saying the opposite of this. If we require all notification actions to have an associated URL, we'll need to create a route for each one. That is not good. We use routes to represent "pages" in the UI (or UI state in general), we shouldn't have to use them for arbitrary actions too. That would be a misuse of the routing system.

It seems that that solution was arrived at because the rest of the notification configuration is JSON, so it can't contain functions. But there's no reason for the notification configuration to be JSON. The state has to be, but the configuration doesn't. It would be easy and straightforward to pass functions in as part of the notification config to act as handlers for any buttons, and it wouldn't require us to migrate each action to a route. The swaps notification for example, we could leave the handler exactly as-is.

Does that make sense? Or is there some reason for this change that I'm not seeing?

[danjm]

@Gudahtt I know we discussed this on Friday, but after rereading your comments here, I just want to clarify what may have been a misunderstanding between us.

I never meant to suggest we use a route for each action. My intended meaning was:

I believe that, on click of the action button/link associated with a notification, only 1 thing should happen: to take the user to a new location. That location can either be within MetaMask or an external site. We simply won't support any additional actions being taken on click of those buttons/links. (We would, of course, track metrics for each of those clicks, but that doesn't need to be accounted for in the notification configuration.)

I arrived at that first when learning the requirements of the current notifications:

The "MetaMask Mobile" and "Help Improve MetaMask" links will take the user to external sites.

I'll contend that the swaps notification button only needs to take the user to the swaps page, i.e. something like history.push(BUILD_QUOTE_ROUTE). Considering the other function calls in the current swaps notification handler: onClose() will be handled by the notification component itself; enteredSwapsEvent() feels like it is something that should happen on entering the swaps route, but even if not, metrics for all notifications can be handled by the notification component itself; and dispatch(setSwapsFromToken(swapsEthToken)) is something that either could happen upon entering the swaps route if no token is already set, or we can just decide that changing state is something we won't do on click of a notification's action/link buttons.

I like the simplicity of a notification feature that only does two things: gives the user some information via text, and gives them a link to follow for more information or to take further action. I still think it will be simpler, with this approach, to eventually support remotely hosted notifications. Of course, allowing arbitrary function calls on click is much more powerful... and maybe we will need that some day, so maybe the time is now.

So I fully agree with you that if we want to support "a unique set of methods called / actions taken on click" we should take your recommended approach. I am just skeptical that we really need to support it.

[Gudahtt]

It's not just confining what we can do with the notification action though - it's also more work. I don't agree that it's simpler - even when we consider the possibility of remotely hosted notifications. To be honest I don't follow why you'd think it was simpler.

As for whether routes would serve our needs here just fine, they already don't. In the example you showed for instance, we don't have a way to specify that it should be an ETH swap (the dispatch(setSwapsFromToken(swapsEthToken)) call). We could add it to the route, or rely upon that being a default, but as written today it'd be easier to use a function. The metrics are a similar case - we'll want to collect metrics for these interactions, and that would be easiest with a function. I understand that we could make compromises and do all of these things with a route, but I don't understand why we would do that.

[danjm]

Right, I don't think we should do any of these things with a route. I realize I said some things that could have obscured that further so I'll add some more notes to clarify further.

For metrics, I think we can have a general "Notification action clicked" event, and it can be passed the notification title, or unique id. That is how we handle the metrics actions.

And instead of doing dispatch(setSwapsFromToken(swapsEthToken)), it can just be the responsibility of the swaps component to set a default from token if none is already set upon mount of the component. So we wouldn't handle it by a route or any other means, we would leave it to some other part of the codebase to deal with if we felt that necessary. And so we wouldn't support arbitrary actions like that in the future either...

... maybe we should clarify synchronously

[danjm]

Add actionFunction to the interface here

[danjm]

This logic is correct, but I think there is an elegant way to ensure this condition is met just by using the type definition. Could be to have an action property which is an object, which is optional, but which itself has two properties (function and text) that are both required. You might have to read into some typescript documentation

[danjm]

So instead of returning the action function, we could just call it here.
Replace return notification ? notification.actionFunction as action : null;
with something like
notification?.actionFunction?.()

[Gudahtt]

This could use a rebase - it's behind develop by quite a bit. There has been a TypeScript update and lint changes since then.

[Gudahtt]

LGTM!

[Gudahtt]

We could probably get rid of this property too, since nothing in this controller uses this 🤔
Not a blocker though I suppose! We can always remove it later.