fixes #7666, #7667, #7668 - errata mail notifications #4780

stbenjam · 2014-10-29T14:24:55Z

No description provided.

ehelms · 2014-10-29T17:34:38Z

app/mailers/katello/errata_mailer.rb

@@ -0,0 +1,37 @@
+module Katello


Missed this earlier, but we do tend to put copyright notices at the top of all of our files.

stbenjam · 2014-11-11T12:12:32Z

Updated to include all the mail notifications. Needs a review.

Here's how they look:
http://imgur.com/a/6IaqT

stbenjam · 2014-11-13T13:47:43Z

A couple notes:

Instructions for configuring mail to use an SMTP server are in the foreman manual (email.yaml), for anyone who is testing this: http://theforeman.org/manuals/1.6/index.html#3.5.2ConfigurationOptions
Unfortunately, monolithic HTML and inline CSS for Gmail and Yahoo, as per guidelines @ http://templates.mailchimp.com/development/html/ and http://templates.mailchimp.com/concepts/email-clients/

waldenraines · 2014-11-18T15:18:21Z

app/views/katello/errata_mailer/host_errata.html.erb

+    color: #CCCCCC !important;
+  }
+</style>
+<div style="font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; color: #3f3f3f; background-color: #f1f1f1;">


I wonder about all of these inline styles and how they will affect the ability to easily change the styling of these emails in the future.

We're a bit stuck there, it's the only option for some very popular web mail providers (e.g. Gmail). We're lucky they're supporting CSS at all these days.

I did break some things out as partials, but even then, the styles are in-line on those tables too.

It could all be abstracted away to helpers or something, but the nature of e-mail notifications is that I don't expect the number of e-mail types to ever become more than a handful.

True, I'm fine with using helpers in the future if we ever need to change these or add more. Don't want to create a premature abstraction.

You could create variables with the common css attributes and use those variables when styling.
See: variable (https://github.com/theforeman/foreman/pull/1910/files#diff-4b17df40367067144fb027cc5533ecefR1)
usage: https://github.com/theforeman/foreman/pull/1910/files#diff-4b17df40367067144fb027cc5533ecefR7

Ah that looks nicer. At this point though, I want to get this in for the Katello 2.1 RC so will have to do it as a separate PR later.

stbenjam · 2014-11-21T19:40:37Z

[test]

stbenjam · 2014-11-24T13:20:13Z

[test]

shlomizadok · 2014-12-02T14:06:21Z

app/mailers/katello/errata_mailer.rb

+
+      mail(:to        => user.mail,
+           :subject   => _("Katello Host Advisory"),
+           :date      => Time.now)


Time.zone.now will respect the system's current timezone (see theforeman/foreman#1880)

waldenraines · 2014-12-02T18:50:31Z

Tested this out and it worked for me. I'll hold off on an ACK though as others still may have additional comments.

ehelms · 2014-12-02T19:29:32Z

ACK for me, my comments were addressed, I just couldn't ever get my testing to work.

jlsherrill · 2014-12-02T21:54:40Z

One small issue when syncing a new repo:

NoMethodError: undefined method `updated_at' for nil:NilClass
Backtrace:
/home/jlsherri/git/katello/app/lib/actions/katello/repository/errata_mail.rb:19:in `plan'
/home/jlsherri/.rvm/gems/ruby-1.9.3-p448/gems/dynflow-0.7.5/lib/dynflow/action.rb:411:in `block (3 levels) in execute_plan'
/home/jlsherri/.rvm/gems/ruby-1.9.3-p448/gems/dynflow-0.7.5/lib/dynflow/middleware/stack.rb:26:in `call'
/home/jlsherri/.rvm/gems/ruby-1.9.3-p448/gems/dynflow-0.7.5/lib/dynflow/middleware/stack.rb:26:in `pass'
/home/jlsherri/.rvm/gems/ruby-1.9.3-p448/gems/dynflow-0.7.5/lib/dynflow/middleware.rb:16:in `pass'

jlsherrill · 2014-12-02T22:57:18Z

When I sync a repo pointed to a particular upstream repo, I get an email that shows the proper counts.

However, if i create a 2nd repo pointed to the same upstream url and sync it, i get an email but it shows 0 new errata.

jlsherrill · 2014-12-02T22:59:31Z

app/views/katello/errata_mailer/promote_errata.html.erb

+
+                <% all_errata = @content_hosts.authorized_as(@user, :view_content_hosts).map(&:available_errata).flatten.sort_by(&:id).uniq %>
+
+                <%= (_("The promotion of %{content_view} to <b>%{environment}</b> has completed.  %{count} errata are available to your hosts.") %


I might would say '%{count} needed errata are available to your host.'

stbenjam · 2014-12-02T23:24:19Z

@jlsherrill Is there an upstream repo smaller than EPEL that has errata?

stbenjam · 2014-12-02T23:25:14Z

@jlsherrill Oh disregard, I found https://repos.fedorapeople.org/repos/pulp/pulp/demo_repos/test_simple_errata/. I didn't realize they had more than zoo!

jlsherrill · 2014-12-03T00:07:11Z

@stbenjam i've been using mostly non-upstream repos such as resilient storage and the rhev-agent repos. Its hard to test errata with real freely available repos :/

I need to do a tad more testing with a client, will do that first thing in the morning.

stbenjam · 2014-12-04T10:38:03Z

@jlsherrill Updated, thanks - this should fix the issues you raised:

Use Time.now for calculating errata for repos w/out any (e.g. new repos)
Repo errata is calculated based on the join table, to cover the multiple copies case

stbenjam · 2014-12-04T10:38:54Z

By the way, the pulp repos with errata don't really work... the errata types on those are all marked as "enhancements" instead of "enhancement"

stbenjam · 2014-12-08T09:20:17Z

#4851 needs to be merged for tests to pass again here.

jlsherrill · 2014-12-08T14:55:11Z

app/views/katello/errata_mailer/host_errata.html.erb

+              <table border="0" cellpadding="20" cellspacing="0" width="100%" id="emailFooter" style="background-color: #e1e2e3;" bgcolor="#e1e2e3">
+                <tr>
+                  <td align="left" valign="top" style="font-size: 82%; padding: 0px 20px;">
+                    <p>Note: The number in parentheses reflects applicable errata from the Library environment. You will


I would change:

Note: The number in parentheses reflects applicable errata from the Library environment.
to something like:
Note: The number in parentheses reflects applicable errata unavailable to the system but available from the synced content in Library.

jlsherrill · 2014-12-08T14:55:33Z

ACK pending small wording change and jenkins

fixes #7666, #7667, #7668 - errata mail notifications

theforeman-bot added Needs testing Not yet reviewed labels Oct 29, 2014

ehelms reviewed Oct 29, 2014
View reviewed changes

stbenjam changed the title ~~fixes #7668 - errata summary mail~~ fixes #7666, #7668 - errata notifications for hosts and repo sync Nov 10, 2014

stbenjam changed the title ~~fixes #7666, #7668 - errata notifications for hosts and repo sync~~ fixes #7666, #7667, #7668 - errata mail notifications Nov 11, 2014

waldenraines reviewed Nov 18, 2014
View reviewed changes

shlomizadok reviewed Dec 2, 2014
View reviewed changes

jlsherrill reviewed Dec 2, 2014
View reviewed changes

stbenjam mentioned this pull request Dec 5, 2014

fixes #8588 - make repo indexing idempotent #4851

Merged

jlsherrill reviewed Dec 8, 2014
View reviewed changes

stbenjam added 3 commits December 8, 2014 16:31

fixes #7668 - host errata mail notification

3331f54

fixes #7666 - sync errata mail notification

4c96c39

fixes #7667 - promotion errata mail notification

ae6e415

stbenjam added a commit that referenced this pull request Dec 8, 2014

Merge pull request #4780 from stbenjam/7668

da77ba3

fixes #7666, #7667, #7668 - errata mail notifications

stbenjam merged commit da77ba3 into Katello:master Dec 8, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #7666, #7667, #7668 - errata mail notifications #4780

fixes #7666, #7667, #7668 - errata mail notifications #4780

stbenjam commented Oct 29, 2014

ehelms Oct 29, 2014

stbenjam Nov 3, 2014

ehelms Nov 19, 2014

stbenjam commented Nov 11, 2014

stbenjam commented Nov 13, 2014

waldenraines Nov 18, 2014

stbenjam Nov 18, 2014

waldenraines Nov 18, 2014

shlomizadok Dec 2, 2014

stbenjam Dec 2, 2014

stbenjam commented Nov 21, 2014

stbenjam commented Nov 24, 2014

shlomizadok Dec 2, 2014

waldenraines commented Dec 2, 2014

ehelms commented Dec 2, 2014

jlsherrill commented Dec 2, 2014

jlsherrill commented Dec 2, 2014

jlsherrill Dec 2, 2014

stbenjam commented Dec 2, 2014

stbenjam commented Dec 2, 2014

jlsherrill commented Dec 3, 2014

stbenjam commented Dec 4, 2014

stbenjam commented Dec 4, 2014

stbenjam commented Dec 8, 2014

jlsherrill Dec 8, 2014

jlsherrill commented Dec 8, 2014


		<% all_errata = @content_hosts.authorized_as(@user, :view_content_hosts).map(&:available_errata).flatten.sort_by(&:id).uniq %>

		<%= (_("The promotion of %{content_view} to <b>%{environment}</b> has completed. %{count} errata are available to your hosts.") %

fixes #7666, #7667, #7668 - errata mail notifications #4780

fixes #7666, #7667, #7668 - errata mail notifications #4780

Conversation

stbenjam commented Oct 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stbenjam commented Nov 11, 2014

stbenjam commented Nov 13, 2014

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

Choose a reason for hiding this comment

stbenjam commented Nov 21, 2014

stbenjam commented Nov 24, 2014

Choose a reason for hiding this comment

waldenraines commented Dec 2, 2014

ehelms commented Dec 2, 2014

jlsherrill commented Dec 2, 2014

jlsherrill commented Dec 2, 2014

Choose a reason for hiding this comment

stbenjam commented Dec 2, 2014

stbenjam commented Dec 2, 2014

jlsherrill commented Dec 3, 2014

stbenjam commented Dec 4, 2014

stbenjam commented Dec 4, 2014

stbenjam commented Dec 8, 2014

Choose a reason for hiding this comment

jlsherrill commented Dec 8, 2014