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

              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