Moodle
  1. Moodle
  2. MDL-36551

Onsite preset doesn't retain the advanced search template.

    Details

    • Testing Instructions:
      Hide

      Pre-requisites

      • Testing requires access to the database.
      • The ability to modify records.
      • The ability to delete files from the the data area.

      Test steps

      1. Create a database activity
      2. Add some fields.
      3. Alter the templates so that you know that you will know if they revert back to the default template.
      4. Export the database preset as a zip file and save it on your computer (file 1).
      5. Go back to the presets tab and save the preset in the 'Save as preset' section. This will save the preset file on the system (file 2).
      6. In the same course, create a database activity and import the preset zip file from your computer(file 1).
        • Check that the preset has imported correctly and that the new database activity is identical to the original.
      7. In the same course, create another database activity and select the system saved preset (file 2) and import that.
        • Check that the preset has imported correctly and that the new database activity is identical to the original.

      Backwards compatibility

      To test a preset file created before advanced search templates were included in the preset file, you will need to alter the files. (or create a 1.7 installation)

      External zip file.

      1. Create a database activity.
      2. Add some fields.
      3. Save the templates.
      4. Export the database preset as a zip file and save it on your computer.
      5. Extract the file.
      6. Delete the asearchtemplate.html file and re-compress the file.
      7. Create a new database activity.
      8. Upload the preset zip file from you computer.
        • Check that no error is shown when uploading the preset file.

      On-site preset.

      1. Create a database activity.
      2. Add some fields.
      3. Save the templates.
      4. Save as a preset ('Save as preset' area, click continue).
      5. Open the database manager of your choice.
      6. Browse through mdl_files and find your preset (check filepath).
      7. When the filepath is the preset file that you created, check filename for asearchtemplate.html.
      8. make note of the contenthash.
      9. Go to the moodledata / filedir directory.
      10. The first two characters of the contenthash are the first directory, enter that directory.
      11. the third and fourth characters of the contenthash are the next directory. Enter that directory.
      12. Delete the file in that directory (make sure that the file name is exactly the same as the contenthash.
      13. Delete the record that corresponds to the file that you just deleted from the database.
      14. Create a new database activity.
      15. Go to the preset page and select the on-site preset that you just hacked.
        • Check that no errors are displayed when using this preset.

      Please note that viewing the advanced search template immediately after uploading the preset will display nothing. Clicking the 'Search' tab will automatically generate a default template.

      Show
      Pre-requisites Testing requires access to the database. The ability to modify records. The ability to delete files from the the data area. Test steps Create a database activity Add some fields. Alter the templates so that you know that you will know if they revert back to the default template. Export the database preset as a zip file and save it on your computer (file 1). Go back to the presets tab and save the preset in the 'Save as preset' section. This will save the preset file on the system (file 2). In the same course, create a database activity and import the preset zip file from your computer(file 1). Check that the preset has imported correctly and that the new database activity is identical to the original. In the same course, create another database activity and select the system saved preset (file 2) and import that. Check that the preset has imported correctly and that the new database activity is identical to the original. Backwards compatibility To test a preset file created before advanced search templates were included in the preset file, you will need to alter the files. (or create a 1.7 installation) External zip file. Create a database activity. Add some fields. Save the templates. Export the database preset as a zip file and save it on your computer. Extract the file. Delete the asearchtemplate.html file and re-compress the file. Create a new database activity. Upload the preset zip file from you computer. Check that no error is shown when uploading the preset file. On-site preset. Create a database activity. Add some fields. Save the templates. Save as a preset ('Save as preset' area, click continue). Open the database manager of your choice. Browse through mdl_files and find your preset (check filepath). When the filepath is the preset file that you created, check filename for asearchtemplate.html. make note of the contenthash. Go to the moodledata / filedir directory. The first two characters of the contenthash are the first directory, enter that directory. the third and fourth characters of the contenthash are the next directory. Enter that directory. Delete the file in that directory (make sure that the file name is exactly the same as the contenthash. Delete the record that corresponds to the file that you just deleted from the database. Create a new database activity. Go to the preset page and select the on-site preset that you just hacked. Check that no errors are displayed when using this preset. Please note that viewing the advanced search template immediately after uploading the preset will display nothing. Clicking the 'Search' tab will automatically generate a default template.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-36551-master
    • Rank:
      46024

      Description

      Using a preset zip file will keep all the information correctly when importing into a new database activity, but a locally stored preset will not.

        Issue Links

          Activity

          Hide
          Adrian Greeve added a comment -

          I split line 2139 as it was causing a strict notification on the page.
          The previous code was checking to see if the file existed for the advanced search template, but the way that it was checking meant that if the file was stored on the site, it would never return the template.
          I see no reason as to why this would be set as "optional" so I removed that if statement and put it with the other templates.

          Show
          Adrian Greeve added a comment - I split line 2139 as it was causing a strict notification on the page. The previous code was checking to see if the file existed for the advanced search template, but the way that it was checking meant that if the file was stored on the site, it would never return the template. I see no reason as to why this would be set as "optional" so I removed that if statement and put it with the other templates.
          Hide
          Dan Poltawski added a comment -

          Hmm.

          This was made optional by 2dc6be3f776826f5f9660fc8fa5b8d5121a24da0 (MDL-13188)

          Show
          Dan Poltawski added a comment - Hmm. This was made optional by 2dc6be3f776826f5f9660fc8fa5b8d5121a24da0 ( MDL-13188 )
          Hide
          Dan Poltawski added a comment -

          Hi Adrian,

          Its not clear to me that removing that optional bit is the right idea. (It might be).

          But it looks like one part of the problem is that file_exists call is not using the file api, and maybe should be.

          Show
          Dan Poltawski added a comment - Hi Adrian, Its not clear to me that removing that optional bit is the right idea. (It might be). But it looks like one part of the problem is that file_exists call is not using the file api, and maybe should be.
          Hide
          Adrian Greeve added a comment -

          Hi Dan,

          I've had a closer look and I still stand by my original patch.
          I don't see why the advanced search template is any different to the other templates in the preset. There isn't an option (as far as I know) for not including it when saving the preset.

          I did test the patch with importing the preset zip and the on site preset. I didn't encounter the problems mentioned in MDL-13188.

          But perhaps there is some reason, that I can't think of, as to why we would specifically check for the advanced search template and not the other templates.

          Show
          Adrian Greeve added a comment - Hi Dan, I've had a closer look and I still stand by my original patch. I don't see why the advanced search template is any different to the other templates in the preset. There isn't an option (as far as I know) for not including it when saving the preset. I did test the patch with importing the preset zip and the on site preset. I didn't encounter the problems mentioned in MDL-13188 . But perhaps there is some reason, that I can't think of, as to why we would specifically check for the advanced search template and not the other templates.
          Hide
          Andrew Davis added a comment -

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [NA] Language
          [NA] Databases
          [N] Testing
          [NA] Security
          [NA] Documentation
          [Y] Git
          [Y] Sanity check

          As long as the advanced search template will always exist this looks fine. I'm curious why someone felt the need to add that if file exists check in the first place.

          Can you rework the testing instructions to make them clearer? I'm not entirely sure of the difference between step 6 and step 7 in particular.

          Show
          Andrew Davis added a comment - [Y] Syntax [Y] Output [Y] Whitespace [NA] Language [NA] Databases [N] Testing [NA] Security [NA] Documentation [Y] Git [Y] Sanity check As long as the advanced search template will always exist this looks fine. I'm curious why someone felt the need to add that if file exists check in the first place. Can you rework the testing instructions to make them clearer? I'm not entirely sure of the difference between step 6 and step 7 in particular.
          Hide
          Adrian Greeve added a comment -

          Thanks for having a look at this for me Andrew. It's nice to get another opinion about this issue. I have been scratching my head trying to figure out why the advanced search template has been singled out for checking if it exists.

          I've touched up the testing instructions and hopefully they are clearer.

          Show
          Adrian Greeve added a comment - Thanks for having a look at this for me Andrew. It's nice to get another opinion about this issue. I have been scratching my head trying to figure out why the advanced search template has been singled out for checking if it exists. I've touched up the testing instructions and hopefully they are clearer.
          Hide
          Sam Hemelryk added a comment -

          I was just looking at this, Dan is right in noting that the asearchtemplate.html file was made optional in MDL-13188.
          That was done because Moodle 1.9.0 and lower wasn't including it in exported presets.
          It looks like anyone with a database preset created back before 1.9.1 will get an error after this change should they try to restore it. It may be that the preset was downloaded, or just saved on the site and then the site upgrade right the way through till now.
          Is this something we care about?
          Personally I think if its not too much trouble it is something that we should care about and deal with.

          Show
          Sam Hemelryk added a comment - I was just looking at this, Dan is right in noting that the asearchtemplate.html file was made optional in MDL-13188 . That was done because Moodle 1.9.0 and lower wasn't including it in exported presets. It looks like anyone with a database preset created back before 1.9.1 will get an error after this change should they try to restore it. It may be that the preset was downloaded, or just saved on the site and then the site upgrade right the way through till now. Is this something we care about? Personally I think if its not too much trouble it is something that we should care about and deal with.
          Hide
          Adrian Greeve added a comment -

          Hi Sam,

          After our discussion I went and had a look at the 1.9.0 code (I tried installing 1.9.0 but there are issues with MoodleQuickForm that I couldn't resolve in a sufficient amount of time).

          If you check out the code for 1.9.0 and look at mod/data/templates.php line 148 - 154 you can see that a default advanced search template is being generated with all of the other templates.

          I suspect that the code that Petr put in was to cover people upgrading from an earlier version of Moodle (say 1.7 where the advanced search template was not automatically generated) so that their presets would not break.

          I feel that this is a situation where we could clean up the code and remove this check for advanced search template presets that are pre 1.9.

          Show
          Adrian Greeve added a comment - Hi Sam, After our discussion I went and had a look at the 1.9.0 code (I tried installing 1.9.0 but there are issues with MoodleQuickForm that I couldn't resolve in a sufficient amount of time). If you check out the code for 1.9.0 and look at mod/data/templates.php line 148 - 154 you can see that a default advanced search template is being generated with all of the other templates. I suspect that the code that Petr put in was to cover people upgrading from an earlier version of Moodle (say 1.7 where the advanced search template was not automatically generated) so that their presets would not break. I feel that this is a situation where we could clean up the code and remove this check for advanced search template presets that are pre 1.9.
          Hide
          Eloy Lafuente (stronk7) added a comment -
          • Without the patch, the advance search template is never get.
          • With the patch, all presets from 1.9 and upwards will get the search template.
          • With the patch, older presets (pre 1.9) or incomplete presets (missing some of the templates) will break with exception (because get_content() does that if the file is missing of it's not a file).

          So I think that, the best solution is to, simply, consider Adrian patch but... modifying a bit data_preset_get_file_contents() so, if the call to $file = $filestorage->get_file() returns false (the file does not exist), then NULL is returned and get_content() is not called.

          That way, the 3 cases above will work always, no matter if it's one old / custom present missing some template or no.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Without the patch, the advance search template is never get. With the patch, all presets from 1.9 and upwards will get the search template. With the patch, older presets (pre 1.9) or incomplete presets (missing some of the templates) will break with exception (because get_content() does that if the file is missing of it's not a file). So I think that, the best solution is to, simply, consider Adrian patch but... modifying a bit data_preset_get_file_contents() so, if the call to $file = $filestorage->get_file() returns false (the file does not exist), then NULL is returned and get_content() is not called. That way, the 3 cases above will work always, no matter if it's one old / custom present missing some template or no. Ciao
          Hide
          Sam Hemelryk added a comment -

          Cool thanks for looking Eloy

          Show
          Sam Hemelryk added a comment - Cool thanks for looking Eloy
          Hide
          Adrian Greeve added a comment -

          Thanks Eloy,

          I've worked over the patch and I feel now that I have covered all possible situations. It should now handle all three scenarios that you mentioned.

          Show
          Adrian Greeve added a comment - Thanks Eloy, I've worked over the patch and I feel now that I have covered all possible situations. It should now handle all three scenarios that you mentioned.
          Hide
          Sam Hemelryk added a comment -

          Thanks Adrian, has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Adrian, has been integrated now
          Hide
          Ankit Agarwal added a comment -

          This works as described!
          Cheers!

          Show
          Ankit Agarwal added a comment - This works as described! Cheers!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Y E S !

          Closing as fixed, many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: