Moodle
  1. Moodle
  2. MDL-29060

no access for question plug ins to method to export file as part of export of questions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.5, 2.1.2, 2.2
    • Fix Version/s: 2.0.6, 2.1.3
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      Really, the testing needs to be repeated for 21, 20 and master, since the change would not cherry-pick.

      1. Create some questions in the question bank that have images embedded in the question text.
      2. Export the questions as Moodle XML format.
      3. Re-import into a new course, and verify that the images came through OK.

      Show
      Really, the testing needs to be repeated for 21, 20 and master, since the change would not cherry-pick. 1. Create some questions in the question bank that have images embedded in the question text. 2. Export the questions as Moodle XML format. 3. Re-import into a new course, and verify that the images came through OK.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      18586

      Description

      qformat_default writefiles should be a public method not protected

        Activity

        Hide
        Jamie Pratt added a comment -

        Before making it a public method we should probably change it's name to write_files

        Show
        Jamie Pratt added a comment - Before making it a public method we should probably change it's name to write_files
        Hide
        Jamie Pratt added a comment -

        Actually maybe we should keep this protected and make a higher level function public that takes all the files in a file area and exports them.

        Show
        Jamie Pratt added a comment - Actually maybe we should keep this protected and make a higher level function public that takes all the files in a file area and exports them.
        Hide
        Tim Hunt added a comment -

        Yes. This (or something better) needs to be public so export methods in question types can use it. But, shall we sort out MDL-29059 first?

        Show
        Tim Hunt added a comment - Yes. This (or something better) needs to be public so export methods in question types can use it. But, shall we sort out MDL-29059 first?
        Hide
        Tim Hunt added a comment -

        Is this really still unresolved?

        Show
        Tim Hunt added a comment - Is this really still unresolved?
        Hide
        Jamie Pratt added a comment -

        Yes. I copied the code from the method into one of my question types so I had access to the code in the question type plugin. I am waiting for us to resolve this so I can call the method on the format class instead and removed the copied code.

        Show
        Jamie Pratt added a comment - Yes. I copied the code from the method into one of my question types so I had access to the code in the question type plugin. I am waiting for us to resolve this so I can call the method on the format class instead and removed the copied code.
        Hide
        Jamie Pratt added a comment -

        I am writing the code now for exporting the ddmarker questions as xml. We still have not resolved this. I think we just need to change the access from protected to public. The method name should have an underscore in it but I guess we just ignore this.

        Show
        Jamie Pratt added a comment - I am writing the code now for exporting the ddmarker questions as xml. We still have not resolved this. I think we just need to change the access from protected to public. The method name should have an underscore in it but I guess we just ignore this.
        Hide
        Jamie Pratt added a comment -

        It seems that the second parameter of writefiles is not used anywhere and doesn't do anything. Are there format plug ins that might be calling this code on the parent class? If not maybe we could rename it and replace the second param with an indent level??

        Show
        Jamie Pratt added a comment - It seems that the second parameter of writefiles is not used anywhere and doesn't do anything. Are there format plug ins that might be calling this code on the parent class? If not maybe we could rename it and replace the second param with an indent level??
        Hide
        Tim Hunt added a comment -

        Sorry, Jamie, I forgot all about this. I think this change does the necessary. Could you give it a quick review.

        I decided this really had to be fixed in all branches since 2.0, but then found that the changes would not cherry-pick, so had to do it three times.

        Even though this looks like an API change, it is not really, because the old method was not public, even though it should have been.

        Show
        Tim Hunt added a comment - Sorry, Jamie, I forgot all about this. I think this change does the necessary. Could you give it a quick review. I decided this really had to be fixed in all branches since 2.0, but then found that the changes would not cherry-pick, so had to do it three times. Even though this looks like an API change, it is not really, because the old method was not public, even though it should have been.
        Hide
        Tim Hunt added a comment -

        You are right that there is no need for the second argument to write-files. However I left the encoding="base64" in the XML. Being explicit about the encoding there seems like a good idea (in case we find a better encoding later) however, there is no point in allowing the qtypes to specify the encoding when they call write_files.

        Show
        Tim Hunt added a comment - You are right that there is no need for the second argument to write-files. However I left the encoding="base64" in the XML. Being explicit about the encoding there seems like a good idea (in case we find a better encoding later) however, there is no point in allowing the qtypes to specify the encoding when they call write_files.
        Hide
        Jamie Pratt added a comment -

        I tested this code in 2.0, 2.1 and master branches. Looks good.

        Show
        Jamie Pratt added a comment - I tested this code in 2.0, 2.1 and master branches. Looks good.
        Hide
        Tim Hunt added a comment -

        Thanks Jamie, submitting for integration now.

        Show
        Tim Hunt added a comment - Thanks Jamie, submitting for integration now.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
        Hide
        Rajesh Taneja added a comment -

        Works Great
        Thanks for fixing this, Tim.

        Show
        Rajesh Taneja added a comment - Works Great Thanks for fixing this, Tim.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        And this has landed upstream, just on time for the upcoming new releases week. Thanks for it!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - And this has landed upstream, just on time for the upcoming new releases week. Thanks for it! Ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: