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

Add com.delighted schemas #660

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

miike
Copy link
Contributor

@miike miike commented Oct 29, 2017

This PR adds two schemas for the delighted.com NPS tool

  • Survey response created for when a survey response has been created (i.e., a NPS response has been received with an associated score)
  • Survey response updated for when a survey response has been updated (i.e., a NPS response was recorded with a score and an optional comment)

@snowplowcla
Copy link

@miike has signed the Software Grant and Corporate Contributor License Agreement. Thanks so much

Copy link
Member

@alexanderdean alexanderdean left a comment

Choose a reason for hiding this comment

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

The schema smash is off to a great start! I've left some feedback. Can you also split the PR into two commits, one each for #662 and #663

"$.data.event_data.person.phone_number",
"$.data.event_data.person_properties.Browser",
"$.data.event_data.person_properties.Country",
"$.data.event_data.person_properties.Device Type",
Copy link
Member

Choose a reason for hiding this comment

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

I am sure there's a good reason for going with these property names, but the spaces make me very nervous. How difficult would it be to conform them to the rest of the fields, like device_type or user_agent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment unfortunately the webhook sends these fields through with spaces so without an adapter we're stuck with the spaces.

"description": "Responder name"
},
"created_at": {
"type": "integer",
Copy link
Member

Choose a reason for hiding this comment

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

Using integers for all the timestamps is going to bleed complexity downstream into all consumers of these events. How difficult would it be to convert these in the adapter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've thought about the best way to do this a bit. A large number of webhooks just give epoch timestamps rather than nicely formatted ISO datetimes. It's possible to do these quite easily in the adapter but it does mean that all webhooks with epochs require adapters.

},
"phone_number": {
"type": ["string", "null"],
"description": "Responser mobile phone number",
Copy link
Member

Choose a reason for hiding this comment

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

Typo ("responser")

},
"permalink": {
"type": "string",
"maxLength": 255,
Copy link
Member

Choose a reason for hiding this comment

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

Feels a bit short? Also, consider the "format": "uri" option

},
"user_id": {
"type": ["string", "null"],
"maxLength": 100,
Copy link
Member

Choose a reason for hiding this comment

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

"ref_root" VARCHAR(255) ENCODE ZSTD NOT NULL,
"ref_tree" VARCHAR(1500) ENCODE ZSTD NOT NULL,
"ref_parent" VARCHAR(255) ENCODE ZSTD NOT NULL,
"event_data.comment" VARCHAR(512) ENCODE ZSTD,
Copy link
Member

Choose a reason for hiding this comment

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

I suspect a lot of these fields should be NOT NULL - to trigger this in Igluctl, you will need to add those fields into the required: [] array.

@alexanderdean alexanderdean changed the title Add com.delighted/[survey_response_created,survey_response_updated]/jsonschema/1-0-0 Add com.delighted schemas Oct 30, 2017
@alexanderdean
Copy link
Member

Hi @miike - where did we get to with this one. Did you want us to write an adapter for it?

@miike
Copy link
Contributor Author

miike commented Jan 4, 2018

I'm part way through writing an adapter for some Vero schemas and I might tackle this one after that.

@alexanderdean
Copy link
Member

If you want a AMA session with one of our data engineers on writing adapters just ping me an email!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants