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

improve the email delivery to not send emails like we do now #171

Merged
merged 8 commits into from
Jul 4, 2016

Conversation

Panioglo
Copy link
Member

Fixes #157, #167, #170

@Panioglo Panioglo self-assigned this Jun 27, 2016
@Chocksy
Copy link
Member

Chocksy commented Jun 27, 2016

@Panioglo there is also an issue that we are not looking at the notification settings defined per website right now. We are sending those emails even if we change the notification settings so there should be a fix for that here too!

@codecov-io
Copy link

codecov-io commented Jun 30, 2016

Current coverage is 93.15%

Merging #171 into master will decrease coverage by 0.04%

@@             master       #171   diff @@
==========================================
  Files           183        183          
  Lines          5099       5140    +41   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4752       4788    +36   
- Misses          347        352     +5   
  Partials          0          0          

Powered by Codecov. Last updated by 99c7264...2c0d56b

return all_headers if header.nil?
all_headers.find { |h| h[header] }.try(:values).try(:first)
end

def http_data(key = nil)
all_data = get_interfaces(:http)._data
return '<no information>' if all_data.blank?
Copy link
Member

Choose a reason for hiding this comment

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

same thing here with this '' message. Why do you need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the image you gave me, I remember there was a message in the error occurred email in the Url section.

This was my approach
%p.gray_text Url
%pre
%code
%small #{@issue.http_data(:url)}

Copy link
Member

Choose a reason for hiding this comment

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

Ok i guess we can keep it then

Website.joins(:grouped_issues).where('grouped_issues.updated_at > ?', date).uniq.each do |website|
GroupedIssueMailer.notify_daily(website).deliver_later
def self.custom_report(date, field)
WebsiteMember.where("website_members.#{field} = ?", true).joins(website: :grouped_issues).where('grouped_issues.updated_at > ? AND muted = ?', date, false).uniq.each do |member|
Copy link
Member

Choose a reason for hiding this comment

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

for these use ... .find_each(batch_size: 500) do |member| so if we are going to have a gazillion number of users we will get them in batches of 500.

@Chocksy Chocksy force-pushed the feat-improve-emails branch from 875ef32 to 2c0d56b Compare July 4, 2016 14:56
@Chocksy Chocksy force-pushed the feat-improve-emails branch from 2c0d56b to 096b7c4 Compare July 4, 2016 14:59
@Chocksy Chocksy force-pushed the feat-improve-emails branch from 096b7c4 to e8c7523 Compare July 4, 2016 15:03
@Chocksy Chocksy merged commit e8c7523 into master Jul 4, 2016
@Chocksy Chocksy deleted the feat-improve-emails branch July 4, 2016 15:03
@Chocksy Chocksy temporarily deployed to epiclogger-staging July 4, 2016 15:06 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants