Default record agreement #2094
Default record agreement #2094
Conversation
WalkthroughThree new boolean user attributes— Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API
participant DB
User->>Frontend: Toggle consent fields in settings
Frontend->>API: PATCH /users/{id} with consent fields
API->>DB: Update user record with new consent values
DB-->>API: Confirmation
API-->>Frontend: Updated user resource (incl. consent fields)
Frontend->>User: Show updated consent status
sequenceDiagram
participant User
participant Frontend
participant API
User->>Frontend: Opens profile/settings or join modal
Frontend->>API: GET /users/me
API-->>Frontend: User data with consent fields
Frontend->>User: Show consent checkboxes/toggles pre-filled
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
database/migrations/2025_03_30_213313_add_default-record-agreement.php (1)
1-1: Migration filename contains a future date.The date in the migration filename (2025_03_30) is in the future. Migration filenames typically use the date when the migration was created.
Consider renaming the file to use the current date to avoid confusion in version control history.
app/Models/User.php (1)
42-44: Fillable property update looks good with minor formatting issue.The new agreement fields are correctly added to the
$fillablearray, but there's a trailing comma at the end of line 42 that's inconsistent with the code style.Consider removing the trailing comma for consistency:
-'firstname', 'lastname', 'email', 'password', 'external_id', 'guid', 'domain', 'locale', 'bbb_skip_check_audio', +'firstname', 'lastname', 'email', 'password', 'external_id', 'guid', 'domain', 'locale', 'bbb_skip_check_audio', 'record_agreement', 'record_video_agreement', 'record_attendance_agreement', 'authenticator',lang/de/admin.php (1)
451-453: LGTM with minor formatting issue.The German translations for the three recording agreement fields have been correctly added to the users section. There's a small formatting inconsistency - line 453 has extra whitespace at the end.
- 'record_attendance_agreement' => 'Zustimmung zur Anwesenheitsaufzeichnung', + 'record_attendance_agreement' => 'Zustimmung zur Anwesenheitsaufzeichnung',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/Http/Controllers/api/v1/UserController.php(1 hunks)app/Http/Requests/UserRequest.php(1 hunks)app/Http/Resources/User.php(1 hunks)app/Models/User.php(2 hunks)database/migrations/2025_03_30_213313_add_default-record-agreement.php(1 hunks)lang/de/admin.php(1 hunks)lang/de/validation.php(1 hunks)lang/en/admin.php(1 hunks)lang/en/validation.php(1 hunks)resources/js/components/RoomJoinButton.vue(3 hunks)resources/js/components/UserTabOtherSettings.vue(4 hunks)
🔇 Additional comments (14)
app/Http/Resources/User.php (1)
85-87: Implementation of new agreement fields looks good.The addition of these three agreement fields (record_agreement, record_video_agreement, and record_attendance_agreement) follows the established pattern for exposing user attributes in the API response.
app/Http/Controllers/api/v1/UserController.php (1)
210-218: New agreement field handling looks good.The implementation for updating the three agreement fields follows the established pattern, checking if each attribute exists in the request before updating it on the user model.
app/Models/User.php (1)
65-67: Boolean casting for agreement fields looks good.Properly casting the new agreement fields as booleans ensures consistent type handling throughout the application.
lang/en/validation.php (1)
42-44: LGTM: New validation attributes added properly.The three new validation attributes for recording agreements have been correctly added to the attributes array. These entries will provide human-readable labels when these fields appear in validation messages.
lang/de/validation.php (1)
42-44: LGTM: German validation attributes added properly.The three new validation attributes have been correctly added to the German validation file with appropriate translations. These align perfectly with the English version and provide proper localization support.
app/Http/Requests/UserRequest.php (1)
23-25: LGTM: Validation rules correctly implemented.The validation rules for the three new agreement fields are correctly defined as optional boolean values using the 'sometimes', 'required', and 'boolean' rules. This approach ensures that when these fields are present in the request, they must be valid boolean values.
lang/en/admin.php (1)
451-453: LGTM: New translations for record agreement settings.The addition of these three new record agreement translations for user settings is clear and consistent with the existing naming patterns in the file.
resources/js/components/RoomJoinButton.vue (3)
84-91: Good addition of profile link for agreement settings.Adding a link to the user profile provides a convenient way for users to manage their agreement preferences.
128-135: Good addition of profile link for agreement settings.Similar to the previous link, this provides users with a convenient way to manage their recording agreement preferences.
212-215: LGTM: Initialization of agreement values from user preferences.Good implementation of initializing the agreement values from the user's stored preferences. This ensures consistent user experience by remembering their previous choices.
resources/js/components/UserTabOtherSettings.vue (4)
28-96: LGTM: Well-structured UI for agreement settings.The implementation of the three agreement toggle switches follows the same pattern as existing settings, maintaining consistency in the UI. All necessary elements are included:
- Proper labels with translations
- Toggle switches bound to model properties
- Form validation error handling
- Appropriate test attributes
117-117: LGTM: Auth store import and instantiation.Proper import and instantiation of the auth store for accessing and updating the current user data.
Also applies to: 141-142
174-176: LGTM: Including new agreement fields in API request.The agreement fields are correctly included in the data sent to the API when updating the user.
179-185: LGTM: Refreshing current user data after update.Good implementation of refreshing the current user data after a successful update if the updated user is the currently authenticated user. This ensures that the UI reflects the latest user settings.
| $table->boolean('record_agreement')->default(false)->after('bbb_skip_check_audio'); | ||
| $table->boolean('record_video_agreement')->default(false)->after('bbb_skip_check_audio'); | ||
| $table->boolean('record_attendance_agreement')->default(false)->after('bbb_skip_check_audio'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix column ordering approach in migration.
Using the same after('bbb_skip_check_audio') for all three columns could lead to unpredictable column ordering, as each addition modifies the table structure.
For predictable column ordering, please chain the after clauses:
-$table->boolean('record_agreement')->default(false)->after('bbb_skip_check_audio');
-$table->boolean('record_video_agreement')->default(false)->after('bbb_skip_check_audio');
-$table->boolean('record_attendance_agreement')->default(false)->after('bbb_skip_check_audio');
+$table->boolean('record_agreement')->default(false)->after('bbb_skip_check_audio');
+$table->boolean('record_video_agreement')->default(false)->after('record_agreement');
+$table->boolean('record_attendance_agreement')->default(false)->after('record_video_agreement');| const authStore = useAuthStore(); | ||
| console.log("authStore", authStore.currentUser); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debugging console.log statement.
This console.log statement appears to be for debugging purposes and should be removed before merging to production.
-console.log("authStore", authStore.currentUser);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("authStore", authStore.currentUser); |
|
@q16marvin Thanks for your PR. However, we are planning to merge #1697, which includes several changes to the join dialog, etc. Just to let you know, there will be some changes required. |
|
@q16marvin Please rebase this PR, due to the changes of #1697 and add software tests. It might make sence to also add the new "streaming" consent |
samuelwei
force-pushed
the
develop
branch
2 times, most recently
from
88ee280 to
bc9287a
Compare
2 participants
Type
Checklist
Changes
Other information
Summary by CodeRabbit