WIP: Feature: Greenlight3 import #2665
Conversation
WalkthroughAdds a Greenlight 3 import Artisan command, extends access_code support to alphanumeric legacy codes across backend and frontend, updates room formatting/input masking, and adds unit and E2E tests for GL3 import and access-code flows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
app/Http/Middleware/RoomAuthenticate.php (1)
86-99: Access code comparison now supports non‑numeric values; consider avoiding type jugglingSwitching to
$room->access_code == $accessCodecorrectly allows alphanumeric codes from headers. Ifaccess_codeis stored as a string in the DB, consider normalizing both sides to string and using strict comparison to avoid subtle PHP type coercion (e.g."012345"vs12345):$accessCode = (string) $request->header('Access-Code'); if ((string) $room->access_code === $accessCode) { $authenticated = true; // ... }Not required for correctness given your current tests, but would make the comparison behavior more explicit and future‑proof.
app/Models/Room.php (1)
383-388: Moderator invitation code formatting now depends onlegacy_code; verify GL3 behaviorConditionally formatting the code via
$this->legacy_code ? implode('-', str_split(...)) : $this->access_codeis a good step toward separating legacy vs non‑legacy display.Two things to double‑check:
- That
legacy_codeistrueonly for the legacy formats you intend (e.g. old 6‑digit numeric codes) and not for new 6‑character alphanumeric GL3 codes, otherwise those GL3 codes will still be split intoxxx-xxxhere while the share button shows them unformatted.- That this difference from the frontend behavior (which still groups all numeric codes into chunks) is intentional; if not, consider centralizing the formatting logic so moderator messages and the share popover stay in sync.
resources/js/components/RoomShareButton.vue (1)
117-123: Formatted access code now preserves non‑numeric GL3 codesThe
formattedAccessCodecomputed correctly avoids chunking non‑numeric codes (e.g. GL3012abc) while preserving the existing grouped format for numeric codes. This matches the new backend behavior and room tests.If you ever want to tighten the numeric check, you could swap
isNaNfor a simple regex like/^\d+$/for more predictable behavior, but it's not required here.tests/Backend/Unit/Console/helper/Greenlight3User.php (1)
5-27: Test helper DTO looks fine; consider typed properties for clarityThe
Greenlight3Userhelper cleanly captures the GL3 user shape and aligns with the existingGreenlight3Roomhelper.Optionally, you could add property types for quicker static feedback in tests:
public string $id; public string $name; public string $email; public ?string $external_id; public ?string $password_digest;Not necessary for functionality, but can improve IDE support and reduce test helper misuse.
app/Console/Commands/ImportGreenlight3Command.php (2)
104-158: User import logic is reasonable; be explicit about password and role semanticsThe user import behaves as expected:
- De‑duplicates by email, mapping multiple GL3 references to the same existing user.
- Derives
authenticatorfromexternal_idand preservespassword_digestfor local users, while generating a random password for external (OIDC) users.- Sets locale and timezone from your app settings and attaches the selected default role only for non‑external users.
Two small considerations:
- Document somewhere (e.g., in command help) that existing local users are not modified (authenticator, external_id, roles), only linked, so operators know what to expect.
- If your
Rolepivot uses anautomaticflag, and you want these imported roles to behave like “automatic” assignments, you may preferattach([$defaultRole => ['automatic' => true]])instead of bareattach($defaultRole).
169-316: Room and shared access import mapping is clear; verify option mapping and ID collision policyThe room and shared access import functions are well structured:
- Rooms:
- Skip import when a room with the same
friendly_idalready exists, and intentionally don’t add it to$roomMapso shared accesses for that ID are skipped too.- Require a mapped owner in
$userMap, collecting failures for reporting instead of throwing.- Copy key options (
glAnyoneCanStart,glAnyoneJoinAsModerator,glRequireAuthentication,glViewerAccessCode,guestPolicy,muteOnStart,record) into the correspondingRoomfields, plus attach the chosenRoomType.- Shared accesses:
- Only add memberships when both user and room were successfully imported, assigning
RoomUserRole::MODERATORviasyncWithoutDetaching, which is appropriate for GL’s shared access.A few behavioral points worth confirming:
- For
guestPolicy, mapping onlyASK_MODERATORtoRoomLobby::ENABLEDand treating all other values asDISABLEDis a design choice; if GL3 has multiple guest policies you care about, you may want a more granular mapping later.- The current ID‑collision behavior (skipping rooms whose friendly ID already exists and not importing their shared accesses) is safe, but it might surprise admins who expect shared accesses to be merged for matching rooms. If that merge is ever desired, you’d need to change the
continuepath to still populate$roomMap.Overall, nothing blocking, just trade‑offs to be aware of.
resources/js/views/RoomsView.vue (1)
118-136: Access-code mask/placeholder correctly handle legacy alphanumeric codesConditionally switching to a 6‑character mask and neutral placeholder for
room.legacy_codecleanly enables GL3‑style alphanumeric access codes while preserving the existing 9‑digit format for non‑legacy rooms. The interaction withlogin()(dash stripping) also stays correct for both paths.If this mask/placeholder logic ends up needed in more places (e.g. other components), consider extracting a small computed helper for reuse.
tests/Frontend/e2e/RoomsViewSettings.cy.js (1)
1239-1304: GL3 access-code settings flow is well coveredThis new test exercises loading a non‑numeric
"fck4fd"access code, editing settings, and asserting that the PUT payload preserves that value along with the expected flags. That gives good coverage for the GL3 access‑code scenario.Optionally, you could add a post‑save assertion on
#room-setting-access_codeto confirm the UI still shows"fck4fd"after the update, mirroring the payload check.tests/Backend/Unit/Console/helper/Greenlight3Room.php (1)
5-27: DTO is fine; consider typed properties for clarityThe helper does its job as a simple value object for mocked rows. If you want slightly stronger guarantees and better static analysis, you could add typed properties, e.g.:
class Greenlight3Room { public string $id; public string $friendly_id; public string $user_id; public string $name; public bool $deleted; public function __construct(string $id, string $friendly_id, string $user_id, string $name, bool $deleted = false) { $this->id = $id; $this->friendly_id = $friendly_id; $this->user_id = $user_id; $this->name = $name; $this->deleted = $deleted; } }Purely a nicety; not blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/Console/Commands/ImportGreenlight3Command.php(1 hunks)app/Http/Middleware/RoomAuthenticate.php(1 hunks)app/Http/Requests/UpdateRoomSettings.php(1 hunks)app/Models/Room.php(1 hunks)resources/js/components/RoomShareButton.vue(1 hunks)resources/js/views/RoomsView.vue(1 hunks)tests/Backend/Feature/api/v1/Room/RoomTest.php(1 hunks)tests/Backend/Unit/Console/ImportGreenlight3Test.php(1 hunks)tests/Backend/Unit/Console/helper/Greenlight3Room.php(1 hunks)tests/Backend/Unit/Console/helper/Greenlight3User.php(1 hunks)tests/Frontend/e2e/RoomsViewGeneral.cy.js(2 hunks)tests/Frontend/e2e/RoomsViewSettings.cy.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/Backend/Unit/Console/ImportGreenlight3Test.php (5)
tests/Backend/Unit/Console/helper/Greenlight3Room.php (1)
Greenlight3Room(5-28)tests/Backend/Unit/Console/helper/Greenlight3User.php (1)
Greenlight3User(5-28)tests/Backend/Unit/Console/helper/GreenlightSharedAccess.php (1)
GreenlightSharedAccess(5-22)app/Models/RoomType.php (1)
rooms(37-40)app/Models/Room.php (2)
owner(214-217)members(263-266)
tests/Backend/Unit/Console/helper/Greenlight3User.php (1)
tests/Backend/Unit/Console/helper/Greenlight3Room.php (1)
__construct(20-27)
app/Console/Commands/ImportGreenlight3Command.php (5)
app/Models/Role.php (2)
Role(15-112)users(35-38)app/Settings/GeneralSettings.php (1)
GeneralSettings(7-29)app/Models/RoomType.php (1)
rooms(37-40)app/Enums/RoomUserRole.php (1)
label(16-25)app/Models/Room.php (2)
owner(214-217)members(263-266)
tests/Backend/Unit/Console/helper/Greenlight3Room.php (1)
tests/Backend/Unit/Console/helper/Greenlight3User.php (1)
__construct(20-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Docker Build
- GitHub Check: Backend
🔇 Additional comments (4)
tests/Backend/Feature/api/v1/Room/RoomTest.php (1)
561-566: Good coverage for legacy alphanumeric access codeThe extra assertion for a 6‑character alphanumeric legacy access code (
012abc) nicely exercises the new backend behavior for guests; no issues spotted.app/Console/Commands/ImportGreenlight3Command.php (1)
35-95: Overall command flow and transaction handling look solidThe command wiring is coherent:
- Dynamically registers a
greenlightpgsql connection and reads users, rooms, and shared accesses from it.- Prompts for room type, optional name prefix, and default role using
Laravel\Prompts.- Wraps all local DB writes (users, rooms, memberships) in a single transaction with a final
confirm()gate, so the operator can review counts before committing.This is a good balance between safety (rollback on failure or cancel) and usability for a one‑shot migration command.
app/Http/Requests/UpdateRoomSettings.php (1)
45-56: Verify GL3 code case handling during import and in validationThe access code rules correctly distinguish legacy 6-character vs new 9-digit codes. However, there's a potential issue with imported GL3 codes:
GL3 viewer access codes are imported without case normalization (stored as-is from the source database). When users later update room settings without changing the access code, if that code contains uppercase letters, the validation will fail because the alphanumeric legacy case enforces
lowercasevalidation rule (line 54).If GL3 instances ever store codes in non-lowercase format, either:
- Normalize to lowercase during import in
ImportGreenlight3Command, or- Apply case-insensitive comparison when detecting legacy codes to preserve and remove the
lowercaserule for GL3 importstests/Frontend/e2e/RoomsViewGeneral.cy.js (1)
386-408: Legacy access-code E2E updated consistently to alphanumericUsing
"012abc"both for the input and theaccess-codeheader expectation matches the new legacy alphanumeric behaviour and keeps this test meaningful for GL3 imports. No issues from my side here.
| $rooms = []; | ||
| $rooms[] = new Greenlight3Room(1, 'abc-def-xyz-123', $users[0]->id, 'Test Room 1'); | ||
| $rooms[] = new Greenlight3Room(2, 'abc-def-xyz-234', $users[1]->id, 'Test Room 2'); | ||
| $rooms[] = new Greenlight3Room(3, 'abc-def-xyz-345', $users[2]->id, 'Test Room 3'); | ||
| $rooms[] = new Greenlight3Room(4, 'abc-def-xyz-456', $users[3]->id, 'Test Room 4'); | ||
|
|
||
| $rooms[] = new Greenlight3Room(5, 'hij-klm-xyz-123', $users[0]->id, 'Test Room 5'); | ||
| $rooms[] = new Greenlight3Room(6, 'hij-klm-xyz-234', $users[0]->id, 'Test Room 6'); | ||
| $rooms[] = new Greenlight3Room(7, 'hij-klm-xyz-456', 99, 'Test Room 9', '012345abcd'); | ||
| $rooms[] = new Greenlight3Room(8, $existingRoom->id, $users[0]->id, 'Test Room 10'); | ||
|
|
There was a problem hiding this comment.
Fix mismatched constructor argument for Greenlight3Room
Here:
$rooms[] = new Greenlight3Room(7, 'hij-klm-xyz-456', 99, 'Test Room 9', '012345abcd');the 5th parameter is "012345abcd", but Greenlight3Room::__construct declares that argument as bool $deleted = false. PHP will coerce this non‑empty string to true, so $deleted becomes true for room 7, and the string itself is never used (the viewer access code for room 5 already comes from the room_meeting_options mock).
This is confusing and could break if strict types are ever enabled or if deleted starts to matter in the test.
Consider either:
- Dropping the 5th argument entirely, if this room isn’t meant to be “deleted”:
- $rooms[] = new Greenlight3Room(7, 'hij-klm-xyz-456', 99, 'Test Room 9', '012345abcd');
+ $rooms[] = new Greenlight3Room(7, 'hij-klm-xyz-456', 99, 'Test Room 9');or, if you intended to mark it as deleted for future assertions, pass a boolean:
- $rooms[] = new Greenlight3Room(7, 'hij-klm-xyz-456', 99, 'Test Room 9', '012345abcd');
+ $rooms[] = new Greenlight3Room(7, 'hij-klm-xyz-456', 99, 'Test Room 9', true);Either way, removing the stray string argument will better reflect the actual data model.
🤖 Prompt for AI Agents
tests/Backend/Unit/Console/ImportGreenlight3Test.php around lines 195-205: the
Greenlight3Room constructor call for room id 7 passes a string "012345abcd" as
the 5th argument which maps to the bool $deleted parameter and will coerce to
true; update that call to either remove the 5th argument entirely (if the room
is not meant to be deleted) or replace it with an explicit boolean (true if you
intend it to be deleted, false otherwise) so the parameter type matches the
constructor and the viewer access code remains sourced from the
room_meeting_options mock.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2665 +/- ##
===========================================
Coverage 96.75% 96.75%
- Complexity 1816 1848 +32
===========================================
Files 434 435 +1
Lines 12483 12643 +160
Branches 2078 2080 +2
===========================================
+ Hits 12078 12233 +155
- Misses 405 410 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pizkaz
force-pushed
the
feat/greenlight3-import
branch
from
2e670e0 to
c38131b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
resources/js/components/RoomShareButton.vue (1)
117-123: Consider using an explicit check for all-numeric strings.The
isNaN()function performs type coercion before checking, which can lead to unexpected behavior with edge cases (e.g., empty strings, null). While the current guards prevent these cases, using an explicit regex test would be more robust and clearer.Apply this diff to use a regex test:
const formattedAccessCode = computed(() => { - return isNaN(props.room.access_code) - ? props.room.access_code - : String(props.room.access_code) + const code = String(props.room.access_code); + return /^\d+$/.test(code) + ? code .match(/.{1,3}/g) - .join("-"); + .join("-") + : code; });This explicitly checks whether the code contains only digits, making the intent clearer and avoiding type coercion quirks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/Http/Middleware/RoomAuthenticate.php(1 hunks)app/Http/Requests/UpdateRoomSettings.php(1 hunks)app/Models/Room.php(1 hunks)resources/js/components/RoomShareButton.vue(1 hunks)resources/js/views/RoomsView.vue(1 hunks)tests/Backend/Feature/api/v1/Room/RoomTest.php(1 hunks)tests/Frontend/e2e/RoomsViewGeneral.cy.js(2 hunks)tests/Frontend/e2e/RoomsViewSettings.cy.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/Backend/Feature/api/v1/Room/RoomTest.php
- app/Http/Middleware/RoomAuthenticate.php
- app/Models/Room.php
- resources/js/views/RoomsView.vue
- tests/Frontend/e2e/RoomsViewSettings.cy.js
- tests/Frontend/e2e/RoomsViewGeneral.cy.js
- app/Http/Requests/UpdateRoomSettings.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Backend
- GitHub Check: Docker Build
| return String(props.room.access_code) | ||
| .match(/.{1,3}/g) | ||
| .join("-"); | ||
| return isNaN(props.room.access_code) |
There was a problem hiding this comment.
That would format 6 digit numeric codes with a dash.
However when entering the 6 digit code there is no dash input mask
| $alphanumeric = $current == $incoming && ! is_numeric($current); | ||
| $digits = ($current == $incoming && strlen($current) == 6) ? 6 : 9; | ||
|
|
||
| $rules = ['numeric', 'digits:'.$digits, 'bail']; | ||
| $rules = $alphanumeric | ||
| ? ['alpha_num:ascii', 'lowercase', 'size:6', 'bail'] | ||
| : ['numeric', 'digits:'.$digits, 'bail']; |
There was a problem hiding this comment.
| $alphanumeric = $current == $incoming && ! is_numeric($current); | |
| $digits = ($current == $incoming && strlen($current) == 6) ? 6 : 9; | |
| $rules = ['numeric', 'digits:'.$digits, 'bail']; | |
| $rules = $alphanumeric | |
| ? ['alpha_num:ascii', 'lowercase', 'size:6', 'bail'] | |
| : ['numeric', 'digits:'.$digits, 'bail']; | |
| $legacy = $current == $incoming && strlen($current) == 6); | |
| $rules = $legacy | |
| ? ['alpha_num:ascii', 'lowercase', 'size:6', 'bail'] | |
| : ['numeric', 'digits:9', 'bail']; |
2 participants
Fixes #2664
Type
Checklist
Changes
import:greenlight-v3Other information
ToDo:
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.