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

Send one mail instead of n mail to supporter on checkout of n grants #8258

Merged
merged 3 commits into from
Feb 1, 2021

Conversation

tarunbhm
Copy link
Contributor

Description
  • Made process_grant_contribution task return grant and created subscription objects
  • Added batch_process_grant_contributions task to process multiple grants in one task
  • Added batch_thank_you_for_supporting method to render and send email for multiple grant subscriptions
  • Added batch_thank_you_for_supporting.html and batch_thank_you_for_supporting.txt templates
  • Updated grants/views.py to use new batch_process_grant_contributions task
Refers/Fixes

Fixes: #8238

Testing

- Made process_grant_contribution task return grant and created subscription objects
- Added batch_process_grant_contributions task to process multiple grants in one task
- Added batch_thank_you_for_supporting method to render and send email for multiple grant subscriptions
- Added batch_thank_you_for_supporting.html and batch_thank_you_for_supporting.txt templates
- Updated grants/views.py to use new batch_process_grant_contributions task

Fixes: gitcoinco#8238
@thelostone-mc
Copy link
Member

could you add a video @tarunbhm ?

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

@tarunbhm code wise this make sense !
Although as opposed to adding new email template, could we replace the old ones ?
thank_you_for_supporting is used in one more place in helper.py

This is when grants are created using CELO / ZIL address

@tarunbhm
Copy link
Contributor Author

tarunbhm commented Jan 19, 2021

@thelostone-mc There is no frontend change so did not add video. In case it's still required do you want to see mail in the video? I had tested by adding logs for generated mails and had verified that only one mail was generated. I wan to avoid creating an account on sendgrid.

@thelostone-mc
Copy link
Member

@tarunbhm the email being sent out is new code added ->. they does qualify as having frontend changes
Plus having a video attached -> make it easier for the non-engineers to review the flow as well

I had tested by adding logs for generated mails and had verified that only one mail was generated.

I would be a lot more comfortable if we could have this tested out as well cause having to rollback in cause is something was wrong in the email is painful

@tarunbhm
Copy link
Contributor Author

I would be a lot more comfortable if we could have this tested out as well cause having to rollback in cause is something was wrong in the email is painful

I had tested by logging rendered email body both txt and html so we are covered there.

As you mentioned video is required for non tech people, I will create a video with mail in inbox.

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

image

Tested multiple ETH grants + 1 cross chain grants
Flow seems to work without any issues

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.

chore: send 1 mail instead of n mails
3 participants