r105448 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r105447‎ \| r105448 \| r105449 >
Date:	18:37, 7 December 2011
Author:	gregchiasson
Status:	resolved (Comments)
Tags:
Comment:	Making long-delayed schema updates to AFTv5, per Roan's feedback in the CodeReview.
Modified paths:	/trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/api/ApiArticleFeedbackv5.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/sql/ArticleFeedbackv5.sql (modified) (history) /trunk/extensions/ArticleFeedbackv5/sql/alter.sql (modified) (history)

Diff [purge]

Index: trunk/extensions/ArticleFeedbackv5/sql/ArticleFeedbackv5.sql
—	—	@@ -11,15 +11,15 @@
12	12	DROP TABLE IF EXISTS /_/aft_article_hits;
13	13	DROP TABLE IF EXISTS /_/aft_article_feedback_properties;
14	14
	15	+-- Stores feedback records: "user X submitted feedback on page Y, at time Z"
15	16	CREATE TABLE IF NOT EXISTS /_/aft_article_feedback (
16	17	-- Row ID (primary key)
17	18	af_id integer unsigned NOT NULL PRIMARY KEY AUTO_INCREMENT,
18	19	-- Foreign key to page.page_id
19	20	af_page_id integer unsigned NOT NULL,
20		~~- -- User Id (0 if anon)~~
	21	+ -- User Id (0 if anon), and ip address
21	22	af_user_id integer NOT NULL,
22		~~- -- Username or IP address~~
23		~~- af_user_text varbinary(255) NOT NULL,~~
	23	+ af_user_ip varchar(32) NOT NULL,
24	24	-- Unique token for anonymous users (to facilitate ratings from multiple users on the same IP)
25	25	af_user_anon_token varbinary(32) NOT NULL DEFAULT '',
26	26	-- Foreign key to revision.rev_id
—	—	@@ -31,49 +31,77 @@
32	32	af_cta_id integer unsigned NOT NULL DEFAULT 0,
33	33	-- Which link the user clicked on to get to the widget. Default of 0 is "none".
34	34	af_link_id integer unsigned NOT NULL DEFAULT 0,
35		~~- af_created timestamp NULL DEFAULT CURRENT_TIMESTAMP,~~
36		~~- af_modified timestamp NULL~~
	35	+ -- Creation timetamp
	36	+ af_created binary(14) NOT NULL DEFAULT ''
37	37	) /$wgDBTableOptions/;
38		~~-CREATE INDEX /i/af_page_user_token_id ON /_/aft_article_feedback (af_page_id, af_user_text, af_user_anon_token, af_id);~~
	38	+CREATE INDEX /i/af_page_user_token_id ON /_/aft_article_feedback (af_page_id, af_user_id, af_user_anon_token, af_id);
39	39	CREATE INDEX /i/af_revision_id ON /_/aft_article_feedback (af_revision_id);
40	40	-- Create an index on the article_feedback.af_timestamp field
41	41	CREATE INDEX /i/article_feedback_timestamp ON /_/aft_article_feedback (af_created);
42	42	CREATE INDEX /i/af_page_id ON /_/aft_article_feedback (af_page_id, af_created);
43	43
	44	+-- Allows for organizing fields into fieldsets, for reporting or rendering.
	45	+-- A group is just a name and an ID.
44	46	CREATE TABLE IF NOT EXISTS /_/aft_article_field_group (
45	47	afg_id integer unsigned NOT NULL PRIMARY KEY AUTO_INCREMENT,
46	48	afg_name varchar(255) NOT NULL UNIQUE
47	49	) /$wgDBTableOptions/;
48	50
	51	+-- Stores article fields, zero or more of which are used by each feedback widget
	52	+-- We already used af_ as a prefix above, so this is afi_ instead
49	53	CREATE TABLE IF NOT EXISTS /_/aft_article_field (
50	54	afi_id integer unsigned NOT NULL PRIMARY KEY AUTO_INCREMENT,
51	55	afi_name varchar(255) NOT NULL,
	56	+ -- Allowed data types - relates directly to which aa_response_* field gets
	57	+ -- set in aft_article_answer, and where we check for answers when fetching
52	58	afi_data_type ENUM('text', 'boolean', 'rating', 'option_id'),
53	59	-- FKey to article_field_groups.group_id
54	60	afi_group_id integer unsigned NULL,
	61	+ -- Which 'bucket' this field should be rendered in.
55	62	afi_bucket_id integer unsigned NOT NULL
56	63	) /$wgDBTableOptions/;
57	64
	65	+-- Stores options for multi-value feedback fields (ie, select boxes)
58	66	CREATE TABLE IF NOT EXISTS /_/aft_article_field_option (
59	67	afo_option_id integer unsigned NOT NULL PRIMARY KEY AUTO_INCREMENT,
	68	+ -- foreign key to aft_article_field.afi_id
60	69	afo_field_id integer unsigned NOT NULL,
61	70	afo_name varchar(255) NOT NULL
62	71	) /$wgDBTableOptions/;
63	72
	73	+-- Stores individual answers for each feedback record - for a given feedback
	74	+-- record, what did the user answer for each individual question/input on
	75	+-- the form with which they were presented.
