feature: managed settings. - #10589
Draft
camilasan wants to merge 22 commits into
Draft
Conversation
It should cover upgrade, downgrade as well as upgrade from unbranded to branded, from legacy to branded and from legacy to unbranded. Signed-off-by: Camila Ayres <hello@camilasan.com>
Signed-off-by: Camila Ayres <hello@camilasan.com>
If no legacy file is found, it means it might be upgrade only. Signed-off-by: Camila Ayres <hello@camilasan.com>
…to ConfigFile. Signed-off-by: Camila Ayres <hello@camilasan.com>
…n class. - Make all Migration class members static. - Rename Migration class members. Signed-off-by: Camila Ayres <hello@camilasan.com>
Signed-off-by: Camila Ayres <hello@camilasan.com>
Signed-off-by: Camila Ayres <hello@camilasan.com>
Signed-off-by: Camila Ayres <hello@camilasan.com>
isUpgrade() and isDowngrade() were mutating _upgradeType as a side effect but only when the condition was true, never clearing it. This meant a stale _upgradeType value could cause both methods to return a wrong result if called after state had already been set. Make both methods (and versionChanged/shouldTryToMigrate) const and compute the result directly from version comparison, with no writes to shared state. Add Migration::resetForTesting() to allow unit tests to restore all static fields to their initial values between test cases. Fix a double-semicolon typo in the static initialiser. Signed-off-by: Camila Ayres <hello@camilasan.com>
…Migration. Migration().isDowngrade() was constructing an anonymous temporary object when a instance was already in scope. Use the existing local variable instead. Signed-off-by: Camila Ayres <hello@camilasan.com>
- Add init() slot that calls Migration::resetForTesting() before each test case so static state does not leak between tests. - Add test cases covering: isInProgress for all phases, rollback prevention, isUpgrade/isDowngrade without side effects, downgrade scenario, version unchanged path, shouldTryToMigrate for both true and false cases, and shared static state across instances. Signed-off-by: Camila Ayres <hello@camilasan.com>
Replace the old linear arrow list with an indented graph that shows branch points and phase transitions matching the current implementation. Signed-off-by: Camila Ayres <hello@camilasan.com>
…ng setter. init() was only resetting the Migration static state, not the QSettings file on disk. testVersionUnchanged writes clientPreviousVersionString to the config file, causing testShouldTryToMigrate_trueOnUpgrade to see the current version as the previous one, making isUpgrade() return false and the test fail. testIsDowngrade_noSideEffects was also calling setClientVersionString instead of setClientPreviousVersionString, so isDowngrade() which reads previousVersion() was always returning false. Signed-off-by: Camila Ayres <hello@camilasan.com>
legacyData() calls setDiscoveredLegacyConfigPath() and setLegacyData() on this, which are non const methods. Marking the function const was wrong since it has side effects and GCC/Clang both reject it with -Werror. Remove the const qualifier. Signed-off-by: Camila Ayres <hello@camilasan.com>
…ration. These four constexpr variables were already moved to migration.cpp as part of the refactor. The leftover copies in accountmanager.cpp were causing -Wunused-const-variable errors on Linux and macOS. Signed-off-by: Camila Ayres <hello@camilasan.com>
legacyData() returned a QSharedPointer while also stashing a co owning copy in the static _legacyData, and restoreFromLegacySettings adopted that same raw pointer into a std::unique_ptr. The QSettings then had two owners and was freed twice at shutdown or on the next reset. Make LegacyData a std::unique_ptr, move it to the caller, drop the unused _legacyData static, and std::move it into settings at the call site so a single owner remains. Add tests for legacy config discovery and the no legacy config case, and remove a stale call to the nonexistent ConfigFile::setForceLoginV2 that kept the test from compiling. Assisted-by: Claude Code:claude-opus-4-8 Signed-off-by: Camila Ayres <hello@camilasan.com>
…nary shouldTryToMigrate() compared clientVersion against clientPreviousVersion through isClientVersionSet(). Once an upgrade settled those two values to be equal, later upgrades were no longer detected, while a freshly upgraded config whose clientVersion already matched the running binary still fired a redundant migration pass. Restore the previous behaviour: compare the stored config version against the running binary, as the old ConfigFile::hasVersionChanged() did. Remove the now unused isClientVersionSet(). Add tests for the already current config and the upgrade from equal versions cases. Assisted-by: Claude Code:claude-opus-4-8 Signed-off-by: Camila Ayres <hello@camilasan.com>
isUnbrandedToBrandedMigration() returned brandingType() == UnbrandedToBranded, but that flag was only latched inside shouldTryUnbrandedToBrandedMigration() once the SetupFolders phase was reached. Callers running during SetupConfigFile and SetupUsers (proxy password in AccountManager, policy and system settings in ConfigFile) therefore read from the branded location instead of the unbranded one for a branded client migrating from an unbranded install. Make both queries self contained and const, matching the old ConfigFile::isUnbrandedToBrandedMigrationInProgress semantics, and drop the side effect that mutated shared state from a query. Only observable in branded builds, so it is not unit tested here: appName equals the unbranded name in the default build. Assisted-by: Claude Code:claude-opus-4-8 Signed-off-by: Camila Ayres <hello@camilasan.com>
…gration Application::configVersionMigration inlined the same launch on startup, prompt delete default and cfg backup loop that ConfigFile::backupConfigFiles already provides, so the two copies could drift. Call the method instead. Assisted-by: Claude Code:claude-opus-4-8 Signed-off-by: Camila Ayres <hello@camilasan.com>
All Migration state is process global, but the API was instance based so every call site created a throwaway `Migration migration;`. Make the methods static, delete the constructor to forbid instantiation, and update every call site to `Migration::` in Application, AccountManager, AccountState, FolderMan, ConfigFile and the tests. Also add per test teardown to the tests (folder manager reset, AccountManager shutdown) so created accounts do not bleed across tests through the singleton, and drop the now meaningless state shared across instances test. Assisted-by: Claude Code:claude-opus-4-8 Signed-off-by: Camila Ayres <hello@camilasan.com>
Assisted-by: Claude Code:claude-opus-4-8 Signed-off-by: Camila Ayres <hello@camilasan.com>
Resolves the effective value of a setting from a set of injectable sources and returns metadata (winning source, default or locked). Precedence: locked policy, then user config, then the highest default, then the builtin default. Assisted-by: Claude Code:claude-opus-4-8 Signed-off-by: Camila Ayres <hello@camilasan.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Resolves
#5497
This PR is branched off #9191, that why the 22 commits (for now).
Summary
First slice of managed settings support for the desktop client as described in #5497. It adds a small resolver that computes the effective value of a setting from a set of sources and reports where the value came from and whether it is locked. No production settings are routed through it yet; this is the foundation the later work builds on.
This starts from the Migration logic refactoring in PR #9191, which ties the settings readers this will extend.
TODO
See plan in #5497 (comment).
Checklist
AI (if applicable)