-
Notifications
You must be signed in to change notification settings - Fork 10.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: update cached setting immediately at the time of updating the db #32742
Conversation
🦋 Changeset detectedLatest commit: 8931608 The changes in this PR will be included in the next version bump. This PR includes changesets to release 32 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Looks like this PR is ready to merge! 🎉 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #32742 +/- ##
========================================
Coverage 55.54% 55.54%
========================================
Files 2634 2634
Lines 57248 57274 +26
Branches 11853 11864 +11
========================================
+ Hits 31797 31815 +18
- Misses 22757 22760 +3
- Partials 2694 2699 +5
Flags with carried forward coverage won't be shown. Click here to find out more. |
apps/meteor/ee/tests/unit/server/settings/SettingsRegistry.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO too many modifications to a simple regression. the fix/regression has by far more changes than the original fix.
To be honest, I can't understand exactly what error is being corrected. I didn't find explanations in the task or in the pull request. It seems to me that the original correction was not so correct?
The only thing that catches my attention is the
const settingFromCodeOverwritten = overwriteSetting(settingFromCode);
const settingOverwrittenDefault = overrideSetting(settingFromCode);
and then
const updatedSettingAfterApplyingOverwritten = isOverwritten ? settingFromCodeOverwritten : settingOverwrittenDefault;
In my idea, both should have the same value
I would like to see a revert of what was done and the same correction again on top of the original code, this way the original code is completely modified, to the point that I no longer remember that we wanted to update the cache at the time of the overwrite
apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Kevin Aleman <[email protected]>
apps/meteor/tests/unit/app/settings/server/functions/compareSettingsMetadata.tests.ts
Outdated
Show resolved
Hide resolved
…ttingsMetadata.tests.ts
0a0a629
to
88ae469
Compare
* test: cached settings expecting delay because changestream * test: change update test to not expect delays (expect to fail) * fix: change `SettingsRegistry` to update immediately
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-545
Steps to test or reproduce
Further comments