Skip to content
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

Adding initial hyper t schemas. #1401

Closed
wants to merge 4 commits into from
Closed

Adding initial hyper t schemas. #1401

wants to merge 4 commits into from

Conversation

misterpig
Copy link
Contributor

@misterpig misterpig commented May 15, 2024

The table here lists which schema (event and entity) is currently implemented in which environment (Shopify app, DBT models and data gen app).

"maxLength": 255
},
"item_type": {
"type": "string",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this. be a required property or should we allow it to be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So after discussing with the other guys we agree that the item type field should be required.
It is also a join key used in our data models so to ensure that users will use the correct values lest we break any downstream models we've also opted for making it an enum field.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense! I just meant that if it is required then we should have it in the list of required properties (currently there is only item_id)? Otherwise it's in a weird state where we don't accept null for it but it can be missing from payload.

@miike miike requested a review from igneel64 May 16, 2024 09:45
"minimum": 0,
"maximum": 2147483647
},
"subtotal": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel the subtotal, shipping_cost and tax belong in the /order/ schema. Am I right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We think for purpose of analysing behaviour with events before an order is created it'll be helpful to have these fields with the cart entity, so worth to leave them in here as well as on the order/payment entities.

@@ -0,0 +1,28 @@
{
"description": "When the submit_search is completed, this event will be triggered along with the contexts view_collection (the viewing of the search results) and search (the search event parameters).",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tempted to name it search_result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have submit_search probably best to leave that naming convention as is. Also the name search_result denotes that the search returned with some result but it is possible to have searches that have no results.

…efinitions for local refs; added collection_id to click_item event.
@misterpig misterpig requested a review from igneel64 June 17, 2024 23:33
Copy link
Contributor

@matus-tomlein matus-tomlein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple more comments, but otherwise LGTM! This is a huge piece of work! 👏

…_trigger no longer accepts null; order payment_id now required; click_item collection_id now required; added more desc.
Copy link
Contributor

@igneel64 igneel64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, excellent effort 🚀

@misterpig misterpig closed this Jul 1, 2024
@misterpig misterpig deleted the schemas/hyper-t branch July 1, 2024 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants