Fixing #4822 daily vs weekly subject #4955
Conversation
Generated by 🚫 Danger |
|
Hey @jywarren, I think the code climate issues are very trivial and can be ignored. However, I fail to understand the travis build fail. Could you please explain what happened and what can be done to fix it? |
app/mailers/subscription_mailer.rb
Outdated
| @@ -54,10 +54,14 @@ def notify_tag_added(node, tag, tagging_user) | |||
| ) | |||
| end | |||
|
|
|||
| def send_digest(user_id, top_picks) | |||
| subject = "Your weekly digest" | |||
| def send_digest(user_id, top_picks, k) | |||
There was a problem hiding this comment.
Can we give this parameter a more informative name, so people understand when reading the code? Thanks!
There was a problem hiding this comment.
Also, maybe top picks could just become "nodes", you know?
There was a problem hiding this comment.
Yes absolutely, and I am gonna rename top_picks parameter to nodes then?
| top_picks = content_followed_in_period(1.week.ago, Time.now) | ||
| if top_picks.count.positive? | ||
| SubscriptionMailer.send_digest(id, top_picks).deliver_now | ||
| if self.has_tag('digest:daily') |
app/models/user.rb
Outdated
| if top_picks.count.positive? | ||
| SubscriptionMailer.send_digest(id, top_picks).deliver_now | ||
| if self.has_tag('digest:daily') | ||
| top_picks = content_followed_in_period(Time.now - 1.day, Time.now) |
There was a problem hiding this comment.
Not sure, but I think @siaw23 was able to use 1.day.ago instead of subtracting... maybe worth a try cause it's short?
app/models/user.rb
Outdated
| top_picks = content_followed_in_period(Time.now - 1.week, Time.now) | ||
| k = 1 | ||
| end | ||
| if top_picks.count > 0 |
There was a problem hiding this comment.
This is where the error points to.
ERROR["test_send_digest_email", #<Minitest::Reporters::Suite:0x00005572aa9d18a0 @name="UserTest">, 24.959422129000018]
test_send_digest_email#UserTest (24.96s)
NoMethodError: NoMethodError: undefined method `count' for nil:NilClass
Did you mean? concat
app/models/user.rb:326:in `send_digest_email'
test/unit/user_test.rb:225:in `block in <class:UserTest>'
There was a problem hiding this comment.
Oh yes, so probably because the daily top_picks were empty, that's why it couldn't trigger the condition, should I put a nil check there?
There was a problem hiding this comment.
So I tried to a check like !top_picks.nil? && top_picks.count > 0 , however another error showed up in travis.
There was a problem hiding this comment.
Sure or as i said above, you can start by defining it as []. Great!
There was a problem hiding this comment.
I have tried both, but still, the new error prevails, could you please tell me why?
app/models/user.rb
Outdated
| if self.has_tag('digest:daily') | ||
| top_picks = content_followed_in_period(Time.now - 1.day, Time.now) | ||
| k = 0 | ||
| elsif self.has_tag('digest:weekly') |
There was a problem hiding this comment.
Aha - so if a user has /neither/ tag set, we are still trying to call top_picks. We should set it to [] by default? We could define it above as top_picks = [] before these IFs?
There was a problem hiding this comment.
Yes I thought of doing the same, but I didn't. So sorry!
|
Hmm, i see in Travis: So do you see how to look up that line where the error occurred? See how it points here: https://github.com/publiclab/plots2/blob/master/test/unit/user_test.rb#L225 Are we sure that |
app/models/user.rb
Outdated
| freq = 1 | ||
| end | ||
| if !top_picks.nil? && top_picks.count > 0 | ||
| SubscriptionMailer.send_digest(id, top_picks, freq).deliver_now |
There was a problem hiding this comment.
aha, so could it be that the test was set up such that the test user doesn't actually have these tags, and so is now not getting the digest email?
Also, if you wouldn't mind changing top_picks to nodes or subscribed_content perhaps, that'd be nice, thanks!
There was a problem hiding this comment.
Oh it could be that! Yeah sure, I will change it here as well, as I did in the mailer!
|
Isha this is going really well. You're getting better and better at tracing errors through the logs, figuring out what they mean and what caused them, and identifying the spot in the code where you need to make a change. Travis tests can really help guide your debugging, so it's great to build that skill set! |
|
Thanks a lot! Yes, I think that's very important too. I do know how to look up for the errors and find the files that caused them. Let me try another hand at debugging this one! |
|
@jywarren so the checks have passed, I think I caught the error somewhere in user.rb 🎉 . Kindly review and let me know if its ready to be merged! Thanks a lot! |
|
Fantastic work, @IshaGupta18 !!! |
* Fixing publiclab#4822 daily vs weekly subject * fixing small changes * fixing changes * spaces * trying to fix bugs
* Fixing publiclab#4822 daily vs weekly subject * fixing small changes * fixing changes * spaces * trying to fix bugs
Fixes #4822
So it works locally now on changing the value of k(parameter for daily v/s weekly):
weekly:
daily: