Reports - user usefullness

Reports - user usefullness

[Wladimir Palant, 2012-10-18 15:41:23]

What's missing is the integration into the templates system: a particular status
normally includes the helpfulness. So when saving a status template the
usefulness has to be stored as well and it should be restored when the template
is chosen. Also, the template should be prefixed with "Useful: " or "Useless: "
in the list if helpfullness was saved.

I'm not sure whether we should require that the author sets a value for
helpfulness... I see some advantages here but also disadvantages.

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/bin/pars...
File sitescripts/reports/bin/parseNewReports.py (right):

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/bin/pars...
sitescripts/reports/bin/parseNewReports.py:70: reportData = {'status': '',
'utility': 0, 'warnings': {}, 'requests': [], 'filters': [], 'subscriptions':
[], 'extensions': [], 'errors': [], 'time': time()}
How about replacing "utility" by "helpfulness"?

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/static/r...
File sitescripts/reports/static/reports.js (right):

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/static/r...
sitescripts/reports/static/reports.js:85: utilityRadios = '<br />Utility: <input
type="radio" name="utility" value="0" checked="checked" /> not established
<input type="radio" name="utility" value="1" /> usefull <input type="radio"
name="utility" value="-1" /> harmful';
Not "harmful" but "useless". And it is "useful", not "usefull".

Also, shouldn't setting utilityRadios be dependent on utilityCell rather than
emailCell for consistency?

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/web/upda...
File sitescripts/reports/web/updateReport.py (right):

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/web/upda...
sitescripts/reports/web/updateReport.py:48: reportData['utility'] =
params.get('utility', 0)
Isn't this value a string? The default value should be a string as well then -
'0' rather than 0.

[Andrey Novikov, 2012-10-18 15:51:28]

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/bin/pars...
File sitescripts/reports/bin/parseNewReports.py (right):

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/bin/pars...
sitescripts/reports/bin/parseNewReports.py:70: reportData = {'status': '',
'utility': 0, 'warnings': {}, 'requests': [], 'filters': [], 'subscriptions':
[], 'extensions': [], 'errors': [], 'time': time()}
On 2012/10/18 15:41:24, Wladimir Palant wrote:
> How about replacing "utility" by "helpfulness"?

It's synonym and it's shorter, that's why I have chosen it.

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/static/r...
File sitescripts/reports/static/reports.js (right):

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/static/r...
sitescripts/reports/static/reports.js:85: utilityRadios = '<br />Utility: <input
type="radio" name="utility" value="0" checked="checked" /> not established
<input type="radio" name="utility" value="1" /> usefull <input type="radio"
name="utility" value="-1" /> harmful';
On 2012/10/18 15:41:24, Wladimir Palant wrote:
> Not "harmful" but "useless". And it is "useful", not "usefull".
> 
> Also, shouldn't setting utilityRadios be dependent on utilityCell rather than
> emailCell for consistency?

Done.

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/web/upda...
File sitescripts/reports/web/updateReport.py (right):

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/web/upda...
sitescripts/reports/web/updateReport.py:48: reportData['utility'] =
params.get('utility', 0)
On 2012/10/18 15:41:24, Wladimir Palant wrote:
> Isn't this value a string? The default value should be a string as well then -
> '0' rather than 0.

Done.

[Wladimir Palant, 2012-10-19 10:26:06]

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/bin/pars...
File sitescripts/reports/bin/parseNewReports.py (right):

http://codereview.adblockplus.org/8625042/diff/6/sitescripts/reports/bin/pars...
sitescripts/reports/bin/parseNewReports.py:70: reportData = {'status': '',
'utility': 0, 'warnings': {}, 'requests': [], 'filters': [], 'subscriptions':
[], 'extensions': [], 'errors': [], 'time': time()}
I don't think that "utility" is a synonym for "helpfulness" - only roughly and
in one of a dozen possible meanings. It's very non-obvious what you mean by it.
You should use a better term, regardless of length.

[Andrey Novikov, 2012-10-19 12:56:33]

Now this issue contains a complete update to reports including score calculation
and digest generation.

[Wladimir Palant, 2012-10-22 13:32:52]

I still need to review the JavaScript/HTML parts, want to publish the Python
code comments sooner nevertheless.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/util...
File sitescripts/reports/utils.py (right):

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/util...
sitescripts/reports/utils.py:62: contact = getUserId(reportData.get('email',
None)) if 'email' in reportData else None
If 'email' in reportData is True then it means that there is an email attribute
- but it's value can still be None. With your change you will pass that None to
getUserId() which will probably fail. IMHO the old version was correct.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/util...
sitescripts/reports/utils.py:124: (contact))
Please add a link to the source of this formula.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/util...
sitescripts/reports/utils.py:125: user = cursor.fetchone()
You are selecting the score, not the user - please name the variable
appropriately.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/util...
sitescripts/reports/utils.py:132: return user[0]
How about returning |4 * user[0]| here? We want this factor to have a lot more
weight than the other factors.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/utils.py
File sitescripts/utils.py (right):

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/utils.py#new...
sitescripts/utils.py:17: """
Any reason to introduce a new decorator instead of making cached below consider
arguments properly?

Note that storing values for an unlimited amount of time is generally not
useful, you have no idea how long this process will stay around.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/utils.py#new...
sitescripts/utils.py:23: if (args, str(kwargs)) in self.mem:
How about calculating str(kwargs) once at the beginning of the function? Also,
maybe note somewhere that this approach will only work for simple parameter
types.

[Wladimir Palant, 2012-10-23 05:23:28]

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
File sitescripts/reports/static/reports.js (right):

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:54: return radio[i]
Semicolon missing here.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:119: var template =
this.options[this.selectedIndex].value.split("|");
.split("|", 2) please - just in case. However, it might be better to use some
character that will definitely not be used in normal template text - how about
"\x00"?

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:134: var usefulness =
document.getElementById("usefulnessCell").getAttribute("value");
What if there is no usefulnessCell? Note that you might have an old report using
the updated script.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:135: var radios =
document.getElementById("updateForm").elements.usefulness;
How about declaring that variable before the "change" event listener above and
using it there as well?

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:157: usefulnessText = template[0] > 0 ?
"useful" : template[0] < 0 ? "useless" : "neutral";
I don't think that [neutral] is required - we should only add a tag if the
useful/useless information is included.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:204: templates.sort();
Note that the result of this sort will be: useless, no usefulness set, useful.
That's reasonable I guess...

[Andrey Novikov, 2012-10-23 14:15:29]

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
File sitescripts/reports/static/reports.js (right):

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:54: return radio[i]
On 2012/10/23 05:23:28, Wladimir Palant wrote:
> Semicolon missing here.

Done.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:119: var template =
this.options[this.selectedIndex].value.split("|");
On 2012/10/23 05:23:28, Wladimir Palant wrote:
> .split("|", 2) please - just in case. However, it might be better to use some
> character that will definitely not be used in normal template text - how about
> "\x00"?

Done.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:134: var usefulness =
document.getElementById("usefulnessCell").getAttribute("value");
On 2012/10/23 05:23:28, Wladimir Palant wrote:
> What if there is no usefulnessCell? Note that you might have an old report
using
> the updated script.
It's much better to run our one-time all reports update script.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:135: var radios =
document.getElementById("updateForm").elements.usefulness;
On 2012/10/23 05:23:28, Wladimir Palant wrote:
> How about declaring that variable before the "change" event listener above and
> using it there as well?

Done.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:157: usefulnessText = template[0] > 0 ?
"useful" : template[0] < 0 ? "useless" : "neutral";
On 2012/10/23 05:23:28, Wladimir Palant wrote:
> I don't think that [neutral] is required - we should only add a tag if the
> useful/useless information is included.

Done.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/util...
File sitescripts/reports/utils.py (right):

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/util...
sitescripts/reports/utils.py:62: contact = getUserId(reportData.get('email',
None)) if 'email' in reportData else None
On 2012/10/22 13:32:52, Wladimir Palant wrote:
> If 'email' in reportData is True then it means that there is an email
attribute
> - but it's value can still be None. With your change you will pass that None
to
> getUserId() which will probably fail. IMHO the old version was correct.

Done.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/util...
sitescripts/reports/utils.py:124: (contact))
On 2012/10/22 13:32:52, Wladimir Palant wrote:
> Please add a link to the source of this formula.

Done.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/util...
sitescripts/reports/utils.py:125: user = cursor.fetchone()
On 2012/10/22 13:32:52, Wladimir Palant wrote:
> You are selecting the score, not the user - please name the variable
> appropriately.

Done.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/util...
sitescripts/reports/utils.py:132: return user[0]
On 2012/10/22 13:32:52, Wladimir Palant wrote:
> How about returning |4 * user[0]| here? We want this factor to have a lot more
> weight than the other factors.

Done.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/utils.py
File sitescripts/utils.py (right):

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/utils.py#new...
sitescripts/utils.py:17: """
On 2012/10/22 13:32:52, Wladimir Palant wrote:
> Any reason to introduce a new decorator instead of making cached below
consider
> arguments properly?
> 
> Note that storing values for an unlimited amount of time is generally not
> useful, you have no idea how long this process will stay around.

I thought you were not considering them on purpose.

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/utils.py#new...
sitescripts/utils.py:23: if (args, str(kwargs)) in self.mem:
On 2012/10/22 13:32:52, Wladimir Palant wrote:
> How about calculating str(kwargs) once at the beginning of the function? Also,
> maybe note somewhere that this approach will only work for simple parameter
> types.

Done.

[Wladimir Palant, 2012-10-25 15:12:55]

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
File sitescripts/reports/static/reports.js (right):

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:134: var usefulness =
document.getElementById("usefulnessCell").getAttribute("value");
I don't really like doing that without a very good reason. So I would prefer it
if you fixed this.

http://codereview.adblockplus.org/8625042/diff/22002/sitescripts/utils.py
File sitescripts/utils.py (right):

http://codereview.adblockplus.org/8625042/diff/22002/sitescripts/utils.py#new...
sitescripts/utils.py:16: Decorator that caches a function's return value for a
given number of seconds.
Still need a note here like:

Note that this only works if the string representation of the parameters is
always unique.

http://codereview.adblockplus.org/8625042/diff/22002/sitescripts/utils.py#new...
sitescripts/utils.py:35: return repr(self.wrapped)
I don't think that we should forward this to our wrapper - rather to the
original function. So I think that you should store the func parameter instead
of wrapped.

[Andrey Novikov, 2012-10-29 07:05:08]

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
File sitescripts/reports/static/reports.js (right):

http://codereview.adblockplus.org/8625042/diff/10001/sitescripts/reports/stat...
sitescripts/reports/static/reports.js:134: var usefulness =
document.getElementById("usefulnessCell").getAttribute("value");
On 2012/10/25 15:12:55, Wladimir Palant wrote:
> I don't really like doing that without a very good reason. So I would prefer
it
> if you fixed this.

Done.

http://codereview.adblockplus.org/8625042/diff/22002/sitescripts/utils.py
File sitescripts/utils.py (right):

http://codereview.adblockplus.org/8625042/diff/22002/sitescripts/utils.py#new...
sitescripts/utils.py:16: Decorator that caches a function's return value for a
given number of seconds.
On 2012/10/25 15:12:55, Wladimir Palant wrote:
> Still need a note here like:
> 
> Note that this only works if the string representation of the parameters is
> always unique.

Done.

http://codereview.adblockplus.org/8625042/diff/22002/sitescripts/utils.py#new...
sitescripts/utils.py:35: return repr(self.wrapped)
On 2012/10/25 15:12:55, Wladimir Palant wrote:
> I don't think that we should forward this to our wrapper - rather to the
> original function. So I think that you should store the func parameter instead
> of wrapped.

Done.

[Wladimir Palant, 2012-10-29 08:36:08]

LGTM

Please wait with pushing it - the database needs to be updated first, after the
meeting today.