Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31541

Only one instance of File System Repository is available at any time

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.4, 2.2.1, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Repositories
    • Labels:
    • Testing Instructions:
      Hide

      Enable the File System Repository
      Create 2 folders
      Go to a course forum, post a message then click to add an attachment
      Ensure that both the folders you set up in the File System Repository are visible

      Show
      Enable the File System Repository Create 2 folders Go to a course forum, post a message then click to add an attachment Ensure that both the folders you set up in the File System Repository are visible
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-31541_multiple_repository_instances

      Description

      Due to the sorting algorithm employed when showing the repository list, multiple instances of the File System repository (and possibly other repository types - I haven't checked yet) overwrite each other in the display order , so that only the last instance is actually shown.

      Reported here: http://moodle.org/mod/forum/discuss.php?d=195596
      and confirmed with latest dev version.

      I'll work on a fix

        Gliffy Diagrams

          Issue Links

            Activity

            davosmith Davo Smith created issue -
            davosmith Davo Smith made changes -
            Field Original Value New Value
            Assignee Dongsheng Cai [ dongsheng ] Davo Smith [ davosmith ]
            Hide
            davosmith Davo Smith added a comment -

            This is a very simple fix that ensures that each instance of a repository has a unique 'sortorder' value when retrieved, which stops the repository instances from getting overwritten when sorted by the Javascript UI.

            Without this patch, multiple instances of the same repository ended up with the same 'sortorder' value.

            Show
            davosmith Davo Smith added a comment - This is a very simple fix that ensures that each instance of a repository has a unique 'sortorder' value when retrieved, which stops the repository instances from getting overwritten when sorted by the Javascript UI. Without this patch, multiple instances of the same repository ended up with the same 'sortorder' value.
            davosmith Davo Smith made changes -
            Pull Master Diff URL https://github.com/davosmith/moodle/compare/master...MDL-31541_multiple_repository_instances
            Pull Master Branch MDL-31541_multiple_repository_instances
            Pull from Repository git://github.com/davosmith/moodle.git
            Labels patch
            davosmith Davo Smith made changes -
            Status Open [ 1 ] Waiting for peer review [ 10012 ]
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for working on that, Davo.

            Show
            salvetore Michael de Raadt added a comment - Thanks for working on that, Davo.
            salvetore Michael de Raadt made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            Labels patch patch triaged
            rajeshtaneja Rajesh Taneja made changes -
            Original Estimate 0 minutes [ 0 ]
            Remaining Estimate 0 minutes [ 0 ]
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Peer reviewer rajeshtaneja
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for spot-on patch Davo
            Initially, I thought it will be nice to check for duplicate instances, then going though the code again, it make sense to have a explicit sortorder

            Can you please add documentation explaining why explicit sortorder is used, before this gets pushed for integration review.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for spot-on patch Davo Initially, I thought it will be nice to check for duplicate instances, then going though the code again, it make sense to have a explicit sortorder Can you please add documentation explaining why explicit sortorder is used, before this gets pushed for integration review.
            rajeshtaneja Rajesh Taneja made changes -
            Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
            dongsheng Dongsheng Cai made changes -
            Link This issue has been marked as being related by MDL-31000 [ MDL-31000 ]
            Hide
            dongsheng Dongsheng Cai added a comment -

            Marked this as related to MDL-31000, the patch looks good to me, thanks Davo for working on this.

            Show
            dongsheng Dongsheng Cai added a comment - Marked this as related to MDL-31000 , the patch looks good to me, thanks Davo for working on this.
            Hide
            davosmith Davo Smith added a comment -

            @Rajesh - was planning to add the following comment:

            $options['sortorder'] = $sortorder++; // MDL-31541 - make sure each instance has a unique sortorder value

            Would that be sufficient?

            Show
            davosmith Davo Smith added a comment - @Rajesh - was planning to add the following comment: $options ['sortorder'] = $sortorder++; // MDL-31541 - make sure each instance has a unique sortorder value Would that be sufficient?
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Hello Davo,
            We don't use MDL number in comments, as it's easily traceable. It will be nice to add a comment while defining $sortorder = 1; to let other developer know why this has been added and should not accidentally remove it.

            // Sortorder should be unique which is not true if we try to retrieve
            // it from $record->sortorder.
            $sortorder = 1;

            Show
            rajeshtaneja Rajesh Taneja added a comment - Hello Davo, We don't use MDL number in comments, as it's easily traceable. It will be nice to add a comment while defining $sortorder = 1; to let other developer know why this has been added and should not accidentally remove it. // Sortorder should be unique which is not true if we try to retrieve // it from $record->sortorder. $sortorder = 1;
            Hide
            davosmith Davo Smith added a comment -

            OK - try that.

            (Repo updated)

            Show
            davosmith Davo Smith added a comment - OK - try that. (Repo updated)
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for the quick fix Davo
            Pushing it for integration review.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for the quick fix Davo Pushing it for integration review.
            rajeshtaneja Rajesh Taneja made changes -
            Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Currently in integration Yes [ 10041 ]
            nebgor Aparup Banerjee made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator nebgor
            Hide
            nebgor Aparup Banerjee added a comment -

            Thanks for the work and all the reviews here,
            this has been integrated into master and also cherry-picked (applied cleanly) into MOODLE_21_STABLE and MOODLE_22_STABLE.

            Show
            nebgor Aparup Banerjee added a comment - Thanks for the work and all the reviews here, this has been integrated into master and also cherry-picked (applied cleanly) into MOODLE_21_STABLE and MOODLE_22_STABLE.
            nebgor Aparup Banerjee made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Affects Version/s 2.1.4 [ 11452 ]
            Affects Version/s 2.3 [ 10657 ]
            Fix Version/s 2.1.5 [ 11553 ]
            Fix Version/s 2.2.2 [ 11552 ]
            Fix Version/s STABLE backlog [ 10463 ]
            abgreeve Adrian Greeve made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Tester abgreeve
            Hide
            abgreeve Adrian Greeve added a comment -

            I tried this out pre-patch and post-patch. Now all folders are visible in the file selector.
            Thanks,
            Test passed.

            Show
            abgreeve Adrian Greeve added a comment - I tried this out pre-patch and post-patch. Now all folders are visible in the file selector. Thanks, Test passed.
            abgreeve Adrian Greeve made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

            Closing as fixed, heading to zzzZZZzzz, niao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Integration date 17/Feb/12

              People

              • Votes:
                2 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Mar/12