Moodle
  1. Moodle
  2. MDL-27670

Calculated question creation allways shows "will use the same existing shared dataset as before" even when the data set does not exist

    Details

    • Type: Bug Bug
    • Status: Development in progress
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.9.13, 2.0.4, 2.1.1, 2.2, 2.7
    • Fix Version/s: STABLE backlog
    • Component/s: Quiz
    • Labels:
    • Environment:
      LAMP
    • Database:
      Any
    • Testing Instructions:
      Hide
      • Create a calculated question with data:
        • Question text: "What is {a} plus {b}?"
          ** Answer text: {a}

          +

          {b}, grade 100%
          * Click "Save changes".

          TEST: On the next screen, verify that for "wildcard {a}" and "wildcard {b}

          ", the option "will use a new private dataset" is displayed.

      • Continue with the wizard, adding at least one dataset item on the third wizard screen. Save the question.
      • Edit the question again. Proceed to the second wizard screen.

      TEST: Verify that for "wildcard

      {a}

      " and "wildcard

      {b}

      ", the option "will use the same existing private dataset as before" is displayed.

      Show
      Create a calculated question with data: Question text: "What is {a} plus {b}?" ** Answer text: {a} + {b}, grade 100% * Click "Save changes". TEST: On the next screen, verify that for "wildcard {a}" and "wildcard {b} ", the option "will use a new private dataset" is displayed. Continue with the wizard, adding at least one dataset item on the third wizard screen. Save the question. Edit the question again. Proceed to the second wizard screen. TEST: Verify that for "wildcard {a} " and "wildcard {b} ", the option "will use the same existing private dataset as before" is displayed.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      17331

      Description

      When creating a new calculated question, on step 2 the drop down allways shows "will use the same existing private dataset as before" (keptcategory1 string) instead of "will use a new private dataset" (newlocal1 string).

      The keptcategory1 string is only to be used when editing an existing calculated question with an existing private dataset created to that variable. It shouldn't ever be show on the question creation because no private datasets exist.

      This question type is already very confusing so these small details can make some difference to the user.

        Activity

        Hide
        Henning Bostelmann added a comment -

        I just had a brief look at that (on the master branch). The underlying problem seems to be that the private dataset actually exists at this point; namely, for "mandatory" fields, it is already created when saving the first page of the wizard.

        More technically, qtype_calculated::save_question() calls $this->preparedatasets(), then $this->save_dataset_definitions(), and preparedatasets() deliberately sets up all mandatory datasets, judging by the comment ("always prepare the mandatorydatasets present in the answers" - line 513).

        Maybe the easiest option would be to display "will use a private dataset"? It doesn't really matter so much whether it's new or existing, since it's private anyway.

        Or should one determine "new" vs. "existing" by the question whether the dataset has any items?

        Show
        Henning Bostelmann added a comment - I just had a brief look at that (on the master branch). The underlying problem seems to be that the private dataset actually exists at this point; namely, for "mandatory" fields, it is already created when saving the first page of the wizard. More technically, qtype_calculated::save_question() calls $this->preparedatasets(), then $this->save_dataset_definitions(), and preparedatasets() deliberately sets up all mandatory datasets, judging by the comment ("always prepare the mandatorydatasets present in the answers" - line 513). Maybe the easiest option would be to display "will use a private dataset"? It doesn't really matter so much whether it's new or existing, since it's private anyway. Or should one determine "new" vs. "existing" by the question whether the dataset has any items?
        Hide
        Antonio Vilela added a comment -

        When you create a new question with new variables the options shown in a drop down are:

        • Will use the same existing private data set as before
        • Will use a new shared data set

        To keep the consistence between the 2 options, the options should be show like this:

        • Will use a new private data set
        • Will use a new shared data set

        In my opinion, to display "will use a private dataset" is not the best option because it will be incoherent with the other strings that may be show on this drop down. I added all the possiblie scenarios bellow if you want to confirm.

        So the "new" vs. "existing" determined by the question whether the dataset has any items seems to me a better solution, although you can exit a question without adding any data to the dataset and later edit the question to add data, but that seems a minor problem.

        I tested all other possibilities. Bellow are the tested possibilities, just for your information:

        When you edit a question, the following options appear on a variable created with a private data set. As you can see, this time the string is well suited for the context.:

        • Will use the same existing private data set as before
        • Will use a new shared data set

        When you edit a question, the following options appear on a variable created with a shared data set. The string shown for the private data set is right this time.

        • Will use a new private data set
        • Will use the same existing shared data set as before

        When you create a new question with a variable that already exists as shared, the options shown in a drop down are the following. Once more the string keptcategory1 is incorrectly shown. The problem is the same and will be solved with the proposed solution.

        • Will use the same existing shared data set as before
        • Will use an already existing shared data set
        Show
        Antonio Vilela added a comment - When you create a new question with new variables the options shown in a drop down are: Will use the same existing private data set as before Will use a new shared data set To keep the consistence between the 2 options, the options should be show like this: Will use a new private data set Will use a new shared data set In my opinion, to display "will use a private dataset" is not the best option because it will be incoherent with the other strings that may be show on this drop down. I added all the possiblie scenarios bellow if you want to confirm. So the "new" vs. "existing" determined by the question whether the dataset has any items seems to me a better solution, although you can exit a question without adding any data to the dataset and later edit the question to add data, but that seems a minor problem. I tested all other possibilities. Bellow are the tested possibilities, just for your information: When you edit a question, the following options appear on a variable created with a private data set. As you can see, this time the string is well suited for the context.: Will use the same existing private data set as before Will use a new shared data set When you edit a question, the following options appear on a variable created with a shared data set. The string shown for the private data set is right this time. Will use a new private data set Will use the same existing shared data set as before When you create a new question with a variable that already exists as shared, the options shown in a drop down are the following. Once more the string keptcategory1 is incorrectly shown. The problem is the same and will be solved with the proposed solution. Will use the same existing shared data set as before Will use an already existing shared data set
        Hide
        Henning Bostelmann added a comment -

        OK, let's go with the option that an existing but empty private dataset is considered "new".

        Show
        Henning Bostelmann added a comment - OK, let's go with the option that an existing but empty private dataset is considered "new".
        Hide
        Henning Bostelmann added a comment -

        Tim: This solution might seem a bit of a hack, but I think it's the option that will cause the least regression errors.

        For all I can see, this affects all STABLE branches. The fix is for master only, at the moment. Will backport it once it's agreed.

        Show
        Henning Bostelmann added a comment - Tim: This solution might seem a bit of a hack, but I think it's the option that will cause the least regression errors. For all I can see, this affects all STABLE branches. The fix is for master only, at the moment. Will backport it once it's agreed.
        Hide
        Tim Hunt added a comment -

        Henning, I finally got to looking at this.

        Looks like your fix will do the right thing, but unfortunately we can't use it as is. The problem is that it adds another DB query. Instead, you need to change the previous query to include hasitems. Something like

        {code sql}

        SELECT
        qdd.*,
        CASE WHEN EXISTS (
        SELECT 1 FROM

        {question_dataset_items}

        WHERE definition = qdd.id
        ) THEN 1 ELSE 0 END AS hasitems
        FROM

        {question_dataset_definitions}

        qdd
        JOIN

        {question_datasets}

        qd ON qdd.id = qd.datasetdefinition
        WHERE qdd.type = 1
        AND qd.question = ?
        AND qdd.name = ?

        
        

        There is a bigger problem here, that this code is ultimately called in a loop (from question_dataset_dependent_definitions_form::definition()) would be better to re-organise the code to load all the necessary dataset information in one big DB query, but that is a larger problem for another day. This bug needs a small fix.

        Will you be able to do a revised patch some time?

        Show
        Tim Hunt added a comment - Henning, I finally got to looking at this. Looks like your fix will do the right thing, but unfortunately we can't use it as is. The problem is that it adds another DB query. Instead, you need to change the previous query to include hasitems. Something like {code sql} SELECT qdd.*, CASE WHEN EXISTS ( SELECT 1 FROM {question_dataset_items} WHERE definition = qdd.id ) THEN 1 ELSE 0 END AS hasitems FROM {question_dataset_definitions} qdd JOIN {question_datasets} qd ON qdd.id = qd.datasetdefinition WHERE qdd.type = 1 AND qd.question = ? AND qdd.name = ? There is a bigger problem here, that this code is ultimately called in a loop (from question_dataset_dependent_definitions_form::definition()) would be better to re-organise the code to load all the necessary dataset information in one big DB query, but that is a larger problem for another day. This bug needs a small fix. Will you be able to do a revised patch some time?
        Hide
        Pierre Pichet added a comment -

        This problem arised as an artefact of trying to solve another biggest problem.
        In old (moodle15) versions the three steps calculated editing process relies using sessions data to store all the datas generated in the three steps.
        Given the time to buid a calculated question, peoples often lost their work.
        So I add this saving of datasets between step 1 and step two even if the decision to use them is done in the second step i.e.
        datasetdefinitions_form.php.
        The also move the datasets to another category in case the question already exists.
        So when datasetdefinitions_form.php is used it cannot seen readily if the existing datasets were created automatically from a new question or from editing an old question unless as you noted it has info on existing dataitems.

        In the next weeks, I plan to work on the problem of handling correctly the category datasets and the solution could be to allow in a given category datasets with the same name but related to different questions.

        I think that I can recode the datasetdefinitions_form.php which is where the user choose the datasets to be used, to handle this new feature that will solve the moving category datasets problem.

        Given that the actual bug is an old moodle 16 unnoticed one, let's wait before processing it.

        Show
        Pierre Pichet added a comment - This problem arised as an artefact of trying to solve another biggest problem. In old (moodle15) versions the three steps calculated editing process relies using sessions data to store all the datas generated in the three steps. Given the time to buid a calculated question, peoples often lost their work. So I add this saving of datasets between step 1 and step two even if the decision to use them is done in the second step i.e. datasetdefinitions_form.php. The also move the datasets to another category in case the question already exists. So when datasetdefinitions_form.php is used it cannot seen readily if the existing datasets were created automatically from a new question or from editing an old question unless as you noted it has info on existing dataitems. In the next weeks, I plan to work on the problem of handling correctly the category datasets and the solution could be to allow in a given category datasets with the same name but related to different questions. I think that I can recode the datasetdefinitions_form.php which is where the user choose the datasets to be used, to handle this new feature that will solve the moving category datasets problem. Given that the actual bug is an old moodle 16 unnoticed one, let's wait before processing it.
        Hide
        Pierre Pichet added a comment -

        A simpler solution from the fact the itemcount is already available when buiding the strings.

        A commit will follow soon.

        Show
        Pierre Pichet added a comment - A simpler solution from the fact the itemcount is already available when buiding the strings. A commit will follow soon.
        Hide
        Pierre Pichet added a comment - - edited

        the commit can be found at
        https://github.com/ppichet/moodle/commit/f6dbf0c1cf88b2e1777c9161de73582c429e7dc4

        The lang string used is only a proposal...

        Also the langstrings that refered to a $type other than 1 as

        $string['keptlocal2'] = 'a file from the same question private set of files as before';
        $string['keptlocal3'] = 'a link from the same question private set of links as before';

        can be removed as they have never be used. Only the $type1 has ever been implemented in moodle.

        P.S.Tim, I came to this bug when surveying your activity. It could be usefull if you add me as a watcher on problems like this one.

        P.S.2 I am not sure how to correctly indent things like

                    if ($currentdatasetdef->type == $type
                            and $currentdatasetdef->category == 0) {
                                if($currentdatasetdef->itemcount > 0 ){
                                    $options[$key] = get_string($prefix."keptlocal$type", $langfile);
                                }else {
                                    $options[$key] = get_string($prefix."keptlocalempty$type", $langfile);
                                }
                    } else {
        

        I need to install Tim styling tool

        Show
        Pierre Pichet added a comment - - edited the commit can be found at https://github.com/ppichet/moodle/commit/f6dbf0c1cf88b2e1777c9161de73582c429e7dc4 The lang string used is only a proposal... Also the langstrings that refered to a $type other than 1 as $string ['keptlocal2'] = 'a file from the same question private set of files as before'; $string ['keptlocal3'] = 'a link from the same question private set of links as before'; can be removed as they have never be used. Only the $type1 has ever been implemented in moodle. P.S.Tim, I came to this bug when surveying your activity. It could be usefull if you add me as a watcher on problems like this one. P.S.2 I am not sure how to correctly indent things like if ($currentdatasetdef->type == $type and $currentdatasetdef->category == 0) { if($currentdatasetdef->itemcount > 0 ){ $options[$key] = get_string($prefix."keptlocal$type", $langfile); }else { $options[$key] = get_string($prefix."keptlocalempty$type", $langfile); } } else { I need to install Tim styling tool
        Hide
        Tim Hunt added a comment -

        Sorry, I should have added you as a watcher. I'll try to remember to do that in future.

        There are lots of whitespace errors there. https://github.com/timhunt/moodle-local_codechecker/ is easy to install. Anyway, look at the deleted line in your patch. That was right. The new code is wrong.

        I don't really understand the need for the new string. The problem is that the get_string($prefix."newcategory$type", $langfile); is never being used.

        But you are right that there is no need to query the DB for the number of items, because we have already got it in $currentdatasetdef. It least as far as I can see.

        Show
        Tim Hunt added a comment - Sorry, I should have added you as a watcher. I'll try to remember to do that in future. There are lots of whitespace errors there. https://github.com/timhunt/moodle-local_codechecker/ is easy to install. Anyway, look at the deleted line in your patch. That was right. The new code is wrong. I don't really understand the need for the new string. The problem is that the get_string($prefix."newcategory$type", $langfile); is never being used. But you are right that there is no need to query the DB for the number of items, because we have already got it in $currentdatasetdef. It least as far as I can see.
        Hide
        Pierre Pichet added a comment - - edited

        In the actual code (that could be improved and will be improved when solving the category datasets problem) , in saving the question from the edit_calculated_form.php, the mandatory datasets are saved or created if they do not exist.
        So if the teacher try to use the question without going to the second step, the question will be able to load the question sets but will return no dataitems as the number of items search is the minimum fom the datasets i.e. 0.
        This kind of condition or error can be handled much more easily in the question code, which explain the actual code.

        From the user viewpoint when he goes to the second step i.e datasetdefinitions_form.php, three conditions could appear when I built this code,
        1. editing a new question : the datasets are already created although without dataitems which are created in the third step.
        2. reediting a question that has been completed to the third step so the already used dataset option
        3. completing a question that was saved only at the first step before this new automatic saving code was in place ( moodle 16) i.e. no datasets created. We could remove this third option or keep it given that we are at moodle 2,1.

        The problem is not the same for the category datasets as at this step
        1.either they already exist ( they could be created even when saving from the first step to the second one by another question).
        or
        2.they do not exist so they will be created between step 2 and 3 .

        Show
        Pierre Pichet added a comment - - edited In the actual code (that could be improved and will be improved when solving the category datasets problem) , in saving the question from the edit_calculated_form.php, the mandatory datasets are saved or created if they do not exist. So if the teacher try to use the question without going to the second step, the question will be able to load the question sets but will return no dataitems as the number of items search is the minimum fom the datasets i.e. 0. This kind of condition or error can be handled much more easily in the question code, which explain the actual code. From the user viewpoint when he goes to the second step i.e datasetdefinitions_form.php, three conditions could appear when I built this code, 1. editing a new question : the datasets are already created although without dataitems which are created in the third step. 2. reediting a question that has been completed to the third step so the already used dataset option 3. completing a question that was saved only at the first step before this new automatic saving code was in place ( moodle 16) i.e. no datasets created. We could remove this third option or keep it given that we are at moodle 2,1. The problem is not the same for the category datasets as at this step 1.either they already exist ( they could be created even when saving from the first step to the second one by another question). or 2.they do not exist so they will be created between step 2 and 3 .
        Hide
        Pierre Pichet added a comment -

        The code has been corrected and the code checker is a very useful tool.
        https://github.com/ppichet/moodle/commit/9729eb20d10480301891f1db62f237d72fcd584b

        However I did not elimibate all the white spaces that results from the long history of this code file.
        Should I do it now or include them when working on the category dataset problem?

        Show
        Pierre Pichet added a comment - The code has been corrected and the code checker is a very useful tool. https://github.com/ppichet/moodle/commit/9729eb20d10480301891f1db62f237d72fcd584b However I did not elimibate all the white spaces that results from the long history of this code file. Should I do it now or include them when working on the category dataset problem?
        Hide
        Tim Hunt added a comment -

        Correct. When you are doing a small bug fix like this, you should only fix the whitespace in the lines you need to change anyway.

        Show
        Tim Hunt added a comment - Correct. When you are doing a small bug fix like this, you should only fix the whitespace in the lines you need to change anyway.
        Hide
        Henning Bostelmann added a comment -

        Hi Pierre, thanks for working on this - your solution seems much more elegant than the one I proposed.

        Changing assignee.

        Show
        Henning Bostelmann added a comment - Hi Pierre, thanks for working on this - your solution seems much more elegant than the one I proposed. Changing assignee.
        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this issue.

        We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported.

        If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

        Michael d.

        TW9vZGxlDQo=

        Show
        Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d. TW9vZGxlDQo=
        Hide
        Henning Bostelmann added a comment -

        I just checked - the issue still exists in Moodle 2.7dev (Build: 20131205). Changing affected versions.

        Show
        Henning Bostelmann added a comment - I just checked - the issue still exists in Moodle 2.7dev (Build: 20131205). Changing affected versions.
        Hide
        Pierre Pichet added a comment - - edited

        Thanks Michael for the general reminder of old bugs and Henning for the checking.

        There is real hope that I will have more time to work on calculated and other bugs as I am now in a 3 years pre-retirement program with half (50%) duty.
        However my wife understand that I could put more time on home improvments that are also waiting ...
        Let's see how I will manage my 50% "free" time...

        P.S. If somebody want to proceed on these bugs, feel free to do it.
        I do not release them as suggested elsewhere as this remains me and other relatives of the moodle todo list...

        Show
        Pierre Pichet added a comment - - edited Thanks Michael for the general reminder of old bugs and Henning for the checking. There is real hope that I will have more time to work on calculated and other bugs as I am now in a 3 years pre-retirement program with half (50%) duty. However my wife understand that I could put more time on home improvments that are also waiting ... Let's see how I will manage my 50% "free" time... P.S. If somebody want to proceed on these bugs, feel free to do it. I do not release them as suggested elsewhere as this remains me and other relatives of the moodle todo list...

          People

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

            Dates

            • Created:
              Updated: