Fixes #35503 - Notify user only when new errata are added to repo #10328
Conversation
|
Issues: #35503 |
|
Credits / co-author should rather go to @hao-yu than me :) |
| last_updated = repo.repository_errata.order('updated_at ASC').last.try(:updated_at) || Time.now | ||
| plan_self(:repo => repo.id, :contents_changed => contents_changed, :last_updated => last_updated.to_s) | ||
| def plan(repo) | ||
| plan_self(:repo => repo.id, :associated_errata_before_syncing => repo.repository_errata.pluck(:erratum_id).uniq.sort.reverse, :new_associated_errata => []) |
There was a problem hiding this comment.
Why the .reverse (dtto for run)?
There was a problem hiding this comment.
I previously tried a format where the truncated input remains an Array containing the first few IDs, appended with the truncation message. The reversed order made sense because the ordering of the erratum ids corresponds to the order which the erratum records were created, so with a partially truncated output it means we see the most recently created (which in most cases would be the most recently associated) at the top, which provides a "good intuition" for the cutoff between new vs. prior associated errata.
I then leaned towards just replacing the entire Array with a string containing the count, if the length is greater than some number, reasoning that it's simpler for the user to read (the data types were mixed within the Array in the previous way, so the user would see several integers followed by an ugly double-quoted string) and maybe easier to maintain.
But I chose the Array length for truncation to kick in very arbitrarily as 3, and I'm not sure if there is some better value. In particular if we allow the user to see a larger Array then I think the reverse ordering feels more intuitive. Since Array reversal should be cheap (linear complexity) I just left it in.
What do you think about it?
There was a problem hiding this comment.
Ah I see, then it makes sense to me.
I didn't list @hao-yu as co-author because he is the primary author (of the commit itself). If that is confusing or contrary to convention I could add his information in both places (first time using this GH feature, but it seems very useful) |
Co-authored-by: William Bradford Clark <wclark@redhat.com> Co-authored-by: Pavel Moravec <pmoravec@redhat.com>
wbclark
force-pushed
the
35503_new_repository_errata_only
branch
from
7646f59
to
0a4405a
Compare
Ah ok then. I am ok with that credits then. I just simply didnt want to get more credit than Hao :) |
Co-authored-by: William Bradford Clark wclark@redhat.com
Co-authored-by: Pavel Moravec pmoravec@redhat.com
What are the changes introduced in this pull request?
Instead of using contents_changed for input (and corresponding middleware) we use new inputs associated_errata_before_syncing and new_associated_errata; in planning, we set the former to the current list of associated errata and initialize the latter as an empty array. Later in the run step (after the repository has synced) we re-calculate the associated errata and set new_associated_errata as the difference -- only sending the notification email when this is non-empty.
Considerations taken when implementing this change?
To avoid keeping potentially large arrays in the database (and to create better UX viewing the action in the dynflow console), after we are done using the new inputs to determine what (if any) emails to send, we truncate them and replace with a message displaying the count of errata rather than the complete list of IDs. More than 3 errata was chosen (somewhat arbitrarily) as a cutoff point, because the full information will be available in the email itself. This prevents us from seeing very large inputs for the action when viewing the action in dynflow console after the action has run.
What are the testing steps for this pull request?
associated_errata_before_syncing: []and some number ofnew_associated_errata/var/spool/mail/$userassociated_errata_before_syncingbutnew_associated_erratais[]/var/spool/mail/$user