Moodle
  1. Moodle
  2. MDL-31541

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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

            Hide
            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
            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.
            Hide
            Michael de Raadt added a comment -

            Thanks for working on that, Davo.

            Show
            Michael de Raadt added a comment - Thanks for working on that, Davo.
            Hide
            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
            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.
            Hide
            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 Cai added a comment - Marked this as related to MDL-31000 , the patch looks good to me, thanks Davo for working on this.
            Hide
            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
            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
            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
            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
            Davo Smith added a comment -

            OK - try that.

            (Repo updated)

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

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

            Show
            Rajesh Taneja added a comment - Thanks for the quick fix Davo Pushing it for integration review.
            Hide
            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
            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.
            Hide
            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
            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.
            Hide
            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
            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

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: