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 send_weekly_recap to crontab #4604

Closed
wants to merge 1 commit into from
Closed

Add send_weekly_recap to crontab #4604

wants to merge 1 commit into from

Conversation

iamonuwa
Copy link
Contributor

@iamonuwa iamonuwa commented Jun 10, 2019

Description

Add send_weekly_recap to crontab

Refers/Fixes
Testing

@codecov
Copy link

codecov bot commented Jun 10, 2019

Codecov Report

Merging #4604 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4604      +/-   ##
==========================================
- Coverage   30.12%   30.09%   -0.04%     
==========================================
  Files         210      210              
  Lines       16911    16911              
  Branches     2284     2284              
==========================================
- Hits         5095     5089       -6     
- Misses      11619    11625       +6     
  Partials      197      197
Impacted Files Coverage Δ
app/dashboard/embed.py 28.16% <0%> (-3.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b17efb5...2319cdd. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jun 10, 2019

Codecov Report

Merging #4604 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4604      +/-   ##
==========================================
- Coverage   30.12%   30.09%   -0.04%     
==========================================
  Files         210      210              
  Lines       16911    16911              
  Branches     2284     2284              
==========================================
- Hits         5095     5089       -6     
- Misses      11619    11625       +6     
  Partials      197      197
Impacted Files Coverage Δ
app/dashboard/embed.py 28.16% <0%> (-3.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b17efb5...2319cdd. Read the comment docs.

@thelostone-mc
Copy link
Member

@iamonuwa could you

  • give more context on why this is needed
  • has it been tested / if not why

^ this is to ensure the reviewer has enough info to review this PR to merge it in

@iamonuwa
Copy link
Contributor Author

@thelostone-mc, I figured it was the remaining piece of the puzzle after with @kuhnchris and
@PixelantDesign

@owocki
Copy link
Contributor

owocki commented Jun 11, 2019

whats the reference issue?

are we sure the weekly recap email is high quality enough that we're ready to send it to everyone?

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Jun 11, 2019 via email

@thelostone-mc
Copy link
Member

are we sure the weekly recap email is high quality enough that we're ready to send it to everyone?

@iamonuwa we won't be getting this in until that is verified cause this would endup going to our users if merged ! Would you be able to verify it & provide screenshots ?

@kuhnchris
Copy link
Contributor

@thelostone-mc do we finally have a easy solution to test mails without them actually going out? Because right now we barely can test this at all.

@PixelantDesign
Copy link
Contributor

A screenshot would be great!

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Jun 14, 2019 via email

@thelostone-mc
Copy link
Member

thelostone-mc commented Jun 23, 2019

@kuhnchris well if the needed data is present in db
@iamonuwa could

  • add a new test route like it's done for other emails
  • simply visit that url and see the mail

Taking it a step further you could add SENDGRID_API_KEY in your env and update the code to trigger the mail to be sent only to your mail

^ Those are the 2 ways I can think of !

I know it's cumbersome but we can't afford to merge this in without testing

cc @danlipert / @octavioamu

@iamonuwa
Copy link
Contributor Author

@kuhnchris well if the needed data is present in db
@iamonuwa could

  • add a new test route like it's done for other emails
  • simply visit that url and see the mail

Taking it a step further you could add SENDGRID_API_KEY in your env and update the code to trigger the mail to be sent only to your mail

^ Those are the 2 ways I can think of !

I know it's cumbersome but we can't afford to merge this in without testing

cc @danlipert / @octavioamu

Totally agree. The only issue ATM is if the data is present

@danlipert
Copy link
Contributor

I honestly don't think this is valuable - right now we do manual testing to see if the email is formatted correctly, etc by sending ourselves a test email before sending it out. There are usually issues that need to be addressed, and the timing of when the roundup is ready is not very stable. I'd like to see some consistency in preparing the content before we automate this.

@kuhnchris
Copy link
Contributor

I honestly don't think this is valuable - right now we do manual testing to see if the email is formatted correctly, etc by sending ourselves a test email before sending it out. There are usually issues that need to be addressed, and the timing of when the roundup is ready is not very stable. I'd like to see some consistency in preparing the content before we automate this.

Maybe as a little background: I implemented MailDev a while ago to "locally send mails" without going out through a real mail server. Since we have a docker environment it should be pretty straight forward to include.

(see https://danfarrelly.nyc/MailDev/)

Maybe that's something to consider @danlipert ?

@danlipert
Copy link
Contributor

I honestly don't think this is valuable - right now we do manual testing to see if the email is formatted correctly, etc by sending ourselves a test email before sending it out. There are usually issues that need to be addressed, and the timing of when the roundup is ready is not very stable. I'd like to see some consistency in preparing the content before we automate this.

Maybe as a little background: I implemented MailDev a while ago to "locally send mails" without going out through a real mail server. Since we have a docker environment it should be pretty straight forward to include.

(see https://danfarrelly.nyc/MailDev/)

Maybe that's something to consider @danlipert ?

Very cool! The manual test so much I dont worry about, but its the content being prepared on-time and accurately/bug-free.

@PixelantDesign
Copy link
Contributor

@iamonuwa please let us know if you can help wrap this up by Friday. Otherwise I'll need to bring it in internally. Thanks!

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Aug 7, 2019

You can make it internal

@rafalkowalski
Copy link
Contributor

@danlipert @octavioamu @thelostone-mc What exactly is missing here? Only manual testing or something else?

@danlipert
Copy link
Contributor

danlipert commented Aug 19, 2019 via email

@kuhnchris
Copy link
Contributor

I don't think this is valuable at all - right now the emails are visually checked before going out for grammar issues, etc. and I don't know how we can automate that easily.

-Dan Lipert
On Thu, Aug 8, 2019 at 11:37 PM Rafał Kowalski @.***> wrote: @danlipert https://github.com/danlipert @octavioamu https://github.com/octavioamu @thelostone-mc https://github.com/thelostone-mc What exactly is missing here? Only manual testing or something else? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4604?email_source=notifications&email_token=AAGUHEQ7K5Q6PEWI62WMCMTQDQVQ5A5CNFSM4HWSFYA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD332LZY#issuecomment-519546343>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGUHET7FA7R52S5SXNHZJTQDQVQ5ANCNFSM4HWSFYAQ .

for the weekly mails too? cause this is a mail in the style of "hey we found work for you".

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.

7 participants