add observer role for fw lite projects#1710
Conversation
|
Important Review skippedAuto incremental 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 📝 WalkthroughWalkthroughThis update introduces the "Observer" project role across the backend, frontend, and API layers. It modifies enums, models, permission checks, database schema, and UI components to support and display the new role. Role-based logic for editing and viewing is refined, and feature flags are integrated for write permissions. Supporting migrations and documentation are included. Changes
Poem
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
deployment/Taskfile.yml (1)
79-81: Add adescfield for better self-documentation
To improve discoverability and clarify the purpose of this task, consider adding a brief description:local-fw-headless-forward: + desc: "Port-forward the fw-headless service (5275→8081) for FieldWorks Lite" cmds: - kubectl port-forward service/fw-headless 5275:8081 -n languagedepot --context docker-desktop
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs(6 hunks)backend/FwLite/FwLiteShared/Projects/LexboxProjectService.cs(4 hunks)backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs(1 hunks)backend/FwLite/LcmCrdt/CrdtProject.cs(2 hunks)backend/FwLite/LcmCrdt/CrdtProjectsService.cs(3 hunks)backend/FwLite/LcmCrdt/CurrentProjectService.cs(1 hunks)backend/FwLite/LcmCrdt/Migrations/20250526102635_AddProjectDataRole.Designer.cs(1 hunks)backend/FwLite/LcmCrdt/Migrations/20250526102635_AddProjectDataRole.cs(1 hunks)backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs(1 hunks)backend/LexBoxApi/Controllers/CrdtController.cs(3 hunks)backend/LexBoxApi/Controllers/LegacyProjectApiController.cs(1 hunks)backend/LexBoxApi/GraphQL/LexAuthUserOutOfSyncExtensions.cs(1 hunks)backend/LexBoxApi/Hub/CrdtProjectChangeHub.cs(1 hunks)backend/LexBoxApi/Services/PermissionService.cs(2 hunks)backend/LexCore/Auth/LexAuthUser.cs(3 hunks)backend/LexCore/Entities/Project.cs(2 hunks)backend/LexCore/Entities/ProjectRole.cs(1 hunks)backend/LexCore/ServiceInterfaces/IPermissionService.cs(1 hunks)backend/LexData/Entities/UserEntityConfiguration.cs(1 hunks)deployment/Taskfile.yml(1 hunks)frontend/schema.graphql(1 hunks)frontend/src/lib/components/Projects/FormatUserProjectRole.svelte(1 hunks)frontend/src/lib/forms/ProjectRoleSelect.svelte(2 hunks)frontend/src/lib/i18n/locales/en.json(1 hunks)frontend/src/lib/user.ts(2 hunks)frontend/src/lib/util/enums.ts(0 hunks)frontend/src/routes/(authenticated)/project/[project_code]/+page.svelte(3 hunks)frontend/src/routes/(authenticated)/project/[project_code]/AddProjectMember.svelte(2 hunks)frontend/src/routes/(authenticated)/project/[project_code]/BulkAddProjectMembers.svelte(1 hunks)frontend/src/routes/(authenticated)/project/[project_code]/ChangeMemberRoleModal.svelte(2 hunks)frontend/src/routes/(authenticated)/project/[project_code]/MembersList.svelte(3 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/IProjectModel.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectData.ts(1 hunks)frontend/viewer/src/project/NewEntryButton.svelte(3 hunks)frontend/viewer/src/project/browse/BrowseView.svelte(1 hunks)frontend/viewer/src/project/browse/EntryMenu.svelte(1 hunks)frontend/viewer/src/project/browse/EntryView.svelte(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/lib/util/enums.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/LexData/Entities/UserEntityConfiguration.cs (1)
backend/LexCore/Exceptions/ProjectMembersMustBeVerifiedForRole.cs (2)
ProjectMembersMustBeVerifiedForRole(5-11)ProjectMembersMustBeVerifiedForRole(7-10)
backend/FwLite/LcmCrdt/Migrations/20250526102635_AddProjectDataRole.Designer.cs (2)
backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs (1)
DbContext(13-639)backend/FwLite/LcmCrdt/Migrations/20250526102635_AddProjectDataRole.cs (1)
AddProjectDataRole(8-28)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (63)
frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectData.ts (1)
16-16: Generated type correctly reflects backend changes.The addition of the required
role: numberproperty aligns with the backendProjectDatarecord changes. The numeric type correctly represents theUserProjectRoleenum values from the backend.Consider adding a TypeScript enum or const assertion in the frontend to improve type safety when working with role values, rather than using raw numbers throughout the codebase.
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/IProjectModel.ts (1)
15-15: Consistent type generation for project role support.The
role: numberproperty addition is consistent with theIProjectDatainterface and correctly reflects the backendProjectModelchanges. This enables role-based UI behavior across both project data models.backend/LexCore/Entities/ProjectRole.cs (1)
9-9: LGTM! Observer role properly activated.The Observer role is correctly added with sequential value 4, maintaining the enum's numbering pattern. This change supports the PR objective of introducing observer functionality for fw lite projects.
frontend/src/lib/components/Projects/FormatUserProjectRole.svelte (1)
14-14: Consistent role mapping implementation.The Observer role is properly added to the roles mapping following the same pattern as other roles, correctly using the i18n localization system.
frontend/src/lib/user.ts (1)
1-18: Import reformatting looks good.The import statements have been properly organized with consistent spacing and ordering. No functional changes detected.
frontend/src/routes/(authenticated)/project/[project_code]/MembersList.svelte (3)
31-31: Well-structured optional prop addition.The
showObserverprop is properly typed as optional boolean, following good TypeScript practices.
44-44: Consistent prop destructuring.The
showObserverprop is correctly included in the destructured props, maintaining consistency with the component's prop handling pattern.
170-170: Proper prop propagation to child component.The
showObserverprop is correctly passed to theChangeMemberRoleModalcomponent, enabling conditional Observer role display functionality.backend/LexBoxApi/Controllers/LegacyProjectApiController.cs (1)
81-81: Good documentation improvement for backward compatibility.The comment clearly explains why the Observer role (and potentially future roles) maps to "unknown" in the legacy API, which is important for maintaining compatibility with older FieldWorks clients that don't understand new role types.
backend/LexCore/ServiceInterfaces/IPermissionService.cs (1)
13-13: Well-designed permission method addition.The new
AssertCanDownloadProjectmethod follows the established pattern of other permission assertion methods in the interface. This separation of download and sync permissions is appropriate for supporting the observer role, where observers may have download permissions but limited sync capabilities.frontend/schema.graphql (1)
1273-1273: Proper schema extension for observer role support.The addition of
OBSERVERto theProjectRoleenum follows the established naming convention and maintains logical ordering. This enables full GraphQL support for the new observer role across the API layer.frontend/viewer/src/project/browse/EntryMenu.svelte (1)
53-57: Excellent permission-based UI rendering.Conditionally showing the delete menu item based on the
features.writeflag is a proper implementation of role-based access control in the UI. This ensures that observers and other users without write permissions won't see destructive actions they cannot perform, improving both security and user experience.backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs (1)
50-52: LGTM! Auto-generated migration snapshot correctly reflects schema changes.The addition of the
Roleproperty withINTEGERcolumn type appropriately supports the newUserProjectRoleenum storage in the database.frontend/src/routes/(authenticated)/project/[project_code]/AddProjectMember.svelte (2)
25-25: LGTM! Validation schema correctly includes Observer role.The schema now properly validates the Observer role alongside Editor and Manager, maintaining Editor as the default.
102-102: LGTM! Conditional Observer role display is well-implemented.The
showObserver={project.hasHarmonyCommits}logic correctly limits Observer role visibility to FW Lite projects, which aligns with the PR objectives of Observer being "FieldWorks Lite only".frontend/src/lib/i18n/locales/en.json (1)
607-608: LGTM! Clear and appropriate localization for Observer role.The description "Observer (can not edit, FieldWorks Lite only)" effectively communicates both the read-only nature and the scope limitation of the role.
backend/FwLite/LcmCrdt/CrdtProject.cs (2)
30-31: LGTM! ProjectData record appropriately extended with role support.The addition of the
Roleparameter withUserProjectRole.Unknownas the default value provides good backward compatibility and fallback behavior for existing projects.
41-47: LGTM! Well-structured enum for user project roles.The
UserProjectRoleenum provides comprehensive role coverage with clear, descriptive values. The inclusion ofUnknownas a fallback andObserverfor read-only access aligns perfectly with the PR objectives.frontend/src/lib/forms/ProjectRoleSelect.svelte (1)
10-10: LGTM! Clean conditional role display implementation.The
showObserverprop with defaultfalseprovides a clean way to control Observer role visibility. The implementation follows consistent patterns with proper i18n usage and maintains backward compatibility.Also applies to: 13-13, 23-27
backend/LexBoxApi/Hub/CrdtProjectChangeHub.cs (1)
14-14: Appropriate permission relaxation for Observer role support.Changing from
AssertCanSyncProjecttoAssertCanDownloadProjectis correct for supporting Observer roles. Observers should be able to listen for project changes (to receive real-time updates) but don't need sync permissions, which typically imply write access.frontend/viewer/src/project/browse/EntryView.svelte (1)
22-22: Proper feature flag integration for write permission control.The implementation correctly combines the existing
readonlystate with thefeatures.writefeature flag using logical OR. This ensures the entry editor becomes read-only when either the local readonly state is true OR the write feature is disabled, which aligns perfectly with supporting Observer roles.Also applies to: 28-28, 105-105
frontend/viewer/src/project/NewEntryButton.svelte (1)
16-16: Correct feature flag gating for write operations.The implementation properly gates the "New Entry" button behind the
features.writeflag while preserving the existingisActivelogic. This ensures Observer role users won't see UI elements for operations they can't perform, providing a better user experience.Also applies to: 35-35, 44-44
frontend/src/routes/(authenticated)/project/[project_code]/ChangeMemberRoleModal.svelte (4)
12-12: LGTM! Optional prop correctly typed.The optional
showObserverprop is properly typed and maintains backward compatibility.
15-15: LGTM! Props destructuring updated correctly.The destructuring properly includes the new
showObserverprop with correct TypeScript typing.
18-18: LGTM! Schema validation correctly includes Observer role.The Zod schema properly validates the Observer role alongside Editor and Manager, ensuring form validation works correctly for all supported roles.
55-55: LGTM! Observer prop correctly passed to child component.The
showObserverprop is properly passed to theProjectRoleSelectcomponent, enabling conditional display of the Observer role option.backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs (2)
24-24: LGTM! Dynamic write permission based on user role.Good change from hardcoded
trueto the computedCanWriteproperty, enabling role-based write access control.
27-29: LGTM! Role-based write permission logic is sound.The implementation correctly:
- Allows write access for non-CRDT projects (maintains existing behavior)
- Restricts CRDT projects to Editor and Manager roles only
- Defaults to Editor role for backward compatibility with existing projects
- Effectively excludes Observer role from write permissions
This properly implements the Observer role as read-only access.
backend/FwLite/LcmCrdt/Migrations/20250526102635_AddProjectDataRole.cs (2)
13-18: LGTM! Well-structured database migration.The migration correctly:
- Adds a non-nullable integer Role column to ProjectData table
- Uses default value 0 (likely Editor role) for existing records
- Follows Entity Framework migration conventions
The default value ensures existing projects maintain write access during the migration.
22-27: LGTM! Proper rollback implementation.The Down method correctly removes the Role column, ensuring the migration is fully reversible.
backend/FwLite/LcmCrdt/CurrentProjectService.cs (1)
139-145: LGTM! Well-implemented role update method.The
UpdateUserRolemethod demonstrates good practices:
- Early return optimization prevents unnecessary database operations
- Uses
ExecuteUpdateAsyncfor efficient bulk updates without entity loading- Properly refreshes cached project data to maintain consistency
- Async implementation with proper awaiting
This provides a clean API for role updates while maintaining performance and data consistency.
backend/LexData/Entities/UserEntityConfiguration.cs (1)
72-73: LGTM! Observer role exemption aligns with read-only permissions.The addition of
ProjectRole.Observerto the email verification exemption is appropriate for a read-only role. The updated comment accurately reflects the current logic.backend/LexCore/Entities/Project.cs (2)
3-3: LGTM! Necessary import for JSON serialization.The
System.Text.Json.Serializationnamespace is correctly added to support theJsonStringEnumConverterattribute used in theFieldWorksLiteProjectrecord.
77-83: LGTM! Well-designed record with proper JSON serialization.The
FieldWorksLiteProjectrecord is well-implemented:
- Clean positional record syntax
- Appropriate use of
JsonStringEnumConverterfor theRoleproperty to ensure proper enum serialization- All properties have meaningful types and names
- Enables role-aware project operations
backend/LexBoxApi/GraphQL/LexAuthUserOutOfSyncExtensions.cs (1)
22-31: LGTM! Consistent implementation following established patterns.The new overload correctly implements role-aware synchronization checking:
- Follows the same pattern as existing overloads with admin check and count validation
- Safely handles potential null token membership with
SingleOrDefault- Properly compares roles to detect mismatches
- Integrates well with the new
FieldWorksLiteProjectrecordbackend/LexBoxApi/Controllers/CrdtController.cs (4)
38-38: LGTM! Appropriate permission check for read operation.Changing from
AssertCanSyncProjecttoAssertCanDownloadProjectis correct for theGetSyncStateendpoint, which is a read operation that doesn't modify data.
64-64: LGTM! Consistent permission check for read operation.The permission change to
AssertCanDownloadProjectis appropriate for theChangesendpoint, which retrieves commits without modifying the project.
79-89: LGTM! Enhanced return type with role information.The changes correctly:
- Update return type to
FieldWorksLiteProject[]to include role information- Project to the new record type with proper property mapping
- Include user's role via the relationship query
p.Users.Where(u => u.UserId == loggedInContext.User.Id).Select(m => m.Role).FirstOrDefault()The role lookup logic is sound and will return the user's role in each project or
Unknownif not found.
90-90: LGTM! Improved sync checking with role information.The change to pass full project objects to
IsOutOfSyncWithMyProjectsenables more accurate synchronization checking by comparing role information, not just project membership.backend/FwLite/FwLiteShared/Projects/LexboxProjectService.cs (4)
6-6: LGTM: Import added for role-aware project model.The import of
LexCore.Entitiesis necessary to use theFieldWorksLiteProjecttype that includes role information.
77-77: LGTM: JSON deserialization updated to match new return type.The deserialization target type correctly matches the updated method signature.
87-90: LGTM: New user retrieval method properly implemented.The
GetLexboxUsermethod correctly delegates to the OAuth client to retrieve the current user, which supports role-based functionality.
67-67:Details
❓ Verification inconclusive
Verify breaking change: Return type changed from LexboxProject to FieldWorksLiteProject.
The method signature change from
LexboxProject[]toFieldWorksLiteProject[]is a breaking change that should be verified across all calling code.
🏁 Script executed:
#!/bin/bash # Description: Find all usages of GetLexboxProjects to verify they handle the new return type # Expected: All callers should be updated to handle FieldWorksLiteProject instead of LexboxProject rg -A 3 "GetLexboxProjects" --type csLength of output: 1006
Verify handling of updated return type in callers
All callers of
GetLexboxProjectsnow receiveFieldWorksLiteProject[]instead ofLexboxProject[]. EnsureFieldWorksLiteProjectexposes the same members used by callers.• In
CombinedProjectsService.cs,lexboxProjects.Select(p => new ProjectModel(p.Name, …))now passesFieldWorksLiteProject.Name.
– ConfirmFieldWorksLiteProjectdefinesName.
– Review any other property or method used on the returned items.No other calls to
GetLexboxProjectswere found.Please verify that:
FieldWorksLiteProjectincludes all fields and behaviors expected by each caller.- No downstream code still refers to the old
LexboxProjecttype.- Update any mismatches or add mapping logic if needed.
backend/LexCore/Auth/LexAuthUser.cs (4)
211-211: LGTM: Observer role mapping added consistently.The mapping of
ProjectRole.Observerto character 'o' is consistent with the existing pattern for Manager ('m') and Editor ('e').
223-223: LGTM: Simplified array initialization using modern C# syntax.The use of
[]syntax for array initialization is cleaner and more modern.Also applies to: 228-228
233-233: LGTM: Observer role parsing added consistently.The parsing of character 'o' to
ProjectRole.Observercorrectly mirrors the serialization logic.
303-316:Details
✅ Verification successful
Verify backward compatibility of IsProjectMember method signature change.
The method signature changed from accepting a single nullable
ProjectRoleto accepting multiple roles viaparams Span<ProjectRole>. This is a breaking change that needs verification.The new logic correctly handles:
- Empty roles (line 307): Returns true if user is a member regardless of role
- Multiple roles (lines 309-313): Returns true if user has any of the specified roles
🏁 Script executed:
#!/bin/bash # Description: Find all usages of IsProjectMember to verify they're updated for the new signature # Expected: All callers should be updated to pass roles as individual parameters instead of nullable single role rg -A 3 "IsProjectMember" --type csLength of output: 4058
Backward compatibility verified for
LexAuthUser.IsProjectMembersignature changeAll existing calls to
IsProjectMember(Guid projectId)continue to work (empty-roles path preserved) and callers that need role checks have been updated to pass one or moreProjectRolevalues. No breaking changes detected.backend/FwLite/LcmCrdt/Migrations/20250526102635_AddProjectDataRole.Designer.cs (2)
53-54: LGTM: Role property correctly added to ProjectData entity.The migration designer correctly defines the new
Roleproperty as an integer type in theProjectDataentity, which aligns with theUserProjectRoleenum being stored as an integer.
1-643: LGTM: Auto-generated migration designer file is well-structured.The migration designer file correctly defines the complete database schema including the new Role property and all existing entities with their relationships, indexes, and constraints.
backend/FwLite/LcmCrdt/CrdtProjectsService.cs (3)
49-62: LGTM: UpdateProjectServerInfo method properly implemented with optimization.The method correctly:
- Performs early return if values haven't changed (line 54)
- Uses proper async disposal pattern with scoped services
- Calls appropriate update methods on CurrentProjectService
The conditional check prevents unnecessary database updates when values are unchanged.
101-101: LGTM: Role parameter properly added to CreateProjectRequest record.The optional
UserProjectRole? Roleparameter allows callers to specify a role during project creation while maintaining backward compatibility.
140-140: LGTM: Role parameter properly handled with sensible default.The
ProjectDatainstantiation correctly uses the role from the request, defaulting toUserProjectRole.Editorwhen not specified. This is a sensible default that maintains existing behavior while supporting the new Observer role.backend/LexBoxApi/Services/PermissionService.cs (3)
55-62: LGTM: Sync permission logic correctly restricts Observer role.The updated
CanSyncProjectmethod appropriately restricts sync permissions to only Editor and Manager roles, excluding the new Observer role. This aligns with the read-only nature of the Observer role.
64-71: LGTM: Download permission logic appropriately includes Observer role.The new
CanDownloadProjectmethod correctly allows any project member (including Observer) to download projects while maintaining the same organization manager permissions. This is appropriate since Observer users should be able to access project data for viewing purposes.
83-86: LGTM: Download assertion method follows established pattern.The new
AssertCanDownloadProjectmethod correctly follows the same pattern as other assertion methods in the service.backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs (4)
18-18: LGTM: Appropriate default role for backward compatibility.Setting
ProjectRole.Editoras the default is appropriate for maintaining backward compatibility with existing projects that don't have explicit roles assigned.
73-81: LGTM: Role synchronization logic is well-implemented.The
UpdateProjectServerInfomethod correctly synchronizes user roles between server projects and local CRDT projects, ensuring consistency across the system.
128-144: LGTM: Role conversion methods handle all cases appropriately.The role conversion helper methods correctly map between
ProjectRoleandUserProjectRoleenums, including the new Observer role. The fallback toUnknownfor unmapped roles is appropriate and safe.
105-105: LGTM: Safe handling of missing role data.The null-safe role conversion with fallback to
ProjectRole.Unknownwhen project data is missing is appropriate and prevents runtime errors.frontend/src/routes/(authenticated)/project/[project_code]/+page.svelte (4)
148-148: LGTM: Good refactoring for readability.Extracting
userIsOrgAdmininto a separate derived store improves code readability and eliminates duplication.
160-160: LGTM: Frontend sync permissions align with backend logic.The
canSyncProjectlogic correctly mirrors the backendCanSyncProjectmethod by allowing only admin, Manager, Editor roles, and org admins. This appropriately excludes Observer roles from sync operations.
368-430: LGTM: Improved project action button logic.The updated conditional logic for project action buttons correctly uses
canSyncProjectto determine when to show sync-related actions, which aligns with the new permission model. The restructured dropdown content improves code organization.
593-593: LGTM: Appropriate Observer role visibility.Showing the Observer role option only when
project.hasHarmonyCommitsis true correctly limits Observer role to FW lite projects, which aligns with the PR objective of adding Observer role specifically for FW lite projects.
Co-authored-by: Tim Haasdyk <tim_haasdyk@sil.org>
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
2 participants
closes #1708 Should we indicate in the UI when in read-only mode?