-
Notifications
You must be signed in to change notification settings - Fork 30
feat: Add option in room to always use sytem-wide default presentation #2746
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
base: develop
Are you sure you want to change the base?
Changes from all commits
eac6018
9732da9
ce6c95d
3da24f5
802d541
5d4340a
6541e56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| <?php | ||
|
|
||
| namespace App\Http\Requests; | ||
|
|
||
| use Illuminate\Foundation\Http\FormRequest; | ||
|
|
||
| class UpdateRoomSystemDefaultPresentation extends FormRequest | ||
| { | ||
| public function rules() | ||
| { | ||
| return [ | ||
| 'use_in_meeting' => ['required', 'boolean'], | ||
| 'default' => ['required', 'boolean'], | ||
| ]; | ||
| } | ||
samuelwei marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,39 @@ | ||||||||||||||||||||||||||||||
| <?php | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| use App\Models\RoomFile; | ||||||||||||||||||||||||||||||
| use Illuminate\Database\Migrations\Migration; | ||||||||||||||||||||||||||||||
| use Illuminate\Database\Schema\Blueprint; | ||||||||||||||||||||||||||||||
| use Illuminate\Support\Facades\Schema; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return new class extends Migration | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Run the migrations. | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| public function up(): void | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| Schema::table('rooms', function (Blueprint $table) { | ||||||||||||||||||||||||||||||
| $table->boolean('use_system_default_presentation_in_meeting')->default(true); | ||||||||||||||||||||||||||||||
| $table->boolean('use_system_default_presentation_as_default')->default(true); | ||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| RoomFile::where('default', true) | ||||||||||||||||||||||||||||||
| ->where('use_in_meeting', true) | ||||||||||||||||||||||||||||||
| ->each(function (RoomFile $file) { | ||||||||||||||||||||||||||||||
| $file->room->use_system_default_presentation_as_default = false; | ||||||||||||||||||||||||||||||
| $file->room->save(); | ||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
|
Comment on lines
+20
to
+25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Data migration triggers N+1 queries and risks model/observer side effects. Two concerns with using Eloquent models here:
A raw DB update avoids both issues: Proposed fix- RoomFile::where('default', true)
- ->where('use_in_meeting', true)
- ->each(function (RoomFile $file) {
- $file->room->use_system_default_presentation_as_default = false;
- $file->room->save();
- });
+ DB::table('rooms')
+ ->whereIn('id', function ($query) {
+ $query->select('room_id')
+ ->from('room_files')
+ ->where('default', true)
+ ->where('use_in_meeting', true);
+ })
+ ->update(['use_system_default_presentation_as_default' => false]);You'll need to add 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Reverse the migrations. | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| public function down(): void | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| Schema::table('rooms', function (Blueprint $table) { | ||||||||||||||||||||||||||||||
| $table->dropColumn('use_system_default_presentation_in_meeting'); | ||||||||||||||||||||||||||||||
| $table->dropColumn('use_system_default_presentation_as_default'); | ||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,12 +107,14 @@ | |
| 'feature_disabled_system' => ':name is disabled. Please contact the administrator.', | ||
| 'files' => [ | ||
| 'confirm_delete' => 'Do you want to delete this file :filename?', | ||
| 'default' => 'Default', | ||
| 'default' => 'Default presentation', | ||
| 'delete' => 'Delete file', | ||
| 'download_hidden' => 'Download hidden', | ||
| 'download_visible' => 'Download visible', | ||
| 'download_allowed' => 'Download allowed', | ||
| 'download_not_allowed' => 'Download not allowed', | ||
| 'downloadable' => 'Downloadable', | ||
| 'edit' => 'Edit file', | ||
| 'configure' => 'Configure file', | ||
|
Comment on lines
+112
to
+115
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check for dangling references to removed translation keys
rg -n --type=vue --type=js --type=php \
-e "files\.download_hidden" \
-e "files\.download_visible" \
-e "files\.edit['\"]" \
-e "use_in_next_meeting" \
-e "download_hidden" \
-e "download_visible" \
.Repository: THM-Health/PILOS Length of output: 87 🏁 Script executed: #!/bin/bash
# Also search in JSON files and any files that might reference these keys
rg -n "download_hidden|download_visible|use_in_next_meeting" \
--type=json --type=ts --type=jsx .Repository: THM-Health/PILOS Length of output: 87 🏁 Script executed: #!/bin/bash
# Check for other locale files that may need updates
fd -e php -path "*/lang/*" | head -20Repository: THM-Health/PILOS Length of output: 228 🏁 Script executed: #!/bin/bash
# Read the actual file to understand the context of changes
cat -n lang/en/rooms.php | sed -n '100,150p'Repository: THM-Health/PILOS Length of output: 2927 🏁 Script executed: #!/bin/bash
# Search for dangling references to removed keys with simpler patterns
echo "=== Checking for references to removed keys ==="
rg "download_hidden|download_visible" -n --no-heading
rg "files\.edit" -n --no-heading
rg "use_in_next_meeting" -n --no-headingRepository: THM-Health/PILOS Length of output: 7710 🏁 Script executed: #!/bin/bash
# Find all locale files
echo "=== Finding all locale translation files ==="
find . -path "*/lang/*/rooms.php" -type fRepository: THM-Health/PILOS Length of output: 231 🏁 Script executed: #!/bin/bash
# Check for files that reference the old keys
rg "download_hidden" . 2>/dev/null | head -20Repository: THM-Health/PILOS Length of output: 1063 🏁 Script executed: #!/bin/bash
# Look for Vue component files that might reference these translation keys
find . -name "*.vue" -o -name "*.js" | head -20 | xargs rg -l "download_hidden|download_visible|use_in_next_meeting" 2>/dev/null || echo "No matches found"Repository: THM-Health/PILOS Length of output: 76 Critical: Translation keys were changed in English but not propagated to other locales or E2E tests. The removed keys (
This creates a critical inconsistency where E2E tests will fail and non-English locales will have mismatched translation keys. All language files must be updated together with the same keys, and E2E tests must be updated to check for the new keys ( Additionally, line 141 has a key naming inconsistency: 🤖 Prompt for AI Agents |
||
| 'configure_dialog_title' => 'Configure ":name"', | ||
| 'configure_system_default' => 'Configure system-wide default presentation', | ||
| 'filter' => [ | ||
| 'all' => 'All files', | ||
| 'downloadable' => 'Downloadable files', | ||
|
|
@@ -126,6 +128,8 @@ | |
| 'filename' => 'Filename', | ||
| 'uploaded_at' => 'Added', | ||
| ], | ||
| 'system_default' => 'System-wide default presentation', | ||
| 'system_default_description' => 'Automatically used when no other presentation is enabled for the next video conference', | ||
| 'terms_of_use' => [ | ||
| 'accept' => 'I accept the terms of use', | ||
| 'required' => 'You must accept the terms of use before downloading this file.', | ||
|
|
@@ -134,8 +138,9 @@ | |
| 'title' => 'Files', | ||
| 'upload' => 'Upload files', | ||
| 'uploaded' => 'File \':name\' uploaded', | ||
| 'use_in_next_meeting' => 'Use in the next meeting', | ||
| 'use_in_next_meeting_disabled' => 'Not available in video conference', | ||
| 'always_available_in_meeting' => 'Always available in next video conference', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Key name is inconsistent with its siblings and the value text is ambiguous.
✏️ Proposed rename and wording fix- 'always_available_in_meeting' => 'Always available in next video conference',
+ 'always_available_in_next_meeting' => 'Always available in video conferences',Any component referencing 🤖 Prompt for AI Agents |
||
| 'available_in_next_meeting' => 'Available in next video conference', | ||
| 'not_available_in_next_meeting' => 'Not available in next video conference', | ||
| 'view' => 'View file', | ||
| ], | ||
| 'first_and_lastname' => 'First- and last name', | ||
|
|
||
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.
Two issues in
updateSystemDefault().1. Inconsistent boolean access — Lines 147 and 159 use
$request->use_in_meetingand$request->default(raw property access), whileupdate()at lines 113/117 uses$request->boolean(...). Raw access can pass through strings like"1"or"0"rather than actual booleans. Use$request->boolean()for consistency.2. Setting
use_in_meeting = falsedoesn't clearas_default— If the user disables the system presentation for the meeting (use_in_meeting = false) without explicitly sendingdefault = false, the room ends up withuse_system_default_presentation_as_default = truewhileuse_system_default_presentation_in_meeting = false. This is an inconsistent state — the presentation can't be the default if it's not included in the meeting.Proposed fix
public function updateSystemDefault(UpdateRoomSystemDefaultPresentation $request, Room $room) { if ($request->has('use_in_meeting')) { - $room->use_system_default_presentation_in_meeting = $request->use_in_meeting; + $room->use_system_default_presentation_in_meeting = $request->boolean('use_in_meeting'); + + // If system default is removed from the meeting, it cannot remain the default + if (!$request->boolean('use_in_meeting')) { + $room->use_system_default_presentation_as_default = false; + } } if ($request->has('default')) { // Make other files not the default - if ($request->default === true) { + if ($request->boolean('default')) { $room->files()->update(['default' => false]); // If system default is set as default, it must also be used in the next meeting $room->use_system_default_presentation_in_meeting = true; } - $room->use_system_default_presentation_as_default = $request->default; + $room->use_system_default_presentation_as_default = $request->boolean('default'); } $room->save();🤖 Prompt for AI Agents