digest email initial functionality #2711

ViditChitkara · 2018-05-10T18:54:23Z

closes #2694
part of #2104
A test digest button will be given in the user profile, only the user's with tag 'digest-weekly' would be getting the mails. (All the digest-weekly users will get the mails at once).

PublicLabBot · 2018-05-10T19:06:12Z

	1 Message
📖	@ViditChitkara Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

namangupta01 · 2018-05-15T11:55:06Z

@ViditChitkara Which part of #2104 are you working?

ViditChitkara · 2018-05-17T20:14:31Z

@jywarren as discussed, I added a simple list of subscribed content to the email content. It would be great, if you could review the code.

jywarren

This looks great. Let's try merging it when it's ready (just ping me!) and then we can subscribe ourselves to see how it works!

jywarren · 2018-05-19T03:58:45Z

app/jobs/digest_mail_job.rb

+class DigestMailJob < ActiveJob::Base
+  queue_as :default
+
+  def perform(*args)


This looks cool but could you add an inline comment explaining a little more about what it does? Thanks!!

jywarren · 2018-05-19T03:59:15Z

app/models/user.rb

@@ -377,6 +377,11 @@ def social_link(site)
    nil
  end

+  def send_digest_email
+    top_picks = self.content_followed_in_period(Time.now - 1.week, Time.now)


Ooh cool! @StlMaris123 check out this being used!!!

jywarren · 2018-05-19T04:00:18Z

Can you also share a screenshot of what it looks like now, in this PR? Thank you very much!!!

jywarren · 2018-05-19T04:00:40Z

Are there any CodeClimate recommendations you'd like to adjust as well?

👍 👍 👍

ViditChitkara · 2018-05-21T18:04:27Z

Are there any CodeClimate recommendations you'd like to adjust as well?

Sure I'll do!!

ViditChitkara · 2018-05-21T18:12:48Z

This looks great. Let's try merging it when it's ready (just ping me!) and then we can subscribe ourselves to see how it works!

For testing this out, we would require resque gem for asynchronous testing. However, if no. of users are limited we could create a rake task and test it out on test servers or something. Creating another pr for resque integration.

ViditChitkara · 2018-05-21T18:58:48Z

Here is a screenshot for a simple list of subscribed content (currently let's show just title). Once the functionality would work properly, we could work on the design.

ViditChitkara · 2018-05-21T19:01:03Z

I hope this looks good to be merged. What do you say?

ViditChitkara · 2018-05-21T19:36:58Z

Working on #2738 for adding resque as queuing backend.

jywarren · 2018-05-22T10:07:45Z

Great can you add the rescue change to this PR so they can be merged together? With both merged, should it work or do we need extra configuration from @icarito?

…

On Mon, May 21, 2018, 3:37 PM Vidit ***@***.***> wrote: Working on #2738 <#2738> for adding resque as queuing backend. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2711 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ_WmaZIWnACSHSPoVC90cNRvPvYMks5t0xdagaJpZM4T6X2X> .

ViditChitkara · 2018-05-22T12:03:54Z

Maybe we would require some redis related configs on our server(don't exactly how that would be done). Maybe @icarito would be able to describe the exact requirements.

ViditChitkara · 2018-05-26T08:45:37Z

@jywarren should I add resque changes here or should we merge them separately?? We can do it either ways and also, should we switch to sidekiq?

icarito · 2018-05-27T05:27:54Z

Hi, we need the Redis service for this to work. Fortunately this can be setup directly from the docker-compose.yml file. I gave instructions on #2738

ViditChitkara · 2018-06-02T17:20:46Z

travis is running fine here!!

jywarren

Let's add a test here before merging please? Even something simple.

What will trigger the digest sending? At this point it'll be if you have the right user tag right? Can you annotate in this PR description what exact user tag is needed to test it? Thanks!!!

jywarren · 2018-06-02T17:25:36Z

Gemfile.lock

@@ -218,7 +218,6 @@ GEM
      mini_portile2 (~> 2.3.0)
    nokogumbo (1.5.0)
      nokogiri
-    oauth (0.5.4)


Hmm I think this needs to be reversed to not affect oath work?

Woah!! Don't know where it came from!! I'll remove gemfile.lock related changes.

For triggering, can we give a button or something which would be visible only to admins and moderators?

ViditChitkara · 2018-06-02T17:51:56Z

adding tests!!

ViditChitkara · 2018-06-02T19:12:56Z

@jywarren written the tests and added a button to profile page for testing out.

jywarren · 2018-06-02T19:23:24Z

Wow awesome! Ready to merge then?

…

On Sat, Jun 2, 2018, 1:13 PM Vidit ***@***.***> wrote: @jywarren <https://github.com/jywarren> written the tests and added a button to profile page for testing out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2711 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ-AGYOVsWEFGC8S7ZsjwqMgp8efVks5t4uO6gaJpZM4T6X2X> .

ViditChitkara · 2018-06-02T20:27:02Z

Yes

jywarren · 2018-06-02T20:50:32Z

Oh weird. Can you check the tests and see if you can correct them?

jywarren · 2018-06-02T21:02:59Z

Dangerfile

@@ -61,5 +61,4 @@ begin

 rescue => ex
  fail "There was an error with Danger bot's Junit parsing: #{ex.message}"
-  puts ex.inspect # view the entire error output in the log
 end


It didn't like that line? I had that in there so we can read anything that goes wrong ... Is there another way to be able to read the output?

yeah, it says that puts statement should not be included in dangerfile.
Even, that doesn't solve the problem. Now it says:-
Your Dangerfile has had smart quotes sanitised. To avoid issues in the future, you should not use TextEdit for editing it. If you are not using TextEdit, you should turn off smart quotes in your editor of choice.

Should we ignore this error for now and let's put the puts line back??

jywarren · 2018-06-02T21:09:25Z

Ah is that actually an error though? I thought it was only a warning. It's complaining about using separate open and close quotes instead of basic symmetric quotes.

…

On Sat, Jun 2, 2018, 3:07 PM Vidit ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Dangerfile <#2711 (comment)>: > @@ -61,5 +61,4 @@ begin rescue => ex fail "There was an error with Danger bot's Junit parsing: #{ex.message}" - puts ex.inspect # view the entire error output in the log end yeah, it says that puts statement should not be included in dangerfile. Even, that doesn't solve the problem. Now it says:- Your Dangerfile has had smart quotes sanitised. To avoid issues in the future, you should not use TextEdit for editing it. If you are not using TextEdit, you should turn off smart quotes in your editor of choice. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2711 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJxn3hzq21Y4_4EIuLq4ZqlZBg3FOks5t4v5UgaJpZM4T6X2X> .

ViditChitkara · 2018-06-03T06:25:43Z

hi @ViditChitkara, actually line in red was appearing in previous builds too. But , those builds passed after @jywarren updated the token. This time build failed due to this

yep!! Let's see how to fix this!!

grvsachdeva · 2018-06-03T08:13:46Z

It seems its due to the Plotsbot trying to print the message. see this https://api.github.com/repos/publiclab/plots2/issues/comments/388153818 .

jywarren · 2018-06-03T15:50:30Z

OH, hm. That's weird, it's the same old octokit error. I had re-assigned the token to Plotsbot user but maybe that user needs admin rights to the repo. I'll check.

/home/travis/.rvm/gems/ruby-2.4.1/gems/octokit-4.9.0/lib/octokit/response/raise_error.rb:16:in `on_complete': PATCH https://api.github.com/repos/publiclab/plots2/issues/comments/388153818: 403 - Must have admin rights to Repository. // See: https://developer.github.com/v3/issues/comments/#edit-a-comment (Octokit::Forbidden)

jywarren · 2018-06-03T15:53:07Z

OK - i made plotsbot a repo admin so it can edit comments!

jywarren · 2018-06-03T15:53:17Z

er, @PublicLabBot

jywarren · 2018-06-03T15:53:52Z

Also, if there are any changes you want to incorporate from CodeClimate suggestions, go for it, otherwise we can just "approve" that. Thanks!

ViditChitkara · 2018-06-03T16:25:29Z

Followed code-climate suggestions and cool travis works again!!
I guess it's ready now!
Thanks @jywarren

jywarren · 2018-06-03T16:46:01Z

Great work @ViditChitkara !!!!! Thanks everyone for pitching in 🤝 🙌

ViditChitkara · 2018-06-03T16:51:48Z

Great work @ViditChitkara !!!!! Thanks everyone for pitching in handshake raised_hands

Thanks a lot, everyone for helping out on this!! 😄 👍

jywarren · 2018-06-06T22:09:55Z

Testing this now!

Hm, I got a 500 error --

There was an error. Are you sure you have the right address?

I wonder what happened? We can check the logs.

jywarren · 2018-06-06T22:14:01Z

I also added my own profile tag of digest:weekly

https://publiclab.org/profile/warren

jywarren · 2018-06-06T23:30:06Z

OK -- error is:

Started GET "/users/test_digest_email" for 172.56.22.214 at 2018-06-06 22:12:08 +0000
Processing by UsersController#test_digest_email as HTML
Completed 500 Internal Server Error in 7ms (ActiveRecord: 1.2ms)
  
Redis::CannotConnectError (Error connecting to Redis on 127.0.0.1:6379 (Errno::ECONNREFUSED)):
  app/controllers/users_controller.rb:301:in `test_digest_email'

If you'd like to open an issue to track and solve this, please do!

icarito · 2018-06-07T03:38:21Z

Yeah, my reading of this is that according to

plots2/config/initializers/sidekiq.rb

Line 1 in 539229b

sidekiq_config = { url: ENV['JOB_WORKER_URL'] }

we need an environment variable to direct the app to the proper Redis service. By default it looks for 127.0.0.1 (localhost) but in this case it is in a separate container with hostname redis. So the URL should be in the JOB_WORKER_URL environment variable and it should be something like redis://redis

icarito · 2018-06-07T03:41:05Z

I'm thinking now that we are configuring the application with environment variables we should have a good practice of naming these env variables because we could have some confusion. So the appropriate name for this environment variable would be REDIS_URL, I think. Thanks for working on this! 😃

* digest email initial functionality * minor changes * changes in digest email design * added job for digest mailer * codeclimate fixes * minor change * minor change * minor change * minor change * minor change * removed gemfile.lock related changes * written tests * removed puts from dangerfile * minor changes * code-climate fixes * more code-climate fixes * minor changes

ViditChitkara added the summer-of-code label May 10, 2018

ViditChitkara added the review-me label May 18, 2018

jywarren requested changes May 19, 2018

View reviewed changes

ViditChitkara force-pushed the email-digest branch from 67bc0b7 to 8ea2446 Compare June 2, 2018 17:07

ghost assigned ViditChitkara Jun 2, 2018

ViditChitkara added ready and removed review-me labels Jun 2, 2018

jywarren requested changes Jun 2, 2018

View reviewed changes

jywarren reviewed Jun 2, 2018

View reviewed changes

ViditChitkara added 6 commits June 3, 2018 11:50

minor change

ff33596

minor change

461557f

removed gemfile.lock related changes

744f94a

written tests

651f9d6

removed puts from dangerfile

bd4acc7

minor changes

b44bec3

ViditChitkara force-pushed the email-digest branch from 4792ebc to b44bec3 Compare June 3, 2018 06:21

ViditChitkara added 3 commits June 3, 2018 21:37

code-climate fixes

b52fdb9

more code-climate fixes

2968718

minor changes

382a0b7

jywarren approved these changes Jun 3, 2018

View reviewed changes

jywarren merged commit a2b69d4 into publiclab:master Jun 3, 2018

ghost removed the ready label Jun 3, 2018

This was referenced Jun 7, 2018

Error connecting to redis #2779

Closed

Community Check-in #3 Summer Of Code Week 4 #2764

Closed

digest email initial functionality #2711

digest email initial functionality #2711

Conversation

ViditChitkara commented May 10, 2018 • edited

PublicLabBot commented May 10, 2018 • edited by plotsbot

namangupta01 commented May 15, 2018

ViditChitkara commented May 17, 2018

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented May 19, 2018

jywarren commented May 19, 2018

ViditChitkara commented May 21, 2018

ViditChitkara commented May 21, 2018

ViditChitkara commented May 21, 2018

ViditChitkara commented May 21, 2018 • edited

ViditChitkara commented May 21, 2018

jywarren commented May 22, 2018 via email

ViditChitkara commented May 22, 2018 via email • edited

ViditChitkara commented May 26, 2018 • edited

icarito commented May 27, 2018

ViditChitkara commented Jun 2, 2018

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViditChitkara commented Jun 2, 2018

ViditChitkara commented Jun 2, 2018

jywarren commented Jun 2, 2018 via email

ViditChitkara commented Jun 2, 2018

jywarren commented Jun 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Jun 2, 2018 via email

ViditChitkara commented Jun 3, 2018

grvsachdeva commented Jun 3, 2018

jywarren commented Jun 3, 2018

jywarren commented Jun 3, 2018

jywarren commented Jun 3, 2018

jywarren commented Jun 3, 2018

ViditChitkara commented Jun 3, 2018

jywarren commented Jun 3, 2018

ViditChitkara commented Jun 3, 2018

jywarren commented Jun 6, 2018

jywarren commented Jun 6, 2018

jywarren commented Jun 6, 2018

icarito commented Jun 7, 2018

icarito commented Jun 7, 2018

ViditChitkara commented May 10, 2018 •

edited

PublicLabBot commented May 10, 2018 •

edited by plotsbot

ViditChitkara commented May 21, 2018 •

edited

ViditChitkara commented May 22, 2018 via email •

edited

ViditChitkara commented May 26, 2018 •

edited