Refactor raisepriority and track automatic prio bumps via history element #3994
Conversation
bgeuken
changed the title
bgeuken
force-pushed
the
refactor_raisepriority
branch
from
f3bd8f5
to
71558e4
Compare
This move the code that checks if we can raise the priority into a separate method. This allows us to shorten the raisepriority method quite some code duplication. Both methods are now private.
bgeuken
force-pushed
the
refactor_raisepriority
branch
from
71558e4
to
a8d50c6
Compare
Part of the bs request validation is to check, if the priority of a associated bs_request_action is higher than the priority of the bs_request. If that's the case, the priority of the bs_request get's raised. This wasn't logged via the history elements and might be one of the culprits of Issue#3897 – history elements were missing.
bgeuken
force-pushed
the
refactor_raisepriority
branch
from
a8d50c6
to
6d9c684
Compare
Codecov Report
@@ Coverage Diff @@
## master #3994 +/- ##
==========================================
+ Coverage 89.23% 89.23% +<.01%
==========================================
Files 309 309
Lines 18228 18228
==========================================
+ Hits 16265 16266 +1
+ Misses 1963 1962 -1
|
|
@openSUSE/open-build-service Please review @adrianschroeter Speak now or be silent forever;-) |
| return unless persisted? | ||
|
|
||
| # rubocop:disable Style/GuardClause | ||
| if priority_changed? |
There was a problem hiding this comment.
Why you disable rubocop here?
There was a problem hiding this comment.
Because I am not going to write a guard claus for a 7 line statement 🤣
There was a problem hiding this comment.
LGTM. As a side comment though if we are going to be making a lot of changes to the priority stuff here I would suggest refactoring to use a gem like state_machine because it is made for exactly what we're doing and would handle a lot of stuff like validations out-of-the-box as well as give us a consistent interface to define priorities and their related logic.
https://github.com/pluginaweek/state_machine
|
Already spoke with Adrian today and he is fine with the approach => merging |
No description provided.