- 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.
- 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.
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.