Core events

[Mikearaya]

No description provided.

[Mikearaya]

@schmidsi this is the branch that one had a conflict plus it's not the module approach so i created a new branch. that one is deleted now tho

[Mikearaya]

@schmidsi @pozylon
so far implementing order event emitting I've identified this event and its corresponding payloads

event payload

ORDER_CHECKOUT =>  current order object
ORDER_ADD_PRODUCT => Order position
ORDER_ADD_DISCOUNT => reservedDiscount
ORDER_CONFIRMED => current order object
ORDER_FULLFILLED => current order object
ORDER_UPDATE_PAYMENT => current order payment object
ORDER_UPDATE_DELIVERY => current order delivery object
ORDER_SIGN_PAYMENT => { orderPayment,  transactionContext }
ORDER_REMOVE => { orderId }
ASSORTMENT_CREATE => { assortment }
ASSORTMENT_ADD_FILTER => { assortmentFilter }
ASSORTMENT_ADD_LINK => { parentAssortmentId, childAssortmentId }
ASSORTMENT_ADD_PRODUCT => { assortmentProduct }
ASSORTMENT_REMOVE => { assortmentId }
ASSORTMENT_REMOVE_FILTER => { assortmentFilterId }
ASSORTMENT_REMOVE_LINK => { assortmentLinkId }
ASSORTMENT_REORDER_PRODUCTS => {assortmentProducts}
ASSORTMENT_REORDER_FILTERS => {assortmentFilters}
ASSORTMENT_REORDER_LINKS => {assortmentLinks}
ASSORTMENT_SET_BASE => {assortmentId}
ASSORTMENT_UPDATE => {assortmentId}
ASSORTMENT_UPDATE_TEXTS => {assortmentId, assortmentTexts }
PRODUCT_ADD_ASSIGNMENT => {productId, proxyId}
PRODUCT_ADD_MEDIA => {productMedia}
PRODUCT_REVIEW_ADD_VOTE => {productReview}
PRODUCT_CREATE => {product}
PRODUCT_CREATE_BUNDLE_ITEM => {productId}
PRODUCT_REVIEW_CREATE => {prodcutReview} 
PRODUCT_CREATE_VARIATION => {productVariation}
PRODUCT_VARIATION_OPTION_CREATE => {productVariation}
PRODUCT_REMOVE_BUNDLE_ITEM => {productId, item}
PRODUCT_REMOVE => {productId}
PRODUCT_REMOVE_ASSIGNMENT => {productId}
PRODUCT_REMOVE_MEDIA => {productMediaId}
PRODUCT_REMOVE_REVIEW => {productReviewId}
PRODUCT_REMOVE_REVIEW_VOTE => {productReviewId, type, userId}
PRODUCT_REMOVE_VARIATION => {productVariationId}
PRODUCT_REMOVE_VARIATION_OPTION => { productVariationId, productVariationOptionValue }
PRODUCT_REORDER_MEDIA => {productMedias}
PRODUCT_UNPUBLISH => {product}
PRODUCT_PUBLISH => {product}
PRODUCT_UPDATE => {productId, type, ...rest}
PRODUCT_UPDATE_MEDIA_TEXT => {productMedia, mediaTexts}
PRODUCT_UPDATE_REVIEW => {productReview}
PRODUCT_UPDATE_TEXTS => {product, productTexts}
PRODUCT_UPDATE_VARIATION_TEXTS => {productVariation, productVariationTexts}
BOOKMARK_CREATE => {bookmarkId}
BOOKMARK_REMOVE => {bookmarkId}

inputs....looks like there needs to be a lot more decision-making required from the group per module on what the emitted format should be and the content of the event.
I've implemented the above events and we can discuss them over this

[pozylon]

ORDER_UPDATE in general is missing (changed contact, billing, meta, ...) + ORDER_SET_PAYMENT_PROVIDER + ORDER_SET_DELIVERY_PROVIDER

[pozylon]

I wonder what's better when naming events:
ORDER_UPDATED or ORDER_UPDATE?

I think the Event Payload for orders should mostly just contain id's rather than actual objects. The code that hooks into an event must have access to the unchained "context" that is also passed to apollo.

So it's like onEvent({ orderId }, futureSuperContext) => futureSuperContext.modules.orders.findOrder({ orderId })... to get the object

[schmidsi]

I wonder what's better when naming events:
ORDER_UPDATED or ORDER_UPDATE?

Usually, event names are present tense. See: https://nodejs.org/api/http.html#http_class_http_clientrequest

I think the Event Payload for orders should mostly just contain id's rather than actual objects. The code that hooks into an event must have access to the unchained "context" that is also passed to apollo.

So it's like onEvent({ orderId }, futureSuperContext) => futureSuperContext.modules.orders.findOrder({ orderId })... to get the object

Hmm, I had in mind that the event payload should be a JSON (no functions) and should contain all relevant data. Relevant is: So that a read only observer that observes all events of the system should be able to rebuild the state of the system at any point in time (time travel).

If you put only the ID and you query for this id later (from next tick up to minutes, hours or days), the information might already have changed.

[pozylon]

Yeah I see what you mean. 👍Objects would allow for retrospectivity which is what we need.

[Mikearaya]

based on this event names should be CamelCase but I think we are following our own standards when we are using caps. when it comes to the name I also think they should define the action performed (ORDER_UPDATED) rather than the action requested (ORDER_UPDATE).
like Simon said I think limiting the type of user that can read the actual data by only emitting id and letting the user query it from unchained might not be necessary and we could send the actual object the event-related to. the only benefit I see from it is controlling restricted users otherwise anyone can subscribe to an event and get the information they want.

[Mikearaya]

forgot to put the link https://basarat.gitbook.io/typescript/main-1/typed-event

[schmidsi]

based on this event names should be CamelCase but I think we are following our own standards when we are using caps. when it comes to the name I also think they should define the action performed (ORDER_UPDATED) rather than the action requested (ORDER_UPDATE).

I think we should follow community standards if possible. But if you have a really good argument, why we should use UPPER_CASE and past tense, go on.

like Simon said I think limiting the type of user that can read the actual data by only emitting id and letting the user query it from unchained might not be necessary and we could send the actual object the event-related to. the only benefit I see from it is controlling restricted users otherwise anyone can subscribe to an event and get the information they want.

Access control happens at the resolver level. Below, everything is open.

... we could send the actual object the event-related to.

... after the change and only the serialisable part (JSON) of it.

[pozylon]

Upper case kind of signals to me that it is an enum of some kind and it gets highlighted by the IDE as such. And it's usually the style when you define types and enums in GraphQL. Event names for me are to be handled like enums.

PascalCase is obviously also an option 😂 no just joking I don't care. I could personally live with present style because order_checkedout sucks compared to order_checkout

[pozylon]

@schmidsi which community standards about event names do you refer to?

[Mikearaya]

@pozylon https://basarat.gitbook.io/typescript/main-1/typed-event

[schmidsi]

@schmidsi which community standards about event names do you refer to?

I just quickly looked over the event names here: https://nodejs.org/api/http.html#http_class_http_clientrequest I don't know if there is a guide. Personally, I like the UPPER_CASE also. Present tense is just shorter.

[Mikearaya]

@schmidsi @pozylon 1 & 2 done ready for review and move on to 3

[Mikearaya]

Failing test are because of rebase with master

[Mikearaya]

@schmidsi ? you suggest interface over abstract class ?

[schmidsi]

@schmidsi ? you suggest interface over abstract class ?

I'm not a fan of classes in general, but sometimes they make sense. So I do not have a strong opinion there.

I meant following the node.js standard and use emit instead of publish. And on instead of subscribe and off instead of unsubscribe and once for only get the next event'

[pozylon]

From what I see now, I'd suggest some more tasks. I know that most of the following is not part of the steps in the issue but I think that stuff should be part of such a feature, no?

Admin Features:

• Queries that allow to query events (only admin by default)
• Controlpanel Views to view events (only admin by default)
• (GraphQL subscription to subscribe to the event stream (only admin by default))

Additional plugins:

• Out-of-the-box redis pub/sub plugin because we already use redis heavily for apollo and thumbor caching

And:

• EventDirector should log event emit and subscription with verbose or debug log level
• Matomo should log fetch url's together with the event it subscribed with debug log level

And I'm wondering if PAGE_VIEW also catches the browser version from req?

[pozylon]

Else, structure wise I have nothing more to say, looks very good!

[Mikearaya]

Thanks i can work with this

[Mikearaya]

I've added events to some of the other modules now. about testing Redis, yes I can get it up and running in CI but the flow is different because it's a fire and forget where do you test for the event? but this can wait for the next round.

[pozylon]

@Mikearaya I'll do the last minor fixes and merge it, great work!