Allow guest status in organizations #8502
Conversation
📝 Walkthrough""" WalkthroughThis update introduces support for "Guest Users," allowing users already paid for by another organization to join additional organizations without consuming a paid user slot. Backend, frontend, and database logic are updated to recognize and display guest status, adjust user counting, and optimize user queries with new indices. Associated tests and documentation are also updated. Changes
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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 (
|
…o select multiple rows / mutliple multiusers
This reverts commit 5dc046e.
app/mail/DefaultMails.scala
Outdated
| @@ -84,7 +84,6 @@ class DefaultMails @Inject()(conf: WkConf) { | |||
| organizationName: String, | |||
| senderName: String): Mail = { | |||
| val host = Try { new URL(uri) }.toOption.getOrElse(uri) | |||
| println(s"Invite mail to $recipient with link $uri/invite/$inviteTokenValue") | |||
There was a problem hiding this comment.
Re-add this line for testing => enables finding the invite link needed for testing in the console
There was a problem hiding this comment.
by the way, you can also set mail.logToStdout = true in application.conf to see the mail contents in logging
| othersMayEdit BOOLEAN NOT NULL DEFAULT false, | ||
| othersMayEdit BOOLEAN NOT NULL DEFAULT FALSE, |
There was a problem hiding this comment.
@fm3 During development, I noticed that the case spelling off false is not consistent and I corrected here. Do you think it is ok to keep this?
| allowsUpload BOOLEAN NOT NULL DEFAULT true, | ||
| isScratch BOOLEAN NOT NULL DEFAULT FALSE, | ||
| isDeleted BOOLEAN NOT NULL DEFAULT FALSE, | ||
| allowsUpload BOOLEAN NOT NULL DEFAULT TRUE, |
There was a problem hiding this comment.
ok I also corrected a single occurence of true
| CREATE INDEX ON webknossos.users(created); | ||
| CREATE INDEX ON webknossos.users(_organization); | ||
| CREATE INDEX ON webknossos.users(isDeactivated); | ||
| CREATE INDEX ON webknossos.users(isUnlisted); |
There was a problem hiding this comment.
While looking whether using a view with the payingOrganization info backed into the view, I noticed potential improvements in the query plan by using more indices. These are all attributes that are filtered or sorted for, and thus an index makes sense; otherwise, the full table needs to always be scanned.
But as the users table does not have a lot of entries this is still ok the current state and not a must change here. But as these attributes rarely / never change, having an index on the should also be fine IMO
There was a problem hiding this comment.
I havent written a mirgation as it is unclear whether this should be kept
There was a problem hiding this comment.
Migration & reversion are now implemented
![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
♻️ Duplicate comments (1)
tools/postgres/schema.sql (1)
47-50: Consistent Boolean Defaults in Annotations Table
The columns forothersMayEditandisDeletednow useDEFAULT FALSEuniformly. This update resolves the earlier inconsistency noted in previous reviews.
🧹 Nitpick comments (2)
app/models/user/User.scala (1)
296-296: Guest status determination logic.The logic correctly determines a user is a guest when they belong to an organization different from their paying organization.
Consider adding a comment explaining this logic since it's a key part of the new feature.
- (payingOrganization._organization IS NOT NULL AND u._organization != payingOrganization._organization) AS isGuest + -- A user is a guest when they have a paying organization that's different from their current organization + (payingOrganization._organization IS NOT NULL AND u._organization != payingOrganization._organization) AS isGuesttools/postgres/schema.sql (1)
797-800: Index Additions on Users Table
New indexes oncreated,_organization,isDeactivated, andisUnlistedhave been introduced. These indexes should improve query performance for common filtering and sorting operations. It might be beneficial to assign explicit names to these indexes for easier future maintenance and reference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.unreleased.md(1 hunks)app/controllers/AuthenticationController.scala(1 hunks)app/models/user/MultiUser.scala(1 hunks)app/models/user/User.scala(5 hunks)app/models/user/UserService.scala(1 hunks)frontend/javascripts/admin/admin_rest_api.ts(1 hunks)frontend/javascripts/admin/onboarding.tsx(5 hunks)frontend/javascripts/admin/organization/pricing_plan_utils.ts(1 hunks)frontend/javascripts/admin/team/team_list_view.tsx(2 hunks)frontend/javascripts/admin/user/user_list_view.tsx(3 hunks)frontend/javascripts/test/fixtures/dummy_user.ts(1 hunks)frontend/javascripts/types/api_flow_types.ts(1 hunks)tools/postgres/schema.sql(28 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
frontend/javascripts/admin/organization/pricing_plan_utils.ts (1)
app/controllers/UserController.scala (1)
user(46-54)
app/models/user/MultiUser.scala (4)
app/utils/sql/SecuredSQLDAO.scala (1)
readAccessQuery(23-36)app/utils/sql/SqlInterpolation.scala (2)
q(19-38)as(53-73)app/models/dataset/credential/CredentialDAO.scala (1)
columns(25-25)app/utils/sql/SQLDAO.scala (1)
parseAll(40-43)
frontend/javascripts/admin/user/user_list_view.tsx (1)
frontend/javascripts/admin/organization/pricing_plan_utils.ts (1)
getActiveUserCount(31-33)
frontend/javascripts/admin/team/team_list_view.tsx (2)
app/controllers/UserController.scala (1)
user(46-54)frontend/javascripts/libs/format_utils.ts (1)
stringToColor(109-112)
🔇 Additional comments (46)
frontend/javascripts/test/fixtures/dummy_user.ts (1)
24-24: LGTM! Added isGuest property to test fixtureThe addition of the
isGuestproperty set tofalsein the dummy user object keeps the test fixture in sync with the new guest user functionality introduced in this PR.frontend/javascripts/types/api_flow_types.ts (1)
345-345: LGTM! Added isGuest property to APIUser typeCorrectly added the
isGuestproperty to theAPIUsertype with the appropriatereadonlymodifier, maintaining consistency with other user properties.CHANGELOG.unreleased.md (1)
15-15: LGTM! Clear and concise changelog entryThe changelog entry properly documents the new guest user functionality with a clear description and appropriate PR reference link.
frontend/javascripts/admin/admin_rest_api.ts (1)
1531-1536: LGTM! Added API method for getting the paying organizationThis new API method is essential for the guest user functionality, allowing the system to determine which organization is financially responsible for a user.
frontend/javascripts/admin/organization/pricing_plan_utils.ts (1)
31-32: Correctly filters out guest users from active user countThe
getActiveUserCountfunction now excludes users withisGueststatus when calculating the number of active users. This aligns with the PR objective of allowing guest users to join organizations without utilizing a paid slot.app/models/user/MultiUser.scala (1)
146-153: New method to find multiple users by email at onceThis new utility method allows batch retrieval of user records based on email addresses, which is useful for implementing the guest user functionality. The implementation correctly applies access control via
readAccessQuery.app/controllers/AuthenticationController.scala (1)
237-239: Guest user implementation allows bypassing user limits when appropriateThe implementation now checks if the user already belongs to a paying organization before enforcing user limits. This correctly implements the PR objective of allowing users to join organizations as guests without consuming a paid slot if they're already covered in another organization.
frontend/javascripts/admin/user/user_list_view.tsx (3)
307-307: Renamed variable for better semantic clarityThe variable has been renamed from
isUserInvitesDisabledtoisNewUserInvitesDisabledto more accurately reflect its purpose - controlling whether new (non-guest) user invites are disabled.
344-344: UI adapts to show the appropriate invite optionThe button label now conditionally displays "Invite Guests" when regular user invites are disabled due to user limits. This provides a clear indication to the user that they can still invite people as guests even when the organization has reached its user limit.
373-374: Consistent application of renamed variableThese conditional rendering updates correctly use the renamed variable to maintain consistent behavior in the UI, showing the appropriate alerts based on whether new users can be invited.
app/models/user/UserService.scala (1)
405-405: New guest status is properly exposed in the JSON response.Adding the isGuest field to the user JSON representation ensures that the frontend can correctly identify and display users with guest status.
frontend/javascripts/admin/team/team_list_view.tsx (4)
9-9: Added Tooltip component import from antd.The Tooltip component is now correctly imported to support the guest user tooltip functionality.
33-41: Guest user role is properly implemented with helpful tooltip.The implementation correctly checks for guest users and adds an appropriate tag with clear visual indication (lime color) and an informative tooltip explaining that guest users don't count against the organization's user quota.
49-55: Good refactoring with the renderTag helper function.Extracting the Tag rendering logic into a separate function improves code maintainability and reduces duplication.
57-65: Added tooltip support for team role tags.The implementation now conditionally wraps tags with tooltips when tooltipText is provided, enhancing the UI with additional context for users.
app/models/user/User.scala (6)
88-89: Added isGuest field to UserCompactInfo.The isGuest field is properly added to the UserCompactInfo case class, supporting the guest status functionality.
207-207: Updated GetResultUserCompactInfo to include isGuest field.The GetResult implementation is properly updated to include the new isGuest field when parsing database results.
212-224: New query for identifying paying organizations.This subquery correctly identifies users' paying organizations by:
- Finding distinct records per multi-user (avoiding duplicates)
- Filtering out deactivated users
- Only including organizations with paid plans (Team, Power, Custom)
- Ordering by creation date to get the earliest paying organization
303-303: Added left join with paying organization info.The LEFT JOIN is correctly implemented to preserve users without a paying organization while allowing identification of guest users.
387-399: New method to find paying organization for multi-user.This method follows the project's coding style and correctly:
- Finds the paying organization for a multi-user
- Excludes deactivated users
- Only considers organizations with paid plans
- Orders by creation date to get the earliest one
428-435: Updated user count logic to exclude guest users.The countAllForOrganization method now correctly excludes guest users (users with a different paying organization) from the count, which is essential for implementing the guest user feature properly.
frontend/javascripts/admin/onboarding.tsx (7)
234-234: Added check for organization user limit.This variable clearly indicates when the organization has reached its maximum user limit, which is needed to determine when to show guest-specific UI.
254-256: Improved calculation to check if invites would exceed limit.The new variable provides a clearer way to determine if adding new users would exceed the organization's limit, allowing for appropriate UI feedback.
257-257: Simplified getContent function signature.Removing the unnecessary parameter simplifies the function interface since doNewUsersExceedLimit is now used directly.
286-292: Added informative message about guest invitations.This message clearly explains the implications of reaching the user limit and informs users they can only invite guests, including what that means.
314-314: Dynamic modal title based on organization limit status.The title now correctly changes between "Invite Guests" and "Invite Users" based on whether the organization has reached its user limit.
319-321: Invite button is no longer disabled when limit is reached.This change allows organizations to continue inviting guests even when they've reached their user limit, which is a key part of the guest user feature.
329-329: Updated getContent call to match new signature.The function call has been updated to match the modified function signature.
tools/postgres/schema.sql (18)
92-92: Consistent Default in Meshes Table
TheisDeletedcolumn now consistently usesDEFAULT FALSEin the meshes table.
102-102: Uniform Boolean Default in Publications Table
TheisDeletedcolumn here now has its default set toFALSE, in line with the overall schema consistency goal.
118-130: Standardized Boolean Defaults in Datasets Table
The changes forisPublic(line 118),isUsable(line 119), andisDeleted(line 130) ensure that these flags use a consistentDEFAULT FALSEsetting. This improves readability and helps avoid ambiguity in later queries.
184-184: Consistent Boolean Flag in Dataset_Mags Table
ThehasLocalDatacolumn now explicitly defaults toFALSE, matching the style used throughout the schema.
249-249: Uniform Default in Scripts Table
TheisDeletedcolumn in thescriptstable now usesDEFAULT FALSEconsistently.
265-266: Boolean Defaults in TaskTypes Table
Thesettings_volumeInterpolationAllowedandsettings_mergerModecolumns are now set toDEFAULT FALSE, ensuring consistency with other boolean columns.
235-241: Consistent Boolean Flags in Projects Table
The columns (notablypausedon line 236,isBlacklistedFromReporton line 238, andisDeletedon line 240) have been updated to useDEFAULT FALSE. This consistency is beneficial for any business logic relying on these flags.
307-308: Uniform Boolean Defaults in Teams Table
BothisOrganizationTeam(line 307) andisDeleted(line 308) now consistently default toFALSE.
320-320: Standardized Boolean in Timespans Table
TheisDeletedcolumn now usesDEFAULT FALSE, aligning it with similar flags across the schema.
469-469: Consistent Default in Tokens Table
TheisDeletedcolumn is now uniformly set toFALSE, which helps prevent subtle bugs during token filtration.
479-479: Uniform Boolean in Maintenances Table
TheisDeleteddefault is now clearly set toFALSE, which matches the design patterns used in other tables.
446-452: Consistent Boolean Flags in MultiUsers Table
The columnsisSuperUser(line 446),isEmailVerified(line 451), andisDeleted(line 452) have been updated to default toFALSE. This reinforces consistency and aligns with previous stylistic corrections.
411-411: Boolean Default in User_Team_Roles Table
TheisTeamManagercolumn now explicitly defaults toFALSE, which is consistent with the other similar boolean columns in the schema.
558-558: Consistent Boolean in Folders Table
TheisDeletedcolumn is now set toDEFAULT FALSE, ensuring that folder records are correctly marked as active by default.
582-582: Uniform Default in EmailVerificationKeys Table
TheisUsedcolumn now usesDEFAULT FALSE, aligning with the schema’s preferred style for boolean defaults.
931-935: Verification Logic in countsAsTaskInstance Function
The function now returnsRETURN (a.state != 'Cancelled' AND a.isDeleted = FALSE AND a.typ = 'Task');This clearly defines what counts as a valid task instance. The boolean test uses
FALSEconsistently. No issues noted here.
952-958: Trigger Function onInsertAnnotation – Logical Consistency
The conditionIF (NEW.typ = 'Task') AND (NEW.isDeleted = FALSE) AND (NEW.state != 'Cancelled') THENproperly ensures that only active task annotations affect the pending instance count. If there is any future consideration for guest users (which might have different handling), it may be worth revisiting this logic; otherwise, it is correct as is.
989-995: Trigger Function onDeleteAnnotation – Correct Update of Task Counts
The function correctly checks that if an annotation with typeTaskis deleted (and it wasn’t already flagged as deleted or cancelled), the related task’spendingInstancescounter is incremented. The consistent boolean comparisons maintain clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (28)
tools/postgres/schema.sql (28)
47-50: [Duplicate] Consistent Boolean Defaults in Annotations Table
The columnsothersMayEditandisDeletedhave been updated to default to uppercaseFALSE, ensuring consistency with other tables.
92-92: [Duplicate] Standardize Deletion Flag in Meshes
TheisDeletedcolumn in themeshestable now defaults toFALSE, aligning its configuration with the rest of the schema.
102-102: [Duplicate] Standardize Deletion Flag in Publications
The default forisDeletedin thepublicationstable is now set toFALSE, which improves overall consistency.
118-119: [Duplicate] Boolean Defaults for Visibility Flags in Datasets
TheisPublicandisUsablecolumns in thedatasetstable are now set to default toFALSE, ensuring uniform boolean handling.
130-130: [Duplicate] Consistent Deletion Flag in Datasets
TheisDeletedcolumn in thedatasetstable has been updated to default toFALSE, matching the convention used throughout the schema.
184-184: [Duplicate] StandardizehasLocalDataFlag in Dataset Mags
ThehasLocalDatacolumn now defaults toFALSE, ensuring that the flag correctly reflects data availability.
214-216: [Duplicate] Consistent Boolean Defaults in DataStores
The flagsisScratch,isDeleted, andallowsUploadnow have explicit boolean defaults (FALSE,FALSE, andTRUErespectively), which clarifies their intended behavior.
218-218: [Duplicate] Standardize Storage Reporting Flag in DataStores
The columnreportUsedStorageEnablednow consistently defaults toFALSE, aligning with the overall schema conventions.
226-226: [Duplicate] Standardize Deletion Flag in TracingStores
TheisDeletedcolumn in thetracingStorestable now explicitly defaults toFALSE, ensuring consistency across datastore tables.
236-240: [Duplicate] Uniform Boolean Flags in Projects
The columnspaused,isBlacklistedFromReport, andisDeletedin theprojectstable have been updated to use consistent boolean defaults (FALSE), which aids in clarity and predictable behavior.
249-249: [Duplicate] Consistent Deletion Flag in Scripts
TheisDeletedflag in thescriptstable now defaults toFALSE, bringing it in line with the rest of the schema.
265-266: [Duplicate] Update Boolean Settings in TaskTypes
Bothsettings_volumeInterpolationAllowedandsettings_mergerModeare now explicitly set to default toFALSE, which standardizes their configuration.
292-292: [Duplicate] Standardize Deletion Flag in Tasks
TheisDeletedcolumn in thetaskstable now defaults toFALSE, ensuring uniform deletion semantics across tables.
320-320: [Duplicate] Consistent Deletion Flag in Timespans
The default forisDeletedin thetimespanstable has been standardized toFALSE, matching the convention used in other areas.
307-308: [Duplicate] Boolean Flags Update in Teams
TheisOrganizationTeamandisDeletedfields in theteamstable are now consistently defaulted toFALSE, ensuring clarity in team status information.
396-403: [Duplicate] Consistent Boolean Flags in Users
The flagsisDeactivated,isAdmin,isOrganizationOwner,isDatasetManager, andisDeletedin theuserstable now default toFALSE, reinforcing consistency and clarity in user state management.
411-411: [Duplicate] Standardize Team Manager Flag in User-Team Roles
TheisTeamManagerflag now defaults toFALSE, which aligns its handling with similar boolean fields across the schema.
446-452: [Duplicate] Uniform Boolean Defaults in MultiUsers
The columnsisSuperUser,isEmailVerified, andisDeletedin themultiUserstable now have consistent defaults (FALSE), ensuring predictable behavior.
469-469: [Duplicate] Consistent Deletion Flag in Tokens
TheisDeletedcolumn in thetokenstable now explicitly defaults toFALSE, providing clarity and consistency.
479-479: [Duplicate] Uniform Deletion Flag in Maintenances
TheisDeletedcolumn in themaintenancestable has been standardized to default toFALSE, which is consistent with the rest of the schema.
492-492: [Duplicate] Standardized Deletion Flag in Workers
TheisDeletedflag now defaults toFALSEin theworkerstable, aligning its configuration with the established schema standards.
510-514: [Duplicate] Consistent Boolean Flags in Jobs
BothretriedBySuperUserandisDeletedin thejobstable are now given explicit defaults (FALSE), ensuring consistency with the rest of the database schema.
525-525: [Duplicate] Standardize Deletion Flag in Invites
TheisDeletedcolumn in theinvitestable now defaults toFALSE, which helps maintain uniform deletion semantics across tables.
533-533: [Duplicate] Consistent Deletion Flag in Annotation PrivateLinks
The default forisDeletedin theannotation_privateLinkstable has been updated toFALSE, ensuring alignment with schema-wide standards.
552-552: [Duplicate] Standardize Deletion Flag in Credentials
TheisDeletedcolumn in thecredentialstable is now set to default toFALSE, contributing to overall schema consistency.
558-558: [Duplicate] Uniform Deletion Flag in Folders
TheisDeletedflag in thefolderstable now defaults toFALSE, which is consistent with similar flags in other tables.
582-582: [Duplicate] Standardize Verification Flag in EmailVerificationKeys
TheisUsedfield in theemailVerificationKeystable now defaults toFALSE, ensuring a consistent boolean representation.
598-598: [Duplicate] Consistent Deletion Flag in AI Models
TheisDeletedcolumn in theaiModelstable now explicitly defaults toFALSE, maintaining uniformity with the rest of the schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.unreleased.md(1 hunks)app/controllers/AuthenticationController.scala(1 hunks)app/models/user/MultiUser.scala(1 hunks)app/models/user/User.scala(5 hunks)app/models/user/UserService.scala(1 hunks)frontend/javascripts/admin/admin_rest_api.ts(1 hunks)frontend/javascripts/admin/onboarding.tsx(5 hunks)frontend/javascripts/admin/organization/pricing_plan_utils.ts(1 hunks)frontend/javascripts/admin/team/team_list_view.tsx(2 hunks)frontend/javascripts/admin/user/user_list_view.tsx(3 hunks)frontend/javascripts/test/fixtures/dummy_user.ts(1 hunks)frontend/javascripts/types/api_flow_types.ts(1 hunks)tools/postgres/schema.sql(28 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
app/models/user/MultiUser.scala (4)
app/utils/sql/SecuredSQLDAO.scala (2)
readAccessQuery(23-36)existingCollectionName(16-16)app/utils/sql/SqlInterpolation.scala (2)
q(19-38)as(53-73)app/models/dataset/credential/CredentialDAO.scala (1)
columns(25-25)app/utils/sql/SQLDAO.scala (1)
parseAll(40-43)
frontend/javascripts/admin/team/team_list_view.tsx (2)
app/controllers/UserController.scala (1)
user(46-54)frontend/javascripts/libs/format_utils.ts (1)
stringToColor(109-112)
frontend/javascripts/admin/admin_rest_api.ts (2)
webknossos-datastore/app/DsRequestHandler.scala (1)
options(13-21)frontend/javascripts/libs/request.ts (1)
RequestOptions(32-32)
frontend/javascripts/admin/user/user_list_view.tsx (1)
frontend/javascripts/admin/organization/pricing_plan_utils.ts (1)
getActiveUserCount(31-33)
🔇 Additional comments (32)
frontend/javascripts/test/fixtures/dummy_user.ts (1)
24-24: Looks good - adds isGuest property to test fixtureThe addition of the
isGuestproperty to the dummy user object correctly implements the new field required for the guest user feature. Setting the default value tofalseis appropriate for this test fixture.frontend/javascripts/types/api_flow_types.ts (1)
345-345: Correctly implements isGuest property in APIUser typeThe new
readonly isGuest: booleanproperty is properly added to the APIUser type interface. Making it readonly is consistent with the pattern used for other properties in this interface and prevents modification after initialization.CHANGELOG.unreleased.md (1)
15-15: Good documentation of the new guest user featureThe changelog entry clearly describes the functionality being added and includes the correct PR reference. This provides good documentation for users about the new guest user capability.
frontend/javascripts/admin/organization/pricing_plan_utils.ts (1)
32-32: Correctly excludes guest users from active user countThis change is crucial for implementing the guest user feature. By excluding users with
isGuest: truefrom the active user count, the function now properly reflects that guest users don't consume paid slots in an organization's user limit.frontend/javascripts/admin/admin_rest_api.ts (1)
1531-1536:❓ Verification inconclusive
New API method for determining guest status
This function correctly retrieves the organization that's financially responsible for a given user, which is essential for the guest user feature to function properly.
Let's verify that all API endpoints are properly defined in the backend:
🏁 Script executed:
#!/bin/bash # Check if the controller endpoint exists for the new API method rg -A 5 "def.*payingOrganization" app/controllersLength of output: 51
Action: Confirm Backend Endpoint Implementation
The frontend function for retrieving the organization (i.e.,
getOrganizationPayingForActiveUser) looks correct and appropriately calls the/api/user/:activeUserId/payingOrganizationendpoint. However, our initial grep in theapp/controllersdirectory did not reveal a matching backend implementation. Please manually verify that the corresponding API endpoint is defined—either in the expected controller, within the routing configuration (e.g.,config/routes.rb), or elsewhere in the backend code.
- Check for proper route declaration: Ensure that
/api/user/:activeUserId/payingOrganizationis registered in the backend (e.g., inconfig/routes.rb).- Confirm controller implementation: Verify that a controller method handling this endpoint exists and follows the naming convention.
Once the backend endpoint is confirmed, the frontend change can be approved.
frontend/javascripts/admin/user/user_list_view.tsx (3)
307-307: Renamed variable to better reflect guest user functionalityThe variable was renamed from
isUserInvitesDisabledtoisNewUserInvitesDisabledto more accurately reflect that guest invites are still allowed even when paid user limits are reached.
344-344: Dynamic invite button text based on organization limitsThe button now intelligently displays either "Invite Guests" or "Invite Users" based on whether the organization has reached its paid user limit, providing better context to the user.
373-374: Updated conditional rendering to use renamed variableReferences to the renamed variable have been consistently updated in the conditional rendering logic for alerts.
app/controllers/AuthenticationController.scala (1)
237-239: Core guest user functionality implementationThis change implements the central logic for the guest user feature, allowing users to join organizations without counting against the user limit if they already have a paying organization.
However, it might be helpful to add some logging when a user is added as a guest to help with debugging and auditing.
_ <- Fox.runIf(!(requestingMultiUser.isSuperUser || alreadyPayingOrgaForMultiUser.isDefined))(organizationService .assertUsersCanBeAdded(organization._id)(GlobalAccessContext, ec)) ?~> "organization.users.userLimitReached" + _ = alreadyPayingOrgaForMultiUser.foreach(payingOrgaId => + logger.info(s"User ${request.identity._id} joined organization ${organization._id} as guest (paying org: $payingOrgaId)"))app/models/user/UserService.scala (1)
404-406: Adding isGuest field to JSON response is a clean approach.This change properly exposes the new
isGuestfield from theUserCompactInfoclass through the API, making it available to frontend components.frontend/javascripts/admin/team/team_list_view.tsx (3)
9-9: Adding required Tooltip import.Adding the Tooltip import is necessary for the new tooltip functionality for guest users.
33-41: Good implementation of guest user visual indicator.The conditional check for
user.isGuestproperly adds a visual indicator with appropriate styling and a helpful tooltip message explaining that guest users don't count against the organization's user quota.
49-65: Excellent refactoring of tag rendering logic.This refactoring improves the code quality by:
- Extracting tag rendering into a separate function
- Adding conditional tooltip wrapping when needed
- Maintaining the same visual appearance while supporting the new functionality
The change simplifies future modifications and enhances maintainability.
frontend/javascripts/admin/onboarding.tsx (7)
234-234: Clear flag for organization limit status.This variable provides a clear boolean flag to determine if the organization has reached its user limit, which is then used appropriately throughout the component.
254-256: Accurately determining if user limits would be exceeded.This check properly calculates whether adding the invited users would exceed the organization's limit, which is better than the previous implementation as it accounts for the exact number of new invites.
257-272: Improved user feedback on limit exceedance.The alert message now more clearly explains that inviting more users would exceed the organization's limit. It also gives users a direct action to upgrade their plan.
286-292: Good user guidance for organizations at their limit.This conditional message properly explains to users that when their organization has reached its user limit, they can only invite guests who already have a paid account in another organization.
314-315: Dynamic modal title improves user experience.Changing the title between "Invite Guests" and "Invite Users" based on the organization's limit status provides clear context to users about what action they're taking.
319-322: Enabling invite button regardless of limit is correct.Since guest users can be invited even when the organization is at its limit, removing the button disabling is the right approach. The system will handle the appropriate user creation on the server side.
329-330: Simplified function call enhances readability.Removing the unnecessary parameter passing simplifies the code and makes it more maintainable.
app/models/user/User.scala (8)
88-89: Adding isGuest field to UserCompactInfo.This addition to the
UserCompactInfocase class properly extends the model to support the guest user functionality.
207-207: Updated GetResult implementation for modified case class.The
GetResultUserCompactInfoimplicit has been correctly updated to accommodate the newly addedisGuestfield, ensuring proper database mapping.
212-224: Well-structured subquery for paying organization detection.This subquery effectively identifies organizations with paying plans that cover a user. The
DISTINCT ON (_multiUser)combined withORDER BY _multiUser, created ASCensures that for each multi-user, the earliest created paying organization is selected, which is a reasonable approach.
296-296: Clear logic for determining guest status.The condition
(payingOrganization._organization IS NOT NULL AND u._organization != payingOrganization._organization)correctly identifies users who are in multiple organizations, with one of them being a paying organization different from the current one.
303-303: Appropriate left join for paying organization data.Using a left join ensures that users without a paying organization are still included in the results, which is important for maintaining a complete user list.
307-309: Updated GROUP BY clause to include all necessary columns.The GROUP BY clause has been properly updated to include all columns required for the guest user functionality, ensuring SQL correctness.
387-400: Useful helper method for finding paying organization.This new method provides a clean way to find the paying organization ID for a multi-user, which could be reused in other parts of the codebase if needed.
428-435: Updated user count logic accounts for guest status.The
countAllForOrganizationmethod now correctly filters out users who are guests in the specified organization (by checkingpayingOrganization._organization IS NULL OR payingOrganization._organization = $organizationId), ensuring they don't count against the organization's quota.tools/postgres/schema.sql (4)
797-800: Additional Indexes on Users Table
New indexes oncreated,_organization,isDeactivated, andisUnlistedin theuserstable have been introduced to improve performance for filtering and sorting. Please verify that these indexes align with the query patterns in your application.
933-933: Consistent Boolean Check in countsAsTaskInstance Function
The boolean condition now explicitly usesa.isDeleted = FALSE, which leverages the standardized default and improves overall clarity in the function’s logic.
954-954: Consistent Check in onInsertAnnotation Function
The trigger function now relies on a condition that checks(NEW.typ = 'Task') AND (NEW.isDeleted = FALSE) AND (NEW.state != 'Cancelled'), consistently enforcing that only active and non-deleted annotations affect task instance counts.
991-993: Consistent Condition in onDeleteAnnotation Function
The function now properly verifies that an annotation is not marked as deleted and is active before adjusting thependingInstancesin tasks. This change ensures consistency with the standardized boolean defaults.
MichaelBuessemeyer
changed the title
|
@fm3 The guest feature should not be tested via the e2e tests in a kinda indirect fashion: I added a new multiuser to the e2e tests with its own unpaid organization and made this user part of the other two already existing organizations. Moreover, the user of organization Y is now also a guest in organization X. Thus, testing that a orga is paying for a user that joined their organization but has no organization already paying for the user & having a guest user whose own orga already pays for the user. The Important: This is not directly testing whether joining an organization works. Only whether the In case you think, thats enought, please feel free to re-review my changes. |
| @@ -29,7 +29,7 @@ export const powerPlanFeatures = [ | |||
| export const maxInludedUsersInBasicPlan = 3; | |||
|
|
|||
| export function getActiveUserCount(users: APIUser[]): number { | |||
| return users.filter((user) => user.isActive && !user.isSuperUser).length; | |||
| return users.filter((user) => user.isActive && !user.isSuperUser && !user.isGuest).length; | |||
There was a problem hiding this comment.
I’m not sure that superUsers should be skipped here. I think I’d rather check the isUnlisted property here for less confusion.
E.g. I was surprised to see that the sample user doesn’t count in their own sample_organization.
While this was not introduced in this PR, I think it makes sense to change that now.
I think that’s also what we use in the backend to determine if the limit is reached
There was a problem hiding this comment.
Ok I now exposed the isUnlisted field to the frontend. IMO this should be ok as this properly is nothing sensitive that must be protected
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
conf/evolutions/reversions/131-more-indices-on-users.sql (4)
10-15: MarkcountsAsTaskInstanceIMMUTABLE for planner optimizations
This function has no side effects and yields the same result for the same input—declaring itIMMUTABLEcan improve query planning.@@ - $$ LANGUAGE plpgsql; + $$ LANGUAGE plpgsql IMMUTABLE;
17-24: HardenonInsertAnnotationtrigger function
Since this function performs writes towebknossos.tasks, consider addingSECURITY DEFINERto ensure it has the necessary privileges and clearly document its intended trigger timing (e.g.,AFTER INSERT) and filtering via aWHENclause.-- Example enhancement: CREATE OR REPLACE FUNCTION webknossos.onInsertAnnotation() RETURNS trigger LANGUAGE plpgsql SECURITY DEFINER AS $$ BEGIN -- your logic RETURN NULL; END; $$;
26-33: Apply best practices toonDeleteAnnotationas well
Mirror the security and timing enhancements suggested for the insert trigger: useSECURITY DEFINER, specifyAFTER DELETE, and add aWHENclause to scope it to task annotations only.
35-35: Scope the schemaVersion update to a single row
Even ifreleaseInformationcurrently has only one row, adding aWHEREclause can prevent accidental multi-row updates in the future.- UPDATE webknossos.releaseInformation SET schemaVersion = 130; + UPDATE webknossos.releaseInformation + SET schemaVersion = 130 + WHERE id = 1; -- adjust to match your PK
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.unreleased.md(1 hunks)MIGRATIONS.unreleased.md(1 hunks)app/controllers/AuthenticationController.scala(1 hunks)app/models/user/User.scala(5 hunks)app/models/user/UserService.scala(1 hunks)conf/evolutions/131-more-indices-on-users.sql(1 hunks)conf/evolutions/reversions/131-more-indices-on-users.sql(1 hunks)frontend/javascripts/admin/organization/pricing_plan_utils.ts(1 hunks)frontend/javascripts/admin/team/team_list_view.tsx(2 hunks)frontend/javascripts/test/backend-snapshot-tests/users.e2e.ts(2 hunks)frontend/javascripts/test/e2e-setup.ts(2 hunks)frontend/javascripts/types/api_flow_types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- MIGRATIONS.unreleased.md
🚧 Files skipped from review as they are similar to previous changes (9)
- CHANGELOG.unreleased.md
- frontend/javascripts/types/api_flow_types.ts
- app/models/user/UserService.scala
- frontend/javascripts/admin/organization/pricing_plan_utils.ts
- frontend/javascripts/test/e2e-setup.ts
- frontend/javascripts/test/backend-snapshot-tests/users.e2e.ts
- app/controllers/AuthenticationController.scala
- frontend/javascripts/admin/team/team_list_view.tsx
- app/models/user/User.scala
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (6)
conf/evolutions/reversions/131-more-indices-on-users.sql (1)
37-38: Transaction boundaries are properly defined
TheSTART TRANSACTION;/COMMITpairing ensures atomicity of this reversion.conf/evolutions/131-more-indices-on-users.sql (5)
1-4: Transaction control and version assertion are correct
UsingSTART TRANSACTION;along with an assertion onschemaVersion = 130ensures the migration only runs in the expected state.
10-15: FunctioncountsAsTaskInstanceis well-defined
The conditions(a.typ = 'Task') AND (a.state != 'Cancelled') AND (a.isDeleted = FALSE)correctly capture which annotations count as pending task instances. Boolean literal casing is consistent with the rest of your schema evolutions.
17-24: Verify trigger registration foronInsertAnnotation
The trigger function logic is sound, but please confirm that there is anAFTER INSERT ON webknossos.annotationstrigger defined (or replaced) to invokewebknossos.onInsertAnnotation(). Without it, the pendingInstances counter won’t be maintained as intended.
26-33: Trigger functiononDeleteAnnotationlogic is correct
The inverse operation for deletions properly incrementspendingInstances. This complements the insert trigger and maintains consistency in the task instance counts.
35-37: Schema version bump and commit finalize the migration
UpdatingschemaVersionto 131 and committing the transaction wrap up the evolution cleanly.
| START TRANSACTION; | ||
|
|
||
| do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 131, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a deterministic check instead of ASSERT
PL/pgSQL ASSERT statements may be disabled in production builds; an explicit version check with RAISE EXCEPTION ensures the migration always halts on a mismatch.
- do $$ begin ASSERT (
- SELECT schemaVersion FROM webknossos.releaseInformation
-) = 131, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql;
+ do $$ begin
+ IF (SELECT schemaVersion FROM webknossos.releaseInformation) <> 131 THEN
+ RAISE EXCEPTION 'Expected schemaVersion=131, got %', (SELECT schemaVersion FROM webknossos.releaseInformation);
+ END IF;
+ end; $$ LANGUAGE plpgsql;📝 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.
| START TRANSACTION; | |
| do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 131, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql; | |
| START TRANSACTION; | |
| do $$ begin | |
| IF (SELECT schemaVersion FROM webknossos.releaseInformation) <> 131 THEN | |
| RAISE EXCEPTION 'Expected schemaVersion=131, got %', (SELECT schemaVersion FROM webknossos.releaseInformation); | |
| END IF; | |
| end; $$ LANGUAGE plpgsql; |
There was a problem hiding this comment.
@fm3 Do we maybe want to consider changing to exceptions due to the reason named above?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Fine by me. I don’t think we ever had this problem, but the new code looks ok too. Please test it first, though.
- name "user" was used twice. This is now resolved.
MichaelBuessemeyer
dismissed
fm3’s stale review
should be fine now. Moreover, I did double the feature
This PR allows users to join other organizations via invites without occupying on of their paid slots in case they are already part of an organization paying for their account. Such accounts are now called guest accounts.
URL of deployed dev instance (used for testing):
Steps to test:
3.application.confsetisWkorgInstance = trueand start wk.That should roughly test all paths a single time :)
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)