Master to develop 0.95 #4684
Master to develop 0.95 #4684
Conversation
Version bump
[0.92.3] Bugfix unable to edit Callout-Whiteboard templates
[v0.93.0] Roles API, Unauthenticated Explore page (Merge conflict fix)
Authorization saved on subspace level
[0.93.1] #Placeholder title#
Minor version bump
Release: VC Data Patch
* Upgrade whiteboard-collaboration-service to 0.5.0 (#4625) Co-authored-by: Valentin Yanakiev <[email protected]> * change search index pattern var (#4640) * Org verification (#4627) * ensure org verification status is set properly after every event * Update src/domain/community/organization-verification/organization.verification.lifecycle.options.provider.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update src/domain/community/organization-verification/organization.verification.lifecycle.options.provider.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Addressing pr feedback --------- Co-authored-by: Evgeni Dimitrov <[email protected]> Co-authored-by: Valentin Yanakiev <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * subspace as a qualified search type * version bump (#4644) --------- Co-authored-by: Carlos Cano <[email protected]> Co-authored-by: Valentin Yanakiev <[email protected]> Co-authored-by: Neil Smyth <[email protected]> Co-authored-by: Evgeni Dimitrov <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This reverts commit bd130dd.
[0.95.0] Subspace Templates, XState v5 upgrade
…pproval Fix community application self-approval
# Conflicts: # src/domain/access/application/application.service.authorization.ts # src/domain/access/application/application.service.lifecycle.ts # src/domain/access/role-set/role.set.resolver.mutations.ts # src/domain/access/role-set/role.set.service.lifecycle.application.ts
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the addition of a new enumeration value and the inclusion of a Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
src/core/bootstrap/platform-template-definitions/space/bootstrap.space.callouts.ts (1)
19-19: Consider grouping related properties together.The visibility property is correctly typed using the CalloutVisibility enum. However, consider grouping it with other metadata properties like
typeandstatefor better code organization.{ nameID: 'welcome', type: CalloutType.POST, + visibility: CalloutVisibility.PUBLISHED, contributionPolicy: { state: CalloutState.OPEN, }, sortOrder: 1, groupName: CalloutGroupName.HOME, - visibility: CalloutVisibility.PUBLISHED, framing: { profile: {src/core/bootstrap/platform-template-definitions/space-tutorials/bootstrap.space.tutorials.callouts.ts (1)
Line range hint
11-173: Consider implementing a factory pattern for callout creationTo reduce repetition and improve maintainability, consider implementing a factory function for creating callouts with default values. This would centralize the visibility setting and make future changes easier to manage.
Example implementation:
const createCallout = (callout: Partial<CreateCalloutInput>): CreateCalloutInput => ({ visibility: CalloutVisibility.PUBLISHED, contributionPolicy: { state: CalloutState.OPEN, }, ...callout }); export const bootstrapSpaceTutorialsCallouts: CreateCalloutInput[] = [ createCallout({ nameID: 'welcome', type: CalloutType.POST, sortOrder: 1, groupName: CalloutGroupName.HOME, framing: { // ... existing framing configuration }, }), // ... other callouts ];src/core/bootstrap/platform-template-definitions/subspace-knowledge/bootstrap.subspace.knowledge.callouts.ts (2)
Line range hint
83-87: Critical: Duplicate nameID 'vc-profile' detectedThere are two callout objects with the same nameID 'vc-profile'. This could cause issues with uniqueness constraints and make it difficult to reference specific callouts.
Suggestion: Rename one of the instances to be more specific about its purpose, for example:
- nameID: 'vc-profile', + nameID: 'vc-profile-location',Also applies to: 221-225
18-18: Style: Consider standardizing property orderingWhile the
visibilityproperty has been correctly added to all callouts, its placement varies slightly between objects. Consider standardizing the property order across all callout objects for better maintainability.Suggested property order:
- nameID
- type
- visibility
- contributionPolicy
- sortOrder
- groupName
- framing
Also applies to: 41-41, 64-64, 87-87, 110-110, 133-133, 156-156, 179-179, 202-202, 225-225, 248-248
src/domain/collaboration/collaboration/collaboration.service.ts (2)
363-366: LGTM! Consider enhancing the comment.The defensive check for timeline existence before deletion is a good practice. However, the comment could be more descriptive.
Consider updating the comment to be more specific:
- // There's no timeline for collaboration templates + // Skip timeline deletion for collaboration templates as they don't have associated timelines
Line range hint
748-756: Security: Potential SQL injection vulnerability in raw queries.The SQL queries use string interpolation to inject values directly into the query string. This is unsafe and could lead to SQL injection attacks.
Consider using parameterized queries with TypeORM's query builder:
await this.entityManager .createQueryBuilder() .select('COUNT(*)', 'postsCount') .from('collaboration', 'collaboration') .rightJoin('callout', 'callout', 'callout.collaborationId = collaboration.id') .rightJoin('callout_contribution', 'callout_contribution', 'callout_contribution.calloutId = callout.id') .where('collaboration.id = :id', { id: collaboration.id }) .andWhere('callout.visibility = :visibility', { visibility: CalloutVisibility.PUBLISHED }) .andWhere('callout.type = :type', { type: CalloutType.POST_COLLECTION }) .getRawOne();Also applies to: 766-775
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/common/enums/authorization.privilege.ts(1 hunks)src/core/bootstrap/platform-template-definitions/space-tutorials/bootstrap.space.tutorials.callouts.ts(8 hunks)src/core/bootstrap/platform-template-definitions/space/bootstrap.space.callouts.ts(2 hunks)src/core/bootstrap/platform-template-definitions/subspace-knowledge/bootstrap.subspace.knowledge.callouts.ts(12 hunks)src/core/bootstrap/platform-template-definitions/subspace/bootstrap.subspace.callouts.ts(3 hunks)src/domain/collaboration/callout/callout.resolver.mutations.ts(1 hunks)src/domain/collaboration/collaboration/collaboration.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/common/enums/authorization.privilege.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/core/bootstrap/platform-template-definitions/space-tutorials/bootstrap.space.tutorials.callouts.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/core/bootstrap/platform-template-definitions/space/bootstrap.space.callouts.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/core/bootstrap/platform-template-definitions/subspace-knowledge/bootstrap.subspace.knowledge.callouts.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/core/bootstrap/platform-template-definitions/subspace/bootstrap.subspace.callouts.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/collaboration/callout/callout.resolver.mutations.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/collaboration/collaboration/collaboration.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🔇 Additional comments (12)
src/core/bootstrap/platform-template-definitions/space/bootstrap.space.callouts.ts (1)
8-8: LGTM! Clean import statement.
The import follows TypeScript best practices using absolute imports.
src/common/enums/authorization.privilege.ts (2)
33-33: LGTM! New privilege follows consistent naming pattern.
The new COMMUNITY_APPLY_ACCEPT privilege aligns well with existing community-related privileges and completes the logical flow of community application process (apply → accept).
33-33: Verify authorization guards implementation.
Please ensure that authorization guards and related community application acceptance functionality are properly implemented for this new privilege.
Let's verify the implementation:
src/core/bootstrap/platform-template-definitions/subspace/bootstrap.subspace.callouts.ts (3)
7-7: LGTM! Import statement follows consistent pattern
The import of CalloutVisibility enum follows the established pattern and is correctly placed with related imports.
42-42: LGTM! Consistent visibility implementation
The visibility property is consistently implemented across both callouts, maintaining a uniform structure.
18-18: Verify the visibility change impact on existing welcome callout
The addition of visibility property is correct, but let's verify if this change affects any existing behavior or filters.
✅ Verification successful
Let me gather more information about the visibility usage in these files.
Setting visibility to PUBLISHED is consistent with existing welcome callouts
The visibility change to PUBLISHED for the welcome callout is consistent with the existing pattern across the codebase. All welcome callouts in other templates (space, space-tutorials) also use CalloutVisibility.PUBLISHED. The codebase handles published callouts appropriately through visibility checks in queries and event processing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any visibility-dependent queries or filters for welcome callouts
# Look for any code that might be affected by this visibility change
# Search for welcome callout references
rg -l "nameID.*welcome" --type ts
# Search for visibility filters in queries
ast-grep --pattern 'find($$$).where({ visibility: $_ })'
Length of output: 369
Script:
#!/bin/bash
# Check content of files that reference welcome callouts
rg -A 5 -B 5 "nameID.*welcome" --type ts
# Search for any visibility-related filters or checks
rg -A 3 -B 3 "visibility.*Callout" --type ts
# Look for any visibility enum usage
rg -A 2 "CalloutVisibility\." --type ts
Length of output: 47114
src/core/bootstrap/platform-template-definitions/space-tutorials/bootstrap.space.tutorials.callouts.ts (3)
8-8: LGTM: Import statement follows best practices
The import for CalloutVisibility is properly placed and follows the established pattern of using aliased paths.
19-19: LGTM: Visibility property appropriately set for welcome callout
The visibility setting is consistent with the welcome callout's purpose as an introductory element.
42-42: LGTM: Consistent visibility implementation across all callouts
The visibility property has been uniformly set to PUBLISHED across all tutorial callouts, which is appropriate for their educational purpose and public accessibility needs.
Also applies to: 65-65, 88-88, 111-111, 134-134, 157-157
src/core/bootstrap/platform-template-definitions/subspace-knowledge/bootstrap.subspace.knowledge.callouts.ts (2)
7-7: LGTM: Import statement follows conventions
The new import for CalloutVisibility is correctly placed and follows the established pattern for enum imports.
18-18: Verify: All callouts set to PUBLISHED visibility
All callouts have been set to CalloutVisibility.PUBLISHED. Please verify that this is the intended visibility state for all callouts, especially considering some contain setup instructions or example content.
Let's check if there are any callouts in other files with different visibility states:
Also applies to: 41-41, 64-64, 87-87, 110-110, 133-133, 156-156, 179-179, 202-202, 225-225, 248-248
✅ Verification successful
Let me search for any historical changes or other visibility states to better understand if this was a deliberate change.
Let me search for any previous visibility states in these callouts to understand if this was a deliberate change.
Let me check if there are any other visibility states defined in the CalloutVisibility enum to understand the available options.
Setting all callouts to PUBLISHED is appropriate
The visibility change to CalloutVisibility.PUBLISHED for all callouts is correct and intentional because:
- The
CalloutVisibilityenum only has two states:DRAFTandPUBLISHED - These callouts are part of bootstrap/template definitions that are used to initialize spaces and subspaces
- Template callouts should be immediately visible and usable, making
PUBLISHEDthe appropriate state - Having template callouts in
DRAFTstate would prevent users from interacting with them until manually published
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other callout visibility declarations to ensure consistency
rg -A 1 "visibility:" "src/core/bootstrap"
Length of output: 5722
Script:
#!/bin/bash
# Search for git history of visibility changes
git log -p | rg -B 2 -A 2 "visibility:"
Length of output: 77569
Script:
#!/bin/bash
# Search for previous visibility states in git history for these specific files
git log -p -- src/core/bootstrap/platform-template-definitions/subspace-knowledge/bootstrap.subspace.knowledge.callouts.ts | rg -B 2 -A 2 "visibility:"
Length of output: 1701
Script:
#!/bin/bash
# Search for CalloutVisibility enum definition
rg -A 5 "enum CalloutVisibility"
Length of output: 387
src/domain/collaboration/callout/callout.resolver.mutations.ts (1)
Line range hint 125-146: LGTM! Template check prevents unnecessary notifications.
The addition of the template check !savedCallout.isTemplate is a good safeguard to prevent unnecessary notifications and activity logs for template callouts.
Consider separating notification and activity logging concerns.
The method currently handles multiple responsibilities: visibility updates, publish info updates, notifications, and activity logging. Consider extracting these into separate methods for better maintainability.
Example refactor:
- if (!savedCallout.isTemplate && savedCallout.visibility !== oldVisibility) {
- if (savedCallout.visibility === CalloutVisibility.PUBLISHED) {
- // Save published info
- await this.calloutService.updateCalloutPublishInfo(
- savedCallout,
- agentInfo.userID,
- Date.now()
- );
-
- if (calloutData.sendNotification) {
- const notificationInput: NotificationInputCalloutPublished = {
- triggeredBy: agentInfo.userID,
- callout: callout,
- };
- await this.notificationAdapter.calloutPublished(notificationInput);
- }
-
- const activityLogInput: ActivityInputCalloutPublished = {
- triggeredBy: agentInfo.userID,
- callout: callout,
- };
- this.activityAdapter.calloutPublished(activityLogInput);
- }
- }
+ if (this.shouldHandleVisibilityChange(savedCallout, oldVisibility)) {
+ await this.handleCalloutPublished(savedCallout, calloutData, agentInfo);
+ }
+ private shouldHandleVisibilityChange(callout: ICallout, oldVisibility: CalloutVisibility): boolean {
+ return !callout.isTemplate &&
+ callout.visibility !== oldVisibility &&
+ callout.visibility === CalloutVisibility.PUBLISHED;
+ }
+ private async handleCalloutPublished(
+ callout: ICallout,
+ calloutData: UpdateCalloutVisibilityInput,
+ agentInfo: AgentInfo
+ ): Promise<void> {
+ await this.updatePublishInfo(callout, agentInfo);
+ await this.sendNotifications(callout, calloutData, agentInfo);
+ await this.logActivity(callout, agentInfo);
+ }Consider aligning notification and activity log behavior.
The sendNotification flag only controls notifications but not activity logs. Consider either:
- Applying the flag to both notifications and activity logs for consistency
- Renaming the flag to clarify its limited scope
Let's verify the usage of sendNotification flag across the codebase:
✅ Verification successful
Let me generate additional verification scripts to understand the activity logging pattern across the codebase:
This will help us understand if the separation between notifications and activity logs is a consistent pattern throughout the codebase.
Let me try one final search pattern to understand the notification and activity logging patterns:
Based on the codebase analysis, I can now provide a final response regarding the separation between notifications and activity logs:
The separation between notifications and activity logs is an intentional design pattern.
The codebase shows a clear architectural separation between notifications and activity logging:
- Notifications (
NotificationAdapter) are used for real-time user alerts and communications - Activity logs (
ActivityAdapter) are used for tracking and recording system events persistently
This separation is consistent throughout the codebase, where:
- Notifications are conditionally sent based on user preferences (
sendNotificationflag) - Activity logs are always recorded for audit and history purposes
- Both systems have their own dedicated adapters, DTOs, and modules
The current implementation follows this pattern correctly by:
- Using
sendNotificationflag only for notifications - Always logging activities regardless of notification preferences
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of sendNotification to understand the intended behavior
rg "sendNotification" -A 5
Length of output: 2550
Script:
#!/bin/bash
# Search for activity logging patterns to understand if there's a consistent pattern
rg "activityAdapter.*published" -A 5
# Search for other notification and activity pairs
rg "notificationAdapter.*published.*\n.*activityAdapter.*published" -A 5
Length of output: 305
Script:
#!/bin/bash
# Search for activity logging patterns
rg "activityAdapter.*published"
# Search for notification patterns with multiline context
rg "notificationAdapter.*published" -B 2 -A 2
Length of output: 81
Script:
#!/bin/bash
# Search for any activity or notification adapter usage
rg -i "activity.*adapter|notification.*adapter" -B 2 -A 2
# Search for activity logging methods
rg -i "activity.*log" -B 2 -A 2
Length of output: 162358
There's one change left after the merge, but I'll fix it in a separate PR, where I'll remove all unused definitions in
AuthorizationPrivilegeSummary by CodeRabbit
New Features
COMMUNITY_APPLY_ACCEPT.visibilityproperty to multiple callout objects, standardizing their visibility status to published across various tutorials and knowledge base entries.Bug Fixes
Documentation