Update number of nominations required to qualify Beatmap #10984
Conversation
notbakaneko
force-pushed
the
feature/nomination-updates-rebased
branch
from
e2d5e6b
to
21ce02e
Compare
nanaya
changed the title
|
(draft while I fix some missing stuff + rebase) |
adjust required nomination count get main ruleset of beatmapset move nominate logic to own class move legacy nomination logic out
…a ruleset to nominate
notbakaneko
force-pushed
the
feature/nomination-updates-rebased
branch
from
cce3066
to
3657218
Compare
…een the same anyway
…sets check. Also adjust text and localisation key since it's not actually the qualifying nomination.
tests/Models/BeatmapsetTest.php
Outdated
| } | ||
|
|
||
| public function testLimitedBNGQualifyingNominationNATNominated() | ||
| public function testNominateMainRulsetInvariant() |
…ligible during assertion
tests/Models/BeatmapsetTest.php
Outdated
| $this->assertFalse($result['result']); | ||
| $this->assertSame($result['message'], osu_trans('beatmapsets.nominate.full_bn_required')); | ||
| $this->assertTrue($beatmapset->fresh()->isPending()); | ||
| $beatmapset->nominate($nominator, ['taiko']); |
There was a problem hiding this comment.
this test fails if it's $beatmapset->fresh()->nominate(...) instead. Replacing the refresh with $beatmapset = $beatmapset->fresh() doesn't help so the bssProcessQueues() maybe somehow do something to the nominate function? 🤷
Also the nomination throws this error (not sure if it's tested somewhere in this file):
- map with taiko and fruits being eligible
- existing nominations (in order):
- fruits (bn)
- fruits (limited bn)
- trying to nominate (qualify) taiko (bn)
(I copied this test and noticed it doesn't fail unless I call fresh beforehand just like in this test)
There was a problem hiding this comment.
oh it was just the model used for assertion was outdated 🤔 still no idea why I'm getting that nomination error above though
There was a problem hiding this comment.
removing the bssProcessQueues() check doesn't have any affect 🤔
|
|
||
| // First mode with more nominations that others becomes main mode. | ||
| // Implicity implies that limited BN nominations becomes main mode. | ||
| $nominations = $this->beatmapset->beatmapsetNominations()->current()->get(); |
There was a problem hiding this comment.
looks like this needs an explicit order. In my dev db the new row somehow is returned first and messed things up
There was a problem hiding this comment.
either that or the logic needs some rework to not immediately finish on the first ruleset with single nomination...
… by nomination order.
try-catchdance onnominate()expectExceptionCallablewrapper is so that we can run additional assertions after the test throws to make sure it didn't do the thing it wasn't supposed to do.withHypes()test factory isn't used by any test, it's for quickly seeding aBeatmapsetwith necessary number of hypes for nomination.might be a problem with existing nominations since they might fail the nomination count check🤔there's appears to be only 1 beatmapset with a limited nomination that might fail the check so maybe it's better to handle it manually when it arises instead of coding in an exception for it.BeatmapPanelis probably not a good idea...I'm still not quite satisfied with the number of repeated things the tests do each time, but we've had questions in the past about notifications, etc not being sent due to a user setting, when they have been dispatched.
Probationary BNs cannot be the only nomination on their ruleset (i.e. can only nominate the main ruleset)
The new rules for nomination on hybrid sets are:
2 nominations required for the main ruleset and 1 on every other ruleset.
There must be one full nominator on every ruleset.
If 1 and 2 do not result in a clear main ruleset, then any ruleset that satisfies the previous criteria is used.