add missing properties to EventTo

[adelinn]

We need this SDK to send some notifications to a large number of subscribers. Before sending we check if each subscriber that we want to send to exists and we create it if not. This means we make large number of unnecessary API calls to Novu since it already does that when triggering an event...

[toddb]

I don't see any changes to the tests. Have you run them and checked they still pass. Does it need more tests.

I am particularly concerned about a field called Data with an object type and with no tests to show that this actually works and whether there is the need for serialisers.

[adelinn]

I updated the type of Data to dynamic so that one can easily access the properties in there, but I don't know if that's a good practice.
I also wrote a test. Hope this is the intended way to write it, I'm not that familiar with C# actually...
The test passed locally.

Also running JsonConvert.DeserializeObject<EventTo>("{\"subscriberId\":\"1234\",\"email\":\"1234\",\"firstName\":\"1234\",\"lastName\":\"1234\",\"phone\":\"1234\",\"avatar\":\"1234\",\"locale\":\"1234\",\"data\":{\"test\":{\"asd\":\"123\"},\"aaa\":\"llll\"}}"); doesn't trigger any errors so I assume using dynamic should be fine.

[toddb]

Thanks for the test, the use of the dynamic property looks like arbitrary data is always supplied. I don't understand why you have chosen this data and what it is trying to express.

1. Why is it only on the Update and not a create?
2. I don't see any documentation links back to where this is explained in the documentation—why is it snake case caps (for example)

[adelinn]

I added a test for Create too. It passes but I actually don't know if it tests the right thing.
The point was to test if using the dynamic data type works for the Data field which the API specification suggests it can be any object. SubscriberPayloadDto https://api.novu.co/api#model-SubscriberPayloadDto
To write the test I just copy-pasted a previous related test and slightly modified it...

The behavior for creating a subscriber when triggering an event that contains subscriber contact information is described here https://docs.novu.co/subscribers/subscribers#2-inline-of-trigger
Testing showed that a subscriber gets updated if the subscriber id exists already.

[toddb]

@adelinn thanks for the next move—I hope this isn't delaying a production environment release.

A couple of things as I look across the project:

• you've introduced dynamic but we've used object, I wouldn't have inconsistent approach (one or all)
• your tests don't really show how to use the data because you poke it but don't read it—you need to think in static language and show lifecycle. So, I could never imagine writing the code you have in the test project because those setting need to be bound to something. Does that make sense?
• I can't see the notes you made in this thread to direct me in the code base—I would expect to see them inline for future reference (honestly, we haven't done that well so far either)

There is one thing to note that IMO is important. This SDK is trying to build a client binding that is understandable inside a code base rather than simply be a direct binding to the http interfaces. It requires a bit of work from our side.

[adelinn]

I'm giving up on it, me and a colleague spent a few hours today trying to understand how the tests work and why it doesn't update the subscriber data and we weren't able to fix it so I just fall backed to List<AdditionalData> as type for Data and no extra tests.

If this isn't enough for merge, I'm fine with anyone using my code however they want.

[Bernton]

The problem is that the "Acknowledged" does not yet mean that the data has been updated, the update of subscriber seems to happen later, if you put a await Task.Delay(...); between trigger.Data.Acknowledged.Should().BeTrue(); and your await Subscriber.Get(...), it will actually work.

[toddb]

@adelinn

I'm giving up on it, me and a colleague spent a few hours today trying to understand how the tests work

Hey, sorry it is so hard for you. This testing is hard (for me too) and brittle but once you get it lined up it works for everyone.

I am really happy to provide really specific guidance if you show me test data and errors.

As an aside, I am trying to get back to the code to roll out all the changes. I apologise.

[toddb]

@Bernton

await Task.Delay(...); between trigger.Data.Acknowledged.Should().BeTrue(); and your await Subscriber.Get(...)

"work" in inverted commas. Delays in tests and code lead to pain.

Semantically it sounds like an HTTP 202 and will need retries. Any chance of returning the canonical resource?

[adelinn]

But anyways, does the code in this PR really need tests? This is the trigger event endpoint of the SDK, should it care whether the event succeeded or not as long as it was triggered? If yes, that would be kind of out of the scope of this PR since this PR only adds additional properties to the EventTo record. Maybe a test to check if the record is serializable should be made, but there are already other tests that prove that (e.g. the tests for Subscriber endpoints).