Allow specifing database backup paths. #7035
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #7035 +/- ##
===========================================
+ Coverage 63.87% 63.92% +0.05%
===========================================
Files 330 330
Lines 41838 41897 +59
===========================================
+ Hits 26720 26779 +59
Misses 15118 15118
Continue to review full report at Codecov.
|
|
@ijordse The current behavior is to name backups according to the pattern specified and save them under the given path. So a pattern of "{DB_NAME}-{TIME:dd.MM.yyyy}.backup" with a path of "/tmp" and db name "test" would save backups as "/tmp/test-11.10.2021.backup". Is that not what your comment suggests? Could you elaborate? |
droidmonkey
force-pushed
the
feature/3279-choose-backup-location
branch
from
7d74520 to
5166f83
Compare
|
Minor cleanup. I removed the ConfigContext since it doesn't conform to our code standards and it really should be handled in the init() instead. Although I am getting a failure in the backup test locally but in autoreload test on ci. |
Im not sure if this is a good idea, especially against the background of the test failure the last commit caused. I think we need a more robust solution here, it's way to easy to forget adding/removing respective config()->set calls when editing tests. I think it makes sense that the init() method is responsible for providing a clean test environment and would suggest simply reloading the config in init() or resetting it to defaults. In line with this, tests should not depend on the value of specific settings even after calls to init() but rather set those they depend on in the test body (thats essentially why the AutoreloadDatabase test failed). |
|
Would you mind labeling the PR with "hacktoberfest-accepted"? I'm fine with doing further modifications, but it's almost the end of October and i believe that it satisfies their expectations w.r.t. quality at this point. I believe that the windows gui test failure is transient and not caused by the PR. |
|
I was actually working on this as you committed. I simplified the tools function a lot. |
|
I'm going to look into the test failure... |
droidmonkey
force-pushed
the
feature/3279-choose-backup-location
branch
3 times, most recently
from
fdf0fab to
2bccaf8
Compare
Update database saveAs() and save() usages.
- Default backupFilePath is '{DB_FILENAME}.old.kdbx' to conform to existing standards
- Implement backupPathPattern tests.
- Show tooltip on how to format database backup location text field.
Code cleanup
* Remove ConfigContext addition
Remove dependency on default config setting in AutoreloadDatabase, Lines 361-368.
Create substituteBackupFilePathPattern in Tools.cpp.
Add tests for substituteBackupFilePathPattern.
droidmonkey
force-pushed
the
feature/3279-choose-backup-location
branch
from
00394c6 to
0ca3638
Compare
|
Thank you for this feature! That was the only missing thing that prevented me to migrate from classic KeePass. |
phoerious
added
pr: new feature
Resolves #3279.
Allow to specify a custom backup location & filename. The filename may contain patterns "{DB_NAME}" and "{TIME:xxx}" which will be replaced with the database name and the current time, formatted according to "xxx", respectively.
The backup path can be relative or absolute. The former creates the backup relative to the database file.
The following changes have been made:
backupDatabase()now takes a destination file path as argumentsaveAs,saveandperformSavefunctionsgetDefault(key)method toConfig. This returns the default value.backupFileNamePatternandbackupFilePathApplicationSettingsWidgetaccordinglyLimitations/open issues (please comment)
backupFileNamePatternneeds to created. Or do you think it suffices to explain it in a tooltip?Screenshots
Testing strategy
Modified database backup unit test accordingly.
Type of change