Code review comment for lp:~nurlan0000/psiphon/sprint3-610547

Revision history for this message
Rod (rod-psiphon) wrote :

This was a team review. Here are our comments:

- Lack of comments makes review harder (and future maintenance).

- Our pattern is to add a permission (see config.php) and check permissions instead of checking user class when accessing functionality. In this branch, this should be done in profile.php and in delete_account.php.

- Please update the copyright notice year to the current year when a file is edited.

- Include 403 exits automatically. The exit statement is redundant.

- The prompt looks odd: "Delete account Delete account [X]". One "Delete account" as a link is sufficient.

- Why escape (using htmlentities) a msg value? This isn't user input, so escape is unnecessary and we don't do that elsewhere.

- DELETE FROM USER after the other tables to to minimize consistency problems. See the related comment in https://code.launchpad.net/~vasilev/psiphon/sprint3-457431/+merge/34185.

- Both this branch and "https://code.launchpad.net/~vasilev/psiphon/sprint3-457431/+merge/34185" delete from a bunch of related tables when deleting a user. Could refactor both to use a central config list of dependencies. Otherwise, every time someone adds a table that references user ids, they will have to know to go to these two places and add extra DELETEs.

- The new file user.php seems a bit random. Suggest either refactor all user helper functions to this file or add user table helpers to the existing database helper.

- Do not need header_link_prefix in "href="<?php echo $header_link_prefix; ?>delete_account.php">" in profile.php. Also "header('Location: '.$header_link_prefix.'profile.php');" in delete_account.php. $header_link_prefix is for other code that's added in separately and doesn't apply to anything on Launchpad.

- This is a bug: after a delete, the logout redirect isn't correct. The user ends up at e.g., "<proxy>/logout.php?lu=/001". They should end up at "<proxy>/001". As a result of this, the user can't immediately re-login to a different account; instead another redirect occurs.

review: Needs Fixing

« Back to merge proposal