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
    • Rank:
      38094

      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

        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: