Missing option to delete a wiki page

Any comments Ludo ? Thanks.

This is a duplicate of MDL-25696.

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

Dear oh dear...

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.

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.

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

Bumping up priority to get STABLE team attention

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

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

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.

looks good

Thanks Andrew, for all the inputs

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

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

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

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

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.

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.

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

Integrated thanks!

Passed thanks guys

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

Closing and ciao