Fixes #32415 - access only through SettingRegistry #8465
Conversation
|
Issues: #32415 |
ezr-ondrej
force-pushed
the
setting_registry_update
branch
from
1a46e41
to
1f2f72f
Compare
ezr-ondrej
changed the title
ezr-ondrej
force-pushed
the
setting_registry_update
branch
from
1f2f72f
to
ca0f8a7
Compare
ezr-ondrej
force-pushed
the
setting_registry_update
branch
2 times, most recently
from
bc37989
to
fe17545
Compare
|
Merged the blocking PR, please rebase. |
|
@ezr-ondrej, this pull request is currently not mergeable. Please rebase against the develop branch and push again. If you have a remote called 'upstream' that points to this repository, you can do this by running: This message was auto-generated by Foreman's prprocessor |
ezr-ondrej
added
Not yet reviewed
and removed
Blocked by another PR
Waiting on contributor
labels
ezr-ondrej
force-pushed
the
setting_registry_update
branch
from
fe17545
to
8ffd5ad
Compare
|
[test katello] |
|
We are green here for some time, do I need to do anything to get this one merged? :) |
app/services/setting_registry.rb
Outdated
| # rubocop:disable Style/DoubleNegation | ||
| case (!!value == value ? 'boolean' : value.class.to_s.downcase) | ||
| # rubocop:enable Style/DoubleNegation |
There was a problem hiding this comment.
It still surprises me that Ruby doesn't have a Boolean.new(value) method (or similar) to force it as a boolean.
That said: why isn't checking for trueclass / falseclass no longer the right thing?
There was a problem hiding this comment.
I was looking for something shorter, this works the best from what I found and is much easier to read IMHO.
But both are fine, this is shorter. If you think checking for two classes was better, I'm ok with that as well :)
There was a problem hiding this comment.
To me that was easier to understand as a reader. I'm not used to reading !! and the fact Rubocop disables it by default probably also means something.
There was a problem hiding this comment.
Back to using the trueclass and falseclass now :)
ezr-ondrej
force-pushed
the
setting_registry_update
branch
4 times, most recently
from
495e7b8
to
676ed86
Compare
ezr-ondrej
force-pushed
the
setting_registry_update
branch
from
676ed86
to
db3b398
Compare
|
@tbrisker could you give it a look pls? :) |
|
|
||
| def set_user_value(name, value) | ||
| definition = find(name) | ||
| raise ActiveRecord::RecordNotFound.new(_("Setting definition for '%s' not found, can not set") % name, Setting, name) unless definition |
There was a problem hiding this comment.
This is not foreman exception, but ActiveRecord, so we need to translate here.
ezr-ondrej
force-pushed
the
setting_registry_update
branch
from
db3b398
to
11b427c
Compare
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
| when db_record.settings_type | ||
| db_record.value = value | ||
| else | ||
| raise ::Foreman::Exception.new(N_('expected a value of type %s'), @setting.settings_type) |
There was a problem hiding this comment.
You don't need to perform the interpolation of %s here?
Edit: never mind, looks from other code that this is correct.
There was a problem hiding this comment.
yeah, this is correct, the interpolation is for recordnotfound execption, that doesn't do any translation for message.
So the Marek's change request was also wrong, getting it back.
Update and read the settings through SettingRegistry. Adds a layer between controller and Model. SettingRegistry is proxying the value parsing method to the model. This lifts blocker for settings without DB record.
ezr-ondrej
force-pushed
the
setting_registry_update
branch
from
11b427c
to
00bbf55
Compare
|
All green, thanks @ezr-ondrej and sorry for turning you wrong direction. Merging! |
Update and read the settings through SettingRegistry.
Adds a layer between controller and Model.
SettingRegistry is proxying the value parsing method to the model.
This lifts blocker for settings without DB record.
Blocked by #8438