Reports - user profile

Reports - user profile

[Wladimir Palant, 2012-11-07 08:38:15]

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/bin/proc...
File sitescripts/reports/bin/processReports.py (right):

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/bin/proc...
sitescripts/reports/bin/processReports.py:43: print >>sys.stderr, guid
Debug code?

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/bin/proc...
sitescripts/reports/bin/processReports.py:46: saveReport(guid, reportData)
Here as well?

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/template...
File sitescripts/reports/template/report.html (right):

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/template...
sitescripts/reports/template/report.html:89: <td id="emailCell"><a
href="/showUser?id={{uid}}">{{email}}</a></td>
How about /user/{{uid}}? We can add an internal redirect to /showUser to the
server config.

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/template...
File sitescripts/reports/template/showUser.html (right):

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/template...
sitescripts/reports/template/showUser.html:13: <tr><td>Marked as
negative:</td><td>{{negative}}</td></tr>
"Marked as useful/not useful" for consistency?

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/utils.py
File sitescripts/reports/utils.py (right):

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/utils.py...
sitescripts/reports/utils.py:105: if contact:
What if there is an email field but no contact? The check here should be
something like |if reportData.get('email', '')|, the |if contact| check should
only be used to set the uid field.

I realize that the two are connected - but generally it is better to make fewer
assumptions.

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/web/show...
File sitescripts/reports/web/showUser.py (right):

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/web/show...
sitescripts/reports/web/showUser.py:22: email = params.get('email',
[''])[0].lower()
Left-over code? Please remove.

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/web/show...
sitescripts/reports/web/showUser.py:28: user['score'] =
getUserUsefulnessScore(id)
You know that I dislike doing two database queries where one would be
sufficient...

How about changing getUserUsefulnessScore() to call getUser() and then a new
function calculateScore(positive, negative)? calculateScore() would then perform
the calculation you are currently doing in MySQL - and you could use it here as
well.

[Andrey Novikov, 2012-11-07 09:38:48]

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/bin/proc...
File sitescripts/reports/bin/processReports.py (right):

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/bin/proc...
sitescripts/reports/bin/processReports.py:43: print >>sys.stderr, guid
On 2012/11/07 08:38:15, Wladimir Palant wrote:
> Debug code?

Sorry, this file should not be in the review at all. It's technical.

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/template...
File sitescripts/reports/template/report.html (right):

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/template...
sitescripts/reports/template/report.html:89: <td id="emailCell"><a
href="/showUser?id={{uid}}">{{email}}</a></td>
On 2012/11/07 08:38:15, Wladimir Palant wrote:
> How about /user/{{uid}}? We can add an internal redirect to /showUser to the
> server config.

Done.

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/template...
File sitescripts/reports/template/showUser.html (right):

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/template...
sitescripts/reports/template/showUser.html:13: <tr><td>Marked as
negative:</td><td>{{negative}}</td></tr>
On 2012/11/07 08:38:15, Wladimir Palant wrote:
> "Marked as useful/not useful" for consistency?

Done.

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/utils.py
File sitescripts/reports/utils.py (right):

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/utils.py...
sitescripts/reports/utils.py:105: if contact:
Few lines above we have:
contact = getUserId(reportData.get('email', None)) if reportData.get('email',
None) else None

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/web/show...
File sitescripts/reports/web/showUser.py (right):

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/web/show...
sitescripts/reports/web/showUser.py:22: email = params.get('email',
[''])[0].lower()
On 2012/11/07 08:38:15, Wladimir Palant wrote:
> Left-over code? Please remove.

Done.

http://codereview.adblockplus.org/8793015/diff/1/sitescripts/reports/web/show...
sitescripts/reports/web/showUser.py:28: user['score'] =
getUserUsefulnessScore(id)
On 2012/11/07 08:38:15, Wladimir Palant wrote:
> You know that I dislike doing two database queries where one would be
> sufficient...
> 
> How about changing getUserUsefulnessScore() to call getUser() and then a new
> function calculateScore(positive, negative)? calculateScore() would then
perform
> the calculation you are currently doing in MySQL - and you could use it here
as
> well.

Done.

[Wladimir Palant, 2012-11-07 09:46:18]

http://codereview.adblockplus.org/8793015/diff/7001/sitescripts/reports/utils.py
File sitescripts/reports/utils.py (right):

http://codereview.adblockplus.org/8793015/diff/7001/sitescripts/reports/utils...
sitescripts/reports/utils.py:51: count = 1000
We don't need this complicated setup for user reports - it's enough to fetch the
last 100 reports for the user, nobody will be interested in more.

[Andrey Novikov, 2012-11-07 11:39:38]

http://codereview.adblockplus.org/8793015/diff/7001/sitescripts/reports/utils.py
File sitescripts/reports/utils.py (right):

http://codereview.adblockplus.org/8793015/diff/7001/sitescripts/reports/utils...
sitescripts/reports/utils.py:51: count = 1000
On 2012/11/07 09:46:18, Wladimir Palant wrote:
> We don't need this complicated setup for user reports - it's enough to fetch
the
> last 100 reports for the user, nobody will be interested in more.

Done.

[Wladimir Palant, 2012-11-07 13:21:09]

LGTM