-
-
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
Make Events a separate entity in management app #3024
base: master
Are you sure you want to change the base?
Conversation
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.
We probably should have an icon for this:
I am also sensing it may be good to put EventAchievementResource
and EventResource
inside of a navigation group. "Events" as a group name may be redundant and/or confusing with "Events" the entity label.
I'm only ideating, but here's what comes to mind within 30 seconds:
- Event Ops
- Event Operations
- Event Workspace
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 didn't even realize it was there. I'd rather not have it show up at all. Event Achievements are basically just a pivot table connecting one achievement to another.
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'm wondering if it may make sense for the default sort order in the table to be by either active_from
or active_through
. In other words, currently-running events could by default bubble up to the top.
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.
active_through
is a derived value and I couldn't figure out how to sort by it, so the sort is on active_util
and the column displays active_through
.
And active_until
is set as the default sort order: https://github.com/RetroAchievements/RAWeb/pull/3024/files#diff-332a6307e080b58ffc6d1a791d50ed7d4c4fcdc020f1ff913f957c152900e92aR173
$table->bigIncrements('id'); | ||
$table->unsignedBigInteger('legacy_game_id'); | ||
$table->string('image_asset_path', 50)->default('/Images/000001.png'); | ||
$table->string('slug', 20)->nullable(); |
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.
This is nullable but seems like it may be critical for SEO-friendly URLs, especially if the model has no title
field (I assume it's inherited from the game this model is currently wrapping).
Should slug
be required?
It also seems like slugs should be distinct.
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.
slug
s should be distinct.
I didn't want to make it required because we'd have to somehow generate values for all the existing event records. I guess we can just set it to the ID initially.
Done (both distinct and not nullable).
$table->date('active_from')->nullable(); | ||
$table->date('active_until')->nullable(); |
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 wonder if we want indexes on both of these - I reckon they'll probably be queried against very frequently.
app/Models/Event.php
Outdated
'active_through', | ||
]; | ||
|
||
// == logging |
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.
nit: the indentation here is slightly off
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.
Done.
public function setActiveThroughAttribute(Carbon|string|null $value): void | ||
{ | ||
if (is_string($value)) { | ||
$value = Carbon::parse($value); | ||
} | ||
|
||
$this->active_until = $value ? $value->clone()->addDays(1) : null; | ||
} |
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.
It looks like we're using a virtual active_through
attribute that adds days to handle inclusive date ranges.
It makes sense what we're doing. Initially, my brain went to:
// What happens with these cases?
$event->active_through = "2025-12-31";
$event->active_through = "2025-02-28";
After thinking a bit about this, it makes sense, but I'm wondering if the distinction between "until" and "through" is ambiguous. Maybe we should rename this to ends_on
instead, which:
- Better implies inclusivity (an event that "ends on Jan 13" includes Jan 13)
- Pairs naturally with
active_from
. - Is more commonly used in event contexts (understandable by both players and engineers).
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.
until
was chosen for event_achievements to simplify the query (now >= from and now < until
), but through
makes more sense for the UI ("active from 1/1 through 1/7", even though the database has 1/1 and 1/8).
I'm not sure that distinction is necessary for the event record itself, but I could see us filtering on active events too.
While the distinction is slightly muddled, I don't think it's ambiguous.
until
is when the event/achievement stops being active- Dictionary definition of
until
is "up to, but not including" - "The event is active until the 14th"
- Dictionary definition of
through
is the last day (inclusive) during which the event/achievement is active.- One dictionary definition of
through
is "to the end of (an experience or activity)" - "The event is active through the 13th".
- One dictionary definition of
If anything, I feel that ends_on
is ambiguous. If the event "ends on the 13th", that could be at any time during the day. "through" indicates it's available for the whole day. ends_at
would provide a specific time, and that's exactly what active_until
does.
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 think we should still add a type
column, even though there is currently only one type ("achievement_hunt"):
// `EventType` enum? achievement_hunt, quest, competition, etc
$table->string('type', 50)->default('achievement_hunt');
enum EventType: string {
case AchievementHunt = "achievement_hunt"; // I don't know if this is the best name.
}
// models/Event.php
protected $casts = [
'type' => EventType::class,
];
This may seem redundant, but it's a very small change that:
- Documents the current coupling that exists (these are achievement hunt events that need associated games).
- Creates a breadcrumb for future event types that might not need games.
- Doesn't require any major refactoring of the current implementation.
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'm trying to avoid over-engineering this. When we have a new event type, we'll inevitably need to add new columns or new subtables, and then we'll probably have a better idea for classifying the different types of events.
New event types will probably require changes to the related tables as well. event_awards
(as currently proposed) has an achievements_required
column.
}) | ||
->required(), | ||
]) | ||
->modalHeading('Add game to related hub') |
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.
Recommend "Add event to related hub"
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.
Done.
->iconButton() | ||
->requiresConfirmation() | ||
->color('danger') | ||
->modalHeading('Remove related game from related hub') |
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.
Recommend "Remove event from related hub"
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.
Done.
->bulkActions([ | ||
Tables\Actions\BulkAction::make('remove') | ||
->label('Remove selected') | ||
->modalHeading('Remove selected related hub links from hub') |
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.
Recommend "Remove selected events from hub"
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.
Done.
Provides a minimalistic
events
table as suggested here. Much of the data is still managed by the underlying game/achievements objects.At this point, no
events
records will exist. The following query should be run on prod after deployment:This PR also adds a new page to the management console (accessible by Event Managers and Administrators - not accessible by Developers) for editing event records. The raw game records are still editable by Developers, but the event extensions no longer appear in the achievement pages. Event achievements are also now their own resource type.
As such, we will need to ensure appropriate users are given the Event Manager role.
Additionally, I've removed all of the non-event related data from the event game and achievement player-facing pages. I've also removed the dev box from both pages, and the comments field from the achievement page. I believe the only functionality needed from the dev box is adding/removing related games, which is available through the management console.
This includes removing the ability to reset individual event achievements. The player can still reset the entire event via the Progress widget on the event page.
I have not yet migrated the "Create Event" functionality from the admin blade panel. This PR is big enough already.
If you want to populate
events
records for all event games, you can run this query:But I want to try to consolidate some event games into singular event entities after #3020 (and the followup PR for displaying badges) are merged. For example http://localhost:64000/hub/3645 shows 8 event games for AotW 2019. The Bronze/Gold/Silver and # 1, # 2, # 3 games are just for awarding badges, but might require two separate events if players were awarded both. The actual event should be created in a manner similar to the commands listed here. The Spooky and Spring events would be kept separate.