Improve file_target finding logic when repairing unmerged shares

Pick the most recent subshare that has no parenthesis from duplication
which should match whichever name the user picked last. If all
subshares have duplicate parenthesis names, use the least recent group
share's target instead.

diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php
index 353877bb8737d49c99e3da218da236b6b0cfb093..f14e98c17619abcd4e3b6789cf5773f70351e415 100644 (file)
--- a/lib/private/Repair/RepairUnmergedShares.php
+++ b/lib/private/Repair/RepairUnmergedShares.php
@@ -148,6 +148,44 @@ class RepairUnmergedShares implements IRepairStep {
                return $groupedShares;
        }
 
+       /**
+        * Decide on the best target name based on all group shares and subshares,
+        * goal is to increase the likeliness that the chosen name matches what
+        * the user is expecting.
+        *
+        * For this, we discard the entries with parenthesis "(2)".
+        * In case the user also renamed the duplicates to a legitimate name, this logic
+        * will still pick the most recent one as it's the one the user is most likely to
+        * remember renaming.
+        *
+        * If no suitable subshare is found, use the least recent group share instead.
+        *
+        * @param array $groupShares group share entries
+        * @param array $subShares sub share entries, sorted by stime
+        *
+        * @return string chosen target name
+        */
+       private function findBestTargetName($groupShares, $subShares) {
+               $pickedShare = null;
+               // note subShares are sorted by stime from oldest to newest
+               foreach ($subShares as $subShare) {
+                       // skip entries that have parenthesis with numbers
+                       if (preg_match('/\([0-9]*\)/', $subShare['file_target']) === 1) {
+                               continue;
+                       }
+                       // pick any share found that would match, the last being the most recent
+                       $pickedShare = $subShare;
+               }
+
+               // no suitable subshare found
+               if ($pickedShare === null) {
+                       // use least recent group share target instead
+                       $pickedShare = $groupShares[0];
+               }
+
+               return $pickedShare['file_target'];
+       }
+
        /**
         * Fix the given received share represented by the set of group shares
         * and matching sub shares
@@ -171,7 +209,7 @@ class RepairUnmergedShares implements IRepairStep {
                        return false;
                }
 
-               $targetPath = $groupShares[0]['file_target'];
+               $targetPath = $this->findBestTargetName($groupShares, $subShares);
 
                // check whether the user opted out completely of all subshares
                $optedOut = true;

diff --git a/tests/lib/Repair/RepairUnmergedSharesTest.php b/tests/lib/Repair/RepairUnmergedSharesTest.php
index fe9b3e5b96f37ce2ce52e947b6fd951162092919..9fe9a07388102a8c94670047d028fba1ce1c5038 100644 (file)
--- a/tests/lib/Repair/RepairUnmergedSharesTest.php
+++ b/tests/lib/Repair/RepairUnmergedSharesTest.php
@@ -204,7 +204,7 @@ class RepairUnmergedSharesTest extends TestCase {
                        [
                                // #2 bogus share
                                // - outsider shares with group1, group2
-                               // - one subshare for each group share
+                               // - one subshare for each group share, both with parenthesis
                                // - but the targets do not match when grouped
                                [
                                        [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31],
@@ -218,7 +218,7 @@ class RepairUnmergedSharesTest extends TestCase {
                                [
                                        ['/test', 31],
                                        ['/test', 31],
-                                       // reset to original name
+                                       // reset to original name as the sub-names have parenthesis
                                        ['/test', 31],
                                        ['/test', 31],
                                        // leave unrelated alone
@@ -228,6 +228,54 @@ class RepairUnmergedSharesTest extends TestCase {
                        [
                                // #3 bogus share
                                // - outsider shares with group1, group2
+                               // - one subshare for each group share, both renamed manually
+                               // - but the targets do not match when grouped
+                               [
+                                       [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31],
+                                       [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31],
+                                       // child of the previous ones
+                                       [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed (1 legit paren)', 31, 0],
+                                       [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed (2 legit paren)', 31, 1],
+                                       // different unrelated share
+                                       [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31],
+                               ],
+                               [
+                                       ['/test', 31],
+                                       ['/test', 31],
+                                       // reset to less recent subshare name
+                                       ['/test_renamed (2 legit paren)', 31],
+                                       ['/test_renamed (2 legit paren)', 31],
+                                       // leave unrelated alone
+                                       ['/test (4)', 31],
+                               ]
+                       ],
+                       [
+                               // #4 bogus share
+                               // - outsider shares with group1, group2
+                               // - one subshare for each group share, one with parenthesis
+                               // - but the targets do not match when grouped
+                               [
+                                       [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31],
+                                       [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31],
+                                       // child of the previous ones
+                                       [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (2)', 31, 0],
+                                       [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed', 31, 1],
+                                       // different unrelated share
+                                       [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31],
+                               ],
+                               [
+                                       ['/test', 31],
+                                       ['/test', 31],
+                                       // reset to less recent subshare name but without parenthesis
+                                       ['/test_renamed', 31],
+                                       ['/test_renamed', 31],
+                                       // leave unrelated alone
+                                       ['/test (4)', 31],
+                               ]
+                       ],
+                       [
+                               // #5 bogus share
+                               // - outsider shares with group1, group2
                                // - one subshare for each group share
                                // - first subshare not renamed (as in real world scenario)
                                // - but the targets do not match when grouped
@@ -251,7 +299,7 @@ class RepairUnmergedSharesTest extends TestCase {
                                ]
                        ],
                        [
-                               // #4 bogus share:
+                               // #6 bogus share:
                                // - outsider shares with group1, group2
                                // - one subshare for each group share
                                // - non-matching targets
@@ -276,7 +324,7 @@ class RepairUnmergedSharesTest extends TestCase {
                                ]
                        ],
                        [
-                               // #5 bogus share:
+                               // #7 bogus share:
                                // - outsider shares with group1, group2
                                // - one subshare for each group share
                                // - non-matching targets
@@ -301,7 +349,7 @@ class RepairUnmergedSharesTest extends TestCase {
                                ]
                        ],
                        [
-                               // #6 bogus share:
+                               // #8 bogus share:
                                // - outsider shares with group1, group2 and also user2
                                // - one subshare for each group share
                                // - one extra share entry for direct share to user2
@@ -329,7 +377,7 @@ class RepairUnmergedSharesTest extends TestCase {
                                ]
                        ],
                        [
-                               // #7 bogus share:
+                               // #9 bogus share:
                                // - outsider shares with group1 and also user2
                                // - no subshare at all
                                // - one extra share entry for direct share to user2
@@ -350,7 +398,7 @@ class RepairUnmergedSharesTest extends TestCase {
                                ]
                        ],
                        [
-                               // #8 legitimate share with own group:
+                               // #10 legitimate share with own group:
                                // - insider shares with both groups the user is already in
                                // - no subshares in this case
                                [
@@ -368,7 +416,7 @@ class RepairUnmergedSharesTest extends TestCase {
                                ]
                        ],
                        [
-                               // #9 legitimate shares:
+                               // #11 legitimate shares:
                                // - group share with same group
                                // - group share with other group
                                // - user share where recipient renamed
@@ -392,7 +440,7 @@ class RepairUnmergedSharesTest extends TestCase {
                                ]
                        ],
                        [
-                               // #10 legitimate share:
+                               // #12 legitimate share:
                                // - outsider shares with group and user directly with different permissions
                                // - no subshares
                                // - same targets