Go client notes and call logs API support[sc-55271] #67

SoeunSona · 2023-12-13T12:17:11Z

API Specs: https://www.notion.so/chartmogul/API-endpoints-for-adding-notes-call-logs-798b788a08764549a334db5d48d65469?pvs=4

https://app.shortcut.com/chartmogul/story/55271/go-client-notes-and-call-logs-api-support
customer

api.ListCustomerNotes(&cm.ListNotesParams{}, "customerUUID")
api.CreateCustomerNote(&cm.NewNote{}, "customerUUID")

customer notes

api.CreateNote(&cm.NewNote{})
api.RetrieveNote("noteUUID")
api.ListNote(&cm.ListNoteParams{})
api.UpdateNote(&cm.UpdateNote{}, "noteUUID")
api.DeleteNote("noteUUID")

shortcut-integration · 2023-12-13T12:17:14Z

This pull request has been linked to Shortcut Story #55271: Go client notes and call logs API support.

briwa

I have some feedback regarding omitting required fields.

Also @SoeunSona could you request for another person to review? I'm not too comfortable with being the lone reviewer for Golang since I'm not too fluent in it. You may ask for someone else from Integrations, for example, since they work on Golang code too

briwa · 2023-12-14T02:08:25Z

README.md

+
+

Suggested change

briwa · 2023-12-14T02:09:37Z

customers.go

@@ -147,6 +147,7 @@ const (
 	searchCustomersEndpoint        = "customers/search"
 	mergeCustomersEndpoint         = "customers/merges"
 	customerContactsEndpoint       = "customers/:uuid/contacts"
+	customerNotesEndpoin           = "customer_notes"


Suggested change

customerNotesEndpoin = "customer_notes"

customerNotesEndpoint = "customer_notes"

briwa · 2023-12-14T02:10:32Z

customers.go

+	if listCustomerNotesParams != nil {
+		query = append(query, *listCustomerNotesParams)
+	}
+	path := customerNotesEndpoint + "?customer_uuid=" + customerUUID


Why don't you append customer_uuid into the query instead? They're both query params

briwa · 2023-12-14T02:11:21Z

integration_tests/customer_notes_test.go

+	if !reflect.DeepEqual(expectedAllNotes, expectedAllNotes) {
+		spew.Dump(allNotes)
+		spew.Dump(expectedAllNotes)
+		t.Fatal("All notes is not equal!")


Suggested change

t.Fatal("All notes is not equal!")

t.Fatal("All notes are not equal!")

briwa · 2023-12-14T02:17:17Z

notes.go

+	UUID string `json:"uuid,omitempty"`
+	// Basic info
+	CustomerUUID string `json:"customer_uuid,omitempty"`
+	Type         string `json:"type,omitempty"`
+	Text         string `json:"text,omitempty"`
+	Author       string `json:"author,omitempty"`
+	CallDuration uint32 `json:"call_duration,omitempty"`
+	CreatedAt    string `json:"created_at,omitempty"`
+	UpdatedAt    string `json:"updated_at,omitempty"`


I don't think we need omitempty here as these values will always be there from the API (even if it's empty, it will be '' for text or 0 for call_duration). See this article

But maybe my understanding is wrong

briwa · 2023-12-14T02:25:34Z

notes.go

+	CustomerUUID string `json:"customer_uuid,omitempty"`
+	Type         string `json:"type,omitempty"`


Not sure if we should omit empty on these because it should never be empty (obligatory)

briwa · 2023-12-14T02:31:11Z

notes.go

+
+// ListNoteParams = parameters for listing customer notes in API.
+type ListNotesParams struct {
+	CustomerUUID string `json:"customer_uuid,omitempty"`


https://app.shortcut.com/chartmogul/story/55271/go-client-notes-and-call-logs-api-support

daesu · 2023-12-15T16:10:42Z

integration_tests/fixtures/notes.yaml

+    method: POST
+  response:
+    body: '{"id":183831708,"uuid":"cus_9dc5aaf4-99cd-11ee-a3b2-4bab72a136d9","external_id":"ext_customer_1","name":"Test
+      Customer Notes","email":"soeun+staff@chartmogul.com","status":"New Lead","customer-since":null,"attributes":{"custom":{},"clearbit":{},"stripe":{},"tags":[]},"data_source_uuid":"ds_9dad66ec-99cd-11ee-a3b1-272c7e7811c1","data_source_uuids":["ds_9dad66ec-99cd-11ee-a3b1-272c7e7811c1"],"external_ids":["ext_customer_1"],"company":"","country":null,"state":null,"city":"","zip":null,"lead_created_at":null,"free_trial_started_at":null,"address":{"country":null,"state":null,"city":"","address_zip":null},"mrr":0,"arr":0,"billing-system-url":null,"chartmogul-url":"https://app.chartmogul.com/#/customers/183831708-Test_Customer_Notes","billing-system-type":"Import


Maybe you don't want to have this email address on the public repo

We already have few email addresses like this line. But I'm curious if we have a test account for integration testing. I test with localhost with changing our api address this line

But I got errors like this.

=== RUN TestConnectSubscriptions connect_subscription_test.go:34: Request error: Post "http://localhost:3000/api/v1/data_sources": Requested interaction not found

daesu · 2023-12-15T16:11:29Z

notes.go

+	Text         string `json:"text"`
+	AuthorEmail  string `json:"author_email"`
+	CallDuration uint32 `json:"call_duration"`
+	CreatedAt    string `json:"create_at"`


Suggested change

CreatedAt string `json:"create_at"`

CreatedAt string `json:"created_at"`

daesu · 2023-12-15T16:12:33Z

notes.go

+	CreatedAt    string `json:"create_at"`
+	UpdatedAt    string `json:"updated_at"`


We probably don't want to send these timestamps in the request

We have a spec for sending updated_at timestamp for updating a note

Ah I see. This seems like a bad idea, timestamps like these should be auto-generated or at least have a consistent meaning. I presume the reason behind it is there is a need for a timestamp field for something beyond the creation/update of the record in which case it should be a separate column(s).

Can you check to see if this is really the desired behavior?

@daesu The reason is for backfilling notes. Users specifically wanted to have a specific date when they're backfilling. See https://www.notion.so/API-endpoints-for-adding-notes-call-logs-798b788a08764549a334db5d48d65469?d=146bd3b41feb4d4ba436aa8720e184c8&pvs=4#df1e2a9458504230b4101a842d9eccb2 and https://chartmogul.slack.com/archives/C04R1AV2HU4/p1701270734531819.

@briwa I see the requirement from Katarina. Shouldn't we then add something like an external_created_at (and external_updated_at) column to handle the need to set from the API?

If we allow created_at to be set from the API then it loses it's value in terms of auditing, data consistency, and will lead to confusion. What does created_at mean for a record when it's not referring to the timestamp the record was created at? How do we know if it was set by the API or generated automatically?

@daesu I see what you mean.

it loses it's value in terms of auditing, data consistency, and will lead to confusion

In other cases, you might be right, but I'm not too sure if it would lose its value or affect auditing or data consistency too much. Adding two new columns only to identify if the update comes from the API is overkill, IMO. We use the same column (created_at, updated_at) to list the customer notes and also to show them in the UI as a timeline, so if anything, adding the new columns would just simply add unnecessary complexity. If it's just to know if the records have been ever updated by the API, we could either see the log (the endpoint URL is already different than the one in the UI) or we could check our own reporting system which we are flagging it already (example for creating a note).

But this is just my take. Feel free to raise it further if you think this is a security concern, but in my opinion, for this context only, having the same column name and params to be updated for a clear reasoning isn't too much of a harm

@briwa I get what you're saying but I still think it's not ideal. created_at and updated_at are standard columns with a standard meaning. Using fields like external_created_at is also a pretty standard thing to do when we are dealing with sync's from external data sources like this. Some ORM's/Frameworks tools will even automatically create the standard timestamps with predetermined values by default.

I don't think having two extra columns for this would add that much complexity but another option would be to just have the existing columns but rename them. created_at and updated_at could be renamed to something non-standard. For example, note_created_at, note_updated_at. This means we wouldn't have the confusion with the standard columns.

I'll leave it to you and @SoeunSona though to decide.

So I'm thinking about changing the column name, as Paul suggested.

As a payload,

type, text, note_created_at, note_updated_at, ....

For response,

type, text, created_at, update_at ...

But if we need to think about consistency, or if you have different ideas, please feel free to let me know!

@SoeunSona @daesu

changing the column name

I'm still not too sure about renaming the column. In our system (Rails), created_at and updated_at is pretty much the standard to know if a record is created and updated. The references are everywhere so changing the name, while it works the same and not too different (created_at -> note_created_at), still feels unnatural to me, and to change it just to cater to this edge case (backfilling) is actually a lot to change.

I saw Sona's branch on changing the column but that isn't just it -- we need to change the references on the response serializer in the UI, how it's sorted in the customer timeline query response, how it's sorted in the API, how it's being used in the client app, etc...

As I mentioned above, ideally in practice it's not a bad idea and if we wanted to be strict, if customer notes are something that would have an impact on data accuracy or included in the reports, we should probably do something different indeed. But in this context, I still think that updating the columns directly is harmless enough for a note related to a customer.

I'd advise to stick to the original spec and see how the users use it first. Or we could discuss it in a bigger setting instead of here. This is a public repo, anyway. Something like this would probably have more benefit to be discussed internally

notes.go

integration_tests/customer_notes_test.go

SoeunSona force-pushed the soeunlee/sc-55271/go-client-notes-and-call-logs-api-support branch 4 times, most recently from c18d0b1 to 7449e2b Compare December 13, 2023 15:50

SoeunSona marked this pull request as ready for review December 13, 2023 15:51

SoeunSona requested a review from briwa December 13, 2023 15:51

briwa reviewed Dec 14, 2023

View reviewed changes

Go client notes and call logs API support[sc-55271]

c850433

https://app.shortcut.com/chartmogul/story/55271/go-client-notes-and-call-logs-api-support

SoeunSona force-pushed the soeunlee/sc-55271/go-client-notes-and-call-logs-api-support branch from 7449e2b to 9df4dde Compare December 14, 2023 21:58

SoeunSona changed the base branch from v3 to v4 December 14, 2023 21:59

Change code after code review

18f08cb

SoeunSona force-pushed the soeunlee/sc-55271/go-client-notes-and-call-logs-api-support branch from 9df4dde to 18f08cb Compare December 14, 2023 22:02

SoeunSona requested review from daesu and briwa December 15, 2023 11:38

daesu requested changes Dec 15, 2023

View reviewed changes

SoeunSona added 2 commits December 15, 2023 17:56

Change code after code review

ea03cdc

change cursor and hasmore after version changes

4747e09

daesu approved these changes Dec 18, 2023

View reviewed changes

briwa approved these changes Dec 19, 2023

View reviewed changes

SoeunSona merged commit 8f97991 into v4 Dec 19, 2023

SoeunSona deleted the soeunlee/sc-55271/go-client-notes-and-call-logs-api-support branch December 19, 2023 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go client notes and call logs API support[sc-55271] #67

Go client notes and call logs API support[sc-55271] #67

SoeunSona commented Dec 13, 2023 •

edited

shortcut-integration bot commented Dec 13, 2023

briwa left a comment

briwa Dec 14, 2023

briwa Dec 14, 2023

briwa Dec 14, 2023

briwa Dec 14, 2023

briwa Dec 14, 2023

briwa Dec 14, 2023

briwa Dec 14, 2023

daesu Dec 15, 2023

SoeunSona Dec 15, 2023

daesu Dec 15, 2023

daesu Dec 15, 2023

SoeunSona Dec 15, 2023

daesu Dec 15, 2023

briwa Dec 18, 2023

daesu Dec 18, 2023

briwa Dec 18, 2023

daesu Dec 18, 2023

SoeunSona Dec 18, 2023 •

edited

briwa Dec 19, 2023

	customerNotesEndpoin = "customer_notes"
	customerNotesEndpoint = "customer_notes"

	t.Fatal("All notes is not equal!")
	t.Fatal("All notes are not equal!")

		CustomerUUID string `json:"customer_uuid,omitempty"`
		Type string `json:"type,omitempty"`

	CreatedAt string `json:"create_at"`
	CreatedAt string `json:"created_at"`

		CreatedAt string `json:"create_at"`
		UpdatedAt string `json:"updated_at"`

Go client notes and call logs API support[sc-55271] #67

Go client notes and call logs API support[sc-55271] #67

Conversation

SoeunSona commented Dec 13, 2023 • edited

shortcut-integration bot commented Dec 13, 2023

briwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoeunSona Dec 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoeunSona commented Dec 13, 2023 •

edited

SoeunSona Dec 18, 2023 •

edited