Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-29086

deleting database activity preset is not possible

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      1) Login as admin
      2) Create a database activity
      3) Add one Database Field
      4) Create one template (just click save template as soon as you enter the form)
      5) Click the Presets tab
      6) Go to the 'Save as preset' section and click continue
      [TEST] After clicking continue and returning to the Presets tab check to make sure that your preset is now present in the 'Use a preset' section and that there is an X located next to it to delete it.

      [Please note that if you have any presets that you created before this fix they will not have an X next to them]

      7) Delete the preset.
      [TEST] make sure that the preset has been deleted.

      Show
      1) Login as admin 2) Create a database activity 3) Add one Database Field 4) Create one template (just click save template as soon as you enter the form) 5) Click the Presets tab 6) Go to the 'Save as preset' section and click continue [TEST] After clicking continue and returning to the Presets tab check to make sure that your preset is now present in the 'Use a preset' section and that there is an X located next to it to delete it. [Please note that if you have any presets that you created before this fix they will not have an X next to them] 7) Delete the preset. [TEST] make sure that the preset has been deleted.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-29086-master

      Description

      If you use "Save as preset" there is no way to delete this preset. Tested as Admin and Manager. It seems that mod/data:manageuserpresets didn't work.

      Replication steps:

      1. Login as Admin / Manager
      2. Create Database activity
      3. Add one Database Field
      4. Create one template (like Add template)
      5. Save this template
      6. goto Presets
      7. Save as preset

      Now you can see and chose the preset, but there is no way to deleting it.

        Gliffy Diagrams

        1. mdl-29086-no-delete.patch
          0.7 kB
          Matthias Hunstock
        2. mdl-29086-no-userid.patch
          0.7 kB
          Matthias Hunstock
        1. Preset_no_deleting.png
          7 kB

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this.

            I've put that on the backlog.

            In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.
            Hide
            salvetore Michael de Raadt added a comment -

            A comment from the duplicate bug:

            "As far as I can tell from a quick investigation, sharing a preset by 'Save as preset' should but doesn't register the user id of the saver. The display of the delete button is conditioned by user id but since the user id is 0, the button is not displayed."

            Show
            salvetore Michael de Raadt added a comment - A comment from the duplicate bug: "As far as I can tell from a quick investigation, sharing a preset by 'Save as preset' should but doesn't register the user id of the saver. The display of the delete button is conditioned by user id but since the user id is 0, the button is not displayed."
            Hide
            atze Matthias Hunstock added a comment -

            I can confirm the previous comment:
            if the column "userid" in table "mdl_files" is updated to the ID of the user who created the preset then the delete button is shown

            BUT

            the button doesn't work correctly. When pressing it, there is the confirmation page, on confirming the deletion it prints "deleted successfully", but the preset ist still there.

            HTH

            Show
            atze Matthias Hunstock added a comment - I can confirm the previous comment: if the column "userid" in table "mdl_files" is updated to the ID of the user who created the preset then the delete button is shown BUT the button doesn't work correctly. When pressing it, there is the confirmation page, on confirming the deletion it prints "deleted successfully", but the preset ist still there. HTH
            Hide
            atze Matthias Hunstock added a comment -

            In line 187 of mod/data/preset.php there is a call to "fulldelete" (in /lib/filelib.php) which tries to delete the preset from /var/moodledata/data/preset/<id>/<name> but in a fresh install I do not even have /var/moodledata/data. The presets do instead reside in /var/moodledata/filedir. Seems like there was an incomplete API change or something.

            Show
            atze Matthias Hunstock added a comment - In line 187 of mod/data/preset.php there is a call to "fulldelete" (in /lib/filelib.php) which tries to delete the preset from /var/moodledata/data/preset/<id>/<name> but in a fresh install I do not even have /var/moodledata/data. The presets do instead reside in /var/moodledata/filedir. Seems like there was an incomplete API change or something.
            Hide
            atze Matthias Hunstock added a comment -

            Patches the missing userid in the table mdl_files when creating a preset with mod/data.

            Show
            atze Matthias Hunstock added a comment - Patches the missing userid in the table mdl_files when creating a preset with mod/data.
            Hide
            atze Matthias Hunstock added a comment -

            patches a missing function call to delete database presets

            Show
            atze Matthias Hunstock added a comment - patches a missing function call to delete database presets
            Hide
            abgreeve Adrian Greeve added a comment -

            Thanks to Matthias for putting in the hard work for this. I noticed that data_delete_site_preset() was actually just looking for a preset name and not a full file path. I've fixed that. Now heading to peer review.

            Show
            abgreeve Adrian Greeve added a comment - Thanks to Matthias for putting in the hard work for this. I noticed that data_delete_site_preset() was actually just looking for a preset name and not a full file path. I've fixed that. Now heading to peer review.
            Hide
            poltawski Dan Poltawski added a comment -

            Makes sense and looks good please submit for integration!

            As we just discussed the test instructions need a tweak to point out that if the preset existed before the change then they won't have the delete option. Creating another issue about the fact you can overwrite any preset..

            Show
            poltawski Dan Poltawski added a comment - Makes sense and looks good please submit for integration! As we just discussed the test instructions need a tweak to point out that if the preset existed before the change then they won't have the delete option. Creating another issue about the fact you can overwrite any preset..
            Hide
            nebgor Aparup Banerjee added a comment - - edited

            Adrian, the author should be you here since you created this git commmit (even if verbatim). Credits can be mentioned in commit messages.

            Show
            nebgor Aparup Banerjee added a comment - - edited Adrian, the author should be you here since you created this git commmit (even if verbatim). Credits can be mentioned in commit messages.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            So, just for clarification, this only enables deletion of new presets which have userid defined, correct?

            And the linked MDL-31763 will solve BOTH the (potential) overwrite problem AND the deletion of presets without userid assigned.

            Plz, confirm. I'll be happy to integrate this once the Q above is clarified.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited So, just for clarification, this only enables deletion of new presets which have userid defined, correct? And the linked MDL-31763 will solve BOTH the (potential) overwrite problem AND the deletion of presets without userid assigned. Plz, confirm. I'll be happy to integrate this once the Q above is clarified. Ciao
            Hide
            abgreeve Adrian Greeve added a comment -

            Yes, that is correct. Only if the userid has been defined will it allow for the preset to be deleted. MDL-31763 was created to fix presets being overwritten by anyone. I think that it would be a good idea to have the ability to delete any old presets that exist as well. I'll leave a comment says as much on that issue.

            Thanks,

            Show
            abgreeve Adrian Greeve added a comment - Yes, that is correct. Only if the userid has been defined will it allow for the preset to be deleted. MDL-31763 was created to fix presets being overwritten by anyone. I think that it would be a good idea to have the ability to delete any old presets that exist as well. I'll leave a comment says as much on that issue. Thanks,
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Thanks, about to integrate this now...

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Thanks, about to integrate this now...
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks! (21, 22 & master)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (21, 22 & master)
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Eloy,
            I guess that you forgot to integrate it to 21? it works fine in master and 22. I cannot find the commit in the 21 logs. Please have a look.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Eloy, I guess that you forgot to integrate it to 21? it works fine in master and 22. I cannot find the commit in the 21 logs. Please have a look. Thanks
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Eloy for sending in the commit.
            All good now.
            Passing
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Eloy for sending in the commit. All good now. Passing Thanks
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!).

            icao_reverse('arreis olik rebemevon afla letoh ognat');

            Closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!). icao_reverse('arreis olik rebemevon afla letoh ognat'); Closing, ciao

              People

              • Votes:
                9 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Mar/12