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

          Attachments

            Issue Links

              Activity

              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.
              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.
              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.
              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.
              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.
              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.
              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

                People

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

                  Dates

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