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 organizer summary emails #194

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

Add organizer summary emails #194

wants to merge 38 commits into from

Conversation

sampoder
Copy link
Member

@sampoder sampoder commented Aug 22, 2023

Fixes #192, I feel like the copy is missing something...

@garyhtou
Copy link
Member

I'll review this tmrw! I can retroactively send these organizer summaries after the first digest batch is sent

@sampoder
Copy link
Member Author

sampoder commented Aug 22, 2023 via email

@northeastprince
Copy link
Collaborator

Was that a rhyme? 😆

@sampoder
Copy link
Member Author

didn't intend it but sure... that's my email signature for primary school lol

Copy link
Member

@garyhtou garyhtou left a comment

Choose a reason for hiding this comment

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

thanks for working on this Sam! the logic is great. here's what I propose to reorganize this code a little bit:

In the SendDigestsJob, call a new Job called SendOrganizerSummariesJob and pass in sent_digests.

In SendOrganizerSummariesJob, copy in the code that you currently have in DigestMailer#organizer_summary. Change the each loop to call DigestMailer#organizer_summary and pass it a list of sent_digests specific to that hackathon.

In DigestMailer#organizer_summary, take in the list of sent_digests for that hackathon. Compute the count, and call the mail method

@sampoder
Copy link
Member Author

@garyhtou thanks for the review! hopefully I understood you correctly here, just pushed the restructure

test/mailers/hackathons/organizer_summary_test.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,17 @@
require "test_helper"
Copy link
Collaborator

@northeastprince northeastprince Dec 6, 2023

Choose a reason for hiding this comment

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

I would incorporate this test into the existing DigestMailerTest to match the mailer itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you help me with that? I'm not quite sure what you mean, sorry

Copy link
Collaborator

Choose a reason for hiding this comment

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

Organizer summaries are in the existing DigestMailer, so use the existing test file as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I gotcha

@northeastprince
Copy link
Collaborator

I'd like to take care of #190 before we merge this.

@northeastprince northeastprince changed the title Add An Organizer Summary Email Add organizer summary emails Dec 24, 2023
app/jobs/hackathons/send_organizer_summaries_job.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,17 @@
require "test_helper"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Organizer summaries are in the existing DigestMailer, so use the existing test file as well.

app/jobs/hackathons/digests_delivery_job.rb Outdated Show resolved Hide resolved
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.

Hackathon Digest: organizer summary
3 participants