Refactored to save settings only if value changes #9194
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
refs TryGhost#9192 - Each setting is saved individually - Update this to only happen on import, or when a value changes - Reduces the amount of work Ghost does on every setting change
|
@kirrg001 & @kevinansfield I have tested this and it seems to work well. Just wanted to quickly check - can either of you think of anything this might break? That's all I want review for 😄 |
Nothing that I can think of 🤔 Sounds like a sane change to me! |
kevinansfield
approved these changes
Oct 31, 2017
kirrg001
suggested changes
Oct 31, 2017
core/server/models/settings.js
Outdated
| } | ||
|
|
||
| if (setting.hasChanged()) { | ||
| return setting.save(options); |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
@kirrg001 good catch, pushed a fix |
kirrg001
approved these changes
Oct 31, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is quite a small change, but with potentially larger consequences. At the moment, requests from the Ghost Admin come into the settings endpoint with multiple settings to update. There's no guarantee any of them have changed.
On the server side, we blindly processed all the settings, saving each one with the "new" value regardless of whether anything changed. The updated_at value will change every time.
The code in this PR changes this to be smarter. We set relevant properties on the model and then if bookshelf's
hasChangedmethod tells us any of the values are different, we save, else we just return.The benefit is less events flying through the settingsCache system. My end goal here related to #9192 is to know when a setting is really changed to check if a route like
/subscribers/or/amp/should be enabled or not.refs #9192