-
Notifications
You must be signed in to change notification settings - Fork 394
Support multiple mappers on beatmap difficulties #11377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
55dbc21
c3ee82a
72b80d7
5c697f4
c93753a
1195c44
43d9dbc
79adc14
825a01a
498e3c7
2da23ff
5655d38
716957d
f9706cf
e371792
0042a94
93a51de
fdaede5
696a102
50b8c6d
fdbd894
926ee77
eb4e738
ad768a5
6b9ab68
5bb4a44
86dfd22
581259a
6ec895d
187306d
0c84a6a
c539574
65ac60e
3732f20
2e25964
9217224
7f283ea
edce318
bc706ef
736a853
5e481aa
9b807b8
b264688
4b91a83
b3cb55a
ecbd11a
e8d4d00
1c64529
eacb7bc
d7d1b0f
c629115
405a091
7d4f3e2
4f757d4
78bff9f
dcf1d1b
58a28b4
0d3d6f0
bb48bf2
9d7845b
5d0faef
4baa745
9aa564e
7e1e92f
3c2129e
fa1e602
a70c617
a744a1f
eff4003
2d19464
77807ac
40ca43c
31059d6
0a8891d
da5cd9c
384d0b2
de8de24
198e59a
1f31a9d
620c1f7
100f320
de0841e
a24d80b
2a475b3
d587edb
2afa074
c7d6d02
9a63743
ef1721a
0de28c1
0e306c3
0601e19
19a8517
26e5ac9
ef2a2c6
2d0679c
6094968
71bcc08
432b8c6
9961203
79afce7
315563d
4cf6146
e797273
2c8b429
dedf45b
3111767
e6963a8
334a910
a5734ca
32d4239
4f01de0
b6b02cb
6ea93b4
d517e0f
dc6fd75
5178c50
430b091
38a4536
3d94a3f
2f6e7ff
b69335b
1f78bab
0e26615
b0e321b
c0cf615
6b4052c
1167226
b5933ef
b6d904e
85a37d3
5da7c56
0389253
191e312
2e3daee
7b0adcc
a8e2635
9a657eb
48bb628
0e1d857
d263810
78e1d0e
cb7eb31
2840b5d
e589f63
d8f8883
ffe6e91
06f602a
fdcbde7
ecc00eb
b5e8c0a
6ca51ce
ea43258
23d4942
47f9a75
a8256c7
11825d3
900ecb3
edbbabd
e6537e4
8fa7949
f81bb52
8ed9cab
adf7bea
a82db85
20e791c
755efc3
454d9fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
<?php | ||
|
||
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the GNU Affero General Public License v3.0. | ||
// See the LICENCE file in the repository root for full licence text. | ||
|
||
namespace App\Console\Commands; | ||
|
||
use App\Models\Beatmap; | ||
use Illuminate\Console\Command; | ||
|
||
class BeatmapsMigrateOwners extends Command | ||
{ | ||
protected $signature = 'beatmaps:migrate-owners'; | ||
|
||
protected $description = 'Migrates beatmap owners to new table.'; | ||
|
||
public function handle() | ||
{ | ||
$progress = $this->output->createProgressBar(); | ||
|
||
Beatmap::chunkById(1000, function ($beatmaps) use ($progress) { | ||
foreach ($beatmaps as $beatmap) { | ||
$beatmap->beatmapOwners()->firstOrCreate(['user_id' => $beatmap->user_id]); | ||
$progress->advance(); | ||
} | ||
}); | ||
|
||
$progress->finish(); | ||
$this->line(''); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
<?php | ||
|
||
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the GNU Affero General Public License v3.0. | ||
// See the LICENCE file in the repository root for full licence text. | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\Libraries\Beatmapset; | ||
|
||
use App\Exceptions\InvariantException; | ||
use App\Jobs\Notifications\BeatmapOwnerChange; | ||
use App\Models\Beatmap; | ||
use App\Models\BeatmapOwner; | ||
use App\Models\BeatmapsetEvent; | ||
use App\Models\User; | ||
use Ds\Set; | ||
|
||
class ChangeBeatmapOwners | ||
{ | ||
private Set $userIds; | ||
|
||
public function __construct(private Beatmap $beatmap, array $newUserIds, private User $source) | ||
{ | ||
priv_check_user($source, 'BeatmapUpdateOwner', $beatmap->beatmapset)->ensureCan(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems like this should be part of controller (as it previously was) as indicated by the need to create additional user in test |
||
|
||
$this->userIds = new Set($newUserIds); | ||
notbakaneko marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if ($this->userIds->count() > $GLOBALS['cfg']['osu']['beatmaps']['owners_max']) { | ||
throw new InvariantException(osu_trans('beatmaps.change_owner.too_many')); | ||
} | ||
|
||
if ($this->userIds->isEmpty()) { | ||
throw new InvariantException('user_ids must be specified'); | ||
} | ||
} | ||
|
||
public function handle(): void | ||
{ | ||
$currentOwners = new Set($this->beatmap->getOwners()->pluck('user_id')); | ||
if ($currentOwners->xor($this->userIds)->isEmpty()) { | ||
return; | ||
} | ||
|
||
$newUserIds = $this->userIds->diff($currentOwners); | ||
|
||
if (User::whereIn('user_id', $newUserIds->toArray())->default()->count() !== $newUserIds->count()) { | ||
throw new InvariantException('invalid user_id'); | ||
} | ||
|
||
$this->beatmap->getConnection()->transaction(function () { | ||
$params = array_map( | ||
fn ($userId) => ['beatmap_id' => $this->beatmap->getKey(), 'user_id' => $userId], | ||
$this->userIds->toArray() | ||
); | ||
|
||
$this->beatmap->fill(['user_id' => $this->userIds->first()])->saveOrExplode(); | ||
$this->beatmap->beatmapOwners()->delete(); | ||
BeatmapOwner::insert($params); | ||
|
||
$this->beatmap->refresh(); | ||
|
||
$newUsers = $this->beatmap->getOwners()->select('id', 'username')->all(); | ||
$beatmapset = $this->beatmap->beatmapset; | ||
$firstMapper = $newUsers[0]; | ||
|
||
BeatmapsetEvent::log(BeatmapsetEvent::BEATMAP_OWNER_CHANGE, $this->source, $beatmapset, [ | ||
'beatmap_id' => $this->beatmap->getKey(), | ||
'beatmap_version' => $this->beatmap->version, | ||
// TODO: mainly for compatibility during dev when switching branches, can be removed after deployed. | ||
'new_user_id' => $firstMapper['id'], | ||
'new_user_username' => $firstMapper['username'], | ||
'new_users' => $newUsers, | ||
])->saveOrExplode(); | ||
|
||
$beatmapset->update(['eligible_main_rulesets' => null]); | ||
}); | ||
|
||
(new BeatmapOwnerChange($this->beatmap, $this->source))->dispatch(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,11 +10,13 @@ | |
use App\Libraries\Transactions\AfterCommit; | ||
use DB; | ||
use Illuminate\Database\Eloquent\Builder; | ||
use Illuminate\Database\Eloquent\Collection; | ||
use Illuminate\Database\Eloquent\SoftDeletes; | ||
|
||
/** | ||
* @property int $approved | ||
* @property \Illuminate\Database\Eloquent\Collection $beatmapDiscussions BeatmapDiscussion | ||
* @property-read Collection<BeatmapDiscussion> $beatmapDiscussions | ||
* @property-read Collection<BeatmapOwner> $beatmapOwners | ||
* @property int $beatmap_id | ||
* @property Beatmapset $beatmapset | ||
* @property int|null $beatmapset_id | ||
|
@@ -29,20 +31,22 @@ | |
* @property float $diff_drain | ||
* @property float $diff_overall | ||
* @property float $diff_size | ||
* @property \Illuminate\Database\Eloquent\Collection $difficulty BeatmapDifficulty | ||
* @property \Illuminate\Database\Eloquent\Collection $difficultyAttribs BeatmapDifficultyAttrib | ||
* @property-read Collection<BeatmapDifficulty> $difficulty | ||
* @property-read Collection<BeatmapDifficultyAttrib> $difficultyAttribs | ||
* @property float $difficultyrating | ||
* @property \Illuminate\Database\Eloquent\Collection $failtimes BeatmapFailtimes | ||
* @property-read Collection<BeatmapFailtimes> $failtimes | ||
* @property string|null $filename | ||
* @property int $hit_length | ||
* @property \Carbon\Carbon $last_update | ||
* @property int $max_combo | ||
* @property mixed $mode | ||
* @property-read Collection<User> $owners | ||
* @property int $passcount | ||
* @property int $playcount | ||
* @property int $playmode | ||
* @property int $score_version | ||
* @property int $total_length | ||
* @property User $user | ||
* @property int $user_id | ||
* @property string $version | ||
* @property string|null $youtube_preview | ||
|
@@ -107,6 +111,11 @@ public function baseMaxCombo() | |
return $this->difficultyAttribs()->noMods()->maxCombo(); | ||
} | ||
|
||
public function beatmapOwners() | ||
{ | ||
return $this->hasMany(BeatmapOwner::class); | ||
} | ||
|
||
public function beatmapset() | ||
{ | ||
return $this->belongsTo(Beatmapset::class, 'beatmapset_id')->withTrashed(); | ||
|
@@ -267,6 +276,7 @@ public function getAttribute($key) | |
'baseDifficultyRatings', | ||
'baseMaxCombo', | ||
'beatmapDiscussions', | ||
'beatmapOwners', | ||
'beatmapset', | ||
'difficulty', | ||
'difficultyAttribs', | ||
|
@@ -279,6 +289,34 @@ public function getAttribute($key) | |
}; | ||
} | ||
|
||
/** | ||
* @return Collection<User> | ||
*/ | ||
public function getOwners(): Collection | ||
{ | ||
$owners = $this->beatmapOwners->loadMissing('user')->map( | ||
fn ($beatmapOwner) => $beatmapOwner->user ?? new DeletedUser(['user_id' => $beatmapOwner->user_id]) | ||
); | ||
|
||
// TODO: remove when everything writes to beatmap_owners. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't it enough to just create an entry if there's no owners at all? any further changes go through osu-web anyway There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. more like should eventually decide whether an empty collection is acceptable or not (in case of broken owners or just simplifying simple cases) |
||
if (!$owners->contains(fn ($beatmapOwner) => $beatmapOwner->user_id === $this->user_id)) { | ||
$owners->prepend($this->user ?? new DeletedUser(['user_id' => $this->user_id])); | ||
} | ||
|
||
return $owners; | ||
} | ||
|
||
public function isOwner(User $user): bool | ||
notbakaneko marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
if ($this->user_id === $user->getKey()) { | ||
return true; | ||
} | ||
|
||
return $this->relationLoaded('beatmapOwners') | ||
? $this->beatmapOwners->contains('user_id', $user->getKey()) | ||
: $this->beatmapOwners()->where('user_id', $user->getKey())->exists(); | ||
} | ||
|
||
public function maxCombo() | ||
{ | ||
if (!$this->convert) { | ||
|
@@ -305,24 +343,6 @@ public function maxCombo() | |
return $maxCombo?->value; | ||
} | ||
|
||
public function setOwner($newUserId) | ||
{ | ||
if ($newUserId === null) { | ||
throw new InvariantException('user_id must be specified'); | ||
} | ||
|
||
if (User::find($newUserId) === null) { | ||
throw new InvariantException('invalid user_id'); | ||
} | ||
|
||
if ($newUserId === $this->user_id) { | ||
throw new InvariantException('the specified user_id is already the owner'); | ||
} | ||
|
||
$this->fill(['user_id' => $newUserId])->saveOrExplode(); | ||
$this->beatmapset->update(['eligible_main_rulesets' => null]); | ||
} | ||
|
||
public function status() | ||
{ | ||
return array_search($this->approved, Beatmapset::STATES, true); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
<?php | ||
|
||
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the GNU Affero General Public License v3.0. | ||
// See the LICENCE file in the repository root for full licence text. | ||
|
||
namespace App\Models; | ||
|
||
/** | ||
* @property Beatmap $beatmap | ||
* @property int $beatmap_id | ||
* @property User $user | ||
* @property int $user_id | ||
*/ | ||
class BeatmapOwner extends Model | ||
{ | ||
public $incrementing = false; | ||
public $timestamps = false; | ||
|
||
protected $primaryKey = ':composite'; | ||
protected $primaryKeys = ['beatmap_id', 'user_id']; | ||
|
||
public function beatmap() | ||
{ | ||
return $this->belongsTo(Beatmap::class, 'beatmap_id'); | ||
} | ||
|
||
public function user() | ||
{ | ||
return $this->belongsTo(User::class, 'user_id'); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicating same content three times (four if counting
related_users
) doesn't seem like a good idea...?(and I don't think I've seen comment on why
.owners
is a thing, partially duplicatingrelated_users
and not just ids)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a bonus(?), if just need the user ids, it can potentially be selected together with the beatmap itself with something like in
osu-web/app/Models/Multiplayer/Room.php
Lines 296 to 321 in 1fbc73b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
owners
is included onconverts
so the UI side doesn't have to go digging the for the non-convert to check for mappers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
owners
is also contains the minimum to display on pages that don't haverelated_users
like the score page instead of a separate include for just the ids, otherwise, the score page should sideload the users instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just include the ids then?
include it only on that specific page then? or add the related users.
seems like a lot of data being returned for everything else just to deal with single page with its single beatmap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
owners
is just the id and username...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to remove the duplicated data from
converts
but it may or not be part of this PR since it needs some extra type and client-side beatmap lookup changes