-
-
Notifications
You must be signed in to change notification settings - Fork 99
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
add event_awards table #3020
Draft
Jamiras
wants to merge
1
commit into
RetroAchievements:master
Choose a base branch
from
Jamiras:feature/event_award
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
add event_awards table #3020
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,4 +8,5 @@ enum ImageUploadType | |
{ | ||
case News; | ||
case HubBadge; | ||
case EventAward; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
136 changes: 136 additions & 0 deletions
136
app/Filament/Resources/GameResource/RelationManagers/EventAwardsRelationManager.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\Filament\Resources\GameResource\RelationManagers; | ||
|
||
use App\Filament\Enums\ImageUploadType; | ||
use App\Filament\Resources\NewsResource\Actions\ProcessUploadedImageAction; | ||
use App\Models\EventAward; | ||
use App\Models\Game; | ||
use App\Models\System; | ||
use App\Models\User; | ||
use Filament\Forms; | ||
use Filament\Forms\Form; | ||
use Filament\Resources\RelationManagers\RelationManager; | ||
use Filament\Tables; | ||
use Filament\Tables\Table; | ||
use Illuminate\Database\Eloquent\Model; | ||
use Illuminate\Support\Facades\Auth; | ||
|
||
class EventAwardsRelationManager extends RelationManager | ||
{ | ||
protected static string $relationship = 'eventAwards'; | ||
|
||
public static function canViewForRecord(Model $ownerRecord, string $pageClass): bool | ||
{ | ||
if ($ownerRecord->ConsoleID != System::Events) { | ||
return false; | ||
} | ||
|
||
/** @var User $user */ | ||
$user = Auth::user(); | ||
|
||
return $user->can('manage', EventAward::class); | ||
} | ||
|
||
public function form(Form $form): Form | ||
{ | ||
/** @var Game $game */ | ||
$game = $this->getOwnerRecord(); | ||
$nextTier = ($game->eventAwards()->max('tier_index') ?? 0) + 1; | ||
|
||
return $form | ||
->schema([ | ||
Forms\Components\TextInput::make('label') | ||
->minLength(2) | ||
->maxLength(40) | ||
->required(), | ||
|
||
Forms\Components\TextInput::make('achievements_required') | ||
->numeric() | ||
->required(), | ||
|
||
Forms\Components\TextInput::make('tier_index') | ||
->default($nextTier) | ||
->numeric() | ||
->readOnly() | ||
->required(), | ||
|
||
Forms\Components\Section::make('Media') | ||
->icon('heroicon-s-photo') | ||
->schema([ | ||
// Store a temporary file on disk until the user submits. | ||
// When the user submits, put in storage. | ||
Forms\Components\FileUpload::make('image_asset_path') | ||
->label('Badge') | ||
->disk('livewire-tmp') // Use Livewire's self-cleaning temporary disk | ||
->image() | ||
->rules([ | ||
'dimensions:width=96,height=96', | ||
]) | ||
->acceptedFileTypes(['image/jpeg', 'image/png', 'image/gif']) | ||
->maxSize(1024) | ||
->maxFiles(1) | ||
->previewable(true), | ||
]) | ||
->columns(2), | ||
]); | ||
} | ||
|
||
public function table(Table $table): Table | ||
{ | ||
return $table | ||
->recordTitleAttribute('label') | ||
->columns([ | ||
Tables\Columns\TextColumn::make('tier_index'), | ||
|
||
Tables\Columns\ImageColumn::make('badgeUrl') | ||
->label('Badge') | ||
->size(config('media.icon.md.width')), | ||
|
||
Tables\Columns\TextColumn::make('label') | ||
->label('Label'), | ||
|
||
Tables\Columns\TextColumn::make('achievements_required'), | ||
]) | ||
->filters([ | ||
|
||
]) | ||
->headerActions([ | ||
Tables\Actions\CreateAction::make() | ||
->mutateFormDataUsing(function (array $data): array { | ||
$this->processUploadedImage($data, null); | ||
|
||
return $data; | ||
}), | ||
]) | ||
->actions([ | ||
Tables\Actions\ActionGroup::make([ | ||
Tables\Actions\EditAction::make() | ||
->mutateFormDataUsing(function (Model $record, array $data): array { | ||
/** @var EventAward $record */ | ||
$this->processUploadedImage($data, $record); | ||
|
||
return $data; | ||
}), | ||
Tables\Actions\DeleteAction::make(), | ||
]), | ||
]); | ||
} | ||
|
||
protected function processUploadedImage(array &$data, ?EventAward $record): void | ||
{ | ||
$existingImage = $record->image_asset_path ?? '/Images/000001.png'; | ||
|
||
if (isset($data['image_asset_path'])) { | ||
$data['image_asset_path'] = (new ProcessUploadedImageAction())->execute( | ||
$data['image_asset_path'], | ||
ImageUploadType::EventAward, | ||
); | ||
} else { | ||
// If no new image was uploaded, retain the existing image. | ||
$data['image_asset_path'] = $existingImage; | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\Models; | ||
|
||
use App\Support\Database\Eloquent\BaseModel; | ||
use Illuminate\Database\Eloquent\Relations\BelongsTo; | ||
|
||
class EventAward extends BaseModel | ||
{ | ||
protected $table = 'event_awards'; | ||
|
||
protected $fillable = [ | ||
'game_id', | ||
'tier_index', | ||
'label', | ||
'achievements_required', | ||
'image_asset_path', | ||
]; | ||
|
||
// == accessors | ||
|
||
public function getBadgeUrlAttribute(): string | ||
{ | ||
return media_asset($this->image_asset_path); | ||
} | ||
|
||
// == mutators | ||
|
||
// == relations | ||
|
||
/** | ||
* @return BelongsTo<Game, EventAward> | ||
*/ | ||
public function game(): BelongsTo | ||
{ | ||
return $this->belongsTo(Game::class, 'game_id', 'ID'); | ||
} | ||
|
||
// == scopes | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\Policies; | ||
|
||
use App\Models\EventAward; | ||
use App\Models\Role; | ||
use App\Models\User; | ||
use Illuminate\Auth\Access\HandlesAuthorization; | ||
|
||
class EventAwardPolicy | ||
{ | ||
use HandlesAuthorization; | ||
|
||
public function manage(User $user): bool | ||
{ | ||
return $user->hasRole(Role::EVENT_MANAGER); | ||
} | ||
|
||
public function viewAny(?User $user): bool | ||
{ | ||
return true; | ||
} | ||
|
||
public function view(?User $user, EventAward $eventAward): bool | ||
{ | ||
return true; | ||
} | ||
|
||
public function create(User $user): bool | ||
{ | ||
return $user->hasRole(Role::EVENT_MANAGER); | ||
} | ||
|
||
public function update(User $user, EventAward $eventAward): bool | ||
{ | ||
return $user->hasRole(Role::EVENT_MANAGER); | ||
} | ||
|
||
public function delete(User $user, EventAward $eventAward): bool | ||
{ | ||
return $user->hasRole(Role::EVENT_MANAGER); | ||
} | ||
} |
36 changes: 36 additions & 0 deletions
36
database/migrations/2025_01_05_000000_create_event_awards_table.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
use Illuminate\Database\Migrations\Migration; | ||
use Illuminate\Database\Schema\Blueprint; | ||
use Illuminate\Support\Facades\Schema; | ||
|
||
return new class() extends Migration { | ||
public function up(): void | ||
{ | ||
Schema::create('event_awards', function (Blueprint $table) { | ||
$table->bigIncrements('id'); | ||
$table->unsignedBigInteger('game_id'); | ||
$table->integer('tier_index'); | ||
$table->string('label', 40); | ||
$table->integer('achievements_required'); | ||
$table->string('image_asset_path', 50); | ||
$table->timestamps(); | ||
}); | ||
|
||
Schema::table('event_awards', function (Blueprint $table) { | ||
$table->foreign('game_id') | ||
->references('ID') | ||
->on('GameData') | ||
->onDelete('cascade'); | ||
|
||
$table->unique(['game_id', 'tier_index']); | ||
}); | ||
} | ||
|
||
public function down(): void | ||
{ | ||
Schema::dropIfExists('event_awards'); | ||
} | ||
}; |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed all of the code in this PR and it all looks logically sound. I want to steer the conversation towards this migration, as it drives the architectural underpinnings of the PR and could have profound impacts on how the system stores and manages events going forward.
We're creating increasingly tight coupling between events and games, and I'm not entirely sure this is desirable unless we want to establish that events will continue to live on indefinitely as games in the database.
My view could be changed, but I have a feeling that the path we should be driving towards is
events
existing as their own kind of entity. This would facilitate varied kinds of events, an actual events dashboard / home page, and potentially even a quests system in the future.I know we don't have an
events
table yet, but it seems like this may actually be our best opportunity to create one. The migration below is me spitballing and have only thought about this for 5 minutes. The core idea here is Event Awards are associated to a new type of entity -- anEvent
-- not games:With this schema, events are still associated to games via
legacy_game_id
, but event_awards are now associated to events. As more event stuff is built out, it can be attached toevents
rather thanGameData
, which will make extraction of events from games significantly easier in the future.There's both an immediate and long-term benefit of pursuing this path in that we already beginning to break the coupling of events to games while maintaining backwards compatibility through
legacy_game_id
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree with the idea, I don't think we're in a place where it's practical yet.
The way the AotW2025 event is functioning now, it relies on the
player_achievements
table to hold the unlocks and the GameMetrics aggregate jobs to track player counts and per-week participation. And because unlocks are associated to achievements, and achievements are associated to games, there's already a tight coupling that will be a lot harder to fix than adding an extra foreign key to the event_awards table to point an event instead of a game.To do this effectively, we'd have to have a
player_event_achievements
table to track event achievement unlocks, and EventMetrics jobs to update player counts and participation. Then we'd have to duplicate a lot of the widgets on the game page to query and use the alternate data.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do acknowledge these valid concerns regarding event achievement entities. My suggestion is that we begin taking an incremental approach with the
events
table:The goal isn't to decouple everything immediately, but to create a path for progressive decoupling as resources allow. Does that seem practical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll think about this some more. It's probably reasonable to make an
event
object wrap a game, so we can keep all the game-derived functionality working while offloading some of the non-game dependencies.