Tech:Reviewing Code/Example4
From Cyclopath
up to: Tech:Reviewing Code
Date: Fri, 05 Dec 2008 14:19:44 -0600
From: Reid Priedhorsky <reid@umn.edu>
To: xxx
Subject: feedback on yyy
xxx,
Good progress. The code is definitely improved since the last commit I
looked at. However, there are a few more changes that I'd like you to
take care of before I do so.
Here is the order of work that I recommend:
1. Complete the pan/zoom widget, or bring it to a state where you'd like
me to review it.
2. Let me know when that's done, and I'll review your work and give
feedback.
3. Implement the changes requested in this e-mail.
4. I may be able to merge the e-mail work at this point; if so, I will.
4. There might be changes requested on the pan/zoom widget; if so,
implement them.
5. I'll then merge your remaining work.
6. Merge the trunk into your branch and svnsurrender.
Normally, code won't be in a branch un-merged so long.
Below is a diff of what you've done so far, with my comments
interspersed. Please read this carefully and let me know what questions
you have.
> Index: pyserver/wfs_PutUserEmail.py
> ===================================================================
> --- pyserver/wfs_PutUserEmail.py (revision 0)
> +++ pyserver/wfs_PutUserEmail.py (revision 9059)
Regarding your approach to dealing with bouncing e-mails: I don't like
it, because it's not general and it duplicates existing functionality.
Instead, I suggest that you send users to the MediaWiki preferences
page, and update our authentication PHP file to clear the bouncing flag
whenever the e-mail address is set.
What is your process for designing UI elements and processes? We try to
emphasize peer feedback in this project, so you should be asking other
team members for feedback on your UI plans before you implement them.
In particular, this issue could have been resolved before you put in all
this effort on the code.
> @@ -0,0 +1,40 @@
> +import conf
> +import wfs
> +import wfs_op_base
> +import util
> +import re
> +from xml.etree import ElementTree as ET
Style: order of imports is nonstandard.
> +
> +class Op_Handler(wfs_op_base.Op_Handler):
> +
> + __slots__ = ('email')
> +
> +
> + def __init__(self, req):
> + wfs_op_base.Op_Handler.__init__(self, req)
> + self.login_required = True
> +
> + def decode_request(self):
> + wfs_op_base.Op_Handler.decode_request(self)
> + self.email = self.req.doc_in.find('email').text
> +
> + def fetch(self):
> + wfs_op_base.Op_Handler.fetch(self)
> +
> + # check that the user provided email is valid
> +
> + p = re.compile('[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,4}', re.IGNORECASE)
There's got to be a library function to verify e-mail addresses; please
use it (though remember that this file is going away). Otherwise you
have this random regex floating around the code.
This is a good example of a time when it's good to ask for help: while I
don't know specifically where this function would be, I could point you
in the right direction, and I'll bet others on the project could too.
> + if not p.match(self.email) :
> + raise wfs.Error('<%s> is not a valid email address'
> + % ( self.email )
> + , 'email-invalid')
> +
Style: bad spacing.
Aside: You might wonder why I'm so picky about code style. The reason is
because consistent code style reduces the cognitive effort required to
understand the code. One can focus effort on understanding the
functionality, and not parsing the syntax.
> + self.save();
Rogue semicolon.
> +
> + def save(self):
> +
> + self.req.db.sql("""UPDATE user_ SET email = %s, email_bouncing=false WHERE username = %s""",
> + (self.email, self.username))
Style: line too long.
I strongly suggest you edit with 80-character windows. This will
eliminate the tendency to write overlong lines.
Note that I'm not going to point out every style error. Please review
the style guide on the wiki, and then carefully review your code and
correct the others too.
If you like, I'm happy to sit down with you on any given file and review
the style line-by-line. That might be more efficient. (Even so, please
review the style guide.)
> +
> + self.req.db.commit()
> +
> Index: pyserver/emailx.py
> ===================================================================
> --- pyserver/emailx.py (revision 8941)
> +++ pyserver/emailx.py (revision 9059)
> @@ -10,13 +10,14 @@
>
> def addr_get(db, username):
> 'Return the e-mail address for username.'
> - rows = db.sql("""SELECT email, login_permitted
> + rows = db.sql("""SELECT email, login_permitted, email_bouncing, enable_email
> FROM user_
> WHERE username = %s""", (username,))
> assert(len(rows) == 1)
> - if (not rows[0]['login_permitted']):
> + user = rows[0]
> + if ((not user['login_permitted']) or user['email_bouncing'] or (not user['enable_email'])):
> return None
> - return rows[0]['email']
> + return user['email']
>
>
> def send(db, touser, toaddr, subject, body):
> Index: pyserver/wfs_Hello.py
> ===================================================================
> --- pyserver/wfs_Hello.py (revision 8941)
> +++ pyserver/wfs_Hello.py (revision 9059)
> @@ -27,6 +27,7 @@
>
> e = et.Element('preferences')
> util.xa_set(e, 'enable_wr_email', self.wr_email_enabled())
> + util.xa_set(e, 'email_bouncing', self.email_bouncing())
> self.doc.append(e)
>
> def wr_email_enabled(self):
> @@ -36,3 +37,11 @@
> WHERE user_.username = %s''' % (self.req.db.quoted(self.username))
> res = self.req.db.sql(sql)
> return res[0]['enable_wr_email']
> +
> + def email_bouncing(self):
> + sql = '''
> + SELECT email_bouncing
> + FROM user_
> + WHERE user_.username = %s''' % (self.req.db.quoted(self.username))
> + res = self.req.db.sql(sql)
> + return res[0]['email_bouncing']
This email_bouncing flag should be sent on every WFS response, not just
Hello -- logins are fairly rare if users tick the "remember me" box. See
the ban or semiprotect infrastructures for an example of how it could work.
> Index: scripts/email_flags.py
> ===================================================================
> --- scripts/email_flags.py (revision 0)
> +++ scripts/email_flags.py (revision 9059)
> @@ -0,0 +1,149 @@
> +#!/usr/bin/python
> +
> +# description
> +#
> +# set or view a users current email flags
> +#
> +# Currently available flags are:
> +#
> +# enable :
> +# enable a user to receive emails
> +#
> +# enable-research :
> +# enable a user to receive research related emails
> +#
> +# bouncing :
> +# flag a users email address as bouncing
> +
> +usage = '''
> + $ export PYSERVER_HOME= location of your pyserver directory
> + $ ./\%prog -u=USER -f=FLAG { -v | -s=VALUE }
> +'''
Just put the documentation in the usage string; no need to duplicate it
in a comment.
Also, please expand these docs; they are too terse. See
tilecache_update.py for a good example of how to do it.
> +
> +import optparse
> +import sys
> +
> +import pyserver_glue
> +import db_glue
> +
> +# flag to database map
> +
> +flag_db_map = {
> + 'enable' : 'enable_email',
> + 'enable-research' : 'enable_email_research',
> + 'bouncing' : 'email_bouncing'
> + }
> +
> +def main() :
> +
> + # parse args
> +
> + op = optparse.OptionParser(usage=usage)
> + op.add_option('-f','--flag', default='all',
> + help='''[ enable , enable-research , bouncing , all ]''' )
> + op.add_option('-s', '--set', dest='value',
> + help='''set flags for user to VALUE=TRUE|FALSE''')
> + op.add_option('-v', '--view', action='store_true', dest='view',
> + help='''view flags for user''')
> + op.add_option('-u', '--user', dest='user',
> + help='''(required) the user whose flags to view or set''')
I don't care for this UI; it's too complicated. How about:
Username to examine is always needed, so I don't think you need a
command-line switch.
If no setting operations are given, print the user's flags.
--enable-email=X Set the enable_email flag.
Similarly for the other two flags (i.e., give each flag a real option,
instead of a just a suboption of --flag).
E.g.
./email_flags.py --enable-email=1 focht
This interface will also simplify the parsing code, I think.
It may be possible to trivially support multiple flags settings per
script invocation. If so, please do so.
Also: only use triple quotes when they're really needed.
> +
> + (options, args) = op.parse_args()
> +
> + # validate args
> +
> + if (options.user is None) :
> + print 'Error: --user must be set'
> + op.print_help()
> + exit()
Use op.error() here; much more concise.
> +
> + if ((options.view is None and options.value is None) or
> + (options.view is not None and options.value is not None)) :
> + print 'Error: either --set or --view must be requested, but not both'
> + op.print_help()
> + exit()
> +
> + if options.value is not None :
> +
> + if not options.value.lower() in [ 'true' , 'false' ] :
> + print 'Error: --set must be either true or false'
> + op.print_help()
> + exit()
> +
optparse can handle Boolean parsing for you.
> + # set flag
> +
> + value = 1 if options.value.lower() == 'true' else 0
> +
> + (successful, values, error) = flags_set(
> + options.user, options.flag, value)
> +
> + else :
> +
> + # get flag
> +
> + (successful, values, error) = flags_get(options.user, options.flag)
Style: too much space (vertical and horizontal).
> +
> + if successful :
> + results = "\n%s email" % options.user
> + for key in values.keys() :
> + results += ''' :%s => %s''' % (
> + key , "True" if values[key] == 1 else "False" )
> + print results + "\n"
> + else :
> + print error
> + op.print_help()
> +
> +
> +# helper functions
> +
> +def flags_get(username, flag):
> +
> + flag = flag.lower()
> +
> + if not flag in flag_db_map and flag != 'all':
Style: missing parens.
> + return ( False, None, '''Error: flag %s is not recognized''' % flag)
> +
> + db = db_glue.new()
Open just one database connection and then pass it around. Commit at the
end of the script.
Also, you need to use db.begin_transaction() as you only want to write
to the database in serializable mode.
> + user = db.sql("SELECT * FROM user_ WHERE username = %s", (username,))
> +
> + if db.rowcount() == 1 :
> +
> + # populate a dictionary to return
> + user = user[0]
> + if flag == "all" :
Style: use single quotes except for SQL or if otherwise necessary.
> + values = dict()
> + for key in flag_db_map.keys() :
> + values[key] = user[flag_db_map[key]]
> + else :
> + values = {flag:user[flag_db_map[flag]]}
> +
> + return ( True , values , 'no error' )
> +
> + else :
> +
> + return (False, None,
> + '''Error: The username %s does not exist''' % (username))
Just throw an exception directly. No need for this complex
success/failure interface.
> +# set a flag
> +#
> +# param value must be one of 0, 1, True, or False
> +
> +def flags_set(username, flag, value):
> +
> + flag = flag.lower()
> + if not flag in flag_db_map :
> + return ( False, None, '''Error: flag %s is not recognized''' % flag)
> +
> + db = db_glue.new()
> + db.sql('''UPDATE user_ SET %s = %%s WHERE username = %%s'''
> + % (flag_db_map[flag.lower()]),
> + ( 'true' if value == 1 else 'false', username ))
> +
> + if db.rowcount() == 1 :
> + db.commit()
> + return ( True , { flag : value }, 'no error' )
> + else :
> + return (
> + False , None, '''Error: The username %s does not exist''' % ( username ))
> +
> +if (__name__ == '__main__'):
> + main()
>
> Property changes on: scripts/email_flags.py
> ___________________________________________________________________
> Name: svn:executable
> + *
>
> Index: scripts/sql/041-alter-users.sql
> ===================================================================
> --- scripts/sql/041-alter-users.sql (revision 0)
> +++ scripts/sql/041-alter-users.sql (revision 9059)
> @@ -0,0 +1,5 @@
> +/** add email columns to the database **/
> +
> +ALTER TABLE user_ ADD COLUMN enable_email BOOLEAN;
> +ALTER TABLE user_ ADD COLUMN enable_email_research BOOLEAN;
> +ALTER TABLE user_ ADD COLUMN email_bouncing BOOLEAN;
> Index: scripts/sql/041-revert-users.sql
> ===================================================================
> --- scripts/sql/041-revert-users.sql (revision 0)
> +++ scripts/sql/041-revert-users.sql (revision 9059)
> @@ -0,0 +1,5 @@
> +/** remove email columns to the database **/
> +
> +ALTER TABLE user_ DROP COLUMN enable_email;
> +ALTER TABLE user_ DROP COLUMN enable_email_research;
> +ALTER TABLE user_ DROP COLUMN email_bouncing;
> Index: flashclient/Rev_Feedback_Entry.mxml
> ===================================================================
> --- flashclient/Rev_Feedback_Entry.mxml (revision 8941)
> +++ flashclient/Rev_Feedback_Entry.mxml (revision 9059)
> @@ -88,8 +88,7 @@
> rev_ids.push(x.@id);
>
> if (ok && this.valid) {
> - (new WFS_PutRevFeedback(this.input_w.text, rev_ids)).fetch();
> - PopUpManager.removePopUp(this);
> + (new WFS_PutRevFeedback(this.input_w.text, rev_ids, this)).fetch();
> }
>
> if (!ok)
> @@ -158,7 +157,7 @@
>
> <mx:HBox>
> <mx:Button id="ok"
> - label="OK"
> + label="Submit"
> enabled="false"
> click="this.submit_maybe(true);"/>
> <mx:Button id="cancel" label="Cancel" click="this.submit_maybe(false);"/>
> Index: flashclient/WFS.as
> ===================================================================
> --- flashclient/WFS.as (revision 8941)
> +++ flashclient/WFS.as (revision 9059)
> @@ -95,7 +95,7 @@
> requests.remove(this);
> if (this.throb)
> UI.request_completed();
> - Alert.show(ev.text + '\n\nYou may have found a bug. Please copy the error message above and click "Help" for next steps.', 'I/O error fetching data');
> + Alert.show(ev.text + '\n\nURL: ' + this.req.url + '\n\nYou may have found a bug. Please copy the error message above and click "Help" for next steps.', 'I/O error fetching data');
> }
>
> protected function handler_security_error(ev:SecurityErrorEvent) :void
> Index: flashclient/User.as
> ===================================================================
> --- flashclient/User.as (revision 8941)
> +++ flashclient/User.as (revision 9059)
> @@ -13,6 +13,7 @@
> public var token:String;
> public var logged_in:Boolean = false;
> public var enable_wr_email:Boolean;
> + public var email_bouncing:Boolean;
>
> // Read-only, a flag to say whether or not user preferences should
> // be saved in a cookie, too since we won't need to make a hello request
> @@ -82,6 +83,7 @@
>
> (new WFS_Hello(username, password,
> this.login_window.rememberme.selected)).fetch();
> +
> this.login_window.enabled = false;
> }
>
> @@ -89,6 +91,7 @@
> public function login_finish(username:String,
> token:String,
> enable_wr_email:Boolean,
> + email_bouncing:Boolean,
> rememberme:Boolean) :void
> {
> this.username = username;
> @@ -96,6 +99,15 @@
> trace('new token: ', token);
> this.logged_in = true;
> this.enable_wr_email = enable_wr_email;
> + this.email_bouncing = email_bouncing;
> +
> + if (this.email_bouncing) {
> + var email_update_window:Email_Update_Popup = new Email_Update_Popup();
> + PopUpManager.addPopUp(email_update_window, G.app, true);
> + PopUpManager.centerPopUp(email_update_window);
> + email_update_window.email_i.setFocus();
> + }
> +
> if (this.reauthenticating) {
> WFS.retry_all();
> } else {
> @@ -186,6 +198,7 @@
> G.assert(this.logged_in);
> if (this.rememberme)
> G.fcookies.set('enable_wr_email', int(this.enable_wr_email));
> + G.fcookies.set('email_bouncing', int(this.email_bouncing));
I don't think it's necessary to keep this in flash cookies.
Look at the griped_foo variables; I think that's a good model for this.
The difference with enable_wr_email is that's a preference setting users
can control, while email_bouncing is an error flag.
> new WFS_PutPreference(this.enable_wr_email).fetch();
> }
>
> @@ -197,10 +210,11 @@
> var enable_wr_email:Boolean;
>
> enable_wr_email = Boolean(int(G.fcookies.get('enable_wr_email')));
> + email_bouncing = Boolean(int(G.fcookies.get('email_bouncing')));
>
> if (username !== null && token !== null) {
> // found a cookie; use those creds
> - this.login_finish(username, token, enable_wr_email, true);
> + this.login_finish(username, token, enable_wr_email, email_bouncing, true);
Again, not necessary to put email_bouncing here.
> } else {
> this.logout();
> // Hack to force login
> Index: flashclient/Email_Update_Popup.mxml
> ===================================================================
> --- flashclient/Email_Update_Popup.mxml (revision 0)
> +++ flashclient/Email_Update_Popup.mxml (revision 9059)
> @@ -0,0 +1,76 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +
> +<mx:TitleWindow
> + xmlns:mx="http://www.adobe.com/2006/mxml"
> + xmlns="*"
> + borderAlpha="1"
> + layout="vertical"
> + showCloseButton="true"
> + paddingLeft="{G.app.pad}"
> + paddingTop="{G.app.pad}"
> + paddingRight="{G.app.pad}"
> + paddingBottom="{G.app.pad}"
> + title="Please update your email address"
> + defaultButton="{ok}">
> +
> + <mx:Script><![CDATA[
> +
> + import mx.managers.PopUpManager;
> + import mx.controls.Text;
> +
> + // Any failing validation will set this to false
> + protected var valid:Boolean;
> +
> + // Validate. If all checks out, submit; otherwise, do nothing.
> + protected function submit_maybe(ok:Boolean) :void
> + {
> + PopUpManager.removePopUp(this);
> +
> + this.valid = true;
> +
> + // email_v.validate();
Remove unneeded code; don't just comment it out unless there's a
specific reason to keep it around.
> +
> + if (ok && this.valid) {
> + (new WFS_PutUserEmail(this.email_i.text)).fetch()
> + }
> +
> + if (!ok)
> + PopUpManager.removePopUp(this);
> + }
> + ]]></mx:Script>
> +
> + <mx:HBox width="100%" height="100%" verticalAlign="middle">
> + <mx:Text>
> + <mx:htmlText><![CDATA[
> +We've noticed that the email address you provided to Cyclopath
> +has has been bouncing emails. If you would like to receive
> +emails from Cyclopath, please update your email address.
> + ]]></mx:htmlText>
> + </mx:Text>
> + </mx:HBox>
> +
> + <mx:HRule width="100%"/>
> +
> + <mx:Grid width="100%">
> + <mx:GridRow width="100%" verticalAlign="middle">
> + <mx:GridItem><mx:Label text="Email address:"/></mx:GridItem>
> + <mx:GridItem><mx:TextInput id="email_i" width="100%"/></mx:GridItem>
> + </mx:GridRow>
> + <mx:GridRow width="100%" verticalAlign="middle">
> + <mx:GridItem/>
> + <mx:GridItem>
> + <mx:Button id="ok"
> + label="Update"
> + fontWeight="bold"
> + click="submit_maybe(true);"/>
> + <mx:Button id="cancel" label="Not Now" click="submit_maybe(false);"/>
> + </mx:GridItem>
> + </mx:GridRow>
> + </mx:Grid>
> +
> + <mx:EmailValidator id="email_v"
> + required="true"
> + source="{email_i}"
> + invalid="valid = false;"/>
> +
> +</mx:TitleWindow>
This file looks pretty nice.
> Index: flashclient/WFS_Hello.as
> ===================================================================
> --- flashclient/WFS_Hello.as (revision 8941)
> +++ flashclient/WFS_Hello.as (revision 9059)
> @@ -59,6 +59,7 @@
> super.resultset_process(rset);
> G.user.login_finish(this.username, rset.token.text(),
> Boolean(int(rset.preferences.@enable_wr_email)),
> + Boolean(int(rset.preferences.@email_bouncing)),
> this.rememberme);
> }
>
> Index: flashclient/WFS_PutUserEmail.as
> ===================================================================
> --- flashclient/WFS_PutUserEmail.as (revision 0)
> +++ flashclient/WFS_PutUserEmail.as (revision 9059)
> @@ -0,0 +1,41 @@
> +package {
> +
> + import mx.controls.Alert;
> + import mx.core.IFlexDisplayObject;
> +
> +
> + public class WFS_PutUserEmail extends WFS {
> +
> + // *** Instance Variables
> +
> + private var email:String;
> +
> + // *** Constructor
> +
> + public function WFS_PutUserEmail(email:String) :void
> + {
> + var doc:XML = this.doc_empty();
> + var url:String = this.url_base('PutUserEmail');
> +
> + this.email = email;
> +
> + doc.appendChild(<email>{email}</email>);
> +
> + super(url, doc);
> + }
> +
> + //
> + override protected function error_wfs(text:String) :void
> + {
> + Alert.show(text, 'Updating email failed');
> + }
> +
> + //
> + override protected function resultset_process(rset:XML) :void
> + {
> + super.resultset_process(rset);
> + Alert.show('Email address was successfully updated.');
> + }
> +
> + }
> +}
> Index: flashclient/WFS_PutRevFeedback.as
> ===================================================================
> --- flashclient/WFS_PutRevFeedback.as (revision 8941)
> +++ flashclient/WFS_PutRevFeedback.as (revision 9059)
> @@ -1,17 +1,25 @@
> package {
>
> import mx.controls.Alert;
> + import mx.managers.PopUpManager;
> + import mx.core.IFlexDisplayObject;
>
> public class WFS_PutRevFeedback extends WFS {
>
> + // *** Instance variables
> +
> + private var feedback_window:IFlexDisplayObject;
> +
> // *** Constructor
>
> - public function WFS_PutRevFeedback(feedback:String, rev_ids:Array) :void
> + public function WFS_PutRevFeedback(feedback:String, rev_ids:Array, feedback_window:IFlexDisplayObject) :void
> {
> var doc:XML = this.doc_empty();
> var url:String = this.url_base('PutRevFeedback');
> var rev_id_str:String = rev_ids.join(",");
>
> + this.feedback_window = feedback_window;
> +
> doc.appendChild(<feedback rids={rev_ids}>{feedback}</feedback>);
>
> super(url, doc);
> @@ -29,6 +37,7 @@
> super.resultset_process(rset);
> Alert.show('Feedback saved successfully. Note that it may take a few minutes for the "See Discussion" button to work.',
> 'Feedback successful');
> + PopUpManager.removePopUp(this.feedback_window);
> // FIXME: This is rather heavyweight, as we could just twiddle the
> // data already in the history browser. However, this is much easier
> // to implement and deals with special cases.
This is a good fix for #591.
I would have merged it now, but it was tangled up in the same commit
with other changes. Each commit should be a single logical piece of effort.
Also, please put both a description of the work done and the relevant
bug number(s) in the commit message. Including a description (which
should be more technical than the symptom-based descriptions in the bug
DB) is important even if it duplicates the bug DB because it makes the
commit messages comprehensible without continual reference to the bug DB
(which is very slow).
General comments:
You may want to work move slowly and methodically. Correct code is more
important than quickly-written code.
Take better advantage of existing/library code. As a long-term effort,
work on your ability to guess whether a particular task is already coded
in a way that you can leverage.
Focus on style, as noted above.
Lastly, push back on feedback if needed, as I'm human too. I expect you
to let me know if there's feedback you don't understand or disagree
with, and then we'll work it out.
As I noted, this code has improved on the previous. I look forward to
reviewing it again after you have another pass.
Thanks,
Reid
