User accounts notifications

[ignazio-bovo]

Changelog

Implementation choices

1. Notification are not deleted if referred entity has been deleted (eg. notification for a video posted is not deleted if a video is deleted later on). I have chosen this for simplicity of the implementation and also because I find it consistent with what most apps do (eg. chats with a deleted account on telegram)
2. Together with point 1: there is no explicit way for a user to delete a notification (as this was not specified in the design)
3. Two entities for displaying notification to the user EmailDeliveryNotification used to register an notification email delivery and InAppDeliveryNotification. Fetching data for notification center / studio should should use the InAppDeliveryNotification entity and from there walk to the desired Notification fields
4. In order to keep both runtime related and off chain notification under a single entity I have used a tagged id:
   • Notification.id = OnChainNotification-${COUNTER} for runtime related notification
   • Notification.id = OffChainNotification-${COUNTER} for off chain related notification
     The migration for these ids will be slightly akward but doable
5. The design requires for example when a nft is set on auction that the page link will direct the user to the video page with the nft widget open. @attemka confirmed that at the moment this is not possible, so I just have a redirect to the video page
6. The email template color are black blue and white but it doesn't look as good as in the design (this is ok I guess as in v2 we will change the template for a digest-like email)
7. Whenever some infomation is missing for the notification text (eg. untitled video, untitled channel and so on) a "" string is used
8. The email notification will have for "unsuscribe" link the following: https://gleev.xyz/member/${membership.handle}/?tab=preferences confident that the user preferences will have their own dedicated tab @thesan

[ignazio-bovo]

I think we should have some sort of basic integration-tests support in the Orion repo so that while making changes to the existing mappings, we don't introduce any bugs. I see /run-tests.sh file has been added but it does not seem to test anything. Do you plan to add/improve the tests in the same PR?

No, I tried to but it was taking too much time and I need to focus on other projects. I think this can be prioritized however
There is already some proposal #147 but not reviewed and not implemented yet

You are using the uniqueId function to assign Ids to some of the entities. This is not ideal as database entity IDs should not be completely random (could lead to ID collusion; though very very minimal chance) instead they should have some structure, either use crypto.randomUUID function or use uuid npm package for assigning IDs to entities.

Migrating to uuid would be too complex.
Also using the birthday paradox formula approximation:
with number of days = strings of variable length x Base64 encoded. So for x = 8 you would need square_root(1 trillion) = 1 million entities to have to have collision probability of about 1 - exp(- (1 trillion)/(2 * 64^8)) = .17%. I think it's safe. (userId is computed using x=32)

[zeeshanakram3]

Migrating to uuid would be too complex. Also using the birthday paradox formula approximation: with number of days = strings of variable length x Base64 encoded. So for x = 8 you would need square_root(1 trillion) = 1 million entities to have to have collision probability of about 1 - exp(- (1 trillion)/(2 * 64^8)) = .17%. I think it's safe. (userId is computed using x=32)

Yeah, that's why I said very very minimal chance, ok, sounds good. Just make sure that we are using uniqueId(32)instead of uniqueId(8) in all new entities

[ignazio-bovo]

I have refactored all the notification variants, so that text and link now have to be provided by:

• front-end in case of inApp delivery
• email scheduler in case of email delivery (this as agreed on the last Orion weekly is now part of the scope and it will be a PR following this)

[zeeshanakram3]

Introducing the p-limit dependency breaks the both orion_processor & orion_graphql-server instances. The reason is that p-limit@4.0.0 is an ESM module & you can import an ESM module inside a common module. Downgrading to p-limit@3.1.0 fixes the problem

Also, please address the other comments

[zeeshanakram3]

I think it's better to use Postgres SERIAL column type (see: https://www.postgresql.org/docs/current/datatype-numeric.html#DATATYPE-SERIAL), instead of manually incrementing the entity ID in the application.

[ignazio-bovo]

"The data types smallserial, serial and bigserial are not true types, but merely a notational convenience for creating unique identifier columns"
So I don't think it will fit the case, as there will be just one row for the NextEntityId

[ignazio-bovo]

Plus it would be complex to generate @ PrimaryGeneratedColumn for every column of the table, since it is not possible to do so with squid typegen

[zeeshanakram3]

What is the use of this file? I don't see this being referenced anywhere.

[zeeshanakram3]

Just noticed that, mjml dependency seems to be unused

[zeeshanakram3]

seems like this is unused

[zeeshanakram3]

Unused dependency

[zeeshanakram3]

Even for the on-chain events we need to hide, we need to hide the status field from the view, because we don't to expose the information about when the notification was read in the public schema

[ignazio-bovo]

I have made the notification table hidden, for simplicity

[zeeshanakram3]

Seems incorrect? As the OnChainNotification & OffChainNotification entity names do not exist.

[ignazio-bovo]

The NextEntityId table contains pairs entityName and id it doesn't necessary impose that the entityName must be a table in the database. In other words it is just a table with counters

[zeeshanakram3]

An issue I discovered while testing again is that you can you cant mix up the use of overlay (in-memnory cache layer) & em (Entity manager) within the single transaction, especially if one data record you are querying using em was created using the overlay within the current transaction. this would lead to error crashin the orion processor. (see https://github.com/Joystream/orion/blob/c66eba0a805de28d13800b91e10588bef9fc6abc/docs/developer-guide/tutorials/adding-new-event-handlers.md#understanding-the-overlay)

For example, in ChannelCreatedEvent mapping, channel entity is being created using overlay (which means that it's not actually created on db, and it only exists in momry yet). and then in the addNotifications function (basically in getChannelTitle function), the entity is being directly fetch from the db (using em), which would result in error since entity does not exist yet.

[ignazio-bovo]

I noticed the same issue also for email notifications, that's why I had to split entities in case we are using

• entity manager
• overlay

This should work, and it is simple but it is not the best approach, as the overlay has no mechanism to set whether a cached entity has been invalidated

[zeeshanakram3]

the on-chain notification delivery logic is still using the em instead of overlay, in executeMailDelivery function, so the delivery would fail if the cached content is not saved into the db

e.g here is the error in case of channel created event

[ignazio-bovo]

The problem is not in the delivery, is in the content creation (as the error suggest)
It is trying to fetch the entity data (for some notification for which entity data is required) in order to construct the content from orion_db but the overlay hasn't flushed yet

[ignazio-bovo]

I need to fix the link and the text construction part

[ignazio-bovo]

it should no longer be a problem now since all notification data is provided at addNotification call site

[zeeshanakram3]

LGTM, there are still a few improvements that can be added, but that can be left for your notifications scheduler PR. So looks good.