Groups now contain a list of doors they give access to, and this is synced automatically to accessy. #670
Conversation
…ynced automatically to accessy.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces door access management for groups by adding a new database model, API endpoints, frontend components, and synchronization logic with the Accessy system. It includes a new UI tab and page for managing group door access, backend models and migrations, API routes for CRUD operations, and enhanced sync logic to align group access between Makeradmin and Accessy. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Accessy
User->>Frontend: Navigate to Group "Dörråtkomst" tab
Frontend->>Backend: GET /membership/groups/{group_id}/door_access_permissions
Backend->>Frontend: List current door access entries
Frontend->>Backend: GET /multiaccessy/assets
Backend->>Accessy: Fetch asset publications
Accessy-->>Backend: Asset publication list
Backend-->>Frontend: Asset publication list
User->>Frontend: Select asset to add
Frontend->>Backend: POST /membership/group_door_access_permissions
Backend->>Backend: Save new GroupDoorAccess entry
Backend->>Accessy: Sync group access permissions
Backend-->>Frontend: Updated door access list
User->>Frontend: Remove door access entry
Frontend->>Backend: DELETE /membership/group_door_access_permissions/{id}
Backend->>Backend: Remove GroupDoorAccess entry
Backend->>Accessy: Sync group access permissions
Backend-->>Frontend: Updated door access list
Suggested reviewers
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
admin/src/Models/Collection.ts (1)
152-164: Guard againstnullIDs inadd()
remove()now explicitly rejectsitem.id === null, butadd()will happily POST[null], leading to a 400/500 from the backend.
Mirror the same validation you added forremove():if (!this.idListName) { throw new Error( `Container for ${this.type.constructor.name} does not support add.`, ); } +if (item.id === null) { + throw new Error( + `Cannot add item with null ID to ${this.type.constructor.name} collection.`, + ); +}
🧹 Nitpick comments (7)
api/src/service/internal_service.py (1)
92-97:codeparameter silently ignored when a view returns a FlaskResponseNice touch letting views short-circuit by handing back a ready-made
Response.
Just note that whenisinstance(data, Response)isTrue, thecodethat may have been supplied toInternalService.route()is never applied. This is probably fine, but if downstream code relies on thecodeargument for logging/metrics you may want to surface that mismatch (e.g. warn if both are provided).No action required if this is intentional.
admin/src/Membership/Routes.jsx (1)
6-7: Consider lazy-loading the new door-access sub-pageThe import of
GroupBoxDoorAccessand the added<Route>work, but they add another chunk to the main bundle. If the door-access screen is used infrequently, wrapping the component inReact.lazy()/Suspensekeeps the initial load slim without affecting UX.Totally optional.
Also applies to: 40-41
admin/src/Membership/GroupBoxDoorAccess.tsx (1)
111-118: Add akeyprop when rendering listsStatic-analysis is right – the
<img>elements produced by.map()need a stablekeyto avoid reconciliation glitches.- {asset?.asset.images.map((img) => ( - <img + {asset?.asset.images.map((img) => ( + <img + key={img.id ?? img.thumbnailId}api/src/multiaccessy/views.py (1)
4-23: Remove unused imports to keep the module tidy
Tuple,AccessyAsset, andAccessyAssetPublicationaren’t referenced after refactor.-from typing import Any, Optional, Tuple, cast +from typing import Any, Optional, cast ... - AccessyAsset, - AccessyAssetPublication,api/src/multiaccessy/accessy.py (1)
9-9: Drop unusedTupleimport
Tupleis no longer referenced – remove to silence Ruff F401.-from typing import Any, List, Optional, Tuple, Union +from typing import Any, List, Optional, Unionapi/src/multiaccessy/sync.py (1)
2-8: Cull unused imports
itertools,Callable,get_membership_summaries,Span, andcontains_eageraren’t used after the refactor – they just trigger F401.
Please delete them to keep the module tidy.api/src/systest/api/accessy_sync_test.py (1)
7-7: Use the same session object thatself.dbrelies on
db_session.flush()works only as long asself.dbhappens to use the same global session.
Callingflush()onself.db.session(or lettingself.db.create_*return committed/flushed objects) keeps the test independent of the current session-wiring and avoids surprises if the test harness swaps the session for isolation.- db_session.flush() + self.db.session.flush()Also applies to: 144-148
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
admin/dist/css/default.cssis excluded by!**/dist/**
📒 Files selected for processing (14)
admin/src/Membership/GroupBox.jsx(1 hunks)admin/src/Membership/GroupBoxDoorAccess.tsx(1 hunks)admin/src/Membership/MembershipPeriodsInput.js(0 hunks)admin/src/Membership/Routes.jsx(2 hunks)admin/src/Models/Collection.ts(6 hunks)admin/src/Models/GroupDoorAccess.ts(1 hunks)api/src/membership/models.py(1 hunks)api/src/membership/views.py(4 hunks)api/src/migrations/0033_group_accessy_permissions.sql(1 hunks)api/src/multiaccessy/accessy.py(6 hunks)api/src/multiaccessy/sync.py(5 hunks)api/src/multiaccessy/views.py(3 hunks)api/src/service/internal_service.py(2 hunks)api/src/systest/api/accessy_sync_test.py(4 hunks)
💤 Files with no reviewable changes (1)
- admin/src/Membership/MembershipPeriodsInput.js
🧰 Additional context used
🧬 Code Graph Analysis (5)
admin/src/Membership/GroupBox.jsx (1)
admin/src/nav.jsx (2)
NavItem(7-24)NavItem(7-24)
admin/src/Models/GroupDoorAccess.ts (1)
api/src/membership/models.py (2)
GroupDoorAccess(134-147)Base(34-35)
api/src/membership/models.py (2)
admin/src/Models/GroupDoorAccess.ts (1)
GroupDoorAccess(3-18)api/src/shop/models.py (1)
Base(28-29)
api/src/membership/views.py (5)
admin/src/Membership/Routes.jsx (2)
Group(30-43)Member(53-81)api/src/membership/models.py (5)
Group(102-122)GroupDoorAccess(134-147)Key(164-178)Member(48-90)Permission(150-161)admin/src/Models/GroupDoorAccess.ts (1)
GroupDoorAccess(3-18)api/src/service/entity.py (1)
OrmSingeRelation(365-381)api/src/service/internal_service.py (2)
related_entity_routes(224-264)entity_routes(155-222)
api/src/multiaccessy/sync.py (3)
api/src/membership/membership.py (3)
get_members_and_membership(159-163)get_membership_summaries(57-156)get_membership_summary(53-54)api/src/membership/models.py (3)
Group(102-122)GroupDoorAccess(134-147)Member(48-90)api/src/multiaccessy/accessy.py (10)
AccessyMember(124-140)ModificationsDisabledError(28-29)create_access_permission_group(767-780)delete_access_permission_group(782-787)create_access_permission_for_group(805-818)delete_access_permission(798-803)get_pending_invitations(416-430)invite_phone_to_org_and_groups(401-411)add_to_group(393-399)remove_from_group(382-383)
🪛 Biome (1.9.4)
admin/src/Membership/GroupBoxDoorAccess.tsx
[error] 112-118: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🪛 Pylint (3.3.7)
api/src/membership/models.py
[refactor] 134-134: Too few public methods (1/2)
(R0903)
api/src/multiaccessy/accessy.py
[refactor] 230-230: Too many instance attributes (12/7)
(R0902)
[refactor] 261-261: Too many public methods (31/20)
(R0904)
api/src/multiaccessy/sync.py
[refactor] 49-49: Consider using '{}' instead of a call to 'dict'.
(R1735)
[refactor] 50-50: Redefining argument with the local name 'member_id'
(R1704)
[refactor] 122-125: Unnecessary "else" after "continue", remove the "else" and de-indent the code inside it
(R1724)
🪛 Ruff (0.11.9)
api/src/multiaccessy/views.py
4-4: typing.Tuple imported but unused
Remove unused import: typing.Tuple
(F401)
18-18: .accessy.AccessyAsset imported but unused
Remove unused import
(F401)
19-19: .accessy.AccessyAssetPublication imported but unused
Remove unused import
(F401)
api/src/multiaccessy/accessy.py
9-9: typing.Tuple imported but unused
Remove unused import: typing.Tuple
(F401)
api/src/multiaccessy/sync.py
2-2: itertools imported but unused
Remove unused import: itertools
(F401)
6-6: typing.Callable imported but unused
Remove unused import: typing.Callable
(F401)
8-8: membership.membership.get_membership_summaries imported but unused
Remove unused import: membership.membership.get_membership_summaries
(F401)
9-9: membership.models.Span imported but unused
Remove unused import: membership.models.Span
(F401)
12-12: sqlalchemy.orm.contains_eager imported but unused
Remove unused import: sqlalchemy.orm.contains_eager
(F401)
47-47: Comparison to None should be cond is None
Replace with cond is None
(E711)
47-47: Comparison to None should be cond is None
Replace with cond is None
(E711)
150-150: Comparison to None should be cond is None
Replace with cond is None
(E711)
151-151: Comparison to None should be cond is None
Replace with cond is None
(E711)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: stripe-tests
- GitHub Check: unit-test
- GitHub Check: firstrun
🔇 Additional comments (7)
admin/src/Membership/GroupBox.jsx (1)
43-48: Navigation entry looks goodThe extra tab surfaces the new feature cleanly and follows the existing pattern.
admin/src/Models/GroupDoorAccess.ts (1)
10-17: ```shell
#!/bin/bashLocate the GroupDoorAccess model definition
rg -n "class GroupDoorAccess" api/src/membership/models.py
Show the class definition and its column attributes
sed -n '120,200p' api/src/membership/models.py
</details> <details> <summary>admin/src/Membership/GroupBoxDoorAccess.tsx (1)</summary> `102-104`: **Column definition doesn’t match rendered cells** `columns` declares a single column, but each row outputs three `<td>` elements (thumbnail, name, delete). If `CollectionTable` uses `columns.length` to calculate widths or headings, the layout will be off. Please verify that the table renders as intended or extend `columns` accordingly. </details> <details> <summary>api/src/membership/views.py (2)</summary> `203-210`: **Door-access list route uses `PERMISSION_VIEW` – is that intentional?** All other group-scoped lists rely on `GROUP_VIEW` / `GROUP_MEMBER_VIEW`. Using `PERMISSION_VIEW` might unintentionally expose the door-access list to users who can view permissions but not groups. Confirm that `PERMISSION_VIEW` is indeed the desired gate here. --- `244-252`: **Top-level CRUD for door access bound to `GROUP_EDIT` – double-check access policy** Creating/deleting door-access rows requires `GROUP_EDIT`, but listing requires only `GROUP_VIEW` (see above). Ensure this asymmetry matches the security model; otherwise adjust permissions for consistency. </details> <details> <summary>api/src/systest/api/accessy_sync_test.py (2)</summary> `23-33`: **Guard against order-dependent failures in `Diff` assertions** `attribute_adds` / `attribute_removes` ultimately come from a Python `set`. If the implementation of `calculate_diff` ever stops sorting these collections, list order becomes nondeterministic and the `assertEqual` on `Diff(...)` will flicker. Safer pattern: ```python self.assertCountEqual( [(m1_phone, ACCESSY_SPECIAL_LABACCESS_GROUP), (m1_phone, ACCESSY_LABACCESS_GROUP)], diff.attribute_adds, )(or ensure
calculate_diffalways returns the lists in a defined order).
A quick tweak now prevents a future flaky test.Also applies to: 35-40, 41-45, 46-48, 49-56
149-176: Great addition – exercises the new group-GUID mapping path thoroughlyThe test now validates both positive and negative cases for Accessy group propagation, which will catch regressions in the sync logic. Nice one!
| CREATE TABLE IF NOT EXISTS `membership_group_door_access` ( | ||
| `id` int(10) unsigned NOT NULL AUTO_INCREMENT, | ||
| `group_id` int(10) unsigned NOT NULL, | ||
| `accessy_asset_publication_guid` VARCHAR(255) NOT NULL, | ||
| `created_at` datetime NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| `updated_at` datetime DEFAULT NULL ON UPDATE CURRENT_TIMESTAMP, | ||
| `deleted_at` datetime DEFAULT NULL, | ||
| PRIMARY KEY (`id`), | ||
| KEY `membership_group_doors_group_id_index` (`group_id`), | ||
| CONSTRAINT `membership_groups_foreign` FOREIGN KEY (`group_id`) REFERENCES `membership_groups` (`group_id`) | ||
| ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align DDL with ORM model & guard against duplicates
The Python model caps accessy_asset_publication_guid at 64 chars, but the table allows 255. Keeping them in sync avoids silent truncation later.
- `accessy_asset_publication_guid` VARCHAR(255) NOT NULL,
+ `accessy_asset_publication_guid` VARCHAR(64) NOT NULL,While you’re here, consider:
UNIQUE KEY uniq_group_door (group_id, accessy_asset_publication_guid),
ON DELETE CASCADE -- for the FKThis prevents duplicate rows and cleans up automatically when a group is removed.
🤖 Prompt for AI Agents
In api/src/migrations/0033_group_accessy_permissions.sql lines 1 to 11, the
column accessy_asset_publication_guid is defined as VARCHAR(255) but the Python
ORM model limits it to 64 characters; reduce the column size to VARCHAR(64) to
keep them consistent and prevent silent truncation. Additionally, add a UNIQUE
constraint on (group_id, accessy_asset_publication_guid) to prevent duplicate
entries, and modify the foreign key constraint on group_id to include ON DELETE
CASCADE to ensure related rows are automatically removed when a membership group
is deleted.
|
Good catch. Now it will show that no assets were found, and any existing assets that have been added will show up as "Unknown". |
Allows adding permissions for opening accessy doors to specific groups.