MiqReport.seed skips a report when a custom report exists with same name #18377
Conversation
| report.update_attributes!(attrs) | ||
| rescue ActiveRecord::RecordInvalid | ||
| duplicate = find_by(:name => name) | ||
| if duplicate&.rpt_type == "Custom" |
There was a problem hiding this comment.
😩 Nooooo... I hate this syntax...
(pedantic/personal preference)
There was a problem hiding this comment.
Out of curiosity what is the syntax you'd prefer (FWIW I like this syntax :) )
There was a problem hiding this comment.
@Fryguy while it is "Rails specific", and I usually would reach for something out of STDLIB, .try would be preferred, or just explicitly doing it via a not duplicate.nil? and duplicate.rpt_type == "Custom".
The & symbol is already overloaded in my opinion, and is used for AND (&&), and bit-wise AND (&) already, so I just think this is a obscure syntax that doesn't help with readability in any form. Would have preferred a more verbose method like attempt() even over a single character operator. </rant> </2cents>
(Again: My personal preference, and it goes against rubocop's rule set, so not worth the change).
There was a problem hiding this comment.
You would think with a name like lonely operator, it would attract more friends.
I don't like how it looks like invalid syntax or how it tries to make ugliness less ugly but besides that, I can dig it.
| report.update_attributes!(attrs) | ||
| rescue ActiveRecord::RecordInvalid | ||
| duplicate = find_by(:name => name) |
There was a problem hiding this comment.
Thankfully this should be only happening on a rare occasion, so shouldn't be a big deal as far as adding to the N+1 count. 👍
That said, if we wanted, we could maybe raise these all at the end, and do a single find query. Probably not worth it, just a thought.
|
Run |
|
@NickLaMuro I'll fix some of the minor comments |
Prefer the custom report over the seeded report and log a warning when this case occurs.
Fryguy
force-pushed
the
fix_report_seed_dups
branch
from
75031d3 to
f05016c
Compare
|
@NickLaMuro Updated. |
|
Checked commit Fryguy@f05016c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
Prefer the custom report over the seeded report and log a warning when
this case occurs.
@NickLaMuro @jrafanie Please review.
This should fix the issue with some of the UI team having custom reports with the exact same name as the seeded reports and it blowing up.