feat: per-sponsor member permission tracking with JSON column on Sponsor_User#523
Open
feat: per-sponsor member permission tracking with JSON column on Sponsor_User#523
Conversation
…sor_User Signed-off-by: romanetar <roman_ag@hotmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-sponsor permission tracking via a new Sponsor_Users.Permissions JSON column, enables members to belong to multiple sponsors per summit, includes external sponsor users in sponsor-scoped authorization, and propagates sponsor_id/summit_id through sync services, badge-scan flows, and MQ jobs. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Badge Scan Client
participant API as Badge Scan API
participant Service as SponsorUserInfoGrantService
participant Member as Member Model
participant Repo as Sponsor Repository
Client->>API: POST /badge-scan (qr_code, scan_date, [sponsor_id])
API->>Service: addBadgeScan(member, summit, data)
Service->>Member: getAllowedSponsorsBySummit(summit)
Note right of Member: returns 0..N allowed sponsors
alt no sponsors
Service-->>API: Validation error (no sponsor)
else one sponsor
Service->>Repo: select single sponsor
Repo-->>Service: Sponsor
Service-->>API: Create scan (sponsor_id)
else multiple sponsors
Service->>Service: require sponsor_id
Service->>Member: getSponsorBySummitAndId(summit, sponsor_id)
alt Sponsor found
Service-->>API: Create scan (sponsor_id)
else Sponsor not found
Service-->>API: Validation error (invalid sponsor)
end
end
sequenceDiagram
participant Job as MQ Job
participant MQ as Message Payload
participant Sync as SponsorUserSyncService
participant Member as Member Model
participant DB as Sponsor_Users Table
Job->>Job: Parse MQ (user_external_id, group_slug, sponsor_id, summit_id)
Job->>Sync: addSponsorUserToGroup(user_id, group_slug, sponsor_id, summit_id)
Sync->>Member: addSponsorPermission(sponsor_id, group_slug)
Member->>DB: SELECT ... FOR UPDATE (sponsor/member)
DB-->>Member: row (Permissions JSON)
Member->>DB: UPDATE Permissions JSON (append slug if missing)
DB-->>Member: Updated row
alt member not in global group
Sync->>DB: add member to global group
else already in global group
Sync-->>Job: complete (idempotent)
end
Note over Sync,DB: removeSponsorUserFromGroup clears JSON for sponsor<br/>and removes global group only when no other sponsor permissions remain
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-523/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
app/Swagger/SponsorBadgeScanSchemas.php (1)
80-80: Document the conditional requirement forsponsor_idmore explicitly.At Line 80, consider clarifying that
sponsor_idis mandatory when the caller belongs to multiple sponsors in the summit (to reduce client-side trial/error).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Swagger/SponsorBadgeScanSchemas.php` at line 80, Update the OA\Property for 'sponsor_id' in SponsorBadgeScanSchemas.php (the line creating new OA\Property with property: 'sponsor_id') to make the conditional requirement explicit: change the description to state that sponsor_id is required when the caller/user is associated with multiple sponsors for the summit (e.g., "Required when the caller belongs to multiple sponsors for the summit; otherwise omitted"). Optionally add an example value to the OA\Property to clarify expected format.app/Models/Foundation/Main/Member.php (1)
24-24: Unused import added.The
Doctrine\DBAL\ParameterTypeimport doesn't appear to be used in the changed code segments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Foundation/Main/Member.php` at line 24, Remove the unused import Doctrine\DBAL\ParameterType from the Member.php file to clean up imports; locate the top-of-file use statements in the Member class (file: Member.php) and delete the line "use Doctrine\DBAL\ParameterType" since no functions or methods (e.g., any Member methods) reference ParameterType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitBadgeScanApiController.php`:
- Around line 389-390: The CSV export path should apply the same sponsor-user
check as the JSON listing: update the CSV filtering logic to include both
$current_member->isSponsorUser() and $current_member->isExternalSponsorUser()
(not just isSponsorUser()), and when true use
$current_member->getSponsorMembershipIds($summit) to constrain results; find the
CSV filter block in OAuth2SummitBadgeScanApiController (the branch that
currently only checks isSponsorUser()) and extend the condition to OR the
isExternalSponsorUser() check and reuse getSponsorMembershipIds($summit) to
produce consistent access behavior.
- Around line 96-99: In the validation rules array inside
OAuth2SummitBadgeScanApiController, remove the duplicate 'qr_code' entry that
currently overwrites the intended rule; keep the first rule "'qr_code' =>
'required_without:attendee_email|string'" (so the required_without fallback to
'attendee_email' remains) and delete the second "'qr_code' =>
'required|string'"; ensure 'sponsor_id' remains unchanged.
In `@app/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.php`:
- Around line 65-66: The code casts missing payload fields to 0 for $sponsor_id
and $summit_id (and similarly at the nearby block for other IDs), allowing
invalid IDs through; add explicit presence checks and positive-ID validation
before casting and dispatching: verify required keys exist in $data (e.g.,
'sponsor_id', 'summit_id', and the other fields at lines 69–71), ensure
intval($data['...']) > 0, and if validation fails log an error via the job
logger (or processLogger) and abort the job/return early (or throw a meaningful
exception) instead of proceeding to call the service that updates sponsor member
groups; update the validation around $sponsor_id and $summit_id (and the
analogous variables) in UpdateSponsorMemberGroupsMQJob to enforce these checks.
In `@database/migrations/model/Version20260402153110.php`:
- Around line 35-38: The migration Version20260402153110 adds a nullable JSON
column Permissions to self::TableName (Sponsor_Users) but doesn't backfill
existing rows, which will break permission checks; after creating the column (in
the same migration or a follow-up migration), run a safe backfill that updates
all existing Sponsor_Users rows with a sensible default Permissions value (e.g.,
an appropriate JSON array/object representing current implicit access, or an
empty array if that maps to allowed behavior) using the database connection (or
schema builder) so no rows remain NULL before permission checks are enforced;
reference Version20260402153110, self::TableName and the Permissions column when
adding this update.
In `@tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php`:
- Line 112: Rename the test method testAddBadgeScanWithOneSponsorsPerMember to
the grammatically correct testAddBadgeScanWithOneSponsorPerMember; update the
method declaration in OAuth2SummitBadgeScanApiControllerTest (and any
references/calls, e.g., other tests or annotations) to use the new name so
PHPUnit still discovers and runs the test, and run the test suite to ensure no
references were missed.
---
Nitpick comments:
In `@app/Models/Foundation/Main/Member.php`:
- Line 24: Remove the unused import Doctrine\DBAL\ParameterType from the
Member.php file to clean up imports; locate the top-of-file use statements in
the Member class (file: Member.php) and delete the line "use
Doctrine\DBAL\ParameterType" since no functions or methods (e.g., any Member
methods) reference ParameterType.
In `@app/Swagger/SponsorBadgeScanSchemas.php`:
- Line 80: Update the OA\Property for 'sponsor_id' in
SponsorBadgeScanSchemas.php (the line creating new OA\Property with property:
'sponsor_id') to make the conditional requirement explicit: change the
description to state that sponsor_id is required when the caller/user is
associated with multiple sponsors for the summit (e.g., "Required when the
caller belongs to multiple sponsors for the summit; otherwise omitted").
Optionally add an example value to the OA\Property to clarify expected format.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fff501ad-b18e-463d-891f-5b4c1c5eb1ba
📒 Files selected for processing (14)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitBadgeScanApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.phpapp/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.phpapp/Models/Foundation/Main/Member.phpapp/Models/Foundation/Main/Strategies/SponsorMemberSummitStrategy.phpapp/Models/Foundation/Summit/Sponsor.phpapp/Services/Model/ISponsorUserSyncService.phpapp/Services/Model/Imp/SponsorUserInfoGrantService.phpapp/Services/Model/Imp/SponsorUserSyncService.phpapp/Swagger/SponsorBadgeScanSchemas.phpdatabase/migrations/model/Version20260402153110.phptests/Unit/Entities/SponsorPermissionTrackingTest.phptests/Unit/Services/SponsorUserPermissionTrackingTest.phptests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php
💤 Files with no reviewable changes (1)
- app/Models/Foundation/Summit/Sponsor.php
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitBadgeScanApiController.php
Show resolved
Hide resolved
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitBadgeScanApiController.php
Show resolved
Hide resolved
Signed-off-by: romanetar <roman_ag@hotmail.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-523/ This page is automatically updated on each push to this PR. |
1 similar comment
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-523/ This page is automatically updated on each push to this PR. |
Signed-off-by: romanetar <roman_ag@hotmail.com>
romanetar
force-pushed
the
feature/per-sponsor-permission-tracking
branch
from
338c229 to
4391757
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-523/ This page is automatically updated on each push to this PR. |
smarcet
reviewed
Apr 7, 2026
smarcet
reviewed
Apr 7, 2026
smarcet
reviewed
Apr 7, 2026
smarcet
reviewed
Apr 7, 2026
smarcet
reviewed
Apr 7, 2026
smarcet
reviewed
Apr 7, 2026
smarcet
reviewed
Apr 7, 2026
smarcet
reviewed
Apr 7, 2026
smarcet
requested changes
Apr 7, 2026
Collaborator
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@romanetar please review comments
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-523/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
database/migrations/model/Version20260408102410.php (1)
40-50: Migration aggregates all group codes, not just sponsor-related ones.The subquery aggregates every
Group.Codethe member belongs to (e.g.,administrators,summit-admins, etc.), not only the sponsor permission slugs (sponsors,sponsors-external-users). This works because downstreamJSON_CONTAINSchecks will find the relevant slugs regardless of extra data, but it means thePermissionscolumn may contain unrelated group codes.If the intent is to store only sponsor-relevant permissions, the query should filter groups:
Optional refinement to filter only sponsor groups
UPDATE Sponsor_Users su SET su.Permissions = ( SELECT JSON_ARRAYAGG(g.Code) FROM Group_Members gm INNER JOIN `Group` g ON g.ID = gm.GroupID WHERE gm.MemberID = su.MemberID + AND g.Code IN ('sponsors', 'sponsors-external-users') ) WHERE su.Permissions IS NULLOtherwise, document that extra codes are intentionally stored for future extensibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/model/Version20260408102410.php` around lines 40 - 50, The migration Version20260408102410 currently sets Sponsor_Users.Permissions by JSON_ARRAYAGG of all Group.Code via the subquery in the addSql(...) call, which pulls unrelated groups; update that SQL subquery to only aggregate sponsor-related group codes (e.g., add "WHERE g.Code IN ('sponsors','sponsors-external-users')" or a pattern filter like "WHERE g.Code LIKE 'sponsors%'" to the JOINed Group g) so JSON_ARRAYAGG(g.Code) only emits sponsor permission slugs for Sponsor_Users.Permissions, or alternatively add a clear comment in the migration explaining that collecting all group codes is intentional for extensibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.php`:
- Around line 62-65: The code in UpdateSponsorMemberGroupsMQJob currently reads
$payload['data'] without verifying the key exists; update the handler (the
method containing the $payload usage in UpdateSponsorMemberGroupsMQJob) to first
check that isset($payload['data']) and that it is an array (or non-empty) and
throw a ValidationException with a clear message if missing, then assign $data =
$payload['data'] and proceed with the existing validation for user_external_id,
group_slug, sponsor_id and summit_id; ensure you reference the same $data
variable used later so the subsequent isset checks don't trigger undefined index
warnings.
---
Nitpick comments:
In `@database/migrations/model/Version20260408102410.php`:
- Around line 40-50: The migration Version20260408102410 currently sets
Sponsor_Users.Permissions by JSON_ARRAYAGG of all Group.Code via the subquery in
the addSql(...) call, which pulls unrelated groups; update that SQL subquery to
only aggregate sponsor-related group codes (e.g., add "WHERE g.Code IN
('sponsors','sponsors-external-users')" or a pattern filter like "WHERE g.Code
LIKE 'sponsors%'" to the JOINed Group g) so JSON_ARRAYAGG(g.Code) only emits
sponsor permission slugs for Sponsor_Users.Permissions, or alternatively add a
clear comment in the migration explaining that collecting all group codes is
intentional for extensibility.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a94d0265-bd5a-4af2-af10-62e41817f1fb
📒 Files selected for processing (7)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitBadgeScanApiController.phpapp/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.phpapp/Models/Foundation/Main/Member.phpapp/Services/Model/Imp/SponsorUserInfoGrantService.phpapp/Services/Model/Imp/SponsorUserSyncService.phpapp/Services/Model/Imp/SummitSponsorService.phpdatabase/migrations/model/Version20260408102410.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Services/Model/Imp/SponsorUserSyncService.php
Comment on lines
62
to
+65
| $data = $payload['data']; | ||
| if (!isset($data['user_external_id'], $data['group_slug'], $data['sponsor_id'], $data['summit_id'])) { | ||
| throw new ValidationException('Invalid payload: user_external_id, group_slug, sponsor_id and summit_id are required.'); | ||
| } |
There was a problem hiding this comment.
Add check for $payload['data'] existence before accessing it.
Line 62 accesses $payload['data'] without verifying the key exists. If the payload lacks a data key, this will throw an undefined array key error before reaching the validation on lines 63-65.
Proposed fix
- $data = $payload['data'];
- if (!isset($data['user_external_id'], $data['group_slug'], $data['sponsor_id'], $data['summit_id'])) {
+ if (!isset($payload['data'])) {
+ throw new ValidationException('Invalid payload: data is required.');
+ }
+ $data = $payload['data'];
+ if (!isset($data['user_external_id'], $data['group_slug'], $data['sponsor_id'], $data['summit_id'])) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.php` around lines 62
- 65, The code in UpdateSponsorMemberGroupsMQJob currently reads
$payload['data'] without verifying the key exists; update the handler (the
method containing the $payload usage in UpdateSponsorMemberGroupsMQJob) to first
check that isset($payload['data']) and that it is an array (or non-empty) and
throw a ValidationException with a clear message if missing, then assign $data =
$payload['data'] and proceed with the existing validation for user_external_id,
group_slug, sponsor_id and summit_id; ensure you reference the same $data
variable used later so the subsequent isset checks don't trigger undefined index
warnings.
Signed-off-by: romanetar <roman_ag@hotmail.com>
romanetar
force-pushed
the
feature/per-sponsor-permission-tracking
branch
from
4a1e844 to
a027d13
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-523/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/Models/Foundation/Main/Member.php (1)
1905-1906: Redundant group membership check before SQL query.Lines 1905-1906 check
isSponsorUser() || isExternalSponsorUser()which queries theGroup_Memberstable. Then the SQL query immediately follows and also checksJSON_CONTAINSonSponsor_Users.Permissions. The preliminary check is now redundant since the SQL query will return 0 anyway if permissions don't exist.This adds an extra DB roundtrip on every call. Consider removing the short-circuit if performance is a concern, or keep it if you want to fail fast for non-sponsor users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Foundation/Main/Member.php` around lines 1905 - 1906, Remove the redundant pre-check that calls isSponsorUser() || isExternalSponsorUser() and returns false early; this causes an extra DB roundtrip before the subsequent SQL that already verifies Sponsor_Users.Permissions via JSON_CONTAINS. Locate the block where $canHaveSponsorMemberships is set and the immediate if(!$canHaveSponsorMemberships) return false; (references: isSponsorUser(), isExternalSponsorUser()) and delete those two lines so the method relies solely on the SQL/JSON_CONTAINS check; ensure no other code depends on $canHaveSponsorMemberships afterwards and adjust/remove that variable if present.database/migrations/model/Version20260408102410.php (1)
53-58: Destructive rollback clears application-written permissions.The
down()method setsPermissions = NULLfor all rows, which will also clear permissions written by the application after the migration ran. The docblock acknowledges this limitation, but consider whether a more targeted rollback is feasible (e.g., only clearing rows that were backfilled by tracking them).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/model/Version20260408102410.php` around lines 53 - 58, The current down() in Version20260408102410 unconditionally nulls Permissions for all Sponsor_Users which removes any permissions the app wrote after the migration; instead make rollback targeted: modify the migration to mark which rows were backfilled during up() (for example by setting a temporary marker column, inserting keys into an audit/backfill table, or comparing a recorded migration timestamp), and then change down() to only clear Permissions for those marked/backfilled rows (or to remove the backfill markers). Update the migration's up() to record that marker/audit data and update down() to use addSql with a WHERE clause that restricts the UPDATE to only those tracked rows so application-written permissions are preserved.app/Services/Model/Imp/SponsorUserInfoGrantService.php (1)
189-194: Edge case:sponsor_id = 0treated as empty.Line 189 uses
empty($data['sponsor_id'])which will treat0as empty. While sponsor IDs are typically positive integers, using a stricter check would be more defensive:🔧 Suggested improvement
- if (empty($data['sponsor_id'])) + if (!isset($data['sponsor_id']))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Model/Imp/SponsorUserInfoGrantService.php` around lines 189 - 194, The guard using empty($data['sponsor_id']) incorrectly treats 0 as missing; in SponsorUserInfoGrantService change the check to verify the key exists (e.g., isset or array_key_exists on $data['sponsor_id']) and then validate the value is a positive integer before casting to $sponsor_id; after intval($data['sponsor_id']) ensure $sponsor_id > 0 (or explicitly allow 0 only if valid) and only then use $member_sponsors->filter(...)->first(), throwing the same ValidationException if the key is absent or the parsed sponsor_id is invalid or not found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@database/migrations/model/Version20260408102410.php`:
- Around line 40-50: The migration currently aggregates all group codes into
Sponsor_Users.Permissions by selecting from Group_Members and Group g (g.Code),
which backfills unrelated groups; change the UPDATE in Version20260408102410.php
to only aggregate sponsor-related groups by adding a WHERE filter on the joined
Group row (e.g., WHERE g.Slug IN (IGroup::Sponsors,
IGroup::SponsorExternalUsers) or WHERE g.Code IN
('sponsors','sponsor_external_users') depending on which column holds the slug),
so the subquery that builds JSON_ARRAYAGG(g.Code) only includes sponsor groups
and leaves non-sponsor members' Permissions NULL or appropriately untouched.
---
Nitpick comments:
In `@app/Models/Foundation/Main/Member.php`:
- Around line 1905-1906: Remove the redundant pre-check that calls
isSponsorUser() || isExternalSponsorUser() and returns false early; this causes
an extra DB roundtrip before the subsequent SQL that already verifies
Sponsor_Users.Permissions via JSON_CONTAINS. Locate the block where
$canHaveSponsorMemberships is set and the immediate
if(!$canHaveSponsorMemberships) return false; (references: isSponsorUser(),
isExternalSponsorUser()) and delete those two lines so the method relies solely
on the SQL/JSON_CONTAINS check; ensure no other code depends on
$canHaveSponsorMemberships afterwards and adjust/remove that variable if
present.
In `@app/Services/Model/Imp/SponsorUserInfoGrantService.php`:
- Around line 189-194: The guard using empty($data['sponsor_id']) incorrectly
treats 0 as missing; in SponsorUserInfoGrantService change the check to verify
the key exists (e.g., isset or array_key_exists on $data['sponsor_id']) and then
validate the value is a positive integer before casting to $sponsor_id; after
intval($data['sponsor_id']) ensure $sponsor_id > 0 (or explicitly allow 0 only
if valid) and only then use $member_sponsors->filter(...)->first(), throwing the
same ValidationException if the key is absent or the parsed sponsor_id is
invalid or not found.
In `@database/migrations/model/Version20260408102410.php`:
- Around line 53-58: The current down() in Version20260408102410 unconditionally
nulls Permissions for all Sponsor_Users which removes any permissions the app
wrote after the migration; instead make rollback targeted: modify the migration
to mark which rows were backfilled during up() (for example by setting a
temporary marker column, inserting keys into an audit/backfill table, or
comparing a recorded migration timestamp), and then change down() to only clear
Permissions for those marked/backfilled rows (or to remove the backfill
markers). Update the migration's up() to record that marker/audit data and
update down() to use addSql with a WHERE clause that restricts the UPDATE to
only those tracked rows so application-written permissions are preserved.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf08d39e-ab20-416a-a575-e7be26b92d14
📒 Files selected for processing (7)
app/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.phpapp/Models/Foundation/Main/Member.phpapp/Services/Model/Imp/SponsorUserInfoGrantService.phpapp/Services/Model/Imp/SponsorUserSyncService.phpapp/Services/Model/Imp/SummitSponsorService.phpdatabase/migrations/model/Version20260408102410.phptests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Services/Model/Imp/SummitSponsorService.php
- app/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.php
Comment on lines
+40
to
+50
| $this->addSql(<<<SQL | ||
| UPDATE Sponsor_Users su | ||
| SET su.Permissions = ( | ||
| SELECT JSON_ARRAYAGG(g.Code) | ||
| FROM Group_Members gm | ||
| INNER JOIN `Group` g ON g.ID = gm.GroupID | ||
| WHERE gm.MemberID = su.MemberID | ||
| ) | ||
| WHERE su.Permissions IS NULL | ||
| SQL | ||
| ); |
There was a problem hiding this comment.
Backfill aggregates all group codes, not just sponsor-related ones.
The migration backfills Permissions with every group code the member belongs to (via Group_Members), but the application logic in Member.php only checks for IGroup::Sponsors and IGroup::SponsorExternalUsers slugs. This means:
- Unrelated group codes (e.g.,
administrators,summit-admins) will be stored in the JSON array - Members who aren't in the Sponsors/SponsorExternalUsers groups will get permissions populated with irrelevant codes
Consider filtering to only sponsor-related groups:
🔧 Proposed fix
UPDATE Sponsor_Users su
SET su.Permissions = (
SELECT JSON_ARRAYAGG(g.Code)
FROM Group_Members gm
INNER JOIN `Group` g ON g.ID = gm.GroupID
WHERE gm.MemberID = su.MemberID
+ AND g.Code IN ('sponsors', 'sponsor-external-users')
)
WHERE su.Permissions IS NULL📝 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.
Suggested change
| $this->addSql(<<<SQL | |
| UPDATE Sponsor_Users su | |
| SET su.Permissions = ( | |
| SELECT JSON_ARRAYAGG(g.Code) | |
| FROM Group_Members gm | |
| INNER JOIN `Group` g ON g.ID = gm.GroupID | |
| WHERE gm.MemberID = su.MemberID | |
| ) | |
| WHERE su.Permissions IS NULL | |
| SQL | |
| ); | |
| $this->addSql(<<<SQL | |
| UPDATE Sponsor_Users su | |
| SET su.Permissions = ( | |
| SELECT JSON_ARRAYAGG(g.Code) | |
| FROM Group_Members gm | |
| INNER JOIN `Group` g ON g.ID = gm.GroupID | |
| WHERE gm.MemberID = su.MemberID | |
| AND g.Code IN ('sponsors', 'sponsor-external-users') | |
| ) | |
| WHERE su.Permissions IS NULL | |
| SQL | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@database/migrations/model/Version20260408102410.php` around lines 40 - 50,
The migration currently aggregates all group codes into
Sponsor_Users.Permissions by selecting from Group_Members and Group g (g.Code),
which backfills unrelated groups; change the UPDATE in Version20260408102410.php
to only aggregate sponsor-related groups by adding a WHERE filter on the joined
Group row (e.g., WHERE g.Slug IN (IGroup::Sponsors,
IGroup::SponsorExternalUsers) or WHERE g.Code IN
('sponsors','sponsor_external_users') depending on which column holds the slug),
so the subquery that builds JSON_ARRAYAGG(g.Code) only includes sponsor groups
and leaves non-sponsor members' Permissions NULL or appropriately untouched.
Signed-off-by: romanetar <roman_ag@hotmail.com>
romanetar
force-pushed
the
feature/per-sponsor-permission-tracking
branch
from
a027d13 to
04376d4
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-523/ This page is automatically updated on each push to this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://app.clickup.com/t/86b94hpua
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests / Chores