Skip to content
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

Support multiple mappers on beatmap difficulties #11377

Open
wants to merge 92 commits into
base: master
Choose a base branch
from

Conversation

notbakaneko
Copy link
Collaborator

@notbakaneko notbakaneko commented Jul 30, 2024

closes #7602

Also refactors out the username input/search thing a separate component

@peppy peppy requested a review from nanaya August 7, 2024 02:13
@notbakaneko
Copy link
Collaborator Author

I'm thinking all the *Owner should be renamed *Mapper to be consistent, but then there's the existing beatmap events which uses *owner...

@nanaya
Copy link
Collaborator

nanaya commented Aug 8, 2024

can try renaming the events...

Copy link
Collaborator

@nanaya nanaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • decide on either mapper or owner name; not both
  • split the user lookup change to its own pr (and combine with checkUsernameExists while at it)

@nanaya
Copy link
Collaborator

nanaya commented Sep 2, 2024

can use a merge conflict fix

@notbakaneko
Copy link
Collaborator Author

Not sure renaming to mappers is a great idea now that I've gotten to the event itself, but using mapper for the display text at least seems to make more sense than owner...

@notbakaneko notbakaneko force-pushed the feature/beatmaps-multiple-mappers branch from d8cdf20 to 1f31a9d Compare September 12, 2024 08:17
@notbakaneko notbakaneko force-pushed the feature/beatmaps-multiple-mappers branch from 9e11108 to d587edb Compare September 13, 2024 11:07
@notbakaneko
Copy link
Collaborator Author

notbakaneko commented Sep 13, 2024

Should be no new introduction of mapper instead of owner now, I think (I left the existing ones alone) 🤔

@peppy peppy requested a review from nanaya September 27, 2024 08:44
Copy link
Collaborator

@nanaya nanaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

part 1?


// TODO: use select instead (needs newer laravel)
$newUsers = $this->beatmap->owners->map(
fn ($user) => ['id' => $user->user_id, 'username' => ($user ?? new DeletedUser())->username],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't the ->user_id (->getKey()?) fail if $user is null?


$this->beatmap->refresh();

// TODO: use select instead (needs newer laravel)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would the deleted user username fallback work with select though?

}

$this->beatmap->getConnection()->transaction(function () {
$userIds = $this->userIds->toArray();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not iterate over the set itself? (or even straight out map)


$this->fill(['user_id' => $newUserId])->saveOrExplode();
$this->beatmapset->update(['eligible_main_rulesets' => null]);
(new ChangeBeatmapOwners($this, $newUserIds, $source))->handle();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it all that useful over just using the class directly

@@ -2247,7 +2248,9 @@ public function profileBeatmapsetsGuest()
return Beatmapset
::where('user_id', '<>', $this->getKey())
->whereHas('beatmaps', function (Builder $query) {
$query->scoreable()->where('user_id', $this->getKey());
$query->scoreable()->whereHas('beatmapOwners', function (Builder $ownerQuery) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use fn (...) => ?

render() {
return (
<div className='beatmap-owner'>
<UserLink className='beatmap-owner__user' user={this.props.user}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

misclicking this when trying to remove a user and losing all unsaved changes is rather annoying

also the card popup occasionally gets in the way

@@ -99,6 +100,10 @@ export function group<T extends BeatmapJson>(beatmaps?: T[] | null, includeEmpty
return ret;
}

export function hasGuestOwners(beatmap: BeatmapJson, beatmapset: BeatmapsetJson) {
return beatmap.owners?.some((owner) => owner.id !== beatmapset.user_id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it not weird to return undefined if the field isn't included?

--input-bg: hsl(var(--hsl-b6));

flex-direction: row;
padding: 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this one but some padding would be nice

{
priv_check_user($source, 'BeatmapUpdateOwner', $beatmap->beatmapset)->ensureCan();

$this->userIds = new Set($newUserIds);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some kind of limit would be useful


border-radius: 50%;
display: flex;
aspect-ratio: 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't seem to be doing anything. the box needs to be a bit wider first

padding: 1px 3px;
width: auto;
border: 2px solid transparent;
&__owners {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can use some wrapping and more gap between avatar and previous user

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

Allow to choose several mappers as difficulty owners
3 participants