Moodle

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

Details

  • Type: Bug Bug
  • Status: Closed 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:
  • Pull Master Diff URL:

Description

qformat_default writefiles should be a public method not protected

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
Tim Hunt added a comment -

Is this really still unresolved?

Show
Tim Hunt added a comment - Is this really still unresolved?
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Tim Hunt added a comment -

Thanks Jamie, submitting for integration now.

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

Integrated, thanks!

Show
Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
Hide
Permalink
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
Permalink
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

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: