Hi

[yfcheung]

New Commits

[U-238]

Hi Yuk,

This was pretty good, in particular, the way you check against each condition is absolutely correct, also the behaviour is exactly as described.

I have some feedback on the 4 points below; these will also help you with future development. Please fix these and resubmit the pull request.

Cheers,

Mark

1.

Instead of:

userToBeDeleted.first_name + " " + userToBeDeleted.last_name

use this:

userToBeDeleted.get_full_name()

because this will correctly show first_name + " " + last_name for most chapters, and last_name + first_name for the Tokyo chapter (where surname comes first).

The function also works in template files, where you can call it as:

{{userToBeDeleted.get_full_name}}

2.

All text that the user can see, must be translatable, e.g. instead of:

"User " + userToBeDeleted.first_name + " " + userToBeDeleted.last_name + " deleted"

use this:

_("User %s deleted") % userToBeDeleted.get_full_name()

or in template files use

{% trans "text goes here" %}

3.

To return them to the manage members page, you currently have:

return HttpResponseRedirect('/chapters/' + request.user.chapter.myrobogals_url + '/edit/users/?search=&status=' + str(old_status.statusType.pk))

This will take them back to the manage members page of the chapter of the currently logged-in user, but that might not be where they came from (e.g. a superuser looking at another chapter). A better way would be to include a return url like this: /delete/user/1/?return=(url), this is what happens currently when you click to edit a user, to ensure you are returned to the same page you came from. If you have a look at the manage member page and click on someone's username to edit their profile, you'll see what I mean.

4.

Finally, storing "deleteUserPk" in session data could have unpredictable behaviour if the user does the following:

1. in one tab clicks "delete" on user A (but does not confirm)
2. in another tab clicks "delete" on user B (but does not confirm)
3. now goes back to first tab and clicks confirm to delete user A, but the deleteUserPk stored in the session data will be for user B

If you just want to make sure that they go through the confirm page, then that can be seen by whether there is 'delete' or 'alumni' in the post request. So I can't see any reason for having deleteUserPk in session data.