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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

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

garyhtou
garyhtou requested changes Aug 22, 2023
View reviewed changes
Copy link
Member

@garyhtou garyhtou left a comment •
edited
Loading

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

app/views/hackathons/digest_mailer/organizer_summary.html.erb Show resolved Hide resolved
app/views/hackathons/digest_mailer/organizer_summary.html.erb Outdated Show resolved Hide resolved
app/mailers/hackathons/digest_mailer.rb Outdated Show resolved Hide resolved
@sampoder
Copy link
Member Author

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

northeastprince
northeastprince reviewed Dec 6, 2023
View reviewed changes
test/mailers/hackathons/organizer_summary_test.rb Outdated Show resolved Hide resolved
test/mailers/hackathons/organizer_summary_test.rb Outdated
@@ -0,0 +1,17 @@
require "test_helper"
Copy link
Collaborator

@northeastprince northeastprince Dec 6, 2023 •
edited
Loading

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
northeastprince
northeastprince reviewed Dec 25, 2023
View reviewed changes
app/jobs/hackathons/send_organizer_summaries_job.rb Outdated Show resolved Hide resolved
test/mailers/hackathons/organizer_summary_test.rb Outdated
@@ -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
Reviewers

@garyhtou garyhtou garyhtou requested changes

@northeastprince northeastprince Awaiting requested review from northeastprince

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hackathon Digest: organizer summary
3 participants
@sampoder @garyhtou @northeastprince