-
Notifications
You must be signed in to change notification settings - Fork 17
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: re-fetch campaign details after campaign is sent #1116
Conversation
Even though sent_at, created_at, and status_updated_at are represented as Dates in the backend, these fields will be serialized to an ISO string which the frontend then receives. Hence, correct the type definition of these fields.
When sending a campaign for the first time, the `sentAt` field for the in-memory representation of `Campaign` is still undefined. This causes "invalid_Date" to appear in the filename of delivery reports. Fix this by updating the `sentAt` field of the in-memory `Campaign` to the current date when sending the campaign for the first time. Fixes issue #1113
if (sendRate) { | ||
sendUserEvent(GA_USER_EVENTS.USE_SEND_RATE, channelType) | ||
} | ||
updateCampaign({ status: Status.Sending, sentAt: new Date().toISOString() }) |
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'm not sure if we should instead fetch the sent_at
value from the backend instead. In practice, I don't think the disparity between the 2 values will be significant, since the sent_at
value is set immediately in the backend anyway.
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.
Gave some thought to this, would prefer to refetch the campaign details and update campaign
in CampaignContext
. Would prefer not to sacrifice de-sync between local and backend state to save one network call. Was thinking we can make call to backend to refetch campaign
once status changed to Status.Sent
.
Maybe something like that?
useEffect(() => {
let timeoutId: NodeJS.Timeout
async function poll() {
const { status } = await refreshCampaignStats(id)
if (status !== Status.Sent) {
timeoutId = setTimeout(poll, 2000)
} else if (status === Status.Sent) {
const updated = await getCampaignDetails(campaign.id)
setCampaign(updated)
}
}
if (stats.status !== Status.Sent) poll()
return () => {
timeoutId && clearTimeout(timeoutId)
}
}, [id, stats.status, campaign, setCampaign])
To extend this further, can consider refactoring the stats fetching/polling logic in (Email|SMS|Telegram)Detail
component into a custom hook.
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.
That sounds good. I was also a little irked by the implication of de-sync. I'll explore your solution as well as the refactor.
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.
Also, I also have some questions regarding possible de-sync of Campaign
state between the frontend and backend at various steps during campaign creation. I'll look to message you during the weekdays about it :D
Note: make `refreshCampaignStats` a memoized callback to satisfy the useEffect dependencies.
Instead, we'll get the actual sentAt from the source. This reverts commit 2abf7b3.
sentAt
value when sending a campaignconsole.error( | ||
'sentAt is undefined. Using current Date() as a fallback.' | ||
) | ||
sentAtTime = new Date() |
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.
lgtm! Tested the following for each channel and works as expected:
- Download delivery report without refresh on campaign details page
- Download delivery report without refresh on dashboard page
* develop: fix(docker/awscli): upgrade to python3 and install python3-dev (#1140) feat(frontend): set up test fixtures and deployment processes (#1086) feat: allow activation of SNS as fallback for all SMS campaigns (#1019) 1.22.4 fix: re-fetch campaign details after campaign is sent (#1116) chore: upgrade dependencies (#1120) chore: fix build warnings (#1103) fix(moment): disable periodic updates for react-moment components (#1127) fix: avoid calling setState when components are unmounted (#1109) chore: add callback server (#1112)
Problem
When a user first sends a campaign,
sentAt
remainsundefined
. This causes the file name of downloaded delivery reports to containinvalid_Date
. This issue doesn't occur after refreshing the page since the refresh causessentAt
to be refetched from the backend.Closes #1113
Solution
Improvements:
onModalConfirm
handler to a common utilility function.Bug Fixes:
sentAt
field to the currentDate
when sending a campaign. This shouldn't be far off from the actual value in the backend, since the actual value is set when creating sending jobs as part of the API request handler for sending a campaign.sentAt
andstatusUpdatedAt
. These are should be of typestring | undefined
.Before & After Screenshots
There are no visual changes.
Tests
Deploy Notes
There are no deploy notes.