Adds purging for notifications

[NickLaMuro]

• Sets notifications that are older than a week to be purged by default.
• Purging is run daily
• Clears out notification recipients as well

Links

• Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1544890
• Pretty much a copy-pasta of this PR: Add orphan purging for vim_performance_states #16754
  • (minus using the orphan purger)

[miq-bot]

Checked commit NickLaMuro@3820086 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. 👍

[carbonin]

I'm not sure if it would look like this PR, but NotificationType does have an expires_in field. Should we be trying to use that?

Or, if we're going to go this route, should we remove that column entirely? It doesn't look like it gets used anywhere.

[jrafanie]

LGTM, although I'd like @carbonin to review since he added the vim performance states purging 🚌 💥

[jrafanie]

Or, if we're going to go this route, should we remove that column entirely? It doesn't look like it gets used anywhere.

heheh, 🔥 ✂️ 🚽

❤️ 😍

[NickLaMuro]

@carbonin @jrafanie If it is any indicator from the PR, since I basically committed "code plagiarism", I really was just doing some "copy pasta" here, so I really don't have an opinion one way or the other.

That said, I did some git history digging, and this is where the notifications where added:

#10623

Looks like the expires_in column was only added, but never implemented (for the record, it changed to use .to_i_with_method here). Since then, it looks like the seeding is the only place in the code base where this field is used.

---

So, that said, should make the purging for this PR more dynamic and take into account the expires_in timer? I don't mind doing the work to get that into place, and I honestly didn't know that was a column until you mentioned it, so I wasn't considering it in the first place.

[carbonin]

If you're game to try to incorporate expires_in here I think that would be great, but at first look it might be difficult to do using the purging stuff you're using here.

I think the purging mixin assumes that everything in the table should be purged by the same interval so it might need multiple passes or some non-trivial database queries.

If this won't fit into the existing purging mixin then I vote for removing the expires_in and going with this. So maybe create a separate PR for the expires version?

[jrafanie]

If this won't fit into the existing purging mixin then I vote for removing the expires_in and going with this. So maybe create a separate PR for the expires version?

If nothing in the service ui or classic ui or backend code uses or has plans to to use expires_in field, my vote would be to remove the column and treat this table like any other we purge.

[kbrock]

can't get any simpler than this.
I like both PRs. will put some more comments in the other

[Fryguy]

@skateman @gtanzillo Should we care about expires_in for notification purging?

[skateman]

@miq-bot request_review @skateman

[miq-bot]

@skateman unrecognized command 'request_review', ignoring...

Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

[skateman]

oh, this doesn't work yet 😞

[NickLaMuro]

Adding [WIP] label as well to make sure this doesn't get #yoloMerged

[skateman]

@Fryguy yes, I think the expires in reflects when the notification should be purged

[jrafanie]

I'm in favor of this version of the purging of notifications for a few reasons:

• we already have a standard way of purging based on time
• expires_in doesn't convey "this will be purged when it expires"
• expires_in would create a "different" complicated way of purging, a complexity we don't need. Compare the code (and description) of this PR and the alternative of using expires_in here

If we don't have explicit features we'd be losing in ui classic, service ui, or the api by removing expires_in, I'd vote for keeping purging as simple and boring as possible (and the same as everything else) by using this PR and removing the expires_in column if it's not used (in a followup PR).

[skateman]

@jrafanie true story that the expires_in is not being used, so I leave it up to you guys. But I like simplicity 😉

[NickLaMuro]

Not my call either. I leave it up to who ever designed this in the first place or has a more valid stake in it than I am.

Seems like it was Simon and @gtanzillo who first put together the notifications, for what that is worth: #10623

[jrafanie]

I just want to say I advocated for deleting something without using any emoji.

[NickLaMuro]

I just want to say I advocated for deleting something without using any emoji.

@jrafanie False, you totally used emoji to agree with a statement that agreed with your own! Don't think I didn't see you doing that there! 🔍

[skateman]

[jrafanie]

I think we're good if we received @skateman's seal of approval 😉

[gtanzillo]

Seems like it was Simon and @gtanzillo who first put together the notifications, for what that is worth: #10623

I'm 👍 for going with the simple approach since expires_in is not currently being used and it's not clear whether it will ever be used. I'm not even sure if expires_in was actually intended for this purpose, anyway.

If everybody's in agreement, we can un-wip and merge this.

[NickLaMuro]

If 10k is an agreed to value from various performance issues, I'm good with that. If we need to discuss that, ...

Addressed in chat, we will discuss if this is needed at a later date.

[jrafanie]

[jrafanie]

@NickLaMuro can you see if this will easily backport to fine and euwe and add those labels? I think this is a no-brainer if it's easy.

[NickLaMuro]

@jrafanie Looks like there were some additions to config/settings.yml and app/model/miq_schedule_worker/runner.rb that made this a non-conflict backport to those two. I think @carbonin 's PR of #16754 didn't make it back in those branches, so that is part of it, along with whatever code was merged for the binary_blob purging.

Short story: No go on easy backport 😢

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit 920e709435e62754456237d03974730d0d0f9228
Author: Joe Rafaniello <[email protected]>
Date:   Thu Mar 8 15:33:38 2018 -0500

    Merge pull request #17046 from NickLaMuro/notification-purging
    
    Adds purging for notifications
    (cherry picked from commit 427257a185aed602d1596dfb81c623254957fbe6)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1553780

[simaishi]

@NickLaMuro The BZ has cfme-5.8.z flag (but not 5.7.z). Can you create a PR for Fine branch?

[NickLaMuro]

@simaishi short answer: sure, I personally don't mind

---

Long answer: I was under the impression that this was a "new feature" and an RFE BZ by my interpretation, so backporting wasn't something we were trying to target. I was unaware of/didn't notice the flag, so I wasn't planning to even have this backported to G in the first place, let alone Fine.

Personally I don't think it makes sense to introduce a new feature like this into a older version, but I am not the boss around these parts, so will just do as I am told. </2cents>

Will have a PR for the backport shortly.

[simaishi]

@NickLaMuro ha ha, I'm not the boss either! Feel free to ask the 'right' person and get that flag removed 😄

[NickLaMuro]

@simaishi Here you go just in case :)

#17294

[miq-bot]

@NickLaMuro unrecognized command 'add', ignoring...

Accepted commands are: add_label, add_reviewer, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

[NickLaMuro]

@miq-bot add_label fine/yes

Talked with @dmetzger57 and it looks like we are going to go with putting this in fine, which is fine.

[simaishi]

Backported to Fine via #17294

[skateman]

@NickLaMuro we would like to have something similar for the SystemConsole model, but first I'd like to rename it to something more meaningful 😉