Moodle
  1. Moodle
  2. MDL-23520

Missing option to delete a wiki page

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Wiki (2.x)
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      PLEASE TEST THIS UNDER 21_STABLE

      1. Log in as teacher/admin
      2. Select a course and turn editing on
      3. Add a Wiki activity to course
      4. Select wiki and click on create page
      5. Create 4 versions of the page (Repeat below steps 4 times)

      • Click edit
      • Modify text
      • click Save
        6. Add 1 pages
      • Click edit
      • Add [[New page title]]) in page text
      • click save
      • "New page title" link should be visible.
      • Click on "New page title"
        7. Create 4 versions of this page as well. (Same as step 5)
        8. Go back to course and click on recently created wiki activity.
        9. Click on Administration, and select "Delete page versions"
        10. Select both radio buttons for version 3 and click "Delete page versions". (version 3 should no longer be visible)
        11. Select version 2 and version 5 (or the last version which was created) and click "Delete page versions" (Only 1 version should be visible and no "delete page version should be visible")
        12. Go back to course and click on recently created wiki activity.
        13. Click on Administration
        14. "New page title" page should be listed as "orphan page"
        15. Click on Delete icon "x" and page should no longer be visible
        16. Now click on "list all", it should list all pages in the wiki except "New page title"
        13. Now delete all pages, by clicking on "x" (Similar to step 15)
        14. when last page is deleted, it should show "edit page" with the option of creating page
        15. Log in as student and you should not see Administration option and should not be able to delete any wiki by passing the delete link
      Show
      PLEASE TEST THIS UNDER 21_STABLE 1. Log in as teacher/admin 2. Select a course and turn editing on 3. Add a Wiki activity to course 4. Select wiki and click on create page 5. Create 4 versions of the page (Repeat below steps 4 times) Click edit Modify text click Save 6. Add 1 pages Click edit Add [ [New page title] ]) in page text click save "New page title" link should be visible. Click on "New page title" 7. Create 4 versions of this page as well. (Same as step 5) 8. Go back to course and click on recently created wiki activity. 9. Click on Administration, and select "Delete page versions" 10. Select both radio buttons for version 3 and click "Delete page versions". (version 3 should no longer be visible) 11. Select version 2 and version 5 (or the last version which was created) and click "Delete page versions" (Only 1 version should be visible and no "delete page version should be visible") 12. Go back to course and click on recently created wiki activity. 13. Click on Administration 14. "New page title" page should be listed as "orphan page" 15. Click on Delete icon "x" and page should no longer be visible 16. Now click on "list all", it should list all pages in the wiki except "New page title" 13. Now delete all pages, by clicking on "x" (Similar to step 15) 14. when last page is deleted, it should show "edit page" with the option of creating page 15. Log in as student and you should not see Administration option and should not be able to delete any wiki by passing the delete link
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      wip-mdl-23520
    • Rank:
      17115

      Description

      It looks like the functionality to delete ANY wiki page or JUST orphaned pages has not been added yet to Moodle 2.0 .

      Is there any chance to get it soon ? Thanks.

        Issue Links

          Activity

          Hide
          Philippe Siwinski added a comment -

          Any comments Ludo ? Thanks.

          Show
          Philippe Siwinski added a comment - Any comments Ludo ? Thanks.
          Hide
          Jens Jahnke added a comment -

          This is a duplicate of MDL-25696.

          Show
          Jens Jahnke added a comment - This is a duplicate of MDL-25696 .
          Hide
          Helen Foster added a comment -

          Increasing priority as this is lost functionality from 1.9 to 2.0.

          Show
          Helen Foster added a comment - Increasing priority as this is lost functionality from 1.9 to 2.0.
          Hide
          Mark Drechsler added a comment -

          Dear oh dear...

          Show
          Mark Drechsler added a comment - Dear oh dear...
          Hide
          Peter Ruthven-Stuart added a comment -

          Dear oh dear indeed. This issue coupled with MDL-27037 (wiki doesn't work with Groups) is making me feel nostalgic for the standard 1.9 Wiki.

          Show
          Peter Ruthven-Stuart added a comment - Dear oh dear indeed. This issue coupled with MDL-27037 (wiki doesn't work with Groups) is making me feel nostalgic for the standard 1.9 Wiki.
          Hide
          Rob Nielson added a comment -

          This bug still exists in 2.1 as well. Hopefully this will get fixed soon. Wow. Can't believe something this important got left out.

          Show
          Rob Nielson added a comment - This bug still exists in 2.1 as well. Hopefully this will get fixed soon. Wow. Can't believe something this important got left out.
          Hide
          Helen Foster added a comment -

          Thanks everyone for your comments. Increasing priority again and hoping this issue can be fixed asap.

          Show
          Helen Foster added a comment - Thanks everyone for your comments. Increasing priority again and hoping this issue can be fixed asap.
          Hide
          Martin Dougiamas added a comment -

          Bumping up priority to get STABLE team attention

          Show
          Martin Dougiamas added a comment - Bumping up priority to get STABLE team attention
          Hide
          Rajesh Taneja added a comment -
          1. Replicated delete feature in wiki, as it was in 1.9.
          2. Deleting page/version will delete wiki page/version record from database (As it was in 1.9).
          3. Code added is based on current wiki design
          Show
          Rajesh Taneja added a comment - Replicated delete feature in wiki, as it was in 1.9. Deleting page/version will delete wiki page/version record from database (As it was in 1.9). Code added is based on current wiki design
          Hide
          Andrew Davis added a comment -

          mod/wiki/admin.php is there are reason for choosing this name when all it does is delete? admin.php would suggest that it is a general purpose page when all it does is delete stuff.

          $delete = optional_param('delete', 0, PARAM_INT); // page id ID
          

          the comment at the end just needs to be corrected

          // Delete page if it's a delete option
          This comment isn't quite right. Perhaps something like "Delete page if a page ID to delete was supplied" or something like that.

          Within the code that actually does the delete, is the logic correct?

          //if first page is deleted then redirect for creating this new one...
          if ($pageid == $delete) {
          	$params = array('swid' => $wiki->id, 'title' => $wiki->firstpagetitle);
          	$url = new moodle_url('/mod/wiki/create.php', $params);
          	redirect($url);
              }
          

          As I understand that that says "if the current page ID equals the page ID to delete go create the wiki's default page. Either I am misunderstanding or something isn't quite right. Please either alter the code or add a comment to make it more clear what is going on.

          Are you also aware that if deleting occurs the purging of the versions will never happen as the user is redirected before then? Is that intentional?

          Regarding $vcount and $totalversionstodel as a rule don't use abbreviations or acronyms in Moodle code. I know we do in some places. Things like shortening delete to del makes life unnecessarily hard for anyone who may not be a strong English speaker. Similarly

          // vcount is the latest version
          $vcount = wiki_count_wiki_page_versions($pageid) - 1;
          

          Rename $vcount to $versioncount or something else that isnt an abbreviation.

          $strippage = optional_param('strippage', 0, PARAM_ALPHANUM);
          

          What does this variable do? Theres no comment and its name means nothing.

          $string['incorrectstripversions'] = "Striping versions provided are incorrect.";
          

          You want stripping with 2 p's. Also, what do you mean by stripping anyway? Do you just mean purging? Was the word "strip" used in Moodle 1.9? If you mean purge, delete or remove and you havent used the word "strip" specifically because thats what was used in 1.9 use a more common word. Don't introduce new synonyms as its just confusing

          $string['strippages'] = 'Strips all versions selcted between first and second radio buttons';
          

          The word "selected" is mis-spelt. The string also doesn't really make sense. You remove all versions between two versions, not between two radio buttons.

          I know theres still some lurking in old code but dont introduce new triple slash php comments. http://docs.moodle.org/dev/Coding_style#Documentation_and_comments

          The following comments contain spelling errors
          this should dispaly administration view for required users.
          Delete specificed versions of a page or versions created by users"
          //Delete all pae versions
          //dispaly admin menu
          // version to pruge to

          if ($user) {
          	$params['userid'] = $version;
          } else if ($version != 0) {
          	//If version = 0, then remove all versions of this page
          	$params['version'] = $version;
          }
          

          Is that correct? Are you meant to be assigning $version to both userid and version?

          //Make sure anyone who try to access this page should have managewiki permissions
          Should be "make sure anyone trying to access this page has managewiki capabilities"

          /**
          * Sets admin view option
          * @param int $view 1 is delete page view and 2 is strip version view
          * @param bool $listorphan is only valid for view 1.
          */
          public function set_view($view, $listorphan = true) {
          

          The comment for $view makes no sense

          In print_purge_content() you're instantiating stdclass. Do it like this. $row = new stdClass(); (you're missing the brackets and have capitalized the s) http://docs.moodle.org/dev/Coding_style#Classes

          Show
          Andrew Davis added a comment - mod/wiki/admin.php is there are reason for choosing this name when all it does is delete? admin.php would suggest that it is a general purpose page when all it does is delete stuff. $delete = optional_param('delete', 0, PARAM_INT); // page id ID the comment at the end just needs to be corrected // Delete page if it's a delete option This comment isn't quite right. Perhaps something like "Delete page if a page ID to delete was supplied" or something like that. Within the code that actually does the delete, is the logic correct? // if first page is deleted then redirect for creating this new one... if ($pageid == $delete) { $params = array('swid' => $wiki->id, 'title' => $wiki->firstpagetitle); $url = new moodle_url('/mod/wiki/create.php', $params); redirect($url); } As I understand that that says "if the current page ID equals the page ID to delete go create the wiki's default page. Either I am misunderstanding or something isn't quite right. Please either alter the code or add a comment to make it more clear what is going on. Are you also aware that if deleting occurs the purging of the versions will never happen as the user is redirected before then? Is that intentional? Regarding $vcount and $totalversionstodel as a rule don't use abbreviations or acronyms in Moodle code. I know we do in some places. Things like shortening delete to del makes life unnecessarily hard for anyone who may not be a strong English speaker. Similarly // vcount is the latest version $vcount = wiki_count_wiki_page_versions($pageid) - 1; Rename $vcount to $versioncount or something else that isnt an abbreviation. $strippage = optional_param('strippage', 0, PARAM_ALPHANUM); What does this variable do? Theres no comment and its name means nothing. $string['incorrectstripversions'] = "Striping versions provided are incorrect." ; You want stripping with 2 p's. Also, what do you mean by stripping anyway? Do you just mean purging? Was the word "strip" used in Moodle 1.9? If you mean purge, delete or remove and you havent used the word "strip" specifically because thats what was used in 1.9 use a more common word. Don't introduce new synonyms as its just confusing $string['strippages'] = 'Strips all versions selcted between first and second radio buttons'; The word "selected" is mis-spelt. The string also doesn't really make sense. You remove all versions between two versions, not between two radio buttons. I know theres still some lurking in old code but dont introduce new triple slash php comments. http://docs.moodle.org/dev/Coding_style#Documentation_and_comments The following comments contain spelling errors this should dispaly administration view for required users. Delete specificed versions of a page or versions created by users" //Delete all pae versions //dispaly admin menu // version to pruge to if ($user) { $params['userid'] = $version; } else if ($version != 0) { //If version = 0, then remove all versions of this page $params['version'] = $version; } Is that correct? Are you meant to be assigning $version to both userid and version? //Make sure anyone who try to access this page should have managewiki permissions Should be "make sure anyone trying to access this page has managewiki capabilities" /** * Sets admin view option * @param int $view 1 is delete page view and 2 is strip version view * @param bool $listorphan is only valid for view 1. */ public function set_view($view, $listorphan = true ) { The comment for $view makes no sense In print_purge_content() you're instantiating stdclass. Do it like this. $row = new stdClass(); (you're missing the brackets and have capitalized the s) http://docs.moodle.org/dev/Coding_style#Classes
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew for detailed feedback
          I have incorporated few of your suggestions, it will be nice if you can peer review it again for me.

          Feedback which was integrated:
          1. Modification of variable names.
          2. Comments are modified as per your suggestion
          3. Spelling mistake is corrected.
          4. Replaced strip/purge with delete version.

          Legacy things:
          #File was named admin.php and section is called administration.
          #Strip/purge was used in 1.9, but now feel it's too misleading so simplified it to delete version.

          FYI:
          Deleting page will delete all related data for that page, like versions, links etc.

          Show
          Rajesh Taneja added a comment - Thanks Andrew for detailed feedback I have incorporated few of your suggestions, it will be nice if you can peer review it again for me. Feedback which was integrated: 1. Modification of variable names. 2. Comments are modified as per your suggestion 3. Spelling mistake is corrected. 4. Replaced strip/purge with delete version. Legacy things: #File was named admin.php and section is called administration. #Strip/purge was used in 1.9, but now feel it's too misleading so simplified it to delete version. FYI: Deleting page will delete all related data for that page, like versions, links etc.
          Hide
          Andrew Davis added a comment -

          looks good

          Show
          Andrew Davis added a comment - looks good
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew, for all the inputs

          Show
          Rajesh Taneja added a comment - Thanks Andrew, for all the inputs
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, some little things that we should be controlling better:

          1) https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-23520#L0R78 : It seems to assume that version numbers are always consecutive, and I think that could lead to wrong situations. Imagine one page with 10 versions, you delete versions 3 to 8 (ok) and later you try to delete versions 2 to 10. Does that work?

          2) https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-23520#L3R1030 : Seems to be a really dangerous function. Passing no params will empty the whole table! Better control of alternatives should be given.

          3) https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-23520#L3R1058 : Same logic than the previous point. While here it is NOT dangerous, because nothing becomes deleted, the alternatives must be controlled better.

          4) Does it work with javascript disabled?

          5) https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-23520#L5R2328 : switch/case indentation is wrong, or I'm wrong, lol. Perhaps some constants could be better than 1,2 there?

          6) Some "English" points I'm not 100% sure:

          • Is "strip" (both in keyword and content) correct here? Shouldn't be simply, "delete" (page, versions, whatever). Same with "remove" vs "delete".
          • 'listorphan' = 'List candidates' seems a bit wrong, should match better.
          • 'deleteversion' = 'Delete page versions' also requires singular/plural matching.

          7) Surely we are missing various things on deletion. What happens with any of: comments, ratings, tags, files... anything pointing to one page or version should be deleted too.

          Rejecting. +1 for next week. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, some little things that we should be controlling better: 1) https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-23520#L0R78 : It seems to assume that version numbers are always consecutive, and I think that could lead to wrong situations. Imagine one page with 10 versions, you delete versions 3 to 8 (ok) and later you try to delete versions 2 to 10. Does that work? 2) https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-23520#L3R1030 : Seems to be a really dangerous function. Passing no params will empty the whole table! Better control of alternatives should be given. 3) https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-23520#L3R1058 : Same logic than the previous point. While here it is NOT dangerous, because nothing becomes deleted, the alternatives must be controlled better. 4) Does it work with javascript disabled? 5) https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-23520#L5R2328 : switch/case indentation is wrong, or I'm wrong, lol. Perhaps some constants could be better than 1,2 there? 6) Some "English" points I'm not 100% sure: Is "strip" (both in keyword and content) correct here? Shouldn't be simply, "delete" (page, versions, whatever). Same with "remove" vs "delete". 'listorphan' = 'List candidates' seems a bit wrong, should match better. 'deleteversion' = 'Delete page versions' also requires singular/plural matching. 7) Surely we are missing various things on deletion. What happens with any of: comments, ratings, tags, files... anything pointing to one page or version should be deleted too. Rejecting. +1 for next week. Ciao
          Hide
          Rajesh Taneja added a comment -

          Thanks for feedback Eloy.

          For point 1:
          It will work as we try delete all versions between two specified versions. If 3-8 are deleted previously, then while deleting 2-10 it will try deleting 3-8 versions and as they are not present, nothing will be done for those versions. I can add a check to make sure if specified version is not available then nothing should be done.

          Rest of the points you raised are quite serious and I will update the code to rectify them.

          Thanks again for explaining all issues in detail. It helps to pin point issues which were missed in initial implementation

          Show
          Rajesh Taneja added a comment - Thanks for feedback Eloy. For point 1: It will work as we try delete all versions between two specified versions. If 3-8 are deleted previously, then while deleting 2-10 it will try deleting 3-8 versions and as they are not present, nothing will be done for those versions. I can add a check to make sure if specified version is not available then nothing should be done. Rest of the points you raised are quite serious and I will update the code to rectify them. Thanks again for explaining all issues in detail. It helps to pin point issues which were missed in initial implementation
          Hide
          Rajesh Taneja added a comment - - edited

          Integrated Eloy's feedback.

          Following will not be deleted:

          1. File: deletion is not possible for page, as files are uploaded in activity context and page context.
          2. Rank: is not available in wiki.

          Deleting:

          1. versions
          2. comments
          3. tags
          4. locks
          5. links
          6. Synonyms
          Show
          Rajesh Taneja added a comment - - edited Integrated Eloy's feedback. Following will not be deleted: File: deletion is not possible for page, as files are uploaded in activity context and page context. Rank: is not available in wiki. Deleting: versions comments tags locks links Synonyms
          Hide
          Andrew Davis added a comment -

          Just did some testing and when you view the delete page versions page the table is centered until you delete every version but one. Then the table leaps over to the left. I'm using the theme "Standard" just in case that is relevant.

          The testing instructions still reference the "strip versions" strings. Update them to reflect the current strings.

          Also in the testing instructions

          3. Create wiki activity
          4. Add page to the wiki
          

          Make it clear that this is adding a page in addition to the first page that the wiki forces you to create. You can't delete the wiki's first page (I believe) so step 4 needs to be the creation of a second page for the wiki.

          Again in the testing instructions avoid saying "multiple times" and "add more pages". Provide a specific number ie add 1 more page or add 3 more pages. Don't make the tester guess how many they need.

          11. Try delete this page.
          12. Now click on "list all", it should list all pages in the wiki
          

          What should happen when you try to delete the page? Don't tell the tester to do something but then not tell them what should happen. Let them know if the page should be removed from the listing, you should get and error that says "blah" or whatever.

          Both print_delete_version() and print_content() say they use the global $PAGE but actually don't.

          Given that wiki is going to be replaced its probably not worth worrying about but choose_from_radio() is pretty nasty. Html in literal strings and even a sprintf() call instead of using html_writer. Probably just leave that alone however.

          Show
          Andrew Davis added a comment - Just did some testing and when you view the delete page versions page the table is centered until you delete every version but one. Then the table leaps over to the left. I'm using the theme "Standard" just in case that is relevant. The testing instructions still reference the "strip versions" strings. Update them to reflect the current strings. Also in the testing instructions 3. Create wiki activity 4. Add page to the wiki Make it clear that this is adding a page in addition to the first page that the wiki forces you to create. You can't delete the wiki's first page (I believe) so step 4 needs to be the creation of a second page for the wiki. Again in the testing instructions avoid saying "multiple times" and "add more pages". Provide a specific number ie add 1 more page or add 3 more pages. Don't make the tester guess how many they need. 11. Try delete this page. 12. Now click on "list all" , it should list all pages in the wiki What should happen when you try to delete the page? Don't tell the tester to do something but then not tell them what should happen. Let them know if the page should be removed from the listing, you should get and error that says "blah" or whatever. Both print_delete_version() and print_content() say they use the global $PAGE but actually don't. Given that wiki is going to be replaced its probably not worth worrying about but choose_from_radio() is pretty nasty. Html in literal strings and even a sprintf() call instead of using html_writer. Probably just leave that alone however.
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew

          Delete page version table show exactly what history option shows. Just to keep things similar, I have kept that behavior.

          Modified test instructions and removed redundant $PAGE and $CFG call.

          Show
          Rajesh Taneja added a comment - Thanks Andrew Delete page version table show exactly what history option shows. Just to keep things similar, I have kept that behavior. Modified test instructions and removed redundant $PAGE and $CFG call.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated thanks!

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

          Passed thanks guys

          Show
          Sam Hemelryk added a comment - Passed thanks guys
          Hide
          Eloy Lafuente (stronk7) added a comment -

          git & cvs repositories updated with your gorgeous code. Many thanks!

          Closing and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - git & cvs repositories updated with your gorgeous code. Many thanks! Closing and ciao

            People

            • Votes:
              36 Vote for this issue
              Watchers:
              21 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: