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

Add additional information to Email model #2024

Merged
merged 4 commits into from
Nov 7, 2023
Merged

Conversation

KludgeKML
Copy link
Contributor

Adds content_id and notify_status columns to the Email model, and the support code to populate them - the content_id from the immediate email builder and the notify_status from the callback endpoint. Also adds an index on the existing email id field so that we have a fast lookup during the callback endpoint rather than a field scan.

https://trello.com/c/xdMu72UJ/2245-add-new-information-to-email-alert-api-email-model

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

@KludgeKML KludgeKML requested a review from 1pretz1 November 2, 2023 15:32
Copy link
Contributor

@1pretz1 1pretz1 left a comment

Choose a reason for hiding this comment

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

Looks good to me - let a couple minor comments. I think you're probably right about updating the notify status synchronously but agreed we should test it 👍

app/builders/immediate_email_builder.rb Outdated Show resolved Hide resolved
@@ -23,6 +23,13 @@ def create
UnsubscribeAllService.call(subscriber, :non_existent_email) if subscriber
end

begin
Email.where(id: reference).update!(notify_status: status)
Copy link
Contributor

Choose a reason for hiding this comment

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

Email.where(id: reference).update!(notify_status: status) -> Email.find(reference).update!(notify_status: status)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... I have one very mild reservation about this, which is that find returns an array and I can't call explain on it to check how it's doing the find. But I'm going to assume it does the right thing behind the scenes, so I'll make the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think find will only return an array if an array is passed to it, passing just a string should just return the email object (if it exists), but yeah you can't find the query information from the return object. However, I'd be very surprised if find didn't use the newly created index (assume that's what you're verifying). Could also check this via Postgres logs.

Here we're just doing it straight off in the callback. This is probably quick enough (I suspect adding the worker infrastructure to
queue it might make it slower), but we
should confirm that.
@KludgeKML KludgeKML force-pushed the add-content-id-to-email branch from 50be8af to e4ef5f2 Compare November 3, 2023 12:14
@KludgeKML KludgeKML merged commit a2058a3 into main Nov 7, 2023
4 checks passed
@KludgeKML KludgeKML deleted the add-content-id-to-email branch November 7, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants