-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fix: Fix alias api when email receiver is first created #1411
Fix: Fix alias api when email receiver is first created #1411
Conversation
a22844e
to
4affd02
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.
I think this is actually a UI issue. The UI polls for aliases for agents. It should do the same thing for email aliases.
pkg/api/handlers/emailreceiver.go
Outdated
// We should use the alias if the following is true | ||
// 1. Alias is assigned and set to true | ||
// 2. Alias is not assigned but Alias is set at the initial creation. | ||
if (er.AliasAssigned != nil && *er.AliasAssigned) || (er.AliasAssigned == nil && er.Alias != "") { |
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.
We don't want to do it this way because if er.AliasAssigned == nil
then we may be sending back bad data.
The way this works for agent, for example, is the UI polls to make sure it gets back a value for AliasAssigned
(which should remain a pointer. If that value is not set, then the UI knows it should keep polling until it gets true or false.
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.
Ok. I think the UI only poll once when receiver is created. The problem is that the current logic returns undetermistic result from API when AliasAssigned is set, like the address will change from name@address.com
to alias@address.com
when controller is settled. I think we should return empty email address when er.AliasAssigned == nil and have UI poll until the alias is set. what do you think?
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.
Yep. That sounds good to me!
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.
Changed the code so that when webhook/email receiver's alias is set, it will return empty address until alias is being set. Otherwise it will return the right address depends on whether alias has been set successfully or not.
The change UI will need to make is that it should start polling for alias if alias
is set until AliasAssigned is either false or true.
4affd02
to
4b25f81
Compare
4b25f81
to
c8a3a96
Compare
67ef3d4
to
e820d47
Compare
Signed-off-by: Daishan Peng <daishan@acorn.io>
e820d47
to
4a8a904
Compare
@ryanhopperlowe @ivyjeong13 would appreciate you taking a look at the frontend code piece. |
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.
Sorry had this review started, but never submitted
useEffect(() => { | ||
if ( | ||
emailReceivers && | ||
emailReceivers.some( | ||
(receiver) => receiver.aliasAssigned == null && receiver.alias | ||
) | ||
) { | ||
mutateEmailReceivers(); | ||
} | ||
}, [emailReceivers, mutateEmailReceivers]); |
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.
this seems like it would be better handled with polling
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.
see how that is handled in ~/lib/hooks/knowledge/useKnowledgeFiles.ts
as an example
useEffect(() => { | ||
if (webhookData?.alias && webhookData.aliasAssigned === undefined) { | ||
const interval = setInterval(async () => { | ||
await getWebhookData.mutate(); | ||
}, 1000); | ||
|
||
return () => clearInterval(interval); | ||
} | ||
}, [webhookData?.alias, webhookData?.aliasAssigned]); |
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.
same here, let's use the polling capability built into useSWR
Signed-off-by: Daishan Peng <daishan@acorn.io>
When email receiver is first created in ui, the emailAddress will always return with random string at the start even if alias is set. This is problematic as it will change to use alias email on refresh once controller has set the alias.
#1412