From 426a8f75aa3df819eba947f5279a823c9c6aceef Mon Sep 17 00:00:00 2001
From: zadam <zadam.apps@gmail.com>
Date: Thu, 16 Nov 2023 23:31:33 +0100
Subject: [PATCH] fix creating tree cycle by moving clone into another clone,
 closes #4442

---
 src/routes/api/branches.js |  6 +++---
 src/services/branches.js   | 20 +++++++++++---------
 src/services/tree.js       | 12 ++++++++----
 3 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/src/routes/api/branches.js b/src/routes/api/branches.js
index 0af2ae7a0..9a560d5c0 100644
--- a/src/routes/api/branches.js
+++ b/src/routes/api/branches.js
@@ -19,14 +19,14 @@ const ValidationError = require("../../errors/validation_error");
 function moveBranchToParent(req) {
     const {branchId, parentBranchId} = req.params;
 
-    const parentBranch = becca.getBranch(parentBranchId);
     const branchToMove = becca.getBranch(branchId);
+    const targetParentBranch = becca.getBranch(parentBranchId);
 
-    if (!parentBranch || !branchToMove) {
+    if (!branchToMove || !targetParentBranch) {
         throw new ValidationError(`One or both branches '${branchId}', '${parentBranchId}' have not been found`);
     }
 
-    return branchService.moveBranchToBranch(branchToMove, parentBranch, branchId);
+    return branchService.moveBranchToBranch(branchToMove, targetParentBranch, branchId);
 }
 
 function moveBranchBeforeNote(req) {
diff --git a/src/services/branches.js b/src/services/branches.js
index f35db98df..681676ab1 100644
--- a/src/services/branches.js
+++ b/src/services/branches.js
@@ -1,12 +1,12 @@
 const treeService = require("./tree");
 const sql = require("./sql");
 
-function moveBranchToNote(sourceBranch, targetParentNoteId) {
-    if (sourceBranch.parentNoteId === targetParentNoteId) {
+function moveBranchToNote(branchToMove, targetParentNoteId) {
+    if (branchToMove.parentNoteId === targetParentNoteId) {
         return {success: true}; // no-op
     }
 
-    const validationResult = treeService.validateParentChild(targetParentNoteId, sourceBranch.noteId, sourceBranch.branchId);
+    const validationResult = treeService.validateParentChild(targetParentNoteId, branchToMove.noteId, branchToMove.branchId);
 
     if (!validationResult.success) {
         return [200, validationResult];
@@ -15,10 +15,10 @@ function moveBranchToNote(sourceBranch, targetParentNoteId) {
     const maxNotePos = sql.getValue('SELECT MAX(notePosition) FROM branches WHERE parentNoteId = ? AND isDeleted = 0', [targetParentNoteId]);
     const newNotePos = maxNotePos === null ? 0 : maxNotePos + 10;
 
-    const newBranch = sourceBranch.createClone(targetParentNoteId, newNotePos);
+    const newBranch = branchToMove.createClone(targetParentNoteId, newNotePos);
     newBranch.save();
 
-    sourceBranch.markAsDeleted();
+    branchToMove.markAsDeleted();
 
     return {
         success: true,
@@ -26,16 +26,18 @@ function moveBranchToNote(sourceBranch, targetParentNoteId) {
     };
 }
 
-function moveBranchToBranch(sourceBranch, targetParentBranch) {
-    const res = moveBranchToNote(sourceBranch, targetParentBranch.noteId);
+function moveBranchToBranch(branchToMove, targetParentBranch) {
+    const res = moveBranchToNote(branchToMove, targetParentBranch.noteId);
 
     if (!res.success) {
         return res;
     }
 
     // expanding so that the new placement of the branch is immediately visible
-    targetParentBranch.isExpanded = true;
-    targetParentBranch.save();
+    if (!targetParentBranch.isExpanded) {
+        targetParentBranch.isExpanded = true;
+        targetParentBranch.save();
+    }
 
     return res;
 }
diff --git a/src/services/tree.js b/src/services/tree.js
index adc075f28..87a99d379 100644
--- a/src/services/tree.js
+++ b/src/services/tree.js
@@ -8,7 +8,7 @@ const becca = require('../becca/becca');
 
 function validateParentChild(parentNoteId, childNoteId, branchId = null) {
     if (['root', '_hidden', '_share', '_lbRoot', '_lbAvailableLaunchers', '_lbVisibleLaunchers'].includes(childNoteId)) {
-        return { branch: null, success: false, message: `Cannot change this note's location.`};
+        return { branch: null, success: false, message: `Cannot change this note's location.` };
     }
 
     if (parentNoteId === 'none') {
@@ -16,14 +16,14 @@ function validateParentChild(parentNoteId, childNoteId, branchId = null) {
         return { branch: null, success: false, message: `Cannot move anything into 'none' parent.` };
     }
 
-    const existing = becca.getBranchFromChildAndParent(childNoteId, parentNoteId);
+    const existingBranch = becca.getBranchFromChildAndParent(childNoteId, parentNoteId);
 
-    if (existing && (branchId === null || existing.branchId !== branchId)) {
+    if (existingBranch && existingBranch.branchId !== branchId) {
         const parentNote = becca.getNote(parentNoteId);
         const childNote = becca.getNote(childNoteId);
 
         return {
-            branch: existing,
+            branch: existingBranch,
             success: false,
             message: `Note "${childNote.title}" note already exists in the "${parentNote.title}".`
         };
@@ -52,6 +52,10 @@ function validateParentChild(parentNoteId, childNoteId, branchId = null) {
  * Tree cycle can be created when cloning or when moving existing clone. This method should detect both cases.
  */
 function wouldAddingBranchCreateCycle(parentNoteId, childNoteId) {
+    if (parentNoteId === childNoteId) {
+        return true;
+    }
+
     const childNote = becca.getNote(childNoteId);
     const parentNote = becca.getNote(parentNoteId);