Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1
    • Fix Version/s: 2.0.4
    • Component/s: Lesson
    • Labels:
    • Rank:
      17606

      Description

      Lesson import questions use deprecated file picker. Should be updated with new filepicker.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Increasing priority as this is related to a QA test.

          Show
          Michael de Raadt added a comment - Increasing priority as this is related to a QA test.
          Hide
          Rajesh Taneja added a comment -

          Hi! Tim,

          Do you have any sample questions files, which I can import in lesson?

          Thanks for all the help

          Cheers
          Rajesh

          Show
          Rajesh Taneja added a comment - Hi! Tim, Do you have any sample questions files, which I can import in lesson? Thanks for all the help Cheers Rajesh
          Hide
          Tim Hunt added a comment -

          Sorry no.

          I realise that we ought to have a standard file in every supported format, with a full range of question types, but I have never had time to assemble that, so every time I just export some questions then re-import them.

          However, if you look inside the qformat unit tests, you will find some sample fragments that you can put together to make a file.

          Show
          Tim Hunt added a comment - Sorry no. I realise that we ought to have a standard file in every supported format, with a full range of question types, but I have never had time to assemble that, so every time I just export some questions then re-import them. However, if you look inside the qformat unit tests, you will find some sample fragments that you can put together to make a file.
          Hide
          Rajesh Taneja added a comment -

          Thanks Tim

          Show
          Rajesh Taneja added a comment - Thanks Tim
          Hide
          Rajesh Taneja added a comment -

          Quick fix to import questions in lesson.

          Show
          Rajesh Taneja added a comment - Quick fix to import questions in lesson.
          Hide
          Tim Hunt added a comment -

          Rajesh asked me to take a quick look at this.

          • I know almost nothing about the lesson module, so I can't really comment on that part of the changes. They look plausible to me. And, if testing does not find anything obviously broken, then they are probably an improvement.
          • The changes in question and mod/quiz are good.
          • I can't think of anyone else who would be able to review the mod/lesson changes. (Well Sam Hemelryk might possibly.)

          Anyway, a tentative +1 from me.

          Show
          Tim Hunt added a comment - Rajesh asked me to take a quick look at this. I know almost nothing about the lesson module, so I can't really comment on that part of the changes. They look plausible to me. And, if testing does not find anything obviously broken, then they are probably an improvement. The changes in question and mod/quiz are good. I can't think of anyone else who would be able to review the mod/lesson changes. (Well Sam Hemelryk might possibly.) Anyway, a tentative +1 from me.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Tim

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Tim
          Hide
          Sam Hemelryk added a comment -

          I've had a look over this as well - it gets my +1 as well.
          I will shortly create a further issue to investigate a possible bug in the lesson code.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - I've had a look over this as well - it gets my +1 as well. I will shortly create a further issue to investigate a possible bug in the lesson code. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam

          Show
          Rajesh Taneja added a comment - Thanks Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks to Tim and Sam for providing there feedback.
          It seems like it can go ahead for integration.

          Show
          Rajesh Taneja added a comment - Thanks to Tim and Sam for providing there feedback. It seems like it can go ahead for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          1) whitespace @ lesson_import_form
          2) not sure if related to lesson or to import/GIFT limitations but, for curiosity, I tested importing one file (from the MDLQA) and found that some question types are not setting the correct score (truefalse, shortanswer, numerical...)

          The rest seems ok for integration, feel free to decide about 2) and fix 1)

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - 1) whitespace @ lesson_import_form 2) not sure if related to lesson or to import/GIFT limitations but, for curiosity, I tested importing one file (from the MDLQA) and found that some question types are not setting the correct score (truefalse, shortanswer, numerical...) The rest seems ok for integration, feel free to decide about 2) and fix 1) Ciao
          Hide
          Rajesh Taneja added a comment -

          Hello Eloy,

          I will update you if grades can be fixed quickly. So we can get questions imported nicely.

          Cheers
          Rajesh

          Show
          Rajesh Taneja added a comment - Hello Eloy, I will update you if grades can be fixed quickly. So we can get questions imported nicely. Cheers Rajesh
          Hide
          Rajesh Taneja added a comment -

          Hello Eloy,

          I have fixed score import, but have one question for Tim.

          Currently, score is multiple of 100, that means right answer is scored 100.
          Need to check with Tim, if that is right or score for right answer should be 1?

          When student attempts lesson questions, grade is calculated perfectly.
          Everything else seems to be working ok...

          Cheers
          Rajesh

          Show
          Rajesh Taneja added a comment - Hello Eloy, I have fixed score import, but have one question for Tim. Currently, score is multiple of 100, that means right answer is scored 100. Need to check with Tim, if that is right or score for right answer should be 1? When student attempts lesson questions, grade is calculated perfectly. Everything else seems to be working ok... Cheers Rajesh
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I think score used to be 0..1 fraction before Moodle 2.1, but was changed to % (1..100) for quizzes (or something like that) for Moodle 2.1.

          What I don't get is why that affected formats stuff, IMO it should be done for each module. In any case, if lesson continues using 0..1 then it's a matter of not multiplying * 100.

          Surely Tim will explain this better...awaiting for his comments before integration. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I think score used to be 0..1 fraction before Moodle 2.1, but was changed to % (1..100) for quizzes (or something like that) for Moodle 2.1. What I don't get is why that affected formats stuff, IMO it should be done for each module. In any case, if lesson continues using 0..1 then it's a matter of not multiplying * 100. Surely Tim will explain this better...awaiting for his comments before integration. Ciao
          Hide
          Tim Hunt added a comment -

          Note that questions is lesson are nothing to do with the quiz or question bank, so I really don't know what happens there.

          I know the lesson does seem to use some of the question stuff like the question formats, and it amazes me that it does not break.

          In the question bank, grades are always stored internally between 0 and 1. Later, that gets multiplied whatever the maximum mark for the question is set to in the quiz. (And later the total mark for the quiz may be scaled again to get the final grade to go into the gradebook.) See http://docs.moodle.org/dev/Overview_of_the_Moodle_question_engine#A_note_about_scores

          So, the question definitions use 'fractions' on a scale of 0 to 1. (Or sometimes -1 to 1). For most import/export formats (e.g. GIFT and Moodle XML) that values is written to the file as a percentage. That is just how those file formats are defined.

          Does that explanation answer your questions?

          Show
          Tim Hunt added a comment - Note that questions is lesson are nothing to do with the quiz or question bank, so I really don't know what happens there. I know the lesson does seem to use some of the question stuff like the question formats, and it amazes me that it does not break. In the question bank, grades are always stored internally between 0 and 1. Later, that gets multiplied whatever the maximum mark for the question is set to in the quiz. (And later the total mark for the quiz may be scaled again to get the final grade to go into the gradebook.) See http://docs.moodle.org/dev/Overview_of_the_Moodle_question_engine#A_note_about_scores So, the question definitions use 'fractions' on a scale of 0 to 1. (Or sometimes -1 to 1). For most import/export formats (e.g. GIFT and Moodle XML) that values is written to the file as a percentage. That is just how those file formats are defined. Does that explanation answer your questions?
          Hide
          Michael de Raadt added a comment -

          Thanks for your comments, guys. I'm just following up on this in response to a question from Helen, which came from a QA test.

          Raj should be able to wrap this up tomorrow. I think he has gone home this afternoon.

          Michael;

          Show
          Michael de Raadt added a comment - Thanks for your comments, guys. I'm just following up on this in response to a question from Helen, which came from a QA test. Raj should be able to wrap this up tomorrow. I think he has gone home this afternoon. Michael;
          Hide
          Rajesh Taneja added a comment -

          Thanks for providing the useful details Tim , It helped.

          Lesson doesn't support maxmark as Question Bank ($questionattempt->maxmark), so I think it's fine for now to multiply it with fixed number (100).
          I have created another issue MDL-28060, to fix this later.

          If everyone is fine with this then we can close this issue to pass QA test and fix MDL-28060 later.

          Show
          Rajesh Taneja added a comment - Thanks for providing the useful details Tim , It helped. Lesson doesn't support maxmark as Question Bank ($questionattempt->maxmark), so I think it's fine for now to multiply it with fixed number (100). I have created another issue MDL-28060 , to fix this later. If everyone is fine with this then we can close this issue to pass QA test and fix MDL-28060 later.
          Hide
          Tim Hunt added a comment -

          I don't think you should randomly change how the lesson works to start multiplying scores by 100. You should carefully read the current code and preserve the current intended behaviour while fixing bugs.

          Before deciding to implement maxmark, you would need to have a discussion in the lesson forum, and find out if people need that feature, and if so, what for.

          Show
          Tim Hunt added a comment - I don't think you should randomly change how the lesson works to start multiplying scores by 100. You should carefully read the current code and preserve the current intended behaviour while fixing bugs. Before deciding to implement maxmark, you would need to have a discussion in the lesson forum, and find out if people need that feature, and if so, what for.
          Hide
          Rajesh Taneja added a comment -

          Hello Tim,

          Multiplying with 100 is the current behavior. I have not changed it.
          Hence, the last thought was to keep it the way it is and try move it towards Question bank implementation (if required) through MDL-28060.

          Show
          Rajesh Taneja added a comment - Hello Tim, Multiplying with 100 is the current behavior. I have not changed it. Hence, the last thought was to keep it the way it is and try move it towards Question bank implementation (if required) through MDL-28060 .
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho Rajesh, Tim and co,

          sorry but there is something here that I don't like at all. I've tried importing the "GIFT-examples" file in 1.9, 2.0 and master. And this is what I've got:

          • 19_STABLE: scores are imported as 0/1. For example, the first question in the file, with 3 answers get 1 score point with the 3rd "no one" and 0 with the rest.
          • 20_STABLE: Although the import is not working at all (the answers are lost), scores are imported and they seem to work like 19_STABLE, aka 0/1.
          • master: scores are imported as 0/100.

          So, at some point, not sure when/why, the scores, that ALWAYS (1.9, 2.0 and master) have been multiplied * 100, started producing 0-100 scores versus 0-1 ones. I bet it is something in Tim's "artillery" or perhaps some annoying integer casting at some point, but I don't know really the cause for that (nor Tim's explanation above clarified that to me).

          I must say that, surely (in my mind) it shouldn't affect final grade calculation, but my mind isn't a good (safe) place. So the logical approach should be:

          APPROACH A:
          1) Modify master to continue setting scores in the 0-1 range. Surely that implies to take out all those * 100.
          2) Backport the fix to 20_STABLE (but take into account that probably you'll need the x100 there). Right now it's 100% (not 1%, LOL) broken.

          But that 1) has an important drawback that is that scores will be ONLY 0 or 1 as far as the score column in DB is one integer, so it does not accept decimals. See for example how the question "Jesus' hometown::Jesus Christ was from" is imported. Now in 2.1 we are getting correct 0, 25, 75 and 100, but in 1.9 it only returns 0, 0, 0, and 1.

          So, the most I think on this, the most I believe that such a "* 100" was done on purpose exactly for that (to get 0-100 scores). And for some reason it never worked ok in 1.9 and 2.0 and started working ok in 2.1. So that lead me to propose:

          APPROACH B:
          1) Accept current patch and start setting 0-100 scores.
          2) Backport the fix to 20_STABLE (but take into account that probably you'll need the x100 there to get the 0-1 values, who knows).

          But finally, I've been looking how grades are calculated by lesson - lesson_grade() - and it seems that scores are only taken into account if the lesson has been created to use "custom scores", else +1 is used. So, once again, I'm lost, should we be importing the questions in a different way depending of that setting? Also, when scores are used (custom scores = yes), they are all summed globally so, indeed, deciding about to import them like 0-1 or 0-100 is going to have a BIG impact.

          So, to decide:

          APPROACH A: Old one but "rounds" all fractions to 0. Safer because of BC.
          APPROACH B: New one and more "exact", respecting fractions. Horrible (risky) if mixed with old 0-1 questions.

          Really I'm not sold to any of them, specially 2 days before release. Surely I'd go to the APPROACH B so people can start importing those questions better. If that messes their lessons, well, they'll need to adjust the scores. Surely this requires to be documented somewhere.

          So reopen and decide, my +0.8 goes to approach B, break old-wrong behavior and import the questions in a richer way.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho Rajesh, Tim and co, sorry but there is something here that I don't like at all. I've tried importing the "GIFT-examples" file in 1.9, 2.0 and master. And this is what I've got: 19_STABLE: scores are imported as 0/1. For example, the first question in the file, with 3 answers get 1 score point with the 3rd "no one" and 0 with the rest. 20_STABLE: Although the import is not working at all (the answers are lost), scores are imported and they seem to work like 19_STABLE, aka 0/1. master: scores are imported as 0/100. So, at some point, not sure when/why, the scores, that ALWAYS (1.9, 2.0 and master) have been multiplied * 100, started producing 0-100 scores versus 0-1 ones. I bet it is something in Tim's "artillery" or perhaps some annoying integer casting at some point, but I don't know really the cause for that (nor Tim's explanation above clarified that to me). I must say that, surely (in my mind) it shouldn't affect final grade calculation, but my mind isn't a good (safe) place. So the logical approach should be: APPROACH A: 1) Modify master to continue setting scores in the 0-1 range. Surely that implies to take out all those * 100. 2) Backport the fix to 20_STABLE (but take into account that probably you'll need the x100 there). Right now it's 100% (not 1%, LOL) broken. But that 1) has an important drawback that is that scores will be ONLY 0 or 1 as far as the score column in DB is one integer, so it does not accept decimals. See for example how the question "Jesus' hometown::Jesus Christ was from" is imported. Now in 2.1 we are getting correct 0, 25, 75 and 100, but in 1.9 it only returns 0, 0, 0, and 1. So, the most I think on this, the most I believe that such a "* 100" was done on purpose exactly for that (to get 0-100 scores). And for some reason it never worked ok in 1.9 and 2.0 and started working ok in 2.1. So that lead me to propose: APPROACH B: 1) Accept current patch and start setting 0-100 scores. 2) Backport the fix to 20_STABLE (but take into account that probably you'll need the x100 there to get the 0-1 values, who knows). But finally, I've been looking how grades are calculated by lesson - lesson_grade() - and it seems that scores are only taken into account if the lesson has been created to use "custom scores", else +1 is used. So, once again, I'm lost, should we be importing the questions in a different way depending of that setting? Also, when scores are used (custom scores = yes), they are all summed globally so, indeed, deciding about to import them like 0-1 or 0-100 is going to have a BIG impact. So, to decide: APPROACH A: Old one but "rounds" all fractions to 0. Safer because of BC. APPROACH B: New one and more "exact", respecting fractions. Horrible (risky) if mixed with old 0-1 questions. Really I'm not sold to any of them, specially 2 days before release. Surely I'd go to the APPROACH B so people can start importing those questions better. If that messes their lessons, well, they'll need to adjust the scores. Surely this requires to be documented somewhere. So reopen and decide, my +0.8 goes to approach B, break old-wrong behavior and import the questions in a richer way. Ciao
          Hide
          Sam Hemelryk added a comment -

          Hehe just chiming in here my +1 goes to approach B as well - especially given lesson is right up the top of the list for 2.2 and we will hopefully see it properly integrated with questions.

          Show
          Sam Hemelryk added a comment - Hehe just chiming in here my +1 goes to approach B as well - especially given lesson is right up the top of the list for 2.2 and we will hopefully see it properly integrated with questions.
          Hide
          Tim Hunt added a comment -

          I think that, 2 days before release, we should not be changing the behaviour if we don't understand it.

          Importing the same file into 2.0 and 2.1 lesson should write the same data to the DB, unless we can explain exactly what the effect of any change is, and why it is better.

          Presumably, the bit of Rajesh's patch that is causing this is all the ->grade to ->score changes.

          My recommendation is:

          APPROACH C:

          Change ->score back to ->grade in time for the 2.1 release, so the behaviour is the same as in 2.0 and 1.9. Please use those, and no-one has reported the current behaviour as a bug.

          Then, after 2.1 is released, start a discussion in the Lesson forum about how this is supposed to work, and once you know, do any changes necessary in all stable branches.

          Show
          Tim Hunt added a comment - I think that, 2 days before release, we should not be changing the behaviour if we don't understand it. Importing the same file into 2.0 and 2.1 lesson should write the same data to the DB, unless we can explain exactly what the effect of any change is, and why it is better. Presumably, the bit of Rajesh's patch that is causing this is all the ->grade to ->score changes. My recommendation is: APPROACH C: Change ->score back to ->grade in time for the 2.1 release, so the behaviour is the same as in 2.0 and 1.9. Please use those, and no-one has reported the current behaviour as a bug. Then, after 2.1 is released, start a discussion in the Lesson forum about how this is supposed to work, and once you know, do any changes necessary in all stable branches.
          Hide
          Rajesh Taneja added a comment -

          +1 for Approach C.

          Have reverted my last changes for updating score.
          Current import replicates the behavior in 1.9.
          As mentioned by Eloy, it's broken in 2.0, so will provide same patch for 2.0

          Thanks Tim, Eloy, Sam and CO

          Show
          Rajesh Taneja added a comment - +1 for Approach C. Have reverted my last changes for updating score. Current import replicates the behavior in 1.9. As mentioned by Eloy, it's broken in 2.0, so will provide same patch for 2.0 Thanks Tim, Eloy, Sam and CO
          Hide
          Rajesh Taneja added a comment -

          Tim, It will be very kind, if you can please peer review again.
          Just want to make sure, I didn't miss anything.

          Have tested the behavior on 1.9, 2.0 and 2.1 and it's consistent now.
          Checked Database as well, imported data is same for 1.9, 2.0 and 2.1

          Show
          Rajesh Taneja added a comment - Tim, It will be very kind, if you can please peer review again. Just want to make sure, I didn't miss anything. Have tested the behavior on 1.9, 2.0 and 2.1 and it's consistent now. Checked Database as well, imported data is same for 1.9, 2.0 and 2.1
          Hide
          Eloy Lafuente (stronk7) added a comment -

          NP, here, assuming approach C = approach A, all right.

          So:

          1) We are going to import in the 0&1 way.
          2) Knowing it keeps out fractional answers (changes them to 0), so I'd create issue for that (to fix in the 2.2)
          3) (optional) We are going to fix some qtypes currently not being score calculated, like true/false. That's safe and it's already coded.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - NP, here, assuming approach C = approach A, all right. So: 1) We are going to import in the 0&1 way. 2) Knowing it keeps out fractional answers (changes them to 0), so I'd create issue for that (to fix in the 2.2) 3) (optional) We are going to fix some qtypes currently not being score calculated, like true/false. That's safe and it's already coded. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

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

          I've tested this both @ 20_STABLE and master and seems to work as expected (aka, imperfectly)

          Also, it's really a pity to have had the score points working for truefalse/shortanswer/numerical... and, at the end, having them not working (my prev optional point 3) above).

          Anyway, I'll be commenting about that @ MDL-28060.

          Passing test without further action. Will be tested by MDLQA-1017 once this meets upstream.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I've tested this both @ 20_STABLE and master and seems to work as expected (aka, imperfectly) Also, it's really a pity to have had the score points working for truefalse/shortanswer/numerical... and, at the end, having them not working (my prev optional point 3) above). Anyway, I'll be commenting about that @ MDL-28060 . Passing test without further action. Will be tested by MDLQA-1017 once this meets upstream. Ciao
          Hide
          Rajesh Taneja added a comment -

          Thanks Eloy

          Show
          Rajesh Taneja added a comment - Thanks Eloy
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yay, this is now part of the just released Moodle 2.1 ! Thanks for all the hard work!

          Show
          Eloy Lafuente (stronk7) added a comment - Yay, this is now part of the just released Moodle 2.1 ! Thanks for all the hard work!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: