Add endpoint for closing playlists within grace period of creation #11667

bdach · 2024-11-19T12:54:28Z

Intended for use in ppy/osu#30401

nanaya · 2024-11-19T12:58:25Z

app/Http/Controllers/Multiplayer/RoomsController.php

+        } catch (InvariantException $e) {
+            return error_popup($e->getMessage(), $e->getStatusCode());
+        }


this is now done in the global exception handler and thus redundant

nanaya · 2024-11-19T12:58:53Z

app/Http/Controllers/Multiplayer/RoomsController.php

+            return error_popup($e->getMessage(), $e->getStatusCode());
+        }
+
+        return response([], 204);


as it turned out the first parameter should be null

nanaya · 2024-11-19T12:59:29Z

app/Http/Controllers/Multiplayer/RoomsController.php

+        $room = Room::findOrFail($id);
+
+        try {
+            $room->endGame(auth()->user());


I think we're leaning towards using \Auth::user() now

nanaya · 2024-11-19T12:59:47Z

app/Models/Multiplayer/Room.php

+        }
+
+        $gracePeriodMinutes = $GLOBALS['cfg']['osu']['multiplayer']['room_close_grace_period_minutes'];
+        if ($this->starts_at->addMinutes($gracePeriodMinutes) < now()) {


there's an ->isPast()

nanaya · 2024-11-19T13:04:08Z

tests/Controllers/Multiplayer/RoomsControllerTest.php

+        $start = now();
+        $end = $start->addMinutes(60);


the object created by now() mutates itself so the start and end are both the same. Either clone() it first or use Carbon\CarbonImmutable

nanaya · 2024-11-19T13:07:22Z

tests/Controllers/Multiplayer/RoomsControllerTest.php

@@ -429,6 +429,100 @@ public function testJoinWithPassword()
        $this->assertSame($initialUserChannelCount + 1, UserChannel::count());
    }

+    public function testDestroy()
+    {
+        $token = Token::factory()->create(['scopes' => ['*']]);


there's $this->actAsScopedUser($room->user)

I'm not sure how to use that. I assumed it'd be something like

diff --git a/tests/Controllers/Multiplayer/RoomsControllerTest.php b/tests/Controllers/Multiplayer/RoomsControllerTest.php index 7cd7096c82..ae457dcb9d 100644 --- a/tests/Controllers/Multiplayer/RoomsControllerTest.php +++ b/tests/Controllers/Multiplayer/RoomsControllerTest.php @@ -431,11 +431,9 @@ class RoomsControllerTest extends TestCase public function testDestroy() { - $token = Token::factory()->create(['scopes' => ['*']]); $start = now(); $end = $start->clone()->addMinutes(60); $room = Room::factory()->create([ - 'user_id' => $token->user, 'starts_at' => $start, 'ends_at' => $end, 'type' => Room::PLAYLIST_TYPE, @@ -443,8 +441,8 @@ class RoomsControllerTest extends TestCase $end = $room->ends_at; // creation truncates fractional second part, so refetch here $url = route('api.rooms.destroy', ['room' => $room]); + $this->actAsScopedUser($room->user); $this - ->actingWithToken($token) ->delete($url) ->assertSuccessful();

but trying that, it barfs at me with App\Exceptions\InvalidScopeException: * is not allowed with Client Credentials.

oh wow the model doesn't have user relation defined (and thus it passes null for the user) o_O

adding it is probably a good idea...

I think it might be ->host actually. Seems to work locally at least.

osu-web/app/Models/Multiplayer/Room.php

Lines 234 to 237 in 1fbc73b

public function host()

{

return $this->belongsTo(User::class, 'user_id');

}

osu-web/app/Models/Multiplayer/Room.php

Line 557 in 1fbc73b

$this->setRelation('host', $host);

nanaya · 2024-11-19T13:23:38Z

tests/Controllers/Multiplayer/RoomsControllerTest.php

+            'ends_at' => $end,
+            'type' => Room::PLAYLIST_TYPE,
+        ]);
+        $end = $room->ends_at; // creation truncates fractional second part, so refetch here


technically the truncation happens during assignment

nanaya · 2024-11-19T13:26:31Z

app/Http/Controllers/Multiplayer/RoomsController.php

@@ -228,4 +228,17 @@ public function store()
            return error_popup($e->getMessage(), $e->getStatusCode());
        }
    }
+
+    public function destroy($id)


the functions are sorted alphabetically

nanaya · 2024-11-19T13:31:13Z

oh and here's a protip for the routing json thing #11629 (comment)

tsunyoku · 2024-11-19T13:44:47Z

oh and here's a protip for the routing json thing #11629 (comment)

at this point I feel like this should be placed somewhere as it seems to be referenced every time someone makes a PR which adds/changes a route 😆

could this not be automated via CI or something?

- Move in alphabetical order - Remove redundant try-catch - Fix response call to use `null` rather than `[]` - Use different syntax or whatever to reference user

nanaya · 2024-11-19T13:54:49Z

at this point I feel like this should be placed somewhere as it seems to be referenced every time someone makes a PR which adds/changes a route 😆

could this not be automated via CI or something?

I think the point is to make us aware about api changes but yeah it can be better documented at least.

nanaya · 2024-11-19T14:16:42Z

app/Http/Controllers/Multiplayer/RoomsController.php

+    public function destroy($id)
+    {
+        $room = Room::findOrFail($id);
+        $room->endGame(\Auth()->user());


it's a different thing 😔 ::user()

i don't even wanna know

oh apparently i had the braces in there too which i should not have? i give up

it's a different way to access the same thing but slightly faster because the other method (auth()) does additional processing for some unknown reason

app/Http/Controllers/Multiplayer/RoomsController.php

Add endpoint for closing playlists within grace period of creation

7b6f6c6

bdach added the area:multiplayer label Nov 19, 2024

nanaya requested changes Nov 19, 2024

View reviewed changes

Fix many issues with the controller method

4dd3947

- Move in alphabetical order - Remove redundant try-catch - Fix response call to use `null` rather than `[]` - Use different syntax or whatever to reference user

bdach added 4 commits November 19, 2024 14:55

Use nicer syntax for grace period check

23eda8f

Sprinkle some clones in test

95e44d1

Fix incorrect comments

f1b90e5

Regenerate api routes

c674ded

nanaya reviewed Nov 19, 2024

View reviewed changes

I don't know what this does but I've been told this is correct

013e681

bdach force-pushed the destroy-rooms branch from b610dce to 013e681 Compare November 19, 2024 14:23

Tweak test to use actAsScopedUser()

586e82b

bdach self-assigned this Nov 20, 2024

nanaya reviewed Nov 20, 2024

View reviewed changes

app/Http/Controllers/Multiplayer/RoomsController.php Outdated Show resolved Hide resolved

Inline lookup and process

d066a34

nanaya approved these changes Nov 20, 2024

View reviewed changes

nanaya enabled auto-merge November 20, 2024 08:06

nanaya merged commit 65ca10d into ppy:master Nov 20, 2024
3 checks passed

bdach deleted the destroy-rooms branch November 20, 2024 10:45

bdach mentioned this pull request Nov 20, 2024

Add ability to close playlists within grace period after creation ppy/osu#30793

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add endpoint for closing playlists within grace period of creation #11667

Add endpoint for closing playlists within grace period of creation #11667

bdach commented Nov 19, 2024

nanaya Nov 19, 2024

nanaya Nov 19, 2024

nanaya Nov 19, 2024

nanaya Nov 19, 2024

nanaya Nov 19, 2024

nanaya Nov 19, 2024

bdach Nov 19, 2024 •

edited

Loading

nanaya Nov 19, 2024

bdach Nov 19, 2024 •

edited

Loading

nanaya Nov 19, 2024

nanaya Nov 19, 2024

nanaya commented Nov 19, 2024

tsunyoku commented Nov 19, 2024

nanaya commented Nov 19, 2024

nanaya Nov 19, 2024

bdach Nov 19, 2024

bdach Nov 19, 2024 •

edited

Loading

nanaya Nov 19, 2024

	public function host()
	{
	return $this->belongsTo(User::class, 'user_id');
	}

Add endpoint for closing playlists within grace period of creation #11667

Add endpoint for closing playlists within grace period of creation #11667

Conversation

bdach commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanaya commented Nov 19, 2024

tsunyoku commented Nov 19, 2024

nanaya commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach Nov 19, 2024 •

edited

Loading

bdach Nov 19, 2024 •

edited

Loading

bdach Nov 19, 2024 •

edited

Loading