64	76	CREATE TABLE IF NOT EXISTS /_/aft_article_answer (
65	77	-- FKEY to article_feedback.af_id)
66	78	aa_feedback_id integer unsigned NOT NULL,
67	79	-- FKEY to article_field.afi_id)
68	80	aa_field_id integer unsigned NOT NULL,
	81	+ -- Only one of these four columns will be non-null, based on the afi_data_type
	82	+ -- of the aa_field_id related to this record.
69	83	aa_response_rating integer NULL,
70	84	aa_response_text text NULL,
71	85	aa_response_boolean boolean NULL,
72	86	-- FKey to article_field_options.afo_option_id)
73	87	aa_response_option_id integer unsigned NULL,
	88	+ -- Only allow one answer per field per feedback ID.
74	89	PRIMARY KEY (aa_feedback_id, aa_field_id)
75	90	) /$wgDBTableOptions/;
76	91
	92	+/*
	93	+These next four are rollup tables used by the articlefeedback special page.
	94	+The revision tables store per-revision numers, as we (in meetings with WMF)
	95	+agreed that per-revision numbers could be useful in reporting, though
	96	+they aren't currently used on the feedback page. The page-level ones only
	97	+count back to wgArticleFeedbackv5RatingLifetime, so they're a rolling window.
	98	+
	99	+There are tables for ratings and select (ratings includes booleans as well),
	100	+because while the vaue of the rating/boolean is important (Rated 3/5), for
	101	+selects we only want the count for each input, not the value of that input or
	102	+the sum of the values (which will be numerical option_ids, not meaningful
	103	+rating values). The queries were sufficiently different that we deemed multiple
	104	+tables worthwhile.
	105	+*/
77	106	CREATE TABLE IF NOT EXISTS /_/aft_article_feedback_ratings_rollup (
78	107	arr_page_id integer unsigned NOT NULL,
79	108	arr_rating_id integer unsigned NOT NULL,
—	—	@@ -108,7 +136,7 @@
109	137	PRIMARY KEY (arfsr_revision_id, arfsr_option_id)
110	138	) /$wgDBTableOptions/;
111	139
	140	+-- Directly taken from AFTv4
112	141	CREATE TABLE IF NOT EXISTS /_/aft_article_feedback_properties (
113	142	-- Keys to article_feedback.aa_id
114	143	afp_feedback_id integer unsigned NOT NULL,
Index: trunk/extensions/ArticleFeedbackv5/sql/alter.sql
—	—	@@ -24,3 +24,9 @@
25	25
26	26	ALTER TABLE aft_article_feedback ADD COLUMN af_abuse_count integer unsigned NOT NULL DEFAULT 0;
27	27	ALTER TABLE aft_article_feedback ADD COLUMN af_hide_count integer unsigned NOT NULL DEFAULT 0;
	28	+
	29	+ALTER TABLE aft_article_feedback ADD COLUMN af_user_ip varchar(32);
	30	+UPDATE aft_article_feedback SET af_user_ip = af_user_text WHERE af_user_text REGEXP '[0-9\.]+';
	31	+ALTER TABLE aft_article_feedback DROP COLUMN af_user_text;
	32	+ALTER TABLE aft_article_feedback DROP COLUMN af_modified;
	33	+ALTER TABLE aft_article_feedback MODIFY COLUMN af_created binary(14) NOT NULL DEFAULT '';
Index: trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.php
—	—	@@ -158,7 +158,6 @@
159	159	*/
160	160	$wgArticleFeedbackv5LearnToEdit = "http://en.wikipedia.org/wiki/Wikipedia:Tutorial";
161	161
162		~~-# TODO: What's up with the surveys, then?~~
163	162	// Would ordinarily call this articlefeedback but survey names are 16 chars max
164	163	$wgPrefSwitchSurveys['articlerating'] = array(
165	164	'updatable' => false,
Index: trunk/extensions/ArticleFeedbackv5/api/ApiArticleFeedbackv5.php
—	—	@@ -315,6 +315,7 @@
316	316	$link = $params['link'];
317	317	$token = ApiArticleFeedbackv5Utils::getAnonToken( $params );
318	318	$timestamp = $dbw->timestamp();
	319	+ $ip = wfGetIP();
319	320
320	321	# make sure we have a page/user
321	322	if ( !$params['pageid'] \|\| !$wgUser) {
—	—	@@ -332,7 +333,7 @@
333	334	'af_revision_id' => $revId,
334	335	'af_created' => $timestamp,
335	336	'af_user_id' => $wgUser->getId(),
336		~~- 'af_user_text' => $wgUser->getName(),~~
	337	+ 'af_user_ip' => $ip,
337	338	'af_user_anon_token' => $token,
338	339	'af_bucket_id' => $bucket,
339	340	'af_link_id' => $link,
Index: trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php
—	—	@@ -129,18 +129,22 @@
130	130
131	131	$rows = $dbr->select(
132	132	array( 'aft_article_feedback', 'aft_article_answer',
133		~~- 'aft_article_field', 'aft_article_field_option'~~
	133	+ 'aft_article_field', 'aft_article_field_option',
	134	+ 'user'
134	135	),
135	136	array( 'af_id', 'af_bucket_id', 'afi_name', 'afo_name',
136	137	'aa_response_text', 'aa_response_boolean',
137	138	'aa_response_rating', 'aa_response_option_id',
138		~~- 'afi_data_type', 'af_created', 'af_user_text',~~
139		~~- 'af_hide_count', 'af_abuse_count'~~
	139	+ 'afi_data_type', 'af_created', 'user_name',
	140	+ 'af_user_ip', 'af_hide_count', 'af_abuse_count'
140	141	),
141	142	array( 'af_id' => $ids ),
142	143	__METHOD__,
143	144	array( 'ORDER BY' => $order ),
144	145	array(
	146	+ 'user' => array(
	147	+ 'LEFT JOIN', 'user_id = af_user_id'
	148	+ ),
145	149	'aft_article_field' => array(
146	150	'LEFT JOIN', 'afi_id = aa_field_id'
147	151	),
—	—	@@ -154,20 +158,21 @@
155	159	)
156	160	);
157	161
158		~~- foreach($rows as $row) {~~
159		~~- if(!array_key_exists($row->af_id, $rv)) {~~
160		~~- $rv[$row->af_id] = array();~~
	162	+ foreach( $rows as $row ) {
	163	+ if( !array_key_exists( $row->af_id, $rv ) ) {
	164	+ $rv[$row->af_id] = array();
161	165	$rv[$row->af_id][0] = $row;
	166	+ $rv[$row->af_id][0]->user_name = $row->user_name ? $row->user_name : $row->af_user_ip;
162	167	}
163	168	$rv[$row->af_id][$row->afi_name] = $row;
164	169	}
165		-
	170	+
166	171	return $rv;
167	172	}
168	173
169	174	private function getFilterCriteria( $filter ) {
170	175	$where = array();
171		~~- switch($filter) {~~
	176	+ switch( $filter ) {
172	177	case 'all':
173	178	$where = array();
174	179	break;
—	—	@@ -208,7 +213,7 @@
209	214	}
210	215
211	216	private function renderBucket1( $record ) {
212		~~- $name = $record[0]->af_user_text;~~
	217	+ $name = $record[0]->user_name;
213	218	if( $record['found']->aa_response_boolean ) {
214	219	$found = wfMsg(
215	220	'articlefeedbackv5-form1-header-found',
—	—	@@ -227,7 +232,7 @@
228	233	}
229	234
230	235	private function renderBucket2( $record ) {
231		~~- $name = $record[0]->af_user_text;~~
	236	+ $name = $record[0]->user_name;
232	237	$type = $record['tag']->afo_name;
233	238	return wfMsg( 'articlefeedbackv5-form2-header', $name, $type )
234	239	.'<blockquote>'.$record['comment']->aa_response_text
—	—	@@ -235,7 +240,7 @@
236	241	}
237	242
238	243	private function renderBucket3( $record ) {
239		~~- $name = $record[0]->af_user_text;~~
	244	+ $name = $record[0]->user_name;
240	245	$rating = $record['rating']->aa_response_rating;
241	246	return wfMsg( 'articlefeedbackv5-form3-header', $name, $rating )
242	247	.'<blockquote>'.$record['comment']->aa_response_text
—	—	@@ -247,8 +252,8 @@
248	253	}
249	254
250	255	private function renderBucket5( $record ) {
251		~~- $name = $record[0]->af_user_text;~~
252		~~- $rv = wfMsg( 'articlefeedbackv5-form5-header', $name );~~
	256	+ $name = $record[0]->user_name;
	257	+ $rv = wfMsg( 'articlefeedbackv5-form5-header', $name );
253	258	$rv .= '<ul>';
254	259	foreach( $record as $key => $answer ) {
255	260	if( $answer->afi_data_type == 'rating' && $key != '0' ) {

Follow-up revisions

Revision	Commit summary	Author	Date
r105901	AFTv5 - fix a privacy issue introduced by r105448 - apparently we only want t...	gregchiasson	16:32, 12 December 2011

Comments

#Comment by Catrope (talk | contribs) 15:55, 12 December 2011

-			'af_user_text'       => $wgUser->getName(),
+			'af_user_ip'         => $ip,

This means you're putting the user's IP address in the table unconditionally, even if the user is logged in. Please don't do this. We're not completely against tracking this information, but the IP address of logged-in users is considered private information that we don't share with the outside world. By storing it in the af_ table you're 'tainting' it so it can't be made available to the public without being censored. I suggest you just don't store this information (and set user_ip to NULL if user_id != 0) so as to make publishing the data easier.

OK otherwise. Marking fixme for the privacy issue.

Status & tagging log

22:33, 13 December 2011 Catrope (talk | contribs) changed the status of r105448 [removed: fixme added: resolved]
15:55, 12 December 2011 Catrope (talk | contribs) changed the status of r105448 [removed: deferred added: fixme]
08:27, 8 December 2011 Siebrand (talk | contribs) changed the status of r105448 [removed: new added: deferred]