-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
3a4b5ef
to
302af69
Compare
Hi @AmeliaCelline |
Hi, I decided to add more tasks, so for now, consider it incomplete |
Sure, then let's mark it as draft PR. Thanks |
…ty and reconstructing tasks.json
4618738
to
20a2dbb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AmeliaCelline cc @donch1989
It is good you make a document to help me quickly understand HubSpot!
And, in tasks.json, I can see that you think clearly about the product design.
I left product opinions in json schema & coding opinions in the go files.
Today, I do not have enough time (& energy) to check clearly about your implementation.
After you check the opinions and modify the code, in the next stage, I will review the following sections.
- If the coding styles follows what I mentioned?
- If test code is well-structured?
- If the user story can be done under the current product design?
In the implementation part, I think I won't check it too details if the 3 things we checked.
For the implementation part, I will quickly go through it in the next review.
p.s.
Some of the comments are opinions not decisions.
I believe you are already the profession in this domain because you research it longer than us.
So, I'd follow your decision if you have a clear & convincible reasons for each comment.
"TASK_GET_CONTACT", | ||
"TASK_CREATE_CONTACT", | ||
"TASK_GET_DEAL", | ||
"TASK_CREATE_DEAL", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question
It seems the design here is like
- Get Contact
- If there is no Contact, Create Contact (with company info)
- Get Company
- If there is no Company, Create Company
- Get Deal
- If there is no Deal, Create Deal
- Get Ticket
- If there is no Ticket, Create Ticket
- Get Thread
- If there is update from the conversation, Insert Message.
So, now, Does it seem we miss insert Message to Thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question
As a sales manager, he/she wants to supervise the status of each client.
If you fake the sales tool component first, is your design able to fulfil this story request?
If my understanding is correct, we may miss company-id in Contact object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question
Do we need TASK_UPDATE_DEAL for this story?
Question
It seems the design here is like
I will answer these questions together.
-
First regarding the design. Initially when I was designing the component, I was thinking, how does HubSpot component fit together with the other already-made components and future possible components. For the "create" tasks, such as "create deal" and "create contact", I thought users will benefit from this when "Run on Event" pipeline is ready. Example: A WhatsApp message from a new client -> Create a new contact in HubSpot. For the "get" tasks, it is mainly for summarization purpose. With an email of the client, all the information about the client is obtainable (using the retrieve association task), then afterward can use LLM to summarize all those information.
-
Second, regarding the TASK_UPDATE_DEAL. I thought update was not needed because I don't see the use case of it. I don't see how the output of text summarization will lead to the need of update.
So, now, Does it seem we miss insert Message to Thread?
Yes, I was debating on this as well. I think I will create it after all.
If you fake the sales tool component first, is your design able to fulfil this story request?
If my understanding is correct, we may miss company-id in Contact object
Yes, I think you are right after all. I will provide options for the user to be able to create contact -> object association (including company).
I am not sure what you mean by fake the sales tool, feel free to elaborate it thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second, regarding the TASK_UPDATE_DEAL. I thought update was not needed because I don't see the use case of it. I don't see how the output of text summarization will lead to the need of update.
Maybe I missed some points. But, as I known, deal means the case that a sales is processing.
So, it means when a sales have a update with their clients in WhatsApp, the deal will be updated according to their message.
e.g. when a client said "I have sent the contract back to you", it means the deal should be updated as "contracting status".
Is this correct under this domain or industry?
I am not sure what you mean by fake the sales tool, feel free to elaborate it thanks.
Because we do not have any Sales tool component like WhatsApp, the story must not be done by this PR.
So, fake it means "we assume we have WhatsApp, after we add HubSpot, could we fulfil this story?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I missed some points. But, as I known, deal means the case that a sales is processing.
So, it means when a sales have a update with their clients in WhatsApp, the deal will be updated according to their message.
Yes, I think you are right. The update task does have its use case once more sales tool component is integrated. I think I can make "Update Ticket task" and "Update Deal task" as soon as I am done with WhatsApp "send message task". I believe once these 2 additional tasks are completed, the HubSpot design should be able to fulfill the story.
Note: Just wanted to let you know that unfortunately, the WhatsApp component will not have "receive message" task, as it can only be implemented through webhook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chuang8511 Maybe it is better if I create a new PR specifically for those 2 update tasks? I think this PR is getting too big
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to make sure if we lack these 2 tasks, can you build a demo pipeline like I mentioned in Slack?
If yes, I think you can do it in another PR.
If no, I think you need to do it with this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the WhatsApp component will not have "receive message" task, as it can only be implemented through webhook.
Does it mean there is no API to fetch users' message in a Chatroom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to make sure if we lack these 2 tasks, can you build a demo pipeline like I mentioned in Slack?
If yes, I think you can do it in another PR.
If no, I think you need to do it with this PR.
Yes, I believe I can build a demo pipeline without those two tasks.
Does it mean there is no API to fetch users' message in a Chatroom?
From my understanding so far, the official WhatsApp Business Platform API cannot retrieve user messages in a chatroom. The only way to get the content of the message is when we receive the message notification sent to the webhook endpoint that we set up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AmeliaCelline
I left some comments. Please take a look.
If time is not enough, those are not blockers.
We can improve them in the next PR.
Others LGTM.
Please make sure you have gone through all use cases with the pipelines!
Thanks for your contribution!!!
@chuang8511 @AmeliaCelline |
🤖 I have created a release *beep* *boop* --- ## [0.24.0-beta](v0.23.0-beta...v0.24.0-beta) (2024-07-31) ### Features * add audio operator ([#236](#236)) ([fe8abff](fe8abff)) * add handler to auto-fill missing default values ([#210](#210)) ([dcad3f0](dcad3f0)) * add HubSpot component ([#199](#199)) ([b3936a8](b3936a8)) * add Jira component ([#205](#205)) ([51f3ed7](51f3ed7)) * add Ollama component ([#224](#224)) ([810f850](810f850)) * add sql component ([#193](#193)) ([9a373f3](9a373f3)) * add token count for each chunk ([#235](#235)) ([bb69104](bb69104)) * add video operator to fulfil unstructured data process ([#238](#238)) ([a1459d7](a1459d7)) * **document:** add docx doc pptx ppt html to transform to text in markdown format ([#232](#232)) ([2932db9](2932db9)) * **document:** move ConvertToText task from text operator to document operator ([#248](#248)) ([699ca70](699ca70)) * introduce event handler interface ([#253](#253)) ([9599b42](9599b42)) * **restapi:** recategorize the restapi component as a generic component ([#249](#249)) ([fbfc3a3](fbfc3a3)) * **website:** add scrape sitemap function ([#239](#239)) ([8648326](8648326)) ### Bug Fixes * bug of duplicate document ([#256](#256)) ([e028a6e](e028a6e)) * bug of json without setting array for images ([#259](#259)) ([4aeae69](4aeae69)) * change md format to html tag for correct frontend link ([#240](#240)) ([7e16b2b](7e16b2b)) * revert the alias because they are same as package name ([#243](#243)) ([1d9c42d](1d9c42d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Because
This commit
For more information about the task and using it in local VDP environment:
https://docs.google.com/document/d/1dP4cDxdPQfGu48oY5fFq9AlirP-osdRVJmulZ_1Ib1M/edit?usp=sharing
TODO:
After PR