r77302 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r77301‎ \| r77302 \| r77303 >
Date:	21:39, 25 November 2010
Author:	catrope
Status:	ok
Tags:
Comment:	(bug 26014) Allow users to strike their own sign-offs. Implemented with a timestamp_struck field which contains either the timestamp the sign-off was struck or Block::infinity(). This allows us to use the UNIQUE INDEX (which was extended to cover that field too) to prevent duplicate sign-offs and at the same time allow the same sign-off (i.e. same user, revision and flag) to be struck and resubmitted multiple times. The UI mostly resembles the mockup Trevor attached to the bug, but it still needs a bit of CSS love. Documentation of the code I added will follow in a separate commit.
Modified paths:	/trunk/extensions/CodeReview/CodeReview.i18n.php (modified) (history) /trunk/extensions/CodeReview/CodeReview.php (modified) (history) /trunk/extensions/CodeReview/archives/code_signoffs_timestamp_struck.sql (added) (history) /trunk/extensions/CodeReview/backend/CodeRevision.php (modified) (history) /trunk/extensions/CodeReview/backend/CodeSignoff.php (modified) (history) /trunk/extensions/CodeReview/codereview.css (modified) (history) /trunk/extensions/CodeReview/codereview.sql (modified) (history) /trunk/extensions/CodeReview/ui/CodeRevisionCommitter.php (modified) (history) /trunk/extensions/CodeReview/ui/CodeRevisionView.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CodeReview/CodeReview.i18n.php
—	—	@@ -104,11 +104,14 @@
105	105	'code-signoffs' => 'Sign-offs',
106	106	'code-signoff-legend' => 'Add sign-off',
107	107	'code-signoff-submit' => 'Sign off',
	108	+ 'code-signoff-strike' => 'Strike selected sign-offs',
	109	+ 'code-signoff-signoff' => 'Sign off on this revision:',
108	110	'code-signoff-flag-inspected' => 'Inspected',
109	111	'code-signoff-flag-tested' => 'Tested',
110	112	'code-signoff-field-user' => 'User',
111	113	'code-signoff-field-flag' => 'Flag',
112	114	'code-signoff-field-date' => 'Date',
	115	+ 'code-signoff-struckdate' => '$1 (struck $2)',
113	116	'code-pathsearch-legend' => 'Search revisions in this repo by path',
114	117	'code-pathsearch-path' => 'Path:',
115	118	'code-pathsearch-filter' => 'Filter applied:',
Index: trunk/extensions/CodeReview/CodeReview.php
—	—	@@ -199,12 +199,16 @@
200	200
201	201	$updater->addExtensionUpdate( array( 'addField', 'code_signoffs', 'cs_user',
202	202	"$base/archives/code_signoffs_userid.sql", true ) );
	203	+ $updater->addExtensionUpdate( array( 'addField', 'code_signoffs', 'cs_timestamp_struck',
	204	+ "$base/archives/code_signoffs_timestamp_struck.sql", true ) );
203	205	break;
204	206	case 'sqlite':
205	207	$updater->addExtensionUpdate( array( 'addTable', 'code_rev', "$base/codereview.sql", true ) );
206	208	$updater->addExtensionUpdate( array( 'addTable', 'code_signoffs', "$base/archives/code_signoffs.sql", true ) );
207	209	$updater->addExtensionUpdate( array( 'addField', 'code_signoffs', 'cs_user',
208	210	"$base/archives/code_signoffs_userid-sqlite.sql", true ) );
	211	+ $updater->addExtensionUpdate( array( 'addField', 'code_signoffs', 'cs_timestamp_struck',
	212	+ "$base/archives/code_signoffs_timestamp_struck.sql", true ) );
209	213	break;
210	214	case 'postgres':
211	215	// TODO
Index: trunk/extensions/CodeReview/codereview.css
—	—	@@ -99,6 +99,15 @@
100	100	color: #666;
101	101	}
102	102
	103	+.mw-codereview-signoffchecks {
	104	+ float: right;
	105	+}
	106	+
	107	+.mw-codereview-struck td {
	108	+ text-decoration: line-through;
	109	+ background: #aaa;
	110	+}
	111	+
103	112	.mw-codereview-success {
104	113	color: #1a2;
105	114	}
Index: trunk/extensions/CodeReview/archives/code_signoffs_timestamp_struck.sql
—	—	@@ -0,0 +1,4 @@
	2	+ALTER TABLE /_/code_signoffs
	3	+ ADD COLUMN cs_timestamp_struck varbinary(14) not null default 'infinity';
	4	+DROP INDEX /i/cs_repo_rev_user_flag ON /_/code_signoffs;
	5	+CREATE UNIQUE INDEX /i/cs_repo_rev_user_flag_tstruck ON /_/code_signoffs (cs_repo_id, cs_rev_id, cs_user_text, cs_flag, cs_timestamp_struck);
Property changes on: trunk/extensions/CodeReview/archives/code_signoffs_timestamp_struck.sql
___________________________________________________________________
Added: svn:eol-style
1	6	+ native
Index: trunk/extensions/CodeReview/backend/CodeRevision.php
—	—	@@ -632,7 +632,7 @@
633	633	public function getSignoffs( $from = DB_SLAVE ) {
634	634	$db = wfGetDB( $from );
635	635	$result = $db->select( 'code_signoffs',
636		~~- array( 'cs_user', 'cs_user_text', 'cs_flag', 'cs_timestamp' ),~~
	636	+ array( 'cs_user', 'cs_user_text', 'cs_flag', 'cs_timestamp', 'cs_timestamp_struck' ),
637	637	array(
638	638	'cs_repo_id' => $this->mRepoId,
639	639	'cs_rev_id' => $this->mId,
—	—	@@ -664,11 +664,22 @@
665	665	'cs_user_text' => $user->getName(),
666	666	'cs_flag' => $flag,
667	667	'cs_timestamp' => $dbw->timestamp(),
	668	+ 'cs_timestamp_struck' => Block::infinity()
668	669	);
669	670	}
670	671	$dbw->insert( 'code_signoffs', $rows, __METHOD__, array( 'IGNORE' ) );
671	672	}
672	673
	674	+ public function strikeSignoffs( $user, $ids ) {
	675	+ foreach ( $ids as $id ) {
	676	+ $signoff = CodeSignoff::newFromId( $this, $id );
	677	+ // Only allow striking own signoffs
	678	+ if ( $signoff && $signoff->userText === $user->getName() ) {
	679	+ $signoff->strike();
	680	+ }
	681	+ }
	682	+ }
	683	+
673	684	public function getTags( $from = DB_SLAVE ) {
674	685	$db = wfGetDB( $from );
675	686	$result = $db->select( 'code_tags',
Index: trunk/extensions/CodeReview/backend/CodeSignoff.php
—	—	@@ -1,22 +1,74 @@
2	2	<?php
3	3	class CodeSignoff {
4	4	public $rev, $user, $flag, $timestamp, $userText;
	5	+ private $timestampStruck;
5	6
6		~~- public function __construct( $rev, $user, $userText, $flag, $timestamp ) {~~
	7	+ public function __construct( $rev, $user, $userText, $flag, $timestamp, $timestampStruck ) {
7	8	$this->rev = $rev;
8	9	$this->user = $user;
9	10	$this->userText = $userText;
10	11	$this->flag = $flag;
11	12	$this->timestamp = $timestamp;
	13	+ $this->timestampStruck = $timestampStruck;
12	14	}
13	15
	16	+ public function isStruck() {
	17	+ return $this->timestampStruck !== Block::infinity();
	18	+ }
	19	+
	20	+ public function getTimestampStruck() {
	21	+ return wfTimestamp( TS_MW, $this->timestampStruck );
	22	+ }
	23	+
	24	+ public function strike() {
	25	+ if ( $this->isStruck() ) {
	26	+ return;
	27	+ }
	28	+ $dbw = wfGetDB( DB_MASTER );
	29	+ $dbw->update( 'code_signoffs', array( 'cs_timestamp_struck' => $dbw->timestamp() ),
	30	+ array(
	31	+ 'cs_repo_id' => $this->rev->getRepoId(),
	32	+ 'cs_rev_id' => $this->rev->getId(),
	33	+ 'cs_flag' => $this->flag,
	34	+ 'cs_user_text' => $this->userText,
	35	+ 'cs_timestamp_struck' => $this->timestampStruck
	36	+ ), __METHOD__
	37	+ );
	38	+ }
	39	+
	40	+ public function getID() {
	41	+ return implode( '\|', array( $this->flag, $this->timestampStruck, $this->userText ) );
	42	+ }
	43	+
14	44	public static function newFromRow( $rev, $row ) {
15	45	return self::newFromData( $rev, get_object_vars( $row ) );
16	46	}
17	47
18	48	public static function newFromData( $rev, $data ) {
19	49	return new self( $rev, $data['cs_user'], $data['cs_user_text'], $data['cs_flag'],
20		~~- wfTimestamp( TS_MW, $data['cs_timestamp'] )~~
	50	+ wfTimestamp( TS_MW, $data['cs_timestamp'] ), $data['cs_timestamp_struck']
21	51	);
22	52	}
	53	+
	54	+ public static function newFromID( $rev, $id ) {
	55	+ $parts = explode( '\|', $id, 3 );
	56	+ if ( count( $parts ) != 3 ) {
	57	+ return null;
	58	+ }
	59	+ $dbr = wfGetDB( DB_SLAVE );
	60	+ $row = $dbr->selectRow( 'code_signoffs',
	61	+ array( 'cs_user', 'cs_user_text', 'cs_flag', 'cs_timestamp', 'cs_timestamp_struck' ),
	62	+ array(
	63	+ 'cs_repo_id' => $rev->getRepoId(),
	64	+ 'cs_rev_id' => $rev->getId(),
	65	+ 'cs_flag' => $parts[0],
	66	+ 'cs_timestamp_struck' => $parts[1],
	67	+ 'cs_user_text' => $parts[2]
	68	+ ), __METHOD__
	69	+ );
	70	+ if ( !$row ) {
	71	+ return null;
	72	+ }
	73	+ return self::newFromRow( $rev, $row );
	74	+ }
23	75	}
Index: trunk/extensions/CodeReview/codereview.sql
—	—	@@ -236,7 +236,10 @@
237	237	cs_flag varchar(25) not null,
238	238
239	239	-- Timestamp of the sign-off
240		~~- cs_timestamp binary(14) not null default ''~~
	240	+ cs_timestamp binary(14) not null default '',
	241	+
	242	+ -- Timestamp the sign-off was struck, or Block::infinity() if not struck
	243	+ cs_timestamp_struck varbinary(14) not null default 'infinity'
241	244	) /$wgDBTableOptions/;
242		~~-CREATE UNIQUE INDEX /i/cs_repo_rev_user_flag ON /_/code_signoffs (cs_repo_id, cs_rev_id, cs_user_text, cs_flag);~~
	245	+CREATE UNIQUE INDEX /i/cs_repo_rev_user_flag_tstruck ON /_/code_signoffs (cs_repo_id, cs_rev_id, cs_user_text, cs_flag, cs_timestamp_struck);
243	246	CREATE INDEX /i/cs_repo_repo_rev_timestamp ON /_/code_signoffs (cs_repo_id, cs_rev_id, cs_timestamp);
Index: trunk/extensions/CodeReview/ui/CodeRevisionCommitter.php
—	—	@@ -15,7 +15,7 @@
16	16	}
17	17
18	18	$commentId = $this->revisionUpdate( $this->mStatus, $this->mAddTags, $this->mRemoveTags,
19		~~- $this->mSignoffFlags, $this->text, $wgRequest->getIntOrNull( 'wpParent' ),~~
	19	+ $this->mSignoffFlags, $this->mStrikeSignoffs, $this->text, $wgRequest->getIntOrNull( 'wpParent' ),
20	20	$wgRequest->getInt( 'wpReview' )
21	21	);
22	22
—	—	@@ -50,15 +50,15 @@
51	51	* @param string $status Status to set the revision to
52	52	* @param Array $addTags Tags to add to the revision
53	53	* @param Array $removeTags Tags to remove from the Revision
54		~~- * @param Array $signoffFlags~~
	54	+ * @param Array $signoffFlags Array of sign-off flags to add
	55	+ * @param Array $strikeSignoffs Array of sign-off IDs to strike
55	56	* @param string $commentText Comment to add to the revision
56	57	* @param null\|int $parent What the parent comment is (if a subcomment)
57	58	* @param int $review (unused)
58	59	* @return int Comment ID if added, else 0
59	60	*/
60		~~- public function revisionUpdate( $status, $addTags, $removeTags, $signoffFlags, $commentText, $parent = null,~~
61		~~- $review = 0 ) {~~
62		-
	61	+ public function revisionUpdate( $status, $addTags, $removeTags, $signoffFlags, $strikeSignoffs,
	62	+ $commentText, $parent = null, $review = 0 ) {
63	63	if ( !$this->mRev ) {
64	64	return false;
65	65	}
—	—	@@ -88,6 +88,10 @@
89	89	if ( count( $signoffFlags ) && $this->validPost( 'codereview-signoff' ) ) {
90	90	$this->mRev->addSignoff( $wgUser, $signoffFlags );
91	91	}
	92	+ // Strike any signoffs
	93	+ if ( count( $strikeSignoffs ) && $this->validPost( 'codereview-signoff' ) ) {
	94	+ $this->mRev->strikeSignoffs( $wgUser, $strikeSignoffs );
	95	+ }
92	96	// Add any comments
93	97	$commentAdded = false;
94	98	$commentId = 0;
Index: trunk/extensions/CodeReview/ui/CodeRevisionView.php
—	—	@@ -44,7 +44,9 @@
45	45	# Make tag arrays
46	46	$this->mAddTags = $this->splitTags( $this->mAddTags );
47	47	$this->mRemoveTags = $this->splitTags( $this->mRemoveTags );
48		~~- $this->mSignoffFlags = $wgRequest->getArray( 'wpSignoffFlags' );~~
	48	+ $this->mSignoffFlags = $wgRequest->getCheck( 'wpSignoff' ) ? $wgRequest->getArray( 'wpSignoffFlags' ) : array();
	49	+ $this->mSelectedSignoffs = $wgRequest->getArray( 'wpSignoffs' );
	50	+ $this->mStrikeSignoffs = $wgRequest->getCheck( 'wpStrikeSignoffs' ) ? $this->mSelectedSignoffs : array();
49	51	}
50	52
51	53	function execute() {
—	—	@@ -129,14 +131,11 @@
130	132	$html .= $this->formatImgDiff();
131	133	}
132	134	# Show sign-offs
133		~~- $signoffs = $this->formatSignoffs();~~
	135	+ $signoffs = $this->formatSignoffs( $this->canSignoff() );
134	136	if ( $signoffs ) {
135	137	$html .= "<h2 id='code-signoffs'>" . wfMsgHtml( 'code-signoffs' ) .
136	138	"</h2>\n" . $signoffs;
137	139	}
138		~~- if( $this->canSignoff() ) {~~
139		~~- $html .= $this->signoffForm();~~
140		~~- }~~
141	140	# Show code relations
142	141	$relations = $this->formatReferences();
143	142	if ( $relations ) {
—	—	@@ -431,17 +430,19 @@
432	431	return wfMsg( 'code-load-diff' );
433	432	}
434	433
435		~~- protected function formatSignoffs() {~~
	434	+ protected function formatSignoffs( $showButtons ) {
436	435	$signoffs = implode( "\n",
437	436	array_map( array( $this, 'formatSignoffInline' ), $this->mRev->getSignoffs() )
438	437	);
439	438	if ( !$signoffs ) {
440	439	return false;
441	440	}
442		~~- $header = '<th>' . wfMsg( 'code-signoff-field-user' ) . '</th>';~~
	441	+ $header = '<th></th>';
	442	+ $header .= '<th>' . wfMsg( 'code-signoff-field-user' ) . '</th>';
443	443	$header .= '<th>' . wfMsg( 'code-signoff-field-flag' ). '</th>';
444	444	$header .= '<th>' . wfMsg( 'code-signoff-field-date' ). '</th>';
445		~~- return "<table border='1' class='TablePager'><tr>$header</tr>$signoffs</table>";~~
	445	+ $buttonrow = $showButtons ? $this->signoffButtons() : '';
	446	+ return "<table border='1' class='TablePager'><tr>$header</tr>$signoffs$buttonrow</table>";
446	447	}
447	448
448	449	protected function formatComments() {
—	—	@@ -487,12 +488,19 @@
488	489	*/
489	490	protected function formatSignoffInline( $signoff ) {
490	491	global $wgLang;
	492	+ $checkbox = Html::input( 'wpSignoffs[]', $signoff->getID(), 'checkbox' );
491	493	$user = $this->skin->userLink( $signoff->user, $signoff->userText );
492		-
493	494	$flag = htmlspecialchars( $signoff->flag );
494		~~- $date = $wgLang->timeanddate( $signoff->timestamp, true );~~
495		~~- $class = "mw-codereview-signoff-$flag";~~
496		~~- return "<tr class='$class'><td>$user</td><td>$flag</td><td>$date</td></tr>";~~
	495	+ $signoffDate = $wgLang->timeanddate( $signoff->timestamp, true );
	496	+ $class = "mw-codereview-signoff-flag-$flag";
	497	+ if ( $signoff->isStruck() ) {
	498	+ $class .= ' mw-codereview-struck';
	499	+ $struckDate = $wgLang->timeanddate( $signoff->getTimestampStruck(), true );
	500	+ $date = wfMsg( 'code-signoff-struckdate', $signoffDate, $struckDate );
	501	+ } else {
	502	+ $date = $signoffDate;
	503	+ }
	504	+ return "<tr class='$class'><td>$checkbox</td><td>$user</td><td>$flag</td><td>$date</td></tr>";
497	505	}
498	506
499	507	protected function formatCommentInline( $comment ) {
—	—	@@ -655,14 +663,17 @@
656	664	}
657	665
658	666	/** TODO : checkboxes should be disabled if user already has set the flag */
659		~~- protected function signoffForm() {~~
660		~~- $form = Xml::element( 'legend', array(), wfMsg( 'code-signoff-legend' ) );~~
	667	+ protected function signoffButtons() {
	668	+ $strikeButton = Xml::submitButton( wfMsg( 'code-signoff-strike' ), array( 'name' => 'wpStrikeSignoffs' ) );
	669	+ $signoffText = wfMsgHtml( 'code-signoff-signoff' );
	670	+ $signoffButton = Xml::submitButton( wfMsg( 'code-signoff-submit' ), array( 'name' => 'wpSignoff' ) );
	671	+ $checks = '';
661	672	foreach ( CodeRevision::getPossibleFlags() as $flag ) {
662		~~- $form .= Html::input( 'wpSignoffFlags[]', $flag, 'checkbox', array( 'id' => "wpSignoffFlags-$flag" ) ) .~~
663		~~- Xml::label( wfMsg( "code-signoff-flag-$flag" ), "wpSignoffFlags-$flag" ) . "\n";~~
	673	+ $checks .= Html::input( 'wpSignoffFlags[]', $flag, 'checkbox', array( 'id' => "wpSignoffFlags-$flag" ) ) .
	674	+ ' ' . Xml::label( wfMsg( "code-signoff-flag-$flag" ), "wpSignoffFlags-$flag" ) . ' ';
664	675	}
665		~~- $form .= Xml::submitButton( wfMsg( 'code-signoff-submit' ) );~~
666		~~- return Xml::tags( 'fieldset', array(), $form );~~
	676	+ return "<tr class='mw-codereview-signoffbuttons'><td colspan='4'>$strikeButton " .
	677	+ "<div class='mw-codereview-signoffchecks'>$signoffText $checks $signoffButton</div></td></tr>";
667	678	}
668	679
669	680	protected function addActionButtons() {

Follow-up revisions

Revision	Commit summary	Author	Date
r77306	Followup r77302: show signoff section even if there are no signoff, so people...	catrope	22:35, 25 November 2010
r77307	Message documentation for r77302	catrope	22:43, 25 November 2010
r77308	Message tweak for r77302 suggested by Reedy	catrope	22:45, 25 November 2010
r77310	Some documentation for r77302, more tomorrow. Functional change: make getTime...	catrope	23:26, 25 November 2010
r77311	Followup r77302, update ApiRevisionUpdate to give flag removal	reedy	23:41, 25 November 2010
r77326	Document functions added in r75327, r77302 (sign-off feature), fix comments a...	catrope	12:18, 26 November 2010
r81060	CodeReview: Update code_signoff.sql for r77302 (cs_timestamp_struck) so the e...	catrope	22:25, 26 January 2011

Status & tagging log

23:48, 22 December 2010 Reedy (talk | contribs) changed the status of r77302 [removed: new added: ok]