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

Lesson module is not browseable using the Server Files repository

    Details

    • Testing Instructions:
      Hide

      Pre-requisites

      • Open 2 tabs, one in a course, one in your private files

      Test

      1. Create a new lesson with a File pop-up (section Appearance in the edit form)
      2. Make sure you can pick the file in your private files (using the Server Files repository)
      3. Now, create a content page, and add a file to its description
      4. Make sure you can now pick both files in your private files (using the Server Files repository)
      5. Remove the Pop-up File
      6. Make sure you can pick the description file in your private files (using the Server Files repository)

      There is a known thumbnail issue: MDL-43401
      This issue discovered: MDL-43371

      Show
      Pre-requisites Open 2 tabs, one in a course, one in your private files Test Create a new lesson with a File pop-up (section Appearance in the edit form) Make sure you can pick the file in your private files (using the Server Files repository) Now, create a content page, and add a file to its description Make sure you can now pick both files in your private files (using the Server Files repository) Remove the Pop-up File Make sure you can pick the description file in your private files (using the Server Files repository) There is a known thumbnail issue: MDL-43401 This issue discovered: MDL-43371
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-42212-master
    • Sprint:
      FRONTEND Sprint 7
    • Story Points (Obsolete):
      13
    • Sprint:
      FRONTEND Sprint 7

      Description

      When files are added to an activity, a folder is automatically created in Server files>>System>>course with the name of the activity and the type of activity. This folder contains all the files added to the activity. This doesn't happen when adding files to a Lesson activity.
      Steps to reproduce:
      Create a new course.
      Add a forum, database, quiz, and lesson activities.
      Add images to each activity using the HTML editor (I.E. Insert/edit a link, Insert/edit image). Upload file from your local computer using the file picker.
      Once all the activities are created and images have been added. Edit the forum description. Click on Insert/edit image>>Find or upload and image. Notice that the folder Lesson does not appear in Server files>>System>>course.

        Gliffy Diagrams

        1. added to all.png
          50 kB
        2. find_or.png
          31 kB
        3. lesson.png
          23 kB

          Issue Links

            Activity

            Hide
            marina Marina Glancy added a comment -

            responsible functions should be
            lesson_get_file_areas()
            and
            lesson_get_file_info()

            they are called from file_info_context_module()

            From the first glance those functions seem to be ok (except strings hardcoding) but there still might be a hidden bug. Leaving it to Lesson component lead. Also this might be a side effect of bug MDL-41337

            Show
            marina Marina Glancy added a comment - responsible functions should be lesson_get_file_areas() and lesson_get_file_info() they are called from file_info_context_module() From the first glance those functions seem to be ok (except strings hardcoding) but there still might be a hidden bug. Leaving it to Lesson component lead. Also this might be a side effect of bug MDL-41337
            Hide
            moodle.com moodle.com added a comment -

            Unassigning this to allow someone else in the team to take this up before the end of the sprint (while Rosie has gone on leave).

            Show
            moodle.com moodle.com added a comment - Unassigning this to allow someone else in the team to take this up before the end of the sprint (while Rosie has gone on leave).
            Hide
            fred Frédéric Massart added a comment -

            Requesting peer review, a few things were wrong in there, to the point that it didn't work at all. As there are a few areas and one of them is using itemid I had to file browser class. This method is highly inspired from what had been done in other modules like mod_book, mod_folder or mod_forum.

            You will notice:

            • The permission check was wrong, I fixed it
            • The new class to browse the lesson
            • The added logic to make it more user friendly to browse media file (without it you click Mediafile and see another directory called Mediafile)
            • The fetch in database to get the names of the pages (same was mod_book does it)
            • The related issue raised because deleting a page does not delete its content

            I was thinking at ordering the pages, but there is no easy way to do it, and it does not seem that we bother ordering alphabetically anywhere else.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Requesting peer review, a few things were wrong in there, to the point that it didn't work at all. As there are a few areas and one of them is using itemid I had to file browser class. This method is highly inspired from what had been done in other modules like mod_book, mod_folder or mod_forum. You will notice: The permission check was wrong, I fixed it The new class to browse the lesson The added logic to make it more user friendly to browse media file (without it you click Mediafile and see another directory called Mediafile ) The fetch in database to get the names of the pages (same was mod_book does it) The related issue raised because deleting a page does not delete its content I was thinking at ordering the pages, but there is no easy way to do it, and it does not seem that we bother ordering alphabetically anywhere else. Cheers, Fred
            Hide
            marina Marina Glancy added a comment -

            Hi Fred, thanks a lot for working on it. Looks like much more mess than I thought it was

            The patch looks very good. Actually nothing to change there

            But since you are so awesome to work on this issue, could you please also fix the hardcoded strings in lesson_get_file_areas(). I wonder if "new lang_string()" can be used there for performance reasons (so the strings are not evaluated when serving pluginfile.php)

            P.S. Could we use class autoloader in 2.6?

            Show
            marina Marina Glancy added a comment - Hi Fred, thanks a lot for working on it. Looks like much more mess than I thought it was The patch looks very good. Actually nothing to change there But since you are so awesome to work on this issue, could you please also fix the hardcoded strings in lesson_get_file_areas(). I wonder if "new lang_string()" can be used there for performance reasons (so the strings are not evaluated when serving pluginfile.php) P.S. Could we use class autoloader in 2.6?
            Hide
            fred Frédéric Massart added a comment -

            Thanks marina,

            • I have localised the area names, initially I thought I'd leave that to the MDL in the comment, but it turns out that it is not specific to lesson.
            • 2.6 and master are using autoloading. I didn't use namespaces because I don't think that was necessary, and that would be more confusing in a code that doesn't use any.
            • I found and raised MDL-43401, the fix is not as straightforward as I thought, so a new issue makes sense.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Thanks marina, I have localised the area names, initially I thought I'd leave that to the MDL in the comment, but it turns out that it is not specific to lesson. 2.6 and master are using autoloading. I didn't use namespaces because I don't think that was necessary, and that would be more confusing in a code that doesn't use any. I found and raised MDL-43401 , the fix is not as straightforward as I thought, so a new issue makes sense. Cheers, Fred
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi guys,

            this is just a message to share with you that I'm going to perform a test of the automated pre-checker against all the current issues awaiting integration (16 issues).

            So, soon, you'll get some extra comments in this issue with some information from the pre-checker. Note it's not final, but just an experiment and there are lots of things to improve, from the message itself to various false positives in the checkers. So take any report with caution, it's not 100% accurate yet.

            Please, feel free to comment any idea/objection @ MDLSITE-2662. I'll be collecting everything there.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi guys, this is just a message to share with you that I'm going to perform a test of the automated pre-checker against all the current issues awaiting integration (16 issues). So, soon, you'll get some extra comments in this issue with some information from the pre-checker. Note it's not final, but just an experiment and there are lots of things to improve, from the message itself to various false positives in the checkers. So take any report with caution, it's not 100% accurate yet. Please, feel free to comment any idea/objection @ MDLSITE-2662 . I'll be collecting everything there. TIA and ciao
            Hide
            cibot CiBoT added a comment -

            Results for MDL-42212

            Show
            cibot CiBoT added a comment - Results for MDL-42212 Branch MDL-42212 -24 to be integrated into upstream MOODLE_24_STABLE Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/687 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/687/artifact/work/smurf.html Branch MDL-42212 -25 to be integrated into upstream MOODLE_25_STABLE Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/688 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/688/artifact/work/smurf.html Branch MDL-42212 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/689 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/689/artifact/work/smurf.html Branch MDL-42212 -master to be integrated into upstream master Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/690 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/690/artifact/work/smurf.html
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Fred,

            Sending this back this week sorry.

            I found the thumbnail in the server files area for an image used within the lesson pop up wasn't being displayed.
            I had the following show:

            Sorry, the requested file could not be found

            More information about this error
            Debug info:
            Error code: filenotfound
            Stack trace:

            line 476 of /lib/setuplib.php: moodle_exception thrown
            line 1992 of /lib/filelib.php: call to print_error()
            line 4441 of /lib/filelib.php: call to send_file_not_found()
            line 38 of /pluginfile.php: call to file_pluginfile()

            If I selected the image anyway it worked fine - so just the thumbnail.
            If I browsed the image I used for the lesson description the thumbnail for that was shown correctly, so just the popup file has thumbnail issues.

            Merry xmas
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Fred, Sending this back this week sorry. I found the thumbnail in the server files area for an image used within the lesson pop up wasn't being displayed. I had the following show: Sorry, the requested file could not be found More information about this error Debug info: Error code: filenotfound Stack trace: line 476 of /lib/setuplib.php: moodle_exception thrown line 1992 of /lib/filelib.php: call to print_error() line 4441 of /lib/filelib.php: call to send_file_not_found() line 38 of /pluginfile.php: call to file_pluginfile() If I selected the image anyway it worked fine - so just the thumbnail. If I browsed the image I used for the lesson description the thumbnail for that was shown correctly, so just the popup file has thumbnail issues. Merry xmas Sam
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            fred Frédéric Massart added a comment -

            Thanks Sam, but this is a bug that I was aware of and raised in MDL-43401. It was not easy to fix it that's why I thought that it was better to get this done, and then to focus on the thumbnail problem. Are you OK if I push that back for integration?

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Thanks Sam, but this is a bug that I was aware of and raised in MDL-43401 . It was not easy to fix it that's why I thought that it was better to get this done, and then to focus on the thumbnail problem. Are you OK if I push that back for integration? Cheers, Fred
            Hide
            samhemelryk Sam Hemelryk added a comment -

            If its an existing issue, and covered by another issue no probs at all pushing this straight back to integration. Perhaps it would pay to mention it in the test instructions however.

            Show
            samhemelryk Sam Hemelryk added a comment - If its an existing issue, and covered by another issue no probs at all pushing this straight back to integration. Perhaps it would pay to mention it in the test instructions however.
            Hide
            fred Frédéric Massart added a comment -

            Thanks Sam, I've added a mention of it to the testing instructions.

            Show
            fred Frédéric Massart added a comment - Thanks Sam, I've added a mention of it to the testing instructions.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Fred - this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Fred - this has been integrated now.
            Hide
            tsala Helen Foster added a comment -

            Removing docs_required label, as the lesson is now listed as an activity module for which files are available in the server files repository in http://docs.moodle.org/en/Server_files_repository

            Show
            tsala Helen Foster added a comment - Removing docs_required label, as the lesson is now listed as an activity module for which files are available in the server files repository in http://docs.moodle.org/en/Server_files_repository
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Fred,

            On step 3 I did the following:

            1. add a content page
            2. on content description editor, selecting the insert image button from the editor then clicked on insert button
              the popup windows closed immediately (there's no filepicker displayed to select the file).

            I tried this for 2.4 and master, both behaving similarly.

            Could you confirm the behavior please?

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Fred, On step 3 I did the following: add a content page on content description editor, selecting the insert image button from the editor then clicked on insert button the popup windows closed immediately (there's no filepicker displayed to select the file). I tried this for 2.4 and master, both behaving similarly. Could you confirm the behavior please?
            Hide
            fred Frédéric Massart added a comment -

            Hi Rosie,

            I do not have troubles picking an image in a content page inside a Lesson. Even though, that should not be related to the patch. I suggest you purge your cookies and user preferences as maybe there is a glitch somewhere is the repository that the file picker opens by default.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Rosie, I do not have troubles picking an image in a content page inside a Lesson. Even though, that should not be related to the patch. I suggest you purge your cookies and user preferences as maybe there is a glitch somewhere is the repository that the file picker opens by default. Cheers, Fred
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Fred, purging the cookies works well.

            This is working as expected.

            Tested for 2.4, 2.5, 2.6 and master.

            Test passed.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Fred, purging the cookies works well. This is working as expected. Tested for 2.4, 2.5, 2.6 and master. Test passed.
            Hide
            damyon Damyon Wiese added a comment -

            David built a framework for behat
            At first just to test this and that
            10000+ steps written
            Sounds like we're all smitten
            And David should be smiling at that

            Thanks for reporting, patching, and testing this issue. It has been released upstream along with 64 others today.

            Show
            damyon Damyon Wiese added a comment - David built a framework for behat At first just to test this and that 10000+ steps written Sounds like we're all smitten And David should be smiling at that Thanks for reporting, patching, and testing this issue. It has been released upstream along with 64 others today.

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                10 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  13/Jan/14

                  Agile