Conversation
|
3rd commit just fixes branch being behind- ignore. |
a3a5979 to
086c3ac
Compare
086c3ac to
7830ba2
Compare
|
public function testUsernameWithDifferentCasingIsSame()
{
$user = $this->createUser();
$errors = $user->validateChangeUsername('iAmUser')->all();
$expected = [osu_trans('model_validation.user.change_username.username_is_same')];
$this->assertArrayHasKey('username', $errors);
$this->assertArraySubset($expected, $errors['username'], true);
}needs to be checked out, tests fails on there. |
|
Would this not increase the cost of the next namechange? I believe that this is not the case at the moment for namechanges done over email. |
it might, depending whether of not the change is logged into db: public function usernameChangeHistory()
{
return $this->hasMany(UsernameChangeHistory::class);
}public function usernameChangeCost()
{
$minTier = $this->usernameChangeHistory()->paid()->exists() ? 1 : 0;
$tier = max(
$this->usernameChangeHistory()
->paid()
->where('timestamp', '>', Carbon::now()->subYearsNoOverflow(3))
->count(),
$minTier,
);
return match ($tier) {
0 => 0,
1 => 8,
2 => 16,
3 => 32,
4 => 64,
default => 100,
};
} |
private function updateUsername(string $newUsername, string $type): UsernameChangeHistory
{
$oldUsername = $type === 'revert' ? null : $this->getOriginal('username');
$this->username_previous = $oldUsername;
$this->username = $newUsername;
return DB::transaction(function () use ($newUsername, $oldUsername, $type) {
Forum\Forum::where('forum_last_poster_id', $this->user_id)->update(['forum_last_poster_name' => $newUsername]);
// DB::table('phpbb_moderator_cache')->where('user_id', $this->user_id)->update(['username' => $newUsername]);
Forum\Post::where('poster_id', $this->user_id)->update(['post_username' => $newUsername]);
Forum\Topic::where('topic_poster', $this->user_id)
->update(['topic_first_poster_name' => $newUsername]);
Forum\Topic::where('topic_last_poster_id', $this->user_id)
->update(['topic_last_poster_name' => $newUsername]);
$history = $this->usernameChangeHistory()->create([
'username' => $newUsername,
'username_last' => $oldUsername,
'timestamp' => Carbon::now(),
'type' => $type,
]);
if (!$history->exists) {
throw new ModelNotSavedException('failed saving model');
}
$skipValidations = in_array($type, ['inactive', 'revert'], true);
$this->saveOrExplode(['skipValidations' => $skipValidations]);
return $history;
});
}i believe this is where we should change it to not add to db. cleanUsername function removes any cases though so a workaround? |
|
|
||
| if (User::cleanUsername($this->username) === $this->user->username_clean) { | ||
| // Block if new username is exactly the same as current (case-sensitive match) | ||
| if (strcasecmp($this->username, $this->user->username) === 0 && $this->username === $this->user->username) { |
There was a problem hiding this comment.
I wonder why not just compare them. If It blocks when they are exactly the same, there doesn't seem to be any need to do a case-insensitive comparison?
There was a problem hiding this comment.
the cleanUsername strips all possibilities of case comparison
There was a problem hiding this comment.
this could also use a message on the frontend explaining that the username change is free because of only being a capitalization change
besides, i suppose this could also use some form of cooldown so that people don't change this constantly, and also as other people pointed out it needs to handle not incrementing the username change count or does it? i'm not sure how exactly it is handled on the support side
|
|
||
| if (User::cleanUsername($this->username) === $this->user->username_clean) { | ||
| // Block if new username is exactly the same as current (case-sensitive match) | ||
| if (strcasecmp($this->username, $this->user->username) === 0 && $this->username === $this->user->username) { |
There was a problem hiding this comment.
wouldn't a regular string comparison (aka the second check) be sufficient in this case?
| protected $type; | ||
|
|
||
| /** @var User */ | ||
| protected $user; |
There was a problem hiding this comment.
why was this removed? tbh with modern php it could just be:
| protected $user; | |
| protected User $user; |
| if ($this->isCapitalizationOnlyChange()) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Putting this here will allow restricted users to change the capitalization on their usernames, which shouldn't be the case. isCapitalizationOnlyChange should only bypass the supporter check if anything.
| $available = $errors->isEmpty(); | ||
| $message = $available ? "Username '".e($username)."' is available!" : $errors->toSentence(); | ||
| $cost = $available ? Auth::user()->usernameChangeCost() : 0; | ||
| $isCapitalizationOnly = strcasecmp($username, Auth::user()->username) === 0 && $username !== Auth::user()->username; |
There was a problem hiding this comment.
this check should probably be put into helpers.php and used both here and in ChangeUsername
| $cost = 0; | ||
| if ($available) { | ||
| $cost = $isCapitalizationOnly ? 0 : Auth::user()->usernameChangeCost(); | ||
| } |
There was a problem hiding this comment.
you could do it this way instead and it'd be easier to read, especially since we don't have to care about the availability checks if the username has the same capitalization
| $cost = 0; | |
| if ($available) { | |
| $cost = $isCapitalizationOnly ? 0 : Auth::user()->usernameChangeCost(); | |
| } | |
| if ($isCapitalizationOnly) { | |
| $cost = 0; | |
| else { | |
| $cost = $available ? Auth::user()->usernameChangeCost() : 0; | |
| } |
This is not a big need, but the only way to change username capitalization so far has been only to message support (i think?) or to change username twice (temporary username and then other capitalization). This needs to be checked further since i'm not the best at Laravel but should work!
reopened