New crediting system for writers and sound editors#180
New crediting system for writers and sound editors#180ShadyMedic wants to merge 10 commits intodevelopmentfrom
Conversation
We're adding a new user_id-bount column to the quest and npc_quest tables.
This comment was marked as spam.
This comment was marked as spam.
Development
List of quests that were scripted by a user are also displayed on their user page (/cast/)
The newly added information is not integrated into the existing visuals, being displayed as (almost) plain text with hyperlinks.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
Views/cast.phtml (1)
99-111: Consider handling missing script files gracefully.Line 107 generates a link to the script file at
scripts/{degenerated_name}.txt. If the file hasn't been uploaded yet, users will encounter a 404 or storage error. Consider either:
- Only showing the script link when the file exists
- Adding a note in the TODO about verifying file existence
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Views/cast.phtml` around lines 99 - 111, The template renders a script file link for each $scriptedQuest using getDegeneratedName() and storageUrl('scripts/... .txt') regardless of whether the file exists; update the view to check for the script file's existence before emitting the "Script file" anchor (or render a clear placeholder/“Not uploaded” text) — use the same identifiers ($scriptedQuests, $scriptedQuest, getDegeneratedName(), storageUrl) to locate the logic and add a conditional that verifies the storage/file presence and only outputs the link when present, otherwise show a non-clickable notice or omit the link and update the TODO to document existence verification.Models/Website/User.php (1)
571-576: Clarify the 've' alias purpose.The aliases
veandve_id(lines 571-572) don't follow the established naming pattern.va= voice actor,se= sound editor, but what doesvestand for? If this is a typo forva, it should be corrected. If intentional, consider adding a comment explaining the abbreviation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Models/Website/User.php` around lines 571 - 576, The cases 've' and 've_id' in the switch inside Models/Website/User.php are inconsistent with the established aliases (e.g., 'va' = voice actor, 'se' = sound editor); either correct these to the intended alias ('va' and 'va_id') everywhere this switch/mapping is used (update the case labels and any related mapping keys/uses referencing 've'/'ve_id'), or if 've' is intentional, add a concise inline comment next to the case labels explaining that 've' stands for the specific role and why it differs from 'va' to avoid confusion.Models/Website/ContentManager.php (1)
186-200: Add missing return type declaration for consistency.The method is missing a return type declaration. Other methods in this class and the adjacent
getEditorsNpcsByQuests()include explicit return types.♻️ Proposed fix
-public function getWritersQuests(int $writerId) +public function getWritersQuests(int $writerId): array🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Models/Website/ContentManager.php` around lines 186 - 200, The method getWritersQuests is missing a return type declaration; update the method signature for getWritersQuests(int $writerId) to include the return type (declare it returns array) so it matches other methods like getEditorsNpcsByQuests(), ensuring the function signature explicitly reads getWritersQuests(int $writerId): array and keeps the existing behavior that returns an array of Quest instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@liquibase/website/changelog-3.sql`:
- Line 3: The changeset IDs "shady:2" and "shady:1" are duplicated across
changelogs; locate the changeset declarations (e.g., the lines starting with
"changeset shady:2" and "changeset shady:1") and rename them to globally unique
IDs (for example "shady-website:2" or add a namespace like "website:shady:2") in
the website changelogs, and ensure any matching rollback or reference entries
are updated to the new IDs so Liquibase recognizes them as distinct.
In `@Models/Website/Npc.php`:
- Around line 487-495: The getSoundEditor method accesses
$this->soundEditors[$quest->getId()] without verifying the key exists, which can
trigger an "Undefined array key" error; update getSoundEditor (method name:
getSoundEditor, property: $this->soundEditors, use of $quest->getId()) to safely
return null (or appropriate default) when the quest id key is missing by
checking array_key_exists or using a null-coalescing access for that key before
returning it.
In `@Models/Website/Quest.php`:
- Around line 39-49: The setter branch in Quest that handles
'writer'/'scriptAuthor' currently constructs a User when $value is null; update
the branch (in the Quest class handling the 'writer' / 'scriptAuthor' cases) to
explicitly guard against null/empty $value by setting $this->scriptAuthor to
null and breaking early when $value is null (or empty), otherwise maintain the
existing behavior of accepting a User instance or instantiating a User and
calling setData(['id' => $value]); ensure you reference the scriptAuthor
property and the $value parameter in your change so downstream code does not
receive a User with a null id.
- Around line 80-82: The assignment creates a new User even when
$result['writer'] may be null; add the same null guard used in the constructor:
check if $result['writer'] is not null (or empty) before instantiating User and
calling setData, and otherwise set $this->scriptAuthor to null (or leave it
unset). Update the block around $writer / setData / scriptAuthor in Quest to
only create and assign the User when the writer ID is present.
- Around line 164-165: The SQL JOIN for the sound editor should be changed to a
LEFT JOIN to avoid excluding NPCs with npc_quest.editor = NULL; update the query
fragment that currently reads "JOIN user sound_editor ON sound_editor.user_id =
npc_quest.editor" to "LEFT JOIN user sound_editor ON sound_editor.user_id =
npc_quest.editor". Then, when mapping DB rows to objects in the code that builds
the NPC (where you currently instantiate a User for the sound editor and call
$npc->setSoundEditor(...)), guard that creation with a null-check on the sound
editor id (e.g., check $dbRow['se_id'] !== null) before creating the User and
calling $npc->setSoundEditor($this, $se) so NPCs without editors are preserved.
In `@Views/quest.phtml`:
- Around line 37-44: The view dereferences possibly null values
(quest->getScriptAuthor(), npc->getSoundEditor($quest)) which can crash; update
the template to null-check getScriptAuthor() before calling getId()/getName()
and similarly check getSoundEditor($quest) (or
isset($npc->soundEditors[$quest->getId()]) / use null coalescing) before using
getId()/getName(); for missing values render a safe fallback label (e.g.,
"Unknown" or omit the link) so getNpcs(), getScriptAuthor(), and
getSoundEditor($quest) are only dereferenced when non-null.
---
Nitpick comments:
In `@Models/Website/ContentManager.php`:
- Around line 186-200: The method getWritersQuests is missing a return type
declaration; update the method signature for getWritersQuests(int $writerId) to
include the return type (declare it returns array) so it matches other methods
like getEditorsNpcsByQuests(), ensuring the function signature explicitly reads
getWritersQuests(int $writerId): array and keeps the existing behavior that
returns an array of Quest instances.
In `@Models/Website/User.php`:
- Around line 571-576: The cases 've' and 've_id' in the switch inside
Models/Website/User.php are inconsistent with the established aliases (e.g.,
'va' = voice actor, 'se' = sound editor); either correct these to the intended
alias ('va' and 'va_id') everywhere this switch/mapping is used (update the case
labels and any related mapping keys/uses referencing 've'/'ve_id'), or if 've'
is intentional, add a concise inline comment next to the case labels explaining
that 've' stands for the specific role and why it differs from 'va' to avoid
confusion.
In `@Views/cast.phtml`:
- Around line 99-111: The template renders a script file link for each
$scriptedQuest using getDegeneratedName() and storageUrl('scripts/... .txt')
regardless of whether the file exists; update the view to check for the script
file's existence before emitting the "Script file" anchor (or render a clear
placeholder/“Not uploaded” text) — use the same identifiers ($scriptedQuests,
$scriptedQuest, getDegeneratedName(), storageUrl) to locate the logic and add a
conditional that verifies the storage/file presence and only outputs the link
when present, otherwise show a non-clickable notice or omit the link and update
the TODO to document existence verification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d33ab537-1c88-4faa-b7e5-1586670ec75e
📒 Files selected for processing (10)
Controllers/Website/Cast.phpControllers/Website/Quest.phpControllers/Website/WebpageController.phpModels/Website/ContentManager.phpModels/Website/Npc.phpModels/Website/Quest.phpModels/Website/User.phpViews/cast.phtmlViews/quest.phtmlliquibase/website/changelog-3.sql
|
@kmaxii done reviewing |
All NPC pictures in blob storage have been converted to WebP. Updates image references in the NPC detail page, NPC card partial, the no_picture API filter, and the NPC copy operation.
…llow it to run locally (#184)
This update will add credits to writers for creating scripts (and allow users to download them directly from the website) and for editors for editing recordings of individual NPCs in individual quests.
Both groups of staff members will have these contributions listed in a dedicated sections on their user pages, just like voice actors have a section with submitted recordings.
On quest credits pages, there will also be text like "script by ..." somewhere.
Editors will be credited on the quest credits page either individually in each NPC-actor card (might be problematic because of insufficient space) or they might be displayed together under the voice actor attributions as "Sound editors: ..., ..., ...". Alternativelly, this "Sound editors" card might be a drop down, showing the sound editors paired to individual NPCs when expanded.
Development is done on
feat/staff-credits.Resolves #167
Summary by CodeRabbit
Release Notes