Merge pull request #8834 from nanaya/offset-admin

Only allow admin to edit beatmap offset
ppy · Apr 22, 2022 · c84ac7f · c84ac7f
2 parents b315c58 + ba4bb10
commit c84ac7f
Show file tree

Hide file tree

Showing 6 changed files with 85 additions and 33 deletions.
diff --git a/app/Http/Controllers/BeatmapsetsController.php b/app/Http/Controllers/BeatmapsetsController.php
@@ -254,20 +254,31 @@ public function update($id)
             'genre_id:int',
             'language_id:int',
             'nsfw:bool',
+        ]);
+
+        $offsetParams = get_params($params, 'beatmapset', [
             'offset:int',
         ]);
 
+        $updateParams = array_merge($metadataParams, $offsetParams);
+
         if (count($metadataParams) > 0) {
             priv_check('BeatmapsetMetadataEdit', $beatmapset)->ensureCan();
+        }
+
+        if (count($offsetParams) > 0) {
+            priv_check('BeatmapsetOffsetEdit')->ensureCan();
+        }
 
-            DB::transaction(function () use ($beatmapset, $metadataParams) {
+        if (count($updateParams) > 0) {
+            DB::transaction(function () use ($beatmapset, $updateParams) {
                 $oldGenreId = $beatmapset->genre_id;
                 $oldLanguageId = $beatmapset->language_id;
                 $oldNsfw = $beatmapset->nsfw;
                 $oldOffset = $beatmapset->offset;
                 $user = auth()->user();
 
-                $beatmapset->fill($metadataParams)->saveOrExplode();
+                $beatmapset->fill($updateParams)->saveOrExplode();
 
                 if ($oldGenreId !== $beatmapset->genre_id) {
                     BeatmapsetEvent::log(BeatmapsetEvent::GENRE_EDIT, $user, $beatmapset, [

diff --git a/app/Libraries/OsuAuthorize.php b/app/Libraries/OsuAuthorize.php
@@ -849,6 +849,11 @@ public function checkBeatmapsetDownload(?User $user, Beatmapset $beatmapset): st
         return 'ok';
     }
 
+    public function checkBeatmapsetOffsetEdit(): string
+    {
+        return 'unauthorized';
+    }
+
     /**
      * @param User|null $user
      * @return string

diff --git a/app/Transformers/BeatmapsetCompactTransformer.php b/app/Transformers/BeatmapsetCompactTransformer.php
@@ -115,6 +115,7 @@ public function includeCurrentUserAttributes(Beatmapset $beatmapset)
             'can_beatmap_update_owner' => priv_check('BeatmapUpdateOwner', $beatmapset)->can(),
             'can_delete' => !$beatmapset->isScoreable() && priv_check('BeatmapsetDelete', $beatmapset)->can(),
             'can_edit_metadata' => priv_check('BeatmapsetMetadataEdit', $beatmapset)->can(),
+            'can_edit_offset' => priv_check('BeatmapsetOffsetEdit')->can(),
             'can_hype' => $hypeValidation['result'],
             'can_hype_reason' => $hypeValidation['message'] ?? null,
             'can_love' => $beatmapset->isLoveable() && priv_check('BeatmapsetLove')->can(),

diff --git a/resources/assets/lib/beatmapsets-show/metadata-editor.tsx b/resources/assets/lib/beatmapsets-show/metadata-editor.tsx
@@ -25,6 +25,10 @@ export default class MetadataEditor extends React.PureComponent<Props, State> {
   private genres = parseJson<GenreJson[]>('json-genres');
   private languages = parseJson<LanguageJson[]>('json-languages');
 
+  get canEditOffset() {
+    return this.props.beatmapset.current_user_attributes.can_edit_offset;
+  }
+
   get numericOffset() {
     if (this.state.offset !== '') {
       const ret = Number(this.state.offset);
@@ -92,20 +96,22 @@ export default class MetadataEditor extends React.PureComponent<Props, State> {
           </div>
         </label>
 
-        <label className='simple-form__row'>
-          <div className='simple-form__label'>
-            {osu.trans('beatmapsets.show.info.offset')}
-          </div>
+        {this.canEditOffset &&
+          <label className='simple-form__row'>
+            <div className='simple-form__label'>
+              {osu.trans('beatmapsets.show.info.offset')}
+            </div>
 
-          <input
-            className='simple-form__input'
-            maxLength={6}
-            name='beatmapset[offset]'
-            onChange={this.setOffset}
-            type='text'
-            value={this.state.offset}
-          />
-        </label>
+            <input
+              className='simple-form__input'
+              maxLength={6}
+              name='beatmapset[offset]'
+              onChange={this.setOffset}
+              type='text'
+              value={this.state.offset}
+            />
+          </label>
+        }
 
         <div className='simple-form__row'>
           <div className='simple-form__label'>
@@ -161,7 +167,7 @@ export default class MetadataEditor extends React.PureComponent<Props, State> {
         genre_id: this.state.genreId,
         language_id: this.state.languageId,
         nsfw: this.state.nsfw,
-        offset: this.numericOffset,
+        offset: this.canEditOffset ? this.numericOffset : undefined,
       } },
       method: 'PATCH',
     }).done((beatmapset: BeatmapsetJsonForShow) => $.publish('beatmapset:set', { beatmapset }))

diff --git a/resources/assets/lib/interfaces/beatmapset-json.ts b/resources/assets/lib/interfaces/beatmapset-json.ts
@@ -46,6 +46,7 @@ export type BeatmapsetStatus =
 export interface CurrentUserAttributes {
   can_delete: boolean;
   can_edit_metadata: boolean;
+  can_edit_offset: boolean;
   can_hype: boolean;
   can_hype_reason: string;
   can_love: boolean;

diff --git a/tests/Controllers/BeatmapsetsControllerTest.php b/tests/Controllers/BeatmapsetsControllerTest.php
@@ -113,30 +113,26 @@ public function testBeatmapsetUpdateMetadataAsModerator($state)
         ]);
         $newGenre = Genre::factory()->create();
         $newLanguage = Language::factory()->create();
-        $newOffset = $beatmapset->offset + 25;
 
         $moderator = User::factory()->withGroup('nat')->create();
 
         $resultGenreId = $newGenre->getKey();
         $resultLanguageId = $newLanguage->getKey();
 
-        $eventCountBeforeTry = BeatmapsetEvent::count();
+        $this->expectCountChange(fn () => BeatmapsetEvent::count(), 2);
 
         $this->actingAsVerified($moderator)
             ->put(route('beatmapsets.update', ['beatmapset' => $beatmapset->getKey()]), [
                 'beatmapset' => [
                     'genre_id' => $newGenre->getKey(),
                     'language_id' => $newLanguage->getKey(),
-                    'offset' => $newOffset,
                 ],
             ])->assertSuccessful();
 
         $beatmapset->refresh();
 
         $this->assertSame($resultGenreId, $beatmapset->genre_id);
         $this->assertSame($resultLanguageId, $beatmapset->language_id);
-        $this->assertSame($newOffset, $beatmapset->offset);
-        $this->assertSame(BeatmapsetEvent::count(), $eventCountBeforeTry + 3);
     }
 
     /**
@@ -151,13 +147,11 @@ public function testBeatmapsetUpdateMetadataAsOtherUser($state)
         ]);
         $newGenre = Genre::factory()->create();
         $newLanguage = Language::factory()->create();
-        $newOffset = $beatmapset->offset + 25;
 
         $resultGenreId = $beatmapset->genre_id;
         $resultLanguageId = $beatmapset->language_id;
-        $resultOffset = $beatmapset->offset;
 
-        $eventCountBeforeTry = BeatmapsetEvent::count();
+        $this->expectCountChange(fn () => BeatmapsetEvent::count(), 0);
 
         $user = User::factory()->create();
 
@@ -166,16 +160,13 @@ public function testBeatmapsetUpdateMetadataAsOtherUser($state)
                 'beatmapset' => [
                     'genre_id' => $newGenre->getKey(),
                     'language_id' => $newLanguage->getKey(),
-                    'offset' => $newOffset,
                 ],
             ])->assertStatus(403);
 
         $beatmapset->refresh();
 
         $this->assertSame($resultGenreId, $beatmapset->genre_id);
         $this->assertSame($resultLanguageId, $beatmapset->language_id);
-        $this->assertSame($resultOffset, $beatmapset->offset);
-        $this->assertSame(BeatmapsetEvent::count(), $eventCountBeforeTry);
     }
 
     /**
@@ -192,29 +183,54 @@ public function testBeatmapsetUpdateMetadataAsOwner($state)
         ]);
         $newGenre = Genre::factory()->create();
         $newLanguage = Language::factory()->create();
-        $newOffset = $beatmapset->offset + 25;
 
         $resultGenreId = $ok ? $newGenre->getKey() : $beatmapset->genre_id;
         $resultLanguageId = $ok ? $newLanguage->getKey() : $beatmapset->language_id;
-        $resultOffset = $ok ? $newOffset : $beatmapset->offset;
 
-        $eventCountBeforeTry = BeatmapsetEvent::count();
+        $this->expectCountChange(fn () => BeatmapsetEvent::count(), $ok ? 2 : 0);
 
         $this->actingAsVerified($owner)
             ->put(route('beatmapsets.update', ['beatmapset' => $beatmapset->getKey()]), [
                 'beatmapset' => [
                     'genre_id' => $newGenre->getKey(),
                     'language_id' => $newLanguage->getKey(),
-                    'offset' => $newOffset,
                 ],
             ])->assertStatus($ok ? 200 : 403);
 
         $beatmapset->refresh();
 
         $this->assertSame($resultGenreId, $beatmapset->genre_id);
         $this->assertSame($resultLanguageId, $beatmapset->language_id);
-        $this->assertSame($resultOffset, $beatmapset->offset);
-        $this->assertSame(BeatmapsetEvent::count(), $ok ? $eventCountBeforeTry + 3 : $eventCountBeforeTry);
+    }
+
+    /**
+     * @dataProvider dataProviderForTestBeatmapsetUpdateOffset
+     */
+    public function testBeatmapsetUpdateOffset(string $userGroupOrOwner, bool $ok): void
+    {
+        $beatmapset = Beatmapset::factory()->create([
+            'approved' => Beatmapset::STATES['ranked'],
+        ]);
+
+        $user = $userGroupOrOwner === 'owner'
+            ? $beatmapset->user
+            : User::factory()->withGroup($userGroupOrOwner)->create();
+
+        $newOffset = $beatmapset->offset + 25;
+        $expectedOffset = $ok ? $newOffset : $beatmapset->offset;
+
+        $this->expectCountChange(fn () => BeatmapsetEvent::count(), $ok ? 1 : 0);
+
+        $this->actingAsVerified($user)
+            ->put(route('beatmapsets.update', ['beatmapset' => $beatmapset->getKey()]), [
+                'beatmapset' => [
+                    'offset' => $newOffset,
+                ],
+            ])->assertStatus($ok ? 200 : 403);
+
+        $beatmapset->refresh();
+
+        $this->assertSame($expectedOffset, $beatmapset->offset);
     }
 
     public function beatmapsetStatesDataProvider()
@@ -223,4 +239,16 @@ public function beatmapsetStatesDataProvider()
             return [$state];
         }, array_keys(Beatmapset::STATES));
     }
+
+    public function dataProviderForTestBeatmapsetUpdateOffset(): array
+    {
+        return [
+            ['admin', true],
+            ['bng', false],
+            ['default', false],
+            ['gmt', false],
+            ['nat', false],
+            ['owner', false],
+        ];
+    }
 }