Support multiple mappers on beatmap difficulties #11377
Conversation
notbakaneko
force-pushed
the
feature/beatmaps-multiple-mappers
branch
from
2cc68f8 to
8ed9cab
Compare
app/Models/Beatmap.php
Outdated
| ); | ||
|
|
||
| // TODO: remove when everything writes to beatmap_owners. | ||
| if (!$owners->contains(fn ($beatmapOwner) => $beatmapOwner->user_id === $this->user_id) && $this->user !== null) { |
There was a problem hiding this comment.
related to the comment above, can't this be simplified to just if ($owners->empty() && $this->user !== null) {?
There should be only these two possibilities, right?
- unmigrated/new entry with only
->user_idand no->beatmapOwners - osu-web managed entry with
->user_idalready in->beatmapOwners
(there's also a potential weird bug/case already existing here - the array should probably always be filled in with at least a deleted user of id ->user_id)
There was a problem hiding this comment.
Is it a simplification if more states have to be considered instead of user is in or not? 🤨
There was a problem hiding this comment.
what more state? it's just whether there's anything in it or not
There was a problem hiding this comment.
It's also making another unnecessary assumption instead of just user in or not
There was a problem hiding this comment.
it skips looping through the array and so the check is way simpler
There was a problem hiding this comment.
also I think it's a fine assumption - there's either anything in the owners table or there's none
There was a problem hiding this comment.
It's also very pointless bike shedding
There was a problem hiding this comment.
more like optimization instead of bike shedding to me?
also, looking again, it's not an assumption. it's how the system works. the only other case is when user id is not in the owners but that's a broken case and that's where assumption goes (which applies to any way of handling it - be it blindly adding it as owner or just ignoring it)
| private get inputUser() { | ||
| return this.props.userByName.get(normaliseUsername(this.inputUsername)); | ||
| private get canSave() { | ||
| return this.validUsers.size > 0 && normaliseUsername(this.inputUsername).length === 0; |
There was a problem hiding this comment.
normaliseUsername is now just a rather verbose way to do a .trim
| 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.
more like should eventually decide whether an empty collection is acceptable or not (in case of broken owners or just simplifying simple cases)
| user: UserJson; | ||
| } | ||
|
|
||
| function createRemoveOwnerHandler(user: UserJson, onRemoveClick?: NonNullable<Props['onRemoveUser']>) { |
There was a problem hiding this comment.
there's no need for this lint workaround anymore now that the component is split out - this can just be part of the component
| renderUser={this.renderOwner} | ||
| /> | ||
| ) : ( | ||
| this.owners.map((owner) => <BeatmapOwner key={owner.id} editing={this.editing} user={owner} />) |
There was a problem hiding this comment.
this can just call renderUser
| @@ -64,6 +66,15 @@ public function inactive(): static | |||
| ]); | |||
| } | |||
|
|
|||
| public function owner(User $user): static | |||
There was a problem hiding this comment.
this should (will?) probably be owners instead
|
|
||
| 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.
seems like this should be part of controller (as it previously was) as indicated by the need to create additional user in test
closes #7602
Also refactors out the username input/search thing a separate component