Fixes #19314 - Notification for subscr. expiring soon #6752

dLobatog · 2017-04-19T11:39:12Z

Depends on #6750 and https://github.com/theforeman/foreman/pull/4240/files (this https://github.com/theforeman/foreman/pull/4240/files#diff-2190144d88f9376ea36f321f8f5af34dR71 allows to set actions with more freedom than the current structure).

houndci-bot · 2017-04-19T11:39:24Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+      class ExpireSoon
+        class << self
+          def deliver!
+            binding.pry


Remove debugger entry point binding.pry.

houndci-bot · 2017-04-19T11:39:24Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+      class ExpireSoon
+        class << self
+          def deliver!
+            binding.pry


Remove debugger entry point binding.pry.

ohadlevy · 2017-05-09T11:53:35Z

app/services/katello/ui_notifications/pulp/proxy_disk_space.rb

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


file does not belongs to this pr.

ohadlevy · 2017-05-09T11:54:09Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+                :subject => subscription,
+                :initiator => User.anonymous_admin,
+                :audience => Notification::AUDIENCE_ADMIN,
+                :message => ::UINotifications::StringParser.new(


if you have the expiry date of the subscription, maybe we should use it for the notification expiry date too?

ohadlevy · 2017-05-09T11:54:46Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+        class << self
+          def deliver!
+            subs_expiring_soon.each do |subscription|
+              next if notification_already_exists?(subscription)


see comments on #6750 around these lines.

ohadlevy · 2017-05-09T11:55:30Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+              ::Notification.create!(
+                :subject => subscription,
+                :initiator => User.anonymous_admin,
+                :audience => Notification::AUDIENCE_ADMIN,


IMHO this should be the taxonomy users/admins?

I agree, but how do you set this? AUDIENCE_TAXONOMY is subject.user_ids - if subject is the subscription, it will not work. The subject can't be the taxonomy

ohadlevy · 2017-05-09T11:56:24Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+
+          def subs_expiring_soon
+            result = []
+            Katello::Subscription.unscoped.all.each do |subscription|


is there no better query than go over all? at least I would use select instead of the next statement below.

ohadlevy · 2017-05-09T11:56:43Z

db/seeds.d/109-katello-notification-blueprints.rb

+  {
+    group: _('Proxies'),
+    name: 'pulp_low_disk_space',
+    message: _("%{subject}'s disk is %{percentage} full. Since this proxy is running Pulp, it needs disk space to publish content views. Please ensure the disk does not get full."),


not related to this pr.

ohadlevy · 2017-05-09T11:56:52Z

lib/katello/tasks/proxy_disk_space_notification.rake

@@ -0,0 +1,13 @@
+namespace :proxy_disk_space do
+  desc <<-END_DESC
+Check the proxy disk space based on Pulp API results and create notifications if


ohadlevy · 2017-05-09T11:57:07Z

lib/katello/tasks/subs_expiry_notification.rake

@@ -0,0 +1,13 @@
+namespace :subscriptions do
+  desc <<-END_DESC
+Check the proxy disk space based on Pulp API results and create notifications if


wrong description titles etc.

dLobatog · 2017-05-25T11:47:43Z

Updated so that:

Notifications are expired according to the subscription expiration date
Finding expiring_soon subs is simplified by using .select
Fixed the typos in the rake task description
Removed the Pulp low disk space notification commit (https://github.com/Katello/katello/pull/6750/files) from here.

cc @ohadlevy thanks for taking a look before

jlsherrill · 2017-05-31T17:02:08Z

I'm getting an error when running the rake task, i think because Subscription is not taxable:

NoMethodError: undefined method `user_ids' for #<Katello::Subscription:0x00000003164bc8>
/home/vagrant/.rvm/gems/ruby-2.2.4/gems/activemodel-4.2.8/lib/active_model/attribute_methods.rb:433:in `method_missing'
/home/vagrant/git/foreman/app/models/notification.rb:43:in `subscriber_ids'
/home/vagrant/git/foreman/app/models/notification.rb:65:in `set_notification_recipients'
/home/vagrant/.rvm/gems/ruby-2.2.4/gems/activesupport-4.2.8/lib/active_support/callbacks.rb:432:in `block in make_lambda'

akofink · 2017-07-26T19:18:47Z

@dLobatog any update here?

houndci-bot · 2017-08-08T15:31:12Z

lib/katello/scheduled_jobs.rb

+
+# Only create notifications if there isn't a scheduled job
+SendExpireSoonNotifications.perform_later if scheduled_job.blank?
+


1 trailing blank lines detected.

houndci-bot · 2017-08-08T15:31:12Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+
+          def notification_already_exists?(subject)
+            subs_expiration_notification = Notification.unscoped.find_by(:subject => subject)
+            return false unless subs_expiration_notification.present?


Use if subs_expiration_notification.blank? instead of unless subs_expiration_notification.present?.

houndci-bot · 2017-08-08T15:31:13Z

app/jobs/send_expire_soon_notifications.rb

+     self.class.set(:wait => 12.hours).perform_later
+   end
+
+  def perform


Inconsistent indentation detected.

houndci-bot · 2017-08-08T15:31:13Z

app/jobs/send_expire_soon_notifications.rb

@@ -0,0 +1,9 @@
+class SendExpireSoonNotifications < ActiveJob::Base
+   after_perform do


Use 2 (not 3) spaces for indentation.

houndci-bot · 2017-08-08T15:31:13Z

app/jobs/send_expire_soon_notifications.rb

@@ -0,0 +1,9 @@
+class SendExpireSoonNotifications < ActiveJob::Base


Jobs should subclass ApplicationJob.

dLobatog

Updated, @jlsherrill @akofink , the job is scheduled every 12 hours with ActiveJob now

ares · 2017-10-16T07:50:02Z

it seems rubocop is failing, could someone with permissions set WoC? since we don't have the output in jenkins anymore, I'm retriggering tests [test]

dLobatog

Rebased, @ares I think the only failure from hound is that "Jobs should subclass ApplicationJob." - I do agree (it's a convention, not really necessary) but I introduce that class in https://github.com/theforeman/foreman/pull/4240/files in Foreman itself, so at this moment it isn't available here to inherit, and I don't think it's a good idea to have a ApplicationJob class namespaced to be exclusive to Katello.

Best course would be to use ApplicationJob in here when the core PR is merged, but it shouldn't block this one.

ares · 2017-12-04T13:24:22Z

this needs rebase again, ActiveJob is now in core too, but maybe it's better to wait until #6750 gets in

dLobatog

@ares Updated to use ApplicationJob and ::Foreman::Application.dynflow.config.on_init do |world| like #6750

ares · 2018-01-02T10:28:16Z

LGTM, it would be great if someone from katello could take a look

jlsherrill · 2018-01-11T20:18:48Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+              ::Notification.create!(
+                :subject => subscription,
+                :initiator => User.anonymous_admin,
+                :audience => Notification::AUDIENCE_TAXONOMY,


Does this require some update to foreman? I'm getting an error that this constant isn't defined:

NameError: uninitialized constant Notification::AUDIENCE_TAXONOMY from /home/vagrant/git/foreman_hooks/lib/foreman_hooks/as_dependencies_hook.rb:4:in `load_missing_constant' from /home/vagrant/git/katello/app/services/katello/ui_notifications/subscriptions/expire_soon.rb:13:in `block in deliver!' from /home/vagrant/git/katello/app/services/katello/ui_notifications/subscriptions/expire_soon.rb:7:in `each' from /home/vagrant/git/katello/app/services/katello/ui_notifications/subscriptions/expire_soon.rb:7:in `deliver!'

yeah, there's no such audience, I guess we need to find administrators of a given org for which there's no good option atm, @jlsherrill can we find a user who uploaded the manifest for the organization somehow?

We don't track who has uploaded or refreshed a manifest. I would suggest notifying anyone who has access to do that for the given org via the 'import_manifest' permission

dLobatog

Note to self, I'll make a method notification_recipient_ids on the Subscription object, which contains all of the users with import_manifest permissions. Changing the audience to Notification::AUDIENCE_SUBJECT will also be required.

ares · 2018-01-15T16:10:53Z

I think it's not easy to find easily such recipients. There are no relations like this, especially if you consider filtered permissions, where the assignment might be scoped e.g. with "name ~ my". Perhaps we could only search for user who don't have such permission filtered.

dLobatog

@ares I've updated this to use the same permission checking as we do on the API to upload a manifest (e.g: if you can upload a manifest to this organization, you get the notification).

I do realize it's a relatively expensive operation, but I don't see any other way of dealing with it. The task ought to run only twice a day though, so I hope that is not a problem. Another advantage is that by leveraging allowed_to? vs writing complicated permissions logic here, if the permissions model change in Foreman core & that method changes, we're covered and this should work - which wouldn't if I write custom logic here.

ares · 2018-01-22T12:32:14Z

@dLobatog I don't think you need to write custom logic, it's fine to ignore filtering conditions, but I believe it should call user.can?(permission) instead of user.allowed_to?(controller_action), the first does not care about specific path that might change but check for permission assignment

dLobatog

@ares I could use can? but as you say it would mean ignoring specific permissions 'search' scope. With .allowed_to?, I'm able to pass the specific object I want to check the permission for, like:

        user.allowed_to?(
          :controller => 'katello/api/v2/subscriptions',
          :action => 'import',
          :organization_id => organization_id
        )

So it's more precise, I can know for sure the users who should be able to see the notification.

ares · 2018-01-22T15:49:33Z

@dLobatog the allowed_to example that you used only verified that user has permission to access "katello/api/v2/subscriptions/import" route based on mapping in lib/katello/permissions/. The fact you pass organization_id does not matter, parameters are ignored by allowed_to completely. In fact the result is equivalent to user.can?(:create_subscription) which returns true if there's any filter with the this permission assigned to the user. Even better, can? does not have to parse the list of routes and permissions. Using allowed_to? does not make sense since we are really interested in the permission, not whether user has access to specific route. Imagine there would be two routes how you can import manifest...

houndci-bot · 2018-03-01T20:12:03Z

db/seeds.d/109-katello-notification-blueprints.rb

+    group: N_('Subscriptions'),
+    name: 'subs_expire_soon',
+    message: N_('%{expiring_subs} subscriptions are going to expire soon in %{subject}. Please renew them before they expire to guarantee your hosts will continue receiving content.'),
+    level: 'warning',


Avoid comma after the last item of a hash.

houndci-bot · 2018-03-01T20:12:03Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+          end
+
+
+          def actions


Use empty lines between method definitions.

houndci-bot · 2018-03-01T20:12:04Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+            )
+          end
+
+


Extra blank line detected.

houndci-bot · 2018-03-01T20:12:04Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+          def notification_already_exists?(subject)
+            subs_expiration_notification = Notification.unscoped.find_by(:subject => subject)
+            return false if subs_expiration_notification.blank?
+            subs_expiration_notification.notification_blueprint == blueprint


Operator == used in void context.

houndci-bot · 2018-03-01T20:12:04Z

db/seeds.d/109-katello-notification-blueprints.rb

+    group: N_('Subscriptions'),
+    name: 'subs_expire_soon',
+    message: N_('%{expiring_subs} subscriptions are going to expire soon in %{subject}. Please renew them before they expire to guarantee your hosts will continue receiving content.'),
+    level: 'warning',


Avoid comma after the last item of a hash.

houndci-bot · 2018-03-01T20:12:04Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+          end
+
+
+          def actions


Use empty lines between method definitions.

houndci-bot · 2018-03-01T20:12:04Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+            )
+          end
+
+


Extra blank line detected.

houndci-bot · 2018-03-01T20:12:04Z

app/services/katello/ui_notifications/subscriptions/expire_soon.rb

+          def notification_already_exists?(subject)
+            subs_expiration_notification = Notification.unscoped.find_by(:subject => subject)
+            return false if subs_expiration_notification.blank?
+            subs_expiration_notification.notification_blueprint == blueprint


Operator == used in void context.

dLobatog

@jlsherrill @beav Updated, now it creates a notification per organization. It shows the # of subs about to expire in the notification, which links to the search page for that.

Unfortunately for admin users, this will mean that the following case may happen:

admin is in org A, B, C
admin chooses in the UI to be in org A. By design the /subscriptions page forces you to choose.
admin gets notifications about all 3 organizations with subs about to expire.
admin clicks on the link for the subs about to expire in A - it works well.
admin clicks on the link for the subs about to expire in B - since admin is in org A in the UI, it will not show the correct subs. admin needs to change to org B in the UI, then click on the notification.

I hope it's not a dealbreaker, as it's an unlikely case, and moving the user away from the current organization by clicking on the notification seems like an anti-pattern.

dLobatog · 2018-03-05T09:45:41Z

[test katello]

ares · 2018-03-06T08:17:32Z

[test katello]

jyejare · 2018-03-08T11:37:58Z

@dLobatog , Does this PR helps to show an expiration notification before 120 days? Does the notification depend upon the place where the foreman is being accessed (e.g for me, its IST time) ?

dLobatog · 2018-03-08T12:32:43Z

@jyejare Yes, you may change the 120 to any other value in app/models/katello/pool (DAYS_EXPIRING_SOON) although it's not meant to be touched by users. But it can be useful when testing. You can also run Katello::UINotifications::Subscriptions::ExpireSoon.deliver! to ensure it runs the check - otherwise it'll only run every 12 hours.

expiring_soon is an old issue, it should be checking Date.today.utc https://github.com/Katello/katello/blob/master/app/models/katello/pool.rb#L51

jyejare · 2018-03-08T14:41:41Z

@dLobatog @ares , Suggested changes in the content of notification to include the days/date.

dLobatog

@ares @jyejare @jlsherrill @beav

I've updated the notification to include the number of days. I can't be more specific as I'm creating a notification per organization. This means there are multiple subscriptions there, which may vary in their expiring date, so I can't just say 'will expire in X days' as I did before with the 'per-subscription' notification.

To test this:

rake db:seed

Then on rake console:

class Organization
  def expiring_subscriptions
    subscriptions
  end
end
Organization.all.each { |o| Notification.where(:subject => o).destroy_all }
Katello::UINotifications::Subscriptions::ExpireSoon.deliver!

houndci-bot · 2018-03-09T10:47:55Z

app/models/katello/concerns/organization_extensions.rb

+        def notification_recipients_ids
+          User.unscoped.all.find_all do |user|
+            user.can?(:import_manifest, self)
+          end.pluck(:id)


Avoid chaining a method call on a do...end block.

dLobatog · 2018-03-09T23:07:42Z

[test katello]

dLobatog · 2018-03-12T19:27:24Z

#7234

dLobatog

The only tests failing are React ones, seems that the snapshot needs to be updated for ListView, can't be caused by this one, probably theforeman/foreman@e26f63d

dLobatog · 2018-03-12T19:30:48Z

I was missing - https://github.com/Katello/katello/pull/7228/files on my branch, rebasing & pushing again..

jyejare · 2018-03-14T11:15:07Z

@dLobatog , @ares , I tried accessing notifications via org-admin user and via another admin, but I cant ! Looks like there is some issue with users.

The notifications drawer should show a warning when some subscription is about to expire. "expiring_soon?" defined in the subscriptions.rb code says this is 120 days before it expires.

dLobatog

@jyejare Thanks for the feedback, it definitely did NOT work for org admins, there was a bug in the notification_recipients_ids method.

You also asked to configure the expire soon interval, so thanks for that too 😄 Can you check now? It should work 100% fine now.

dLobatog · 2018-03-21T21:55:06Z

app/models/katello/concerns/organization_extensions.rb

+
+        def notification_recipients_ids
+          users = User.unscoped.all.find_all do |user|
+            user.can?(:import_manifest) && user.can?(:view_organizations, self)


@jyejare The problem I had was that previously I was calling user.can?(:import_manifest, self) here. This would not work, because import_manifest is a permission for Subscription, not for Organization. Hence, only admins would return 'true' for that.

tl;dr - It should be fine now for org admins

jyejare · 2018-03-22T08:44:41Z

Tests Performed:

1. Notification should be displayed for expiring subscriptions within given days in settings.
2. Notification should only be shown for expiring subscriptions within given days in settings.
3. Notification should not be shown for subscriptions not expiring within given days in settings.
4. Notification body text should be correct.
5. Notification should be shown for org-admin users as well.
6. No duplication of notifications on multiple time delivers.
7. Pulp Notification for disk storage full with the percent of disk full.

QE ACK.

jlsherrill

Thanks @dLobatog !

theforeman-bot added Needs testing Not yet reviewed labels Apr 19, 2017

houndci-bot reviewed Apr 19, 2017

View reviewed changes

dLobatog force-pushed the 19314 branch from add5b06 to 343725a Compare April 19, 2017 11:40

ohadlevy suggested changes May 9, 2017

View reviewed changes

dLobatog force-pushed the 19314 branch from 343725a to 3b20bab Compare May 25, 2017 11:45

dLobatog force-pushed the 19314 branch from 3b20bab to cd785bd Compare August 8, 2017 15:30

houndci-bot reviewed Aug 8, 2017

View reviewed changes

dLobatog force-pushed the 19314 branch 2 times, most recently from d9f7977 to 51af787 Compare August 8, 2017 15:32

dLobatog commented Aug 8, 2017

View reviewed changes

dLobatog force-pushed the 19314 branch from 51af787 to 447704f Compare October 24, 2017 09:09

dLobatog commented Oct 24, 2017

View reviewed changes

dLobatog force-pushed the 19314 branch from 447704f to f58c484 Compare December 7, 2017 13:16

dLobatog commented Dec 7, 2017

View reviewed changes

jlsherrill reviewed Jan 11, 2018

View reviewed changes

dLobatog commented Jan 15, 2018

View reviewed changes

dLobatog force-pushed the 19314 branch from f58c484 to 9152b01 Compare January 19, 2018 15:02

dLobatog commented Jan 19, 2018

View reviewed changes

dLobatog commented Jan 22, 2018

View reviewed changes

theforeman-bot added the Needs testing label Mar 1, 2018

houndci-bot reviewed Mar 1, 2018

View reviewed changes

dLobatog force-pushed the 19314 branch from 2894adc to 8003ed5 Compare March 1, 2018 20:16

dLobatog commented Mar 1, 2018

View reviewed changes

dLobatog commented Mar 9, 2018

View reviewed changes

dLobatog force-pushed the 19314 branch from 8003ed5 to 8714256 Compare March 9, 2018 10:47

houndci-bot reviewed Mar 9, 2018

View reviewed changes

dLobatog force-pushed the 19314 branch from 8714256 to e13b752 Compare March 9, 2018 10:49

dLobatog mentioned this pull request Mar 12, 2018

Fixes #22867 - Configurable 'expiring soon' days #7234

Merged

dLobatog commented Mar 12, 2018

View reviewed changes

dLobatog force-pushed the 19314 branch from e13b752 to 536e45f Compare March 14, 2018 09:30

dLobatog force-pushed the 19314 branch 2 times, most recently from e72d549 to 343e7b8 Compare March 21, 2018 21:46

Fixes #19314 - Notification for subscr. expiring soon

2ef7204

The notifications drawer should show a warning when some subscription is about to expire. "expiring_soon?" defined in the subscriptions.rb code says this is 120 days before it expires.

dLobatog force-pushed the 19314 branch from 343e7b8 to 2ef7204 Compare March 21, 2018 21:54

dLobatog commented Mar 21, 2018

View reviewed changes

jlsherrill approved these changes Mar 22, 2018

View reviewed changes

jlsherrill merged commit 390b096 into Katello:master Mar 22, 2018

dLobatog deleted the 19314 branch April 3, 2018 08:48


		# Only create notifications if there isn't a scheduled job
		SendExpireSoonNotifications.perform_later if scheduled_job.blank?

		@@ -0,0 +1,9 @@
		class SendExpireSoonNotifications < ActiveJob::Base
		after_perform do

Fixes #19314 - Notification for subscr. expiring soon #6752

Fixes #19314 - Notification for subscr. expiring soon #6752

Conversation

dLobatog commented Apr 19, 2017 • edited

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

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

Choose a reason for hiding this comment

dLobatog commented May 25, 2017

jlsherrill commented May 31, 2017

akofink commented Jul 26, 2017

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

dLobatog left a comment

Choose a reason for hiding this comment

ares commented Oct 16, 2017

dLobatog left a comment

Choose a reason for hiding this comment

ares commented Dec 4, 2017

dLobatog left a comment

Choose a reason for hiding this comment

ares commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dLobatog left a comment

Choose a reason for hiding this comment

ares commented Jan 15, 2018

dLobatog left a comment

Choose a reason for hiding this comment

ares commented Jan 22, 2018

dLobatog left a comment

Choose a reason for hiding this comment

ares commented Jan 22, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dLobatog left a comment • edited

Choose a reason for hiding this comment

dLobatog commented Mar 5, 2018

ares commented Mar 6, 2018

jyejare commented Mar 8, 2018

dLobatog commented Mar 8, 2018

jyejare commented Mar 8, 2018

dLobatog left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dLobatog commented Mar 9, 2018

dLobatog commented Mar 12, 2018

dLobatog left a comment

Choose a reason for hiding this comment

dLobatog commented Mar 12, 2018

jyejare commented Mar 14, 2018

dLobatog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyejare commented Mar 22, 2018

jlsherrill left a comment

Choose a reason for hiding this comment

dLobatog commented Apr 19, 2017 •

edited

dLobatog left a comment •

edited

dLobatog left a comment •

edited