r91561 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r91560‎ \| r91561 \| r91562 >
Date:	16:47, 6 July 2011
Author:	bawolff
Status:	reverted (Comments)
Tags:
Comment:	(Bug 19725) Do not include suppressed edits in the "View X deleted edits" message, and when doing prefix search of special:undelete. I'm not 100% sure this is the right thing to do, see the bug for the details. But basically this doesn't include an edit in the count if its text is hidden and its hidden from admins. (Not sure if it should not be included only if everything is hidden). Its also weird to show people different things depending if they have suppress rights, without really indicating that. Minor db note: This causes the query to no longer use a covering index. I don't think that matters but just thought i'd mention. p.s. The upload page show deleted edits link is broken right now, (from before) I'll fix in a follow-up.
Modified paths:	/trunk/phase3/RELEASE-NOTES-1.18 (modified) (history) /trunk/phase3/includes/Skin.php (modified) (history) /trunk/phase3/includes/SkinTemplate.php (modified) (history) /trunk/phase3/includes/Title.php (modified) (history) /trunk/phase3/includes/specials/SpecialUndelete.php (modified) (history) /trunk/phase3/includes/specials/SpecialUpload.php (modified) (history)

Diff [purge]

Index: trunk/phase3/RELEASE-NOTES-1.18
—	—	@@ -257,6 +257,8 @@
258	258	environments
259	259	* (bug 14977) Fixed $wgServer detection in cases where an IPv6 address is used
260	260	as the server name.
	261	+* (bug 19725) Do not list suppressed edits in the "View X deleted edits" link
	262	+ if user cannot view suppressed edits.
261	263
262	264	=== API changes in 1.18 ===
263	265	* (bug 26339) Throw warning when truncating an overlarge API result.
Index: trunk/phase3/includes/Title.php
—	—	@@ -2344,20 +2344,37 @@
2345	2345	/**
2346	2346	* Is there a version of this page in the deletion archive?
2347	2347	*
	2348	+ * @param $includeSuppressed Boolean Include suppressed revisions?
2348	2349	* @return Int the number of archived revisions
2349	2350	*/
2350		~~- public function isDeleted() {~~
	2351	+ public function isDeleted( $includeSuppressed = false ) {
2351	2352	if ( $this->getNamespace() < 0 ) {
2352	2353	$n = 0;
2353	2354	} else {
2354	2355	$dbr = wfGetDB( DB_SLAVE );
	2356	+ $conditions = array( 'ar_namespace' => $this->getNamespace(), 'ar_title' => $this->getDBkey() );
	2357	+
	2358	+ if( !$includeSuppressed ) {
	2359	+ $suppressedTextBits = REVISION::DELETED_TEXT \| REVISION::DELETED_RESTRICTED;
	2360	+ $conditions[] = $dbr->bitAnd('ar_deleted', $suppressedTextBits ) .
	2361	+ ' != ' . $suppressedTextBits;
	2362	+ }
	2363	+
2355	2364	$n = $dbr->selectField( 'archive', 'COUNT(*)',
2356		~~- array( 'ar_namespace' => $this->getNamespace(), 'ar_title' => $this->getDBkey() ),~~
	2365	+ $conditions,
2357	2366	__METHOD__
2358	2367	);
2359	2368	if ( $this->getNamespace() == NS_FILE ) {
2360		~~- $n += $dbr->selectField( 'filearchive', 'COUNT(*)',~~
2361		~~- array( 'fa_name' => $this->getDBkey() ),~~
	2369	+ $fconditions = array( 'fa_name' => $this->getDBkey() );
	2370	+ if( !$includeSuppressed ) {
	2371	+ $suppressedTextBits = FILE::DELETED_FILE \| FILE::DELETED_RESTRICTED;
	2372	+ $fconditions[] = $dbr->bitAnd('fa_deleted', $suppressedTextBits ) .
	2373	+ ' != ' . $suppressedTextBits;
	2374	+ }
	2375	+
	2376	+ $n += $dbr->selectField( 'filearchive',
	2377	+ 'COUNT(*)',
	2378	+ $fconditions,
2362	2379	__METHOD__
2363	2380	);
2364	2381	}
Index: trunk/phase3/includes/SkinTemplate.php
—	—	@@ -982,7 +982,8 @@
983	983	} else {
984	984	// article doesn't exist or is deleted
985	985	if ( $wgUser->isAllowed( 'deletedhistory' ) ) {
986		~~- $n = $title->isDeleted();~~
	986	+ $includeSuppressed = $wgUser->isAllowed( 'suppressrevision' );
	987	+ $n = $title->isDeleted( $includeSuppressed );
987	988	if( $n ) {
988	989	$undelTitle = SpecialPage::getTitleFor( 'Undelete' );
989	990	// If the user can't undelete but can view deleted history show them a "View .. deleted" tab instead
Index: trunk/phase3/includes/specials/SpecialUndelete.php
—	—	@@ -62,6 +62,7 @@
63	63	* @return ResultWrapper
64	64	*/
65	65	public static function listPagesByPrefix( $prefix ) {
	66	+ global $wgUser;
66	67	$dbr = wfGetDB( DB_SLAVE );
67	68
68	69	$title = Title::newFromText( $prefix );
—	—	@@ -77,6 +78,13 @@
78	79	'ar_namespace' => $ns,
79	80	'ar_title' . $dbr->buildLike( $prefix, $dbr->anyString() ),
80	81	);
	82	+
	83	+ // bug 19725
	84	+ $suppressedText = REVISION::DELETED_TEXT \| REVISION::DELETED_RESTRICTED;
	85	+ if( !$wgUser->isAllowed( 'suppressrevision' ) ) {
	86	+ $conds[] = $dbr->bitAnd('ar_deleted', $suppressedText ) .
	87	+ ' != ' . $suppressedText;
	88	+ }
81	89	return self::listPages( $dbr, $conds );
82	90	}
83	91
Index: trunk/phase3/includes/specials/SpecialUpload.php
—	—	@@ -314,17 +314,20 @@
315	315	$title = Title::makeTitleSafe( NS_FILE, $this->mDesiredDestName );
316	316	// Show a subtitle link to deleted revisions (to sysops et al only)
317	317	if( $title instanceof Title ) {
318		~~- $count = $title->isDeleted();~~
319		~~- if ( $count > 0 && $wgUser->isAllowed( 'deletedhistory' ) ) {~~
320		~~- $link = wfMsgExt(~~
321		~~- $wgUser->isAllowed( 'delete' ) ? 'thisisdeleted' : 'viewdeleted',~~
322		~~- array( 'parse', 'replaceafter' ),~~
323		~~- $this->getSkin()->linkKnown(~~
324		~~- SpecialPage::getTitleFor( 'Undelete', $title->getPrefixedText() ),~~
325		~~- wfMsgExt( 'restorelink', array( 'parsemag', 'escape' ), $count )~~
326		~~- )~~
327		~~- );~~
328		~~- $wgOut->addHTML( "<div id=\"contentSub2\">{$link}</div>" );~~
	318	+ if ( $wgUser->isAllowed( 'deletedhistory' ) ) {
	319	+ $canViewSuppress = $wgUser->isAllowed( 'suppressrevision' );
	320	+ $count = $title->isDeleted( $canViewSuppress );
	321	+ if ( $count > 0 ) {
	322	+ $link = wfMsgExt(
	323	+ $wgUser->isAllowed( 'delete' ) ? 'thisisdeleted' : 'viewdeleted',
	324	+ array( 'parse', 'replaceafter' ),
	325	+ $this->getSkin()->linkKnown(
	326	+ SpecialPage::getTitleFor( 'Undelete', $title->getPrefixedText() ),
	327	+ wfMsgExt( 'restorelink', array( 'parsemag', 'escape' ), $count )
	328	+ )
	329	+ );
	330	+ $wgOut->addHTML( "<div id=\"contentSub2\">{$link}</div>" );
	331	+ }
329	332	}
330	333	}
331	334
Index: trunk/phase3/includes/Skin.php
—	—	@@ -754,8 +754,10 @@
755	755
756	756	if ( $this->getContext()->getUser()->isAllowed( 'deletedhistory' ) &&
757	757	( $this->getTitle()->getArticleId() == 0 \|\| $action == 'history' ) ) {
758		~~- $n = $this->getTitle()->isDeleted();~~
759	758
	759	+ $includeSuppressed = $this->getContext()->getUser()->isAllowed( 'suppressrevision' );
	760	+ $n = $this->getTitle()->isDeleted( $includeSuppressed );
	761	+
760	762	if ( $n ) {
761	763	if ( $this->getContext()->getUser()->isAllowed( 'undelete' ) ) {
762	764	$msg = 'thisisdeleted';

Sign-offs

User	Flag	Date
Catrope	inspected	07:50, 20 August 2011

Follow-up revisions

Revision	Commit summary	Author	Date
r92551	Follow up to r91561: Use the canonical class names.	platonides	19:00, 19 July 2011
r92564	Follow up r91561. Use canonical class names.	platonides	20:06, 19 July 2011
r97297	Revert r91561	reedy	16:55, 16 September 2011
r97299	Revert r91561 out of REL1_18 (essentially merging r97297, but it wasn't, as i...	reedy	17:00, 16 September 2011

Comments

#Comment by Bawolff (talk | contribs) 16:51, 6 July 2011

adding tag 1.18 since relevant bug was a 1.18 blocker. (My personal opinion is its not really that important of a bug, and don't overly care either way if it gets backported)

#Comment by Aaron Schulz (talk | contribs) 00:10, 4 August 2011

Wait, page histories (for live revs) don't do this, but Undelete does?

#Comment by Aaron Schulz (talk | contribs) 20:03, 7 September 2011

Should the $suppressedText condition be in listPages()?

This should be reverted unless it's done everywhere at least (sp:Undelete list for a page, files too) and in regular revision history.

#Comment by Bawolff (talk | contribs) 21:56, 7 September 2011

I'm confused how this applies to regular page histories(?) What exactly do you think this should be doing for normal page histories that it isn't currently?

>Should the $suppressedText condition be in listPages()

Yes, that'd probably be a better place conceptually to put it (instead of listPagesByPrefix), I'll fix that in a follow up. I don't think there is any code path that where moving that bit would make a difference in the behaviour though.

#Comment by Aaron Schulz (talk | contribs) 22:05, 7 September 2011

listAllPages() calls listPages() AFAIK.

If we are ignoring suppressed revs from the deleted rev count, then they should also be visually hidden from sp:undelete or it's inconsistent. But if they are hidden at Undelete, they would still reappear as stubs (all greyed out) on restore, which is surprising to users. So you then have hide suppressed revisions from the history pages of live pages too.

#Comment by Bawolff (talk | contribs) 23:08, 7 September 2011

however listAllPages is never called, so it doesn't matter, but still for code making logical sense is should be in listPages. However I'll wait on committing a follow up until we sort out the other issue, since the other issue quite likely might result in reverting this anyways.

In reply to:

If we are ignoring suppressed revs from the deleted rev count, then they should also be visually hidden from sp:undelete or
it's inconsistent. But if they are hidden at Undelete, they would still reappear as stubs (all greyed out) on restore, which
is surprising to users. So you then have hide suppressed revisions from the history pages of live pages too.

That's basically why I said "I'm not 100% sure this is the right thing to do" in the commit summary. I still think its a good idea to show greyed out stubs for oversighted things. People wanted the ability to make the "view X deleted revisions" to go away when oversighting pages with sensitive titles. Making the view X deleted revisions only count non-oversighted revisions seemed like a possible compromise, allowing hiding a page name ever existed, while still showing the relevant list of (grey-ed out) deleted revisions. After all if there is 3 revisions, and one has been revdeleted, the user can really only view 2 of them.

I'm not really attached to this revision, and wouldn't object to it being reverted on the basis of it being a bad idea (if you think doing this is a bad idea that is).

#Comment by Aaron Schulz (talk | contribs) 23:36, 15 September 2011

Recommending revert. I'll get around to it when I get a chance.

#Comment by Reedy (talk | contribs) 16:50, 16 September 2011

Rarrrrgh

Everything but the RELEASE-NOTES conflict

Status & tagging log

16:55, 16 September 2011 Reedy (talk | contribs) changed the tags for r91561 [removed: design]
16:55, 16 September 2011 Reedy (talk | contribs) changed the status of r91561 [removed: fixme added: reverted]
20:03, 7 September 2011 Aaron Schulz (talk | contribs) changed the status of r91561 [removed: new added: fixme]
22:27, 24 August 2011 Catrope (talk | contribs) changed the tags for r91561 [removed: frontend]
20:25, 23 August 2011 MarkAHershberger (talk | contribs) changed the tags for r91561 [added: frontend,design]
21:23, 18 July 2011 Reedy (talk | contribs) changed the tags for r91561 [removed: 1.18]
16:51, 6 July 2011 Bawolff (talk | contribs) changed the tags for r91561 [added: 1.18]