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

CRM Schemas/Properties CRUD support #17

Merged
merged 1 commit into from
Apr 27, 2023
Merged

Conversation

sblackstone
Copy link
Contributor

@sblackstone sblackstone commented Mar 13, 2023

Hello again!

This patch adds CRUD support for the Hubspot "schemas" API.. (crm/v3/schemas).

Edit: Properties resource is now included.

https://developers.hubspot.com/docs/api/crm/crm-custom-objects

https://developers.hubspot.com/docs/api/crm/properties

Thanks,
Stephen

@sblackstone
Copy link
Contributor Author

As with the previous PR, all the tests have t.SkipNow()

You should be able to run them in order, e.g. Create, List, Get, Update, Delete and they should reference the correct test schema.

Hubspot does require some time between some of these things (e.g. create with an immediate get fails, or immediate update) so I haven't left them as active.

This feature also requires an enterprise account.

@sblackstone sblackstone force-pushed the CrmSchemas branch 2 times, most recently from 8b5dce6 to b008e61 Compare March 13, 2023 20:34
@sblackstone sblackstone changed the title CRM Schemas CRUD support CRM Schemas/Properties CRUD support Mar 13, 2023
@kk-no kk-no added the enhancement New feature or request label Mar 14, 2023
@kk-no kk-no self-requested a review March 14, 2023 10:44
@kk-no
Copy link
Member

kk-no commented Mar 15, 2023

Thank you @sblackstone !
I'll check it later this week, so please wait a moment.

Copy link
Member

@kk-no kk-no left a comment

Choose a reason for hiding this comment

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

Added a PR comment! Please check.
+ I'm updating the belong-inc:main branch, so I'd appreciate it if you could take it in.

crm_properties.go Outdated Show resolved Hide resolved
crm_properties.go Outdated Show resolved Hide resolved
crm_properties.go Outdated Show resolved Hide resolved
crm_schemas.go Outdated Show resolved Hide resolved
crm_schemas.go Outdated Show resolved Hide resolved
@sblackstone sblackstone force-pushed the CrmSchemas branch 2 times, most recently from 7dceb24 to 8ca9a8e Compare March 21, 2023 15:36
@sblackstone
Copy link
Contributor Author

I've attempted to address these issues, all of them should either be resolved or have additional comments..

Thanks!
Stephen

Copy link
Member

@kk-no kk-no left a comment

Choose a reason for hiding this comment

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

Thank you for your response! Sorry for the delay in responding.
I left an additional comment.

crm_properties.go Outdated Show resolved Hide resolved
crm_properties.go Outdated Show resolved Hide resolved
crm_schemas.go Outdated Show resolved Hide resolved
crm_properties.go Outdated Show resolved Hide resolved
crm_properties.go Outdated Show resolved Hide resolved
@sblackstone
Copy link
Contributor Author

I just pushed an update which moves everything to using pointers and wrapped types.

@sblackstone
Copy link
Contributor Author

I believe everything requested here is done... I squashed all the commits...

@sblackstone sblackstone force-pushed the CrmSchemas branch 2 times, most recently from 9cbd710 to 6945458 Compare April 3, 2023 12:29
@kk-no
Copy link
Member

kk-no commented Apr 10, 2023

Thanks for the fix.
I will be able to get back to you with feedback today/tomorrow! Sorry for the wait.

Copy link
Member

@kk-no kk-no left a comment

Choose a reason for hiding this comment

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

Thank you for your fix.
I have reviewed the whole thing and there are a few things I was wondering about around structure definitions. Please check them.

crm_properties_test.go Outdated Show resolved Hide resolved
crm_properties.go Outdated Show resolved Hide resolved
crm_properties.go Show resolved Hide resolved
crm_properties.go Show resolved Hide resolved
crm_properties.go Show resolved Hide resolved
crm_schemas.go Show resolved Hide resolved
crm_schemas.go Outdated Show resolved Hide resolved
@sblackstone
Copy link
Contributor Author

Thanks for the replies, it may be a few days before I can update this...

Stephen

@sblackstone
Copy link
Contributor Author

sblackstone commented Apr 19, 2023

Hello,

Everything here has been updated..

Incidentally, there are some inconsistencies in the Hubspot documentation about what constitutes certain objects. For example,

	Cardinality        *HsStr  `json:"cardinality"`
	InverseCardinality *HsStr  `json:"inverseCardinality"`

in a CrmSchemaAssociation these are not mentioned in the docs for GetSchema but are present.

Another example, CrmSchema can return PortalID in some instances but not others.

I've tried to match everything up best I can..

@sblackstone
Copy link
Contributor Author

I've rebased/squashed this PR..

Copy link
Member

@kk-no kk-no left a comment

Choose a reason for hiding this comment

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

Thank you for your response!
If you execute make fmt and go mod tidy, I think there will be no problem.

Thank you for taking the time to fix this.

go.mod Outdated Show resolved Hide resolved
@sblackstone
Copy link
Contributor Author

Removed spew and ran go fmt

@kk-no kk-no merged commit 74c46a1 into belong-inc:main Apr 27, 2023
@kk-no
Copy link
Member

kk-no commented Apr 27, 2023

@sblackstone
Thank you!! ⭐⭐⭐

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants