Skip to content
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.

Sign up for GitHub

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

New setting: Allow creating subfolders for each profile #69

Merged
merged 8 commits into from
Feb 22, 2024
Merged

New setting: Allow creating subfolders for each profile #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
13c7a73
Allow creating subfolders for each profile
personalizedrefrigerator Feb 7, 2024
e6f152e
Update README.md with new setting info
personalizedrefrigerator Feb 7, 2024
41a2354
Different profile directory name in dev mode
personalizedrefrigerator Feb 15, 2024
cc1d19a
Adjust comment
personalizedrefrigerator Feb 15, 2024
8d89b75
Merge remote-tracking branch 'upstream/develop' into pr/allow-creatin…
personalizedrefrigerator Feb 19, 2024
f284a6d
Fix tests
personalizedrefrigerator Feb 19, 2024
61403f2
Merge remote-tracking branch 'upstream/develop' into pr/allow-creatin…
personalizedrefrigerator Feb 21, 2024
a402958
Fix README duplicated for each subfolder
personalizedrefrigerator Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 78 additions & 6 deletions __test__/backup.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ describe("Backup", function () {
.calledWith("path").mockImplementation(() => Promise.resolve(testPath.backupBasePath))
.calledWith("zipArchive").mockImplementation(() => "no")
.calledWith("execFinishCmd").mockImplementation(() => "")
.calledWith("usePassword").mockImplementation(() => false);
.calledWith("usePassword").mockImplementation(() => false)
.calledWith("createSubfolderPerProfile").mockImplementation(() => false);

/* prettier-ignore */
when(spyOnGlobalValue)
Expand Down Expand Up @@ -200,7 +201,7 @@ describe("Backup", function () {
path.dirname(os.homedir()),
path.join(os.homedir(), "Desktop"),
path.join(os.homedir(), "Documents"),

// Avoid including system-specific paths here. For example,
// testing with "C:\Windows" fails on POSIX systems because it is interpreted
// as a relative path.
Expand All @@ -219,6 +220,71 @@ describe("Backup", function () {
);
});

describe("backups per profile", function () {
test.each([
{
rootProfileDir: testPath.joplinProfile,
profileDir: testPath.joplinProfile,
joplinEnv: "prod",
expectedProfileName: "default",
},
{
rootProfileDir: testPath.joplinProfile,
profileDir: testPath.joplinProfile,
joplinEnv: "dev",
expectedProfileName: "default-dev",
},
{
rootProfileDir: testPath.joplinProfile,
profileDir: path.join(testPath.joplinProfile, "profile-test"),
joplinEnv: "prod",
expectedProfileName: "profile-test",
},
{
rootProfileDir: testPath.joplinProfile,
profileDir: path.join(testPath.joplinProfile, "profile-idhere"),
joplinEnv: "prod",
expectedProfileName: "profile-idhere",
},
{
rootProfileDir: testPath.joplinProfile,
profileDir: path.join(testPath.joplinProfile, "profile-idhere"),
joplinEnv: "dev",
expectedProfileName: "profile-idhere-dev",
},
])(
"should correctly set backupBasePath based on the current profile name (case %#)",
async ({
profileDir,
rootProfileDir,
joplinEnv,
expectedProfileName,
}) => {
when(spyOnsSettingsValue)
.calledWith("path")
.mockImplementation(async () => testPath.backupBasePath);
when(spyOnGlobalValue)
.calledWith("rootProfileDir")
.mockImplementation(async () => rootProfileDir);
when(spyOnGlobalValue)
.calledWith("profileDir")
.mockImplementation(async () => profileDir);
when(spyOnGlobalValue)
.calledWith("env")
.mockImplementation(async () => joplinEnv);

// Should use the folder named "default" for the default profile
backup.createSubfolderPerProfile = true;
await backup.loadBackupPath();
expect(backup.backupBasePath).toBe(
path.normalize(
path.join(testPath.backupBasePath, expectedProfileName)
)
);
}
);
});

describe("Div", function () {
it(`Create empty folder`, async () => {
const folder = await backup.createEmptyFolder(
Expand Down Expand Up @@ -1053,14 +1119,20 @@ describe("Backup", function () {
});

describe("create backup readme", () => {
it.each([{ backupRetention: 1 }, { backupRetention: 2 }])(
"should create a README.md in the backup directory (case %#)",
async ({ backupRetention }) => {
it.each([
{ backupRetention: 1, createSubfolderPerProfile: false },
{ backupRetention: 2, createSubfolderPerProfile: false },
{ backupRetention: 1, createSubfolderPerProfile: true },
])(
"should create a README.md in the backup directory (case %j)",
async ({ backupRetention, createSubfolderPerProfile }) => {
when(spyOnsSettingsValue)
.calledWith("backupRetention")
.mockImplementation(async () => backupRetention)
.calledWith("backupInfo")
.mockImplementation(() => Promise.resolve("[]"));
.mockImplementation(() => Promise.resolve("[]"))
.calledWith("createSubfolderPerProfile")
.mockImplementation(() => Promise.resolve(createSubfolderPerProfile));

backup.backupStartTime = null;
await backup.start();
Expand Down
54 changes: 48 additions & 6 deletions src/Backup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class Backup {
private msgDialog: any;
private backupBasePath: string;
private activeBackupPath: string;
private readmeOutputDirectory: string;
private log: any;
private logFile: string;
private backupRetention: number;
Expand All @@ -29,6 +30,7 @@ class Backup {
private compressionLevel: number;
private singleJex: boolean;
private createSubfolder: boolean;
private createSubfolderPerProfile: boolean;
private backupSetName: string;
private exportFormat: string;
private execFinishCmd: string;
Expand Down Expand Up @@ -273,12 +275,10 @@ class Backup {
);
}

if (this.createSubfolder) {
this.log.verbose("append subFolder");
const orgBackupBasePath = this.backupBasePath;
this.backupBasePath = path.join(this.backupBasePath, "JoplinBackup");
const origBackupBasePath = this.backupBasePath;
const handleSubfolderCreation = async () => {
if (
fs.existsSync(orgBackupBasePath) &&
fs.existsSync(origBackupBasePath) &&
!fs.existsSync(this.backupBasePath)
) {
try {
Expand All @@ -287,6 +287,45 @@ class Backup {
await this.showError(i18n.__("msg.error.folderCreation", e.message));
}
}
};

if (this.createSubfolder) {
this.log.verbose("append subFolder");
this.backupBasePath = path.join(this.backupBasePath, "JoplinBackup");
await handleSubfolderCreation();
}

// Set the README output directory before adding a subdirectory for the profile.
// This gives us one README for all backup subfolders.
this.readmeOutputDirectory = this.backupBasePath;

if (this.createSubfolderPerProfile) {
this.log.verbose("append profile subfolder");
// We assume that Joplin's profile structure is the following
// rootProfileDir/
// | profileDir/
// | | [[profile content]]
// or, if using the default,
// rootProfileDir/
// | [[profile content]]
Comment on lines +304 to +310
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what Joplin's profile structure currently looks like. However, it doesn't seem to be documented anywhere...

Reading profiles.json (which seems to not always exist) in the root profile directory may be another way to get this information. It also doesn't seem to be documented though.

const profileRootDir = await joplin.settings.globalValue(
"rootProfileDir"
);
const profileCurrentDir = await joplin.settings.globalValue("profileDir");

let profileName = path.basename(profileCurrentDir);
if (profileCurrentDir === profileRootDir) {
profileName = "default";
}

// Appending a -dev to the profile name prevents a devmode default Joplin
// profile from overwriting a non-devmode Joplin profile.
if ((await joplin.settings.globalValue("env")) === "dev") {
profileName += "-dev";
}

this.backupBasePath = path.join(this.backupBasePath, profileName);
await handleSubfolderCreation();
}

// Creating a backup can overwrite the backup directory. Thus,
Expand Down Expand Up @@ -320,6 +359,9 @@ class Backup {
public async loadSettings() {
this.log.verbose("loadSettings");
this.createSubfolder = await joplin.settings.value("createSubfolder");
this.createSubfolderPerProfile = await joplin.settings.value(
"createSubfolderPerProfile"
);
await this.loadBackupPath();
this.backupRetention = await joplin.settings.value("backupRetention");

Expand Down Expand Up @@ -499,7 +541,7 @@ class Backup {

const backupDst = await this.makeBackupSet();

await this.writeReadme(this.backupBasePath);
await this.writeReadme(this.readmeOutputDirectory);
await joplin.settings.setValue(
"lastBackup",
this.backupStartTime.getTime()
Expand Down
4 changes: 4 additions & 0 deletions src/locales/en_US.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@
"label": "Create Subfolder",
"description": "Create a subfolder in the the configured {{backupPath}}. Deactivate only if there is no other data in the {{backupPath}}!"
},
"createSubfolderPerProfile": {
"label": "Create subfolder for Joplin profile",
"description": "Create a subfolder within the backup directory for the current profile. This allows multiple profiles from the same Joplin installation to use the same backup directory without overwriting backups made from other profiles. All profiles that use the same backup directory must have this setting enabled."
},
"zipArchive": {
"label": "Create archive",
"description": "Save backup data in a archive, if a password protected backups is set, an archive is always created"
Expand Down
9 changes: 9 additions & 0 deletions src/settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,15 @@ export namespace Settings {
backupPath: i18n.__("settings.path.label"),
}),
},
createSubfolderPerProfile: {
value: false,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should enable this by default ... But not for already installed versions.
Can be made in the function upgradeBackupPluginVersion.

Or what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or what do you think?

Currently, the upstream pull request makes Joplin responsible for setting the default backup directory to the home directory (rather this plugin). As such, there are tradeoffs to having this setting enabled by default.

Some reason(s) to not enable this by default:

  • Extra directories: If the default backup directory for this plugin is kept as the user's profile directory, backups would then look like this: profileDir/JoplinBackup/default. Because each subprofile has its own profile directory, subprofile backups would be in directories like this: profileDir/profile-abcd/JoplinBackup/profile-abcd. Observe that each JoplinBackup directory would have exactly one subdirectory.

Some reasons to enable this by default:

  • Consistency: It will need to be enabled by default in the built-in version of Simple Backup to prevent backups from different profiles from overwriting one another. Enabling it here would then be consistent with the built-in version.
    • The default values of settings in built-in plugins can be overridden here.
  • Data safety: If a user manually changes multiple profiles to back up to the same location, having this setting enabled could prevent backups from being overwritten.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To summarize, I think enabling this by default only makes sense in the version built-in to Joplin. In other versions, the default backup location is within the profile directory, which is different for each profile.

type: SettingItemType.Bool,
section: "backupSection",
public: true,
advanced: true,
label: i18n.__("settings.createSubfolderPerProfile.label"),
description: i18n.__("settings.createSubfolderPerProfile.description"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to use SettingStorage.File here so that this setting can be backed up. This wouldn't be consistent with the other settings though.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My opinion is that there must be a Joplin API to backup all plugin settings and restore settings from selected plugins.
In my opinion, SettingStorage.File is not a good idea and not a good way to proceed. If so, Joplin should automatically convert all settings in one version to SettingStorage.File and not provide x ways.
I also don't understand why all the settings from the database have to be in a json file. But that is a different topic.

},
zipArchive: {
value: "no",
type: SettingItemType.String,
Expand Down
Loading