Accept terms of Service at signup #8193
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the user registration process by integrating terms of service (TOS) acceptance directly into the signup form. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
frcroth
changed the title
|
Backend should be done. @dieknolle3333 do you want to take a look at this for the frontend? |
|
sounds good! |
|
I feel like adding an iframe increases the probability that users actually read the ToS before accepting them, but then the privacy statement should also be an iframe. And two iframes either make the page quite long or are quite small, so users would have to scroll a lot to read them. Thats why I chose to add a link for now. If anybody disagrees with this decision, please let me know! |
|
this is how I built the frontend for now |
|
@dieknolle3333 Thank you so much! One thing I noticed is that the frontend sends an "organization" value used as the org id. However, it is a random string, not the slug-name of the organization as I expected (from the changes from #7386). So, if I create an account as "Monika Musterfrau" I would expect the organization value to be either empty (it will be handled by the backend then) or "monika-musterfrau-lab", not "3c98af371407d694" |
|
I will have a look |
|
@frcroth I implemented the org id generation similar to the org name generation now, and this id is sent to |
![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
🧹 Outside diff range and nitpick comments (9)
frontend/javascripts/admin/auth/registration_form_wkorg.tsx (2)
30-31: Add error and loading states for terms fetching.The terms fetching implementation should handle error states and loading conditions to provide better user feedback.
- const terms = useFetch(getTermsOfService, null, []); + const { data: terms, error: termsError, loading: termsLoading } = useFetch(getTermsOfService, null, []); + + if (termsError) { + // Consider showing an error message or fallback UI + console.error('Failed to fetch terms:', termsError); + }
182-207: Simplify ToS checkbox rendering and enhance accessibility.The conditional rendering logic for the ToS checkbox could be simplified. Also, consider adding aria-labels for better accessibility.
- {terms != null && !terms.enabled ? null : ( + {(!terms || terms.enabled) && ( <FormItem name="tos_check" valuePropName="checked" rules={[ { validator: (_, value) => value ? Promise.resolve() : Promise.reject(new Error(messages["auth.tos_check_required"])), }, ]} > - <Checkbox disabled={terms == null}> + <Checkbox + disabled={terms == null} + aria-label="Accept terms of service" + > I agree to the{" "} {terms == null ? ( "terms of service" ) : ( <a target="_blank" href={terms.url} rel="noopener noreferrer"> terms of service </a> )} . </Checkbox> </FormItem> )}frontend/javascripts/admin/auth/registration_form_generic.tsx (2)
304-329: Simplify terms of service checkbox rendering conditionThe current condition
terms != null && !terms.enabled ? null : (...)is complex and could be simplified.Consider this cleaner approach:
-{terms != null && !terms.enabled ? null : ( +{terms?.enabled && ( <FormItem name="tos_check" valuePropName="checked" rules={[ { validator: (_, value) => value ? Promise.resolve() : Promise.reject(new Error(messages["auth.tos_check_required"])), }, ]} > - <Checkbox disabled={terms == null}> + <Checkbox> I agree to the{" "} - {terms == null ? ( - "terms of service" - ) : ( - <a target="_blank" href={terms.url} rel="noopener noreferrer"> - terms of service - </a> - )} + <a target="_blank" href={terms.url} rel="noopener noreferrer"> + terms of service + </a> . </Checkbox> </FormItem> )}
281-330: Enhance form accessibilityThe checkboxes section needs accessibility improvements.
Consider these enhancements:
- Add aria-label to the checkboxes section
- Add aria-describedby to link the checkbox labels
- Ensure proper focus management
-<div className="registration-form-checkboxes"> +<div + className="registration-form-checkboxes" + role="group" + aria-label="Terms and Privacy" +>frontend/stylesheets/main.less (1)
683-691: LGTM with minor suggestions for improvement.The implementation correctly handles the spacing between checkbox elements in the registration form. Consider these optional improvements:
- Use CSS custom properties for margins to maintain consistency with other spacing values
- Consider a more semantic class name like
registration-form-consent-groupto better indicate purpose-.registration-form-checkboxes { +.registration-form-consent-group { > div:first-of-type { - margin-bottom: 0px; + margin-bottom: 0; } > div:last-of-type { - margin-bottom: 15px; + margin-bottom: var(--ant-margin-md); } }app/controllers/OrganizationController.scala (2)
141-144: Consider adding rate limiting for ToS acceptanceThe ToS acceptance endpoints (
acceptTermsOfServiceandtermsOfServiceAcceptanceNeeded) could benefit from rate limiting to prevent potential abuse. This is especially important since these endpoints are called during critical organization management operations.Consider implementing rate limiting using Play's built-in action composition or a dedicated rate limiting service.
Also applies to: 168-190
Line range hint
168-190: Enhance deadline validation logicThe
termsOfServiceAcceptanceNeededendpoint's deadline validation could be improved:
- Consider using a more precise timestamp comparison
- Add timezone handling for the deadline
- Consider moving this logic to the service layer for consistency
- deadlinePassed = acceptanceDeadline.toEpochMilli < System.currentTimeMillis() + now = java.time.Instant.now() + deadlinePassed = acceptanceDeadline.isBefore(now)frontend/javascripts/router.tsx (1)
Line range hint
196-198: Consider moving Terms of Service check to registration-specific routes.Given that the PR objectives specifically mention integrating ToS acceptance into the signup process, the current global placement might be broader than necessary. Consider moving this component to be rendered only within registration-related routes (e.g.,
/auth/signup,/invite/:token) to better align with the intended functionality.Example approach:
- <DisableGenericDnd /> - <CheckTermsOfServices /> - <Navbar isAuthenticated={isAuthenticated} /> + <DisableGenericDnd /> + <Navbar isAuthenticated={isAuthenticated} />Then add it to specific routes:
<RouteWithErrorBoundary path="/auth/signup" - render={() => (isAuthenticated ? <Redirect to="/" /> : <RegistrationView />)} + render={() => (isAuthenticated ? <Redirect to="/" /> : ( + <> + <CheckTermsOfServices /> + <RegistrationView /> + </> + ))} />app/controllers/AuthenticationController.scala (1)
742-743: Document and validate Terms of Service fieldThe
acceptedTermsOfServicefield has been added correctly, but consider:
- Adding ScalaDoc to document the field's purpose and expected values
- Adding validation to ensure the version number is positive
Apply this improvement:
case class SignUpData(organization: String, organizationName: String, email: String, firstName: String, lastName: String, password: String, inviteToken: Option[String], + /** Version of Terms of Service accepted by the user during signup. + * Must be a positive integer matching the current ToS version. */ acceptedTermsOfService: Option[Int])And in the form mapping:
- "acceptedTermsOfService" -> optional(number) + "acceptedTermsOfService" -> optional(number.verifying(_ > 0))Also applies to: 758-759, 769-770
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
app/controllers/AuthenticationController.scala(6 hunks)app/controllers/OrganizationController.scala(1 hunks)app/models/organization/OrganizationService.scala(4 hunks)conf/application.conf(2 hunks)frontend/javascripts/admin/auth/registration_form_generic.tsx(3 hunks)frontend/javascripts/admin/auth/registration_form_wkorg.tsx(3 hunks)frontend/javascripts/messages.tsx(1 hunks)frontend/javascripts/router.tsx(1 hunks)frontend/stylesheets/main.less(1 hunks)
🔇 Additional comments (16)
frontend/javascripts/admin/auth/registration_form_wkorg.tsx (1)
Line range hint 1-224: Implementation successfully meets PR objectives.
The changes effectively integrate ToS acceptance into the signup process as required. The implementation:
- Fetches ToS information from the API
- Includes ToS acceptance in the form submission
- Properly handles the UI components for ToS acceptance
app/models/organization/OrganizationService.scala (2)
4-4: LGTM: Required imports for ToS functionality
The new imports support timestamp handling and i18n for the Terms of Service feature.
Also applies to: 14-14
169-176: LGTM: Well-implemented ToS acceptance logic
The implementation includes proper validation of ToS settings, version matching, and timestamp recording. Good use of i18n for error messages.
Let's verify the configuration and usage:
✅ Verification successful
Let me gather more information about the DAO implementation and configuration to ensure completeness.
Terms of Service implementation is correctly integrated across the codebase
The verification confirms that:
- The DAO layer properly persists ToS acceptance with version and timestamp
- The service layer correctly validates ToS settings and version matching
- The controller layer implements proper access control (organization owner check)
- The feature is consistently used in both organization management and authentication flows
- The version checking is consistently implemented across all components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ToS configuration and usage across the codebase
# Check if ToS configuration exists
echo "Checking ToS configuration..."
rg "WebKnossos\.TermsOfService\.(enabled|version)"
# Verify consistent version checking across the codebase
echo "Checking ToS version usage..."
rg "WebKnossos\.TermsOfService\.version"
# Find all calls to acceptTermsOfService
echo "Checking acceptTermsOfService usage..."
ast-grep --pattern 'acceptTermsOfService($_, $_)'
Length of output: 1461
Script:
#!/bin/bash
# Check DAO implementation
echo "Checking OrganizationDAO implementation..."
rg "acceptTermsOfService.*Instant" -A 5
# Check configuration structure
echo "Checking ToS configuration structure..."
fd "application.conf" --exec rg "TermsOfService" {} -A 3
# Check controller implementation
echo "Checking controller implementation..."
rg "def acceptTermsOfService" -A 5
Length of output: 3716
conf/application.conf (2)
149-149: Verify the relationship between instance type and ToS functionality
The change to isWkorgInstance = true appears unrelated to the Terms of Service integration. According to the comments, this setting affects how demo datasets are opened. Please clarify:
- Is this change necessary for ToS integration?
- If not, should it be part of a separate PR?
#!/bin/bash
# Description: Check for dependencies between ToS and instance type
# Look for code that might couple ToS functionality with instance type
rg -l "isWkorgInstance.*termsOfService|termsOfService.*isWkorgInstance"119-119: Review development-only configuration changes
According to the PR checklist, development-only changes should be removed from the configuration file. Please verify if enabling Terms of Service in the development configuration is intentional or if this should be handled differently in development vs. production environments.
✅ Verification successful
Development configuration change for Terms of Service is acceptable
The Terms of Service configuration in conf/application.conf is appropriate because:
- The file is clearly marked as a development configuration with the comment "This is a dev deployment"
- No production configuration files were found that would be affected by this change
- The ToS configuration block contains all necessary settings (URL, deadline, version) which indicates it's a complete and intentional setup
- Development environments need ToS enabled for testing the feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other environment-specific ToS configurations
# Look for other configuration files that might contain environment-specific ToS settings
fd -e conf -e config -e properties | xargs rg -l "termsOfService.enabled"
Length of output: 167
Script:
#!/bin/bash
# Let's try a different approach to find configuration files and their content
# First, find all configuration files
fd -e conf -e config -e properties
# Then specifically look for the terms of service configuration in application.conf
cat conf/application.conf | grep -A 5 -B 5 "termsOfService"
Length of output: 726
Script:
#!/bin/bash
# Let's check other configuration files for any production or environment-specific settings
cat webknossos-datastore/deployment/config/datastore-docker.conf
cat webknossos-tracingstore/deployment/config/tracingstore-docker.conf
# Also check if there are any reference configuration files
fd "reference.conf|prod.conf|production.conf"
Length of output: 1367
frontend/javascripts/admin/auth/registration_form_generic.tsx (3)
12-13: LGTM: Clean import additions
The new imports are well-organized and necessary for the Terms of Service functionality.
Line range hint 34-57: Verify form submission with terms of service
The form submission logic needs to be verified to ensure the terms of service acceptance is properly sent to the backend.
Let's check the API integration:
#!/bin/bash
# Search for the API endpoint handling terms of service acceptance
rg -A 10 "createOrganizationWithAdmin|register"
# Check if the terms version is included in the request
ast-grep --pattern 'await Request.sendJSONReceiveJSON($path, { data: $data })'31-31: Verify error handling for terms fetching
The terms fetching implementation looks good, but we should ensure proper error handling for failed API requests.
Let's check if there's error handling in the API utility:
✅ Verification successful
Let me search for the error handling implementation in the Request utility.
Let me try searching for the Request utility implementation with a different path pattern.
Let me search for the Request utility implementation in a different location.
Let me get more context about the Request utility implementation to check its error handling.
Based on the implementation of the Request utility, I can now provide a final response about the error handling for terms fetching.
Error handling is properly implemented for terms fetching
The Request utility has comprehensive error handling that covers the terms fetching implementation:
- HTTP errors are automatically caught and processed
- Failed requests trigger error toasts by default (showErrorToast: true)
- JSON parsing errors are handled gracefully
- Network timeouts can be configured
- Empty responses are handled appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling in the terms of service API implementation
rg -A 5 "getTermsOfService"
Length of output: 122788
Script:
#!/bin/bash
# Search for Request utility implementation
rg -A 10 "Request\.(sendJSONReceiveJSON|sendJSON)" frontend/javascripts/libs/
Length of output: 79
Script:
#!/bin/bash
# Search for Request utility implementation
rg -A 10 "Request\.(sendJSONReceiveJSON|sendJSON)" frontend/javascripts/
Length of output: 78066
Script:
#!/bin/bash
# Search for Request utility implementation
rg -A 10 "class Request" frontend/javascripts/
Length of output: 727
Script:
#!/bin/bash
# Get the full implementation of the Request utility
cat frontend/javascripts/libs/request.ts
Length of output: 11209
frontend/stylesheets/main.less (1)
683-691: Verify visual consistency across themes.
Since this styling affects the registration form which is a critical user journey, let's ensure the margins look consistent across different themes (light/dark).
✅ Verification successful
The margin changes are safe across themes
After analyzing the codebase, I can confirm that the margin changes to .registration-form-checkboxes are theme-independent and safe because:
- The dark theme implementation in
dark.lessdoesn't override any form-related margins or registration form styles - The margins are defined using absolute pixel values (0px and 15px) which remain consistent regardless of theme
- The styling is purely structural (margin spacing) and doesn't involve any theme-specific properties like colors or backgrounds
The changes will maintain visual consistency across both light and dark themes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the registration form styling is consistent across themes
# Look for any theme-specific overrides that might affect the margins
# Check for any theme-specific styles affecting registration forms
rg -l "registration-form|form.*margin|checkbox.*margin" --type less
# Check for any dark theme specific overrides
rg -l "dark.*registration|dark.*form|dark.*checkbox" --type less
Length of output: 207
Script:
#!/bin/bash
# Let's examine the actual styling content in the identified files
# and check for any theme-specific variations
# Check main.less for theme-related styling around registration forms
rg "registration-form|dark-theme|theme" -B 2 -A 2 frontend/stylesheets/main.less
# Check _publications.less for form-related styling
rg "form.*margin|checkbox.*margin" -B 2 -A 2 frontend/stylesheets/_publications.less
# Look for any theme variables or mixins that might affect forms
rg "theme|@dark|@light" --type less
Length of output: 2042
Script:
#!/bin/bash
# Let's check the dark theme implementation and any form-related styles in it
cat frontend/stylesheets/dark.less
# Also check antd theme customization which might affect forms
cat frontend/stylesheets/antd_overwrites.less
# Look for any registration form specific theme overrides
rg "registration|form" frontend/stylesheets/dark.less
Length of output: 8988
app/controllers/OrganizationController.scala (2)
143-143: LGTM: Good refactoring to service layer
Moving the Terms of Service acceptance logic to organizationService improves separation of concerns and maintainability.
141-144: Verify error handling in the service layer
The ToS acceptance flow has proper authorization checks but we should verify the error handling in the service layer, especially for version mismatch scenarios.
✅ Verification successful
Error handling for ToS version mismatch is properly implemented
The service layer has comprehensive error handling for the Terms of Service acceptance flow:
- Validates if ToS feature is enabled via
conf.WebKnossos.TermsOfService.enabled - Checks version match against
conf.WebKnossos.TermsOfService.version - Uses Fox monadic error handling with
?~>to propagate failures with appropriate error messages - Handles version mismatch with a detailed error message including both required and provided versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling in organizationService.acceptTermsOfService
ast-grep --pattern 'def acceptTermsOfService($_, $_) {
$$$
}'
# Look for Fox error handling patterns
rg -A 5 '\?~>' app/models/organization/OrganizationService.scala
Length of output: 2053
frontend/javascripts/router.tsx (2)
72-72: LGTM! Clean import statement.
The import follows the existing pattern and is correctly placed with other component imports.
72-72: Consider the architectural implications of the Terms of Service check placement.
The CheckTermsOfServices component is placed at a high level in the component hierarchy, making it available across all routes. While this ensures consistent Terms of Service checking throughout the application, consider:
- Does this component need to be mounted for all routes, including public/unauthenticated routes?
- Could this impact performance by checking Terms of Service status on every route change?
- Should the check be more targeted to specific routes (e.g., only authenticated routes or specific features)?
Also applies to: 196-198
frontend/javascripts/messages.tsx (1)
411-412: LGTM! The new message key follows conventions and aligns with requirements.
The new Terms of Service message key follows the existing naming pattern and maintains consistency with similar messages (e.g., privacy check). The message clearly communicates the requirement to users.
app/controllers/AuthenticationController.scala (2)
642-648: LGTM: Clean implementation of ToS acceptance
The implementation is well-structured with proper error handling and context management.
624-625: Validate Terms of Service version
Consider adding validation to ensure the provided acceptedTermsOfService version is current and valid before acceptance.
| function generateOrganizationId(firstName: string, lastName: string) { | ||
| return `${firstName.toLowerCase()}-${lastName.toLowerCase()}-lab`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input sanitization to prevent invalid organization IDs.
The organization ID generation could produce invalid or duplicate IDs. Consider:
- Sanitizing special characters and spaces
- Adding uniqueness suffix
- Handling edge cases (empty strings, very long names)
function generateOrganizationId(firstName: string, lastName: string) {
- return `${firstName.toLowerCase()}-${lastName.toLowerCase()}-lab`;
+ const sanitize = (str: string) => str
+ .toLowerCase()
+ .trim()
+ .replace(/[^a-z0-9-]/g, '')
+ .replace(/-+/g, '-');
+ const timestamp = Date.now().toString(36);
+ return `${sanitize(firstName)}-${sanitize(lastName)}-lab-${timestamp}`;
}📝 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.
| function generateOrganizationId(firstName: string, lastName: string) { | |
| return `${firstName.toLowerCase()}-${lastName.toLowerCase()}-lab`; | |
| } | |
| function generateOrganizationId(firstName: string, lastName: string) { | |
| const sanitize = (str: string) => str | |
| .toLowerCase() | |
| .trim() | |
| .replace(/[^a-z0-9-]/g, '') | |
| .replace(/-+/g, '-'); | |
| const timestamp = Date.now().toString(36); | |
| return `${sanitize(firstName)}-${sanitize(lastName)}-lab-${timestamp}`; | |
| } |
| async function onFinish(formValues: Record<string, any>) { | ||
| const { firstName, lastName } = formValues; | ||
| const trimmedFirstName = firstName.trim(); | ||
| const trimmedLastName = lastName.trim(); | ||
| await Request.sendJSONReceiveJSON("/api/auth/createOrganizationWithAdmin", { | ||
| data: { | ||
| ...formValues, | ||
| firstName: formValues.firstName.trim(), | ||
| lastName: formValues.lastName.trim(), | ||
| firstName: trimmedFirstName, | ||
| lastName: trimmedLastName, | ||
| password: { | ||
| password1: formValues.password.password1, | ||
| password2: formValues.password.password1, | ||
| }, | ||
| organization: organizationId.current, | ||
| organizationName: `${formValues.firstName.trim()} ${formValues.lastName.trim()} Lab`, | ||
| organization: generateOrganizationId(trimmedFirstName, trimmedLastName), | ||
| organizationName: generateOrganizationName(trimmedFirstName, trimmedLastName), | ||
| acceptedTermsOfService: terms?.version, | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the findings, I can now generate the final response.
Add error handling for organization creation and display feedback to the user
The form submission should handle API errors and provide user feedback using the antd message component, which is the standard error display mechanism in the codebase.
- Add try-catch block around the API call
- Display error feedback using
message.error - Show success message using
message.success
async function onFinish(formValues: Record<string, any>) {
+ try {
const { firstName, lastName } = formValues;
const trimmedFirstName = firstName.trim();
const trimmedLastName = lastName.trim();
await Request.sendJSONReceiveJSON("/api/auth/createOrganizationWithAdmin", {
data: {
...formValues,
firstName: trimmedFirstName,
lastName: trimmedLastName,
password: {
password1: formValues.password.password1,
password2: formValues.password.password1,
},
organization: generateOrganizationId(trimmedFirstName, trimmedLastName),
organizationName: generateOrganizationName(trimmedFirstName, trimmedLastName),
acceptedTermsOfService: terms?.version,
},
});
+ message.success("Organization created successfully");
+ } catch (error) {
+ message.error("Failed to create organization. Please try again.");
+ console.error("Organization creation failed:", error);
+ return;
+ }🔗 Analysis chain
Add error handling for organization creation.
The form submission lacks proper error handling for the API call. Also, verify the impact of failed organization creation on the subsequent login attempt.
async function onFinish(formValues: Record<string, any>) {
+ try {
const { firstName, lastName } = formValues;
const trimmedFirstName = firstName.trim();
const trimmedLastName = lastName.trim();
await Request.sendJSONReceiveJSON("/api/auth/createOrganizationWithAdmin", {
data: {
...formValues,
firstName: trimmedFirstName,
lastName: trimmedLastName,
password: {
password1: formValues.password.password1,
password2: formValues.password.password1,
},
organization: generateOrganizationId(trimmedFirstName, trimmedLastName),
organizationName: generateOrganizationName(trimmedFirstName, trimmedLastName),
acceptedTermsOfService: terms?.version,
},
});
+ } catch (error) {
+ console.error('Failed to create organization:', error);
+ // Consider showing error message to user
+ return;
+ }Let me gather more context about the error handling patterns in the codebase.
Let me check how error handling is implemented in other form submissions and authentication flows.
Let me check for the Request utility implementation and any error handling patterns it might have.
Let me check how the Request utility handles errors and if there's any global error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for error handling patterns in similar form submission implementations
rg -A 5 "sendJSONReceiveJSON.*createOrganizationWithAdmin"
Length of output: 630
Script:
#!/bin/bash
# Check for error handling patterns in similar form submissions using Request.sendJSONReceiveJSON
rg -B 2 -A 5 "try.*Request\.sendJSONReceiveJSON"
# Look for any notification or error display components used in the codebase
ast-grep --pattern 'import { $_ } from "antd"'
Length of output: 3664
Script:
#!/bin/bash
# Look for similar form submissions in auth-related components
rg -B 3 -A 10 "async.*onFinish.*formValues" frontend/javascripts/admin/auth/
# Check for any existing error message handling in auth components
rg -B 2 -A 5 "message\.(error|warning)" frontend/javascripts/admin/auth/
Length of output: 1545
Script:
#!/bin/bash
# Look for Request utility implementation
rg -B 2 -A 10 "class.*Request"
# Check for any error handling in the Request class
rg -B 2 -A 10 "sendJSONReceiveJSON.*=.*async"
# Look for any error handling patterns in the auth flow
ast-grep --pattern 'catch (error) { $$$_ }'
Length of output: 36038
Script:
#!/bin/bash
# Look for error handling in the Request class implementation
rg -B 5 -A 10 "triggerRequest" frontend/javascripts/libs/request.ts
# Check for any global error handlers or message displays
rg -B 2 -A 5 "message\.(error|success)" frontend/javascripts/
Length of output: 10511
| <Checkbox disabled={terms == null}> | ||
| I agree to the{" "} | ||
| {terms == null ? ( | ||
| "terms of service" | ||
| ) : ( | ||
| <a target="_blank" href={terms.url} rel="noopener noreferrer"> | ||
| terms of service | ||
| </a> | ||
| )} | ||
| . | ||
| </Checkbox> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state for terms of service checkbox
The checkbox is disabled when terms are loading, but there's no visual indication to the user.
Consider adding a loading state:
-<Checkbox disabled={terms == null}>
+<Checkbox disabled={terms == null}>
+ {terms == null ? (
+ <span>
+ <LoadingOutlined spin /> Loading terms of service...
+ </span>
+ ) : (
I agree to the{" "}
<a target="_blank" href={terms.url} rel="noopener noreferrer">
terms of service
</a>
.
+ )}
</Checkbox>Committable suggestion skipped: line range outside the PR's diff.
@fm3 I need a little double-check here. I would have expected the frontend to not generate the orga id but letting the backend do this 🤔. function generateOrganizationId(firstName: string, lastName: string) {
return `${firstName.toLowerCase()}-${lastName.toLowerCase()}-lab`;
}can lead to naming collisions in case of using the same name when creating an orga. 🤔. |
conf/application.conf
Outdated
| enabled = false | ||
| enabled = true |
conf/application.conf
Outdated
| isWkorgInstance = false | ||
| isWkorgInstance = true |
| for { | ||
| _ <- organizationService.acceptTermsOfService(user._organization, termsOfServiceVersion)( | ||
| DBAccessContext(Some(user)), | ||
| m) | ||
| } yield () |
There was a problem hiding this comment.
I'd say this can be simplified as organizationService.acceptTermsOfService already returns a Fox[Unit]
| for { | |
| _ <- organizationService.acceptTermsOfService(user._organization, termsOfServiceVersion)( | |
| DBAccessContext(Some(user)), | |
| m) | |
| } yield () | |
| organizationService.acceptTermsOfService(user._organization, termsOfServiceVersion)(DBAccessContext(Some(user)), m) |
| function generateOrganizationId(firstName: string, lastName: string) { | ||
| return `${firstName.toLowerCase()}-${lastName.toLowerCase()}-lab`; | ||
| } |
There was a problem hiding this comment.
I think this might create naming collisions. I'd like to double check this with @fm3
There was a problem hiding this comment.
And this way of defining the orga id might lead to orga ids with whitespaces in it 🤔
There was a problem hiding this comment.
Why change this in this PR at all? As I understand, the random-character string is supposed to be the typical organization id. The organization name on the other hand is supposed to be created from the user name.
This was introduced in #4439 (though organizationId was still called organizationName then, and what is now the name was then the displayName).
I agree that changing it to firstname-lastname-lab may lead to unwanted collisions.
It’s true that some old organizations have ids like scalable_minds or sample_organization but it is indeed intended that new ones created by this form should have the random string. #7386 did not change that.
There was a problem hiding this comment.
To me it is also kinda strange, why the frontend generates this id string and not the backend 🤔. Maybe this could be changed in the future?
but it is indeed intended that new ones created by this form should have the random string.
@dieknolle3333 Could you please undo this change here? In case we agree to change the id generation, I'd do this in a separate PR. But I'd say that this is kinda unlikely because an id being a random hex string sounds fine to me. But feel free to open up a discussion on this :) (@frcroth)
There was a problem hiding this comment.
It is undone. I interpreted the PR and the issue so that the id should be human readable now.
There was a problem hiding this comment.
Oh that indeed looks like a misunderstanding 🙈
Thanks a lot for undoing the change so fast :) 🙏
There was a problem hiding this comment.
To me it is also kinda strange, why the frontend generates this id string and not the backend 🤔. Maybe this could be changed in the future?
Wrote #8216 to track that and future discussion of what the IDs should look like.
I interpreted the PR and the issue so that the id should be human readable now.
Fair, but it was more of the opposite, really. Before #7386 there was a mongoId and this random string as organizationName, and the actual name (then called displayName), with spaces and all. In the PR, we dropped the mongoId and declared this random string the id, because it already had to be unique and was used as a key in many places.
Thanks to you all for clearing this up here :)
There was a problem hiding this comment.
thanks for removing this, felix, and clearing this up everyone!
This reverts commit 77e2a59.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
CHANGELOG.unreleased.md (1)
19-19: Consider enhancing the changelog entry for better clarity.While the entry is accurate, it could be more descriptive about the user experience improvements.
Consider this enhanced version:
- Terms of Service for Webknossos are now accepted at registration, not afterward. [#8193](https://github.com/scalableminds/webknossos/pull/8193) + Streamlined user registration by integrating Terms of Service acceptance into the signup process, removing the need for post-registration acceptance. [#8193](https://github.com/scalableminds/webknossos/pull/8193)frontend/javascripts/admin/auth/registration_form_wkorg.tsx (5)
10-11: Add error handling for terms of service fetchingConsider handling potential fetch errors and loading states for terms of service to ensure a better user experience.
- const terms = useFetch(getTermsOfService, null, []); + const { data: terms, error: termsError } = useFetch(getTermsOfService, null, []); + + if (termsError) { + console.error('Failed to fetch terms of service:', termsError); + // Consider showing a user-friendly error message + }Also applies to: 35-35
Line range hint
37-49: Consider moving organization ID generation to the backendAs discussed in previous reviews, generating the organization ID in the frontend might not be the best approach. Consider:
- Moving the ID generation to the backend to ensure uniqueness
- Implementing proper collision detection
- Centralizing the ID generation logic
184-184: Simplify the conditional rendering logicThe current condition
terms != null && !terms.enabled ? null : (...)can be simplified.- {terms != null && !terms.enabled ? null : ( + {(!terms?.enabled) ? null : (
197-207: Improve terms checkbox disabled state handlingThe checkbox is disabled when terms are null, but there's no visual indication to the user about why it's disabled or when it will become enabled.
- <Checkbox disabled={terms == null}> + <Checkbox + disabled={terms == null} + title={terms == null ? "Loading terms of service..." : ""} + >
Line range hint
37-52: Add error handling for organization creationThe form submission lacks proper error handling for the API call.
async function onFinish(formValues: Record<string, any>) { + try { await Request.sendJSONReceiveJSON("/api/auth/createOrganizationWithAdmin", { data: { ...formValues, firstName: formValues.firstName.trim(), lastName: formValues.lastName.trim(), password: { password1: formValues.password.password1, password2: formValues.password.password1, }, organization: organizationId.current, organizationName: `${formValues.firstName.trim()} ${formValues.lastName.trim()} Lab`, acceptedTermsOfService: terms?.version, }, }); + } catch (error) { + console.error('Failed to create organization:', error); + message.error('Failed to create organization. Please try again.'); + return; + }app/controllers/AuthenticationController.scala (3)
642-644: Add documentation for the Terms of Service acceptance methodConsider adding ScalaDoc to document the purpose, parameters, and possible failure scenarios of this method.
+ /** + * Accepts the Terms of Service for a user with the specified version. + * + * @param user The user accepting the Terms of Service + * @param termsOfServiceVersion The version of Terms of Service being accepted + * @param m Implicit message provider for i18n + * @return Fox[Unit] Success or failure of the operation + */ private def acceptTermsOfServiceForUser(user: User, termsOfServiceVersion: Int)(implicit m: MessagesProvider) =
738-739: Add Terms of Service version validation in form mappingThe form accepts any integer as a ToS version without validation. Consider adding constraints to ensure the version is valid.
- "acceptedTermsOfService" -> optional(number) + "acceptedTermsOfService" -> optional(number.verifying( + "error.invalidTermsOfServiceVersion", + version => organizationService.isValidTermsOfServiceVersion(version) + ))Also applies to: 754-766
624-625: Consider centralizing Terms of Service version validationTo ensure consistent validation across the application, consider moving ToS version validation logic to
OrganizationServiceor creating a dedicatedTermsOfServiceService.This would help:
- Centralize version validation logic
- Make it easier to update validation rules
- Ensure consistent validation across different entry points
Also applies to: 642-644
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
CHANGELOG.unreleased.md(1 hunks)app/controllers/AuthenticationController.scala(6 hunks)conf/application.conf(2 hunks)frontend/javascripts/admin/auth/registration_form_wkorg.tsx(4 hunks)frontend/javascripts/router.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- conf/application.conf
- frontend/javascripts/router.tsx
| _ <- Fox.runOptional(signUpData.acceptedTermsOfService)(version => | ||
| acceptTermsOfServiceForUser(user, version)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for Terms of Service acceptance
The ToS acceptance is handled after user creation, but failures in acceptTermsOfServiceForUser might not be properly propagated. Consider:
- Validating the ToS version before acceptance
- Handling ToS acceptance failures explicitly
- _ <- Fox.runOptional(signUpData.acceptedTermsOfService)(version =>
- acceptTermsOfServiceForUser(user, version))
+ _ <- Fox.runOptional(signUpData.acceptedTermsOfService) { version =>
+ for {
+ _ <- organizationService.validateTermsOfServiceVersion(version) ?~> "Invalid Terms of Service version"
+ _ <- acceptTermsOfServiceForUser(user, version) ?~> "Failed to accept Terms of Service"
+ } yield ()
+ }Committable suggestion skipped: line range outside the PR's diff.
|
Oh and during testing I happend to notice, that the wkorg sign up form only has a single password input 🤔?! The second one to prevent accidental mistyping is missing. Was this removed intentionally? But IMO there should be a second pwd field to prevent mistyping the password. => I opened an issue for this, so we can tackle this in another PR :D |
| _ <- Fox.runOptional(signUpData.acceptedTermsOfService)(version => | ||
| acceptTermsOfServiceForUser(user, version)) |
There was a problem hiding this comment.
@frcroth I would prefer if the sign up request gets rejected in case acceptedTermsOfService is not set to true or empty but the application conf says that TOS is enabled.
What do you think?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/controllers/AuthenticationController.scala (2)
642-647: Consider enhancing error messages for better user feedback.The implementation is clean and follows functional programming principles. However, the error messages could be more specific.
Consider adding specific error messages for different failure cases:
private def acceptTermsOfServiceForUser(user: User, termsOfServiceVersion: Option[Int])( implicit m: MessagesProvider): Fox[Unit] = for { - acceptedVersion <- Fox.option2Fox(termsOfServiceVersion) + acceptedVersion <- Fox.option2Fox(termsOfServiceVersion) ?~> "Terms of Service version is required" _ <- organizationService.acceptTermsOfService(user._organization, acceptedVersion)(DBAccessContext(Some(user)), m) } yield ()
742-743: LGTM! Consider adding validation for Terms of Service version.The SignUpData case class has been properly updated to include the Terms of Service acceptance field.
Consider adding validation to ensure the Terms of Service version is valid when enabled:
"acceptedTermsOfService" -> optional(number).verifying( "Invalid Terms of Service version", version => !conf.WebKnossos.TermsOfService.enabled || version.isDefined )Also applies to: 758-759
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/controllers/AuthenticationController.scala(6 hunks)conf/application.conf(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- conf/application.conf
🔇 Additional comments (2)
app/controllers/AuthenticationController.scala (2)
624-625: LGTM! Verify Terms of Service configuration.
The Terms of Service acceptance is properly integrated into the organization creation flow and correctly guarded by the configuration flag.
Please ensure that WebKnossos.TermsOfService.enabled is properly set in your configuration files for all environments.
Line range hint 760-770: LGTM! Form mapping is correctly updated.
The form mapping has been properly updated to handle the new Terms of Service field while maintaining backward compatibility.
| private def acceptTermsOfServiceForUser(user: User, termsOfServiceVersion: Option[Int])( | ||
| implicit m: MessagesProvider): Fox[Unit] = | ||
| for { | ||
| acceptedVersion <- Fox.option2Fox(termsOfServiceVersion) |
There was a problem hiding this comment.
While being nitpicky 🙈
Could you please add an error message like
| acceptedVersion <- Fox.option2Fox(termsOfServiceVersion) | |
| acceptedVersion <- Fox.option2Fox(termsOfServiceVersion) ?~> "Terms of service must be accepted." |
? 🙏
There was a problem hiding this comment.
Else some kind of generic 400 error would be returned where an explanation would be useful IMO
There was a problem hiding this comment.
I can do this of course, no problem. Note that the user would not see 400 anyway because the frontend does not allow not accepting TOS.
There was a problem hiding this comment.
Sure, I just thought in case the logic changes in the frontend and a little bug or so is introduced, that the backend 1. ensures that tos is accepted and 2. in case it is not, the error returned is understandable.
I can do this of course, no problem.
Thanks a lot 🙏
|
from my perspective, this PR can be rereviewed :) I left the application.conf edits for testing purposes. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/controllers/AuthenticationController.scala (1)
642-647: Consider adding version validation.While the implementation is solid, consider validating that the provided version is current/valid before accepting it.
private def acceptTermsOfServiceForUser(user: User, termsOfServiceVersion: Option[Int])( implicit m: MessagesProvider): Fox[Unit] = for { acceptedVersion <- Fox.option2Fox(termsOfServiceVersion) ?~> "Terms of service must be accepted." + _ <- organizationService.validateTermsOfServiceVersion(acceptedVersion) ?~> "Invalid terms of service version" _ <- organizationService.acceptTermsOfService(user._organization, acceptedVersion)(DBAccessContext(Some(user)), m) } yield ()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/controllers/AuthenticationController.scala(6 hunks)
🔇 Additional comments (3)
app/controllers/AuthenticationController.scala (3)
742-743: LGTM! Form updates for ToS acceptance.
The SignUpData case class and form mapping are properly updated to handle Terms of Service acceptance.
Also applies to: 758-759, 769-770
624-625: LGTM! Verify configuration-based ToS enforcement.
The Terms of Service check is properly integrated into the organization creation flow and guarded by configuration.
✅ Verification successful
Configuration-based ToS enforcement is properly implemented
The verification shows that the Terms of Service feature is:
- Properly configured in
reference.confwith all necessary settings (enabled flag, version, URL, acceptance deadline) - Consistently checked across multiple components:
- AuthenticationController for signup
- OrganizationController for status checks
- OrganizationService for version validation
- Correctly implemented with proper error handling and version checking in
acceptTermsOfServiceForUser
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ToS configuration usage
# Test: Check if ToS configuration is properly referenced
rg -A 2 "WebKnossos.TermsOfService.enabled"
Length of output: 1455
Script:
#!/bin/bash
# Check the configuration structure and default values
rg -A 5 "TermsOfService" conf/
Length of output: 2514
Script:
#!/bin/bash
# Check if there are any tests verifying this functionality
rg -A 5 "TermsOfService" test/
Length of output: 30
Script:
#!/bin/bash
# Check the implementation of acceptTermsOfServiceForUser
ast-grep --pattern 'def acceptTermsOfServiceForUser($$$) { $$$ }'
Length of output: 67
Script:
#!/bin/bash
# Let's check the implementation with a different pattern since Scala syntax might be different
rg -A 10 "acceptTermsOfServiceForUser"
Length of output: 2285
Script:
#!/bin/bash
# Check the configuration reference file for ToS settings
fd "reference.conf|application.conf" conf/ --exec cat {}
Length of output: 9781
Line range hint 624-647: Verify removal of old ToS acceptance code.
The PR objectives mention removing the previous requirement for ToS acceptance after account creation. Let's verify this has been done.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
conf/application.conf(1 hunks)
🔇 Additional comments (1)
conf/application.conf (1)
120-123: Verify the Terms of Service URL accessibility
Please ensure that the Terms of Service URL is accessible and contains the correct content. This is crucial as it will be presented to users during signup.
✅ Verification successful
Terms of Service URL is accessible and properly referenced
The URL is accessible (HTTP 200) and is referenced consistently in the configuration and migration documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Terms of Service URL is accessible
# Note: Using curl with -L to follow redirects and -f to fail on HTTP errors
curl -Lf "https://webknossos.org/terms-of-service" -o /dev/null -w "%{http_code}\n"
# Check if the URL is referenced elsewhere in the codebase
rg -l "terms-of-service"
Length of output: 553
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
frontend/javascripts/admin/auth/tos_check_form_item.tsx (3)
6-11: Enhance type safety and accessibilityConsider improving the type definition:
type TOSProps = { terms: { enabled: boolean; - url: string; + url: `https://${string}` | `http://${string}`; // Ensure valid URL format + // Consider adding these fields for better accessibility + title?: string; // For custom TOS title + ariaLabel?: string; // For screen readers } | null; };
13-14: Add loading state handlingThe component should show a loading state when terms is null to improve user experience.
-export function TOSCheckFormItem({ terms }: TOSProps) { +export function TOSCheckFormItem({ terms, loading = false }: TOSProps & { loading?: boolean }) { return terms == null || terms.enabled ? (
19-24: Enhance form validation feedbackThe validation message could be more informative and support i18n properly.
{ validator: (_, value) => value ? Promise.resolve() - : Promise.reject(new Error(messages["auth.tos_check_required"])), + : Promise.reject( + new Error( + messages["auth.tos_check_required"] || + "Please accept the terms of service to continue" + ) + ), },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
frontend/javascripts/admin/auth/registration_form_generic.tsx(3 hunks)frontend/javascripts/admin/auth/registration_form_wkorg.tsx(4 hunks)frontend/javascripts/admin/auth/tos_check_form_item.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/admin/auth/registration_form_generic.tsx
- frontend/javascripts/admin/auth/registration_form_wkorg.tsx
🔇 Additional comments (1)
frontend/javascripts/admin/auth/tos_check_form_item.tsx (1)
1-40: Verify integration with registration forms
The component seems correctly implemented, but let's verify its integration with the registration forms mentioned in the PR objectives.
✅ Verification successful
Component is properly integrated with registration forms
The verification confirms that:
- TOSCheckFormItem is correctly imported and used in both registration forms (generic and wkorg)
- Terms data is properly fetched using
getTermsOfServiceAPI - The component receives the required
termsprop in both forms - Terms acceptance is properly handled in form submission (verified in wkorg form)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how this component is used in registration forms
rg -l "TOSCheckFormItem" frontend/javascripts/admin/auth/
Length of output: 235
Script:
#!/bin/bash
# Check the implementation of registration forms to verify integration
rg -A 10 "TOSCheckFormItem" frontend/javascripts/admin/auth/registration_form_wkorg.tsx frontend/javascripts/admin/auth/registration_form_generic.tsx
Length of output: 3845
Script:
#!/bin/bash
# Check if terms prop is properly passed from parent components
rg -A 5 "terms\s*=\s*{" frontend/javascripts/admin/auth/registration_form_wkorg.tsx frontend/javascripts/admin/auth/registration_form_generic.tsx
Length of output: 237
Script:
#!/bin/bash
# Check the terms variable usage in registration forms
rg -A 5 "terms" frontend/javascripts/admin/auth/registration_form_wkorg.tsx frontend/javascripts/admin/auth/registration_form_generic.tsx
Length of output: 3978
| <FormItem | ||
| name="tos_check" | ||
| valuePropName="checked" | ||
| rules={[ | ||
| { | ||
| validator: (_, value) => | ||
| value | ||
| ? Promise.resolve() | ||
| : Promise.reject(new Error(messages["auth.tos_check_required"])), | ||
| }, | ||
| ]} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test attributes and error boundary
The form item should have test attributes and error handling.
<FormItem
name="tos_check"
valuePropName="checked"
+ data-testid="tos-form-item"
+ validateTrigger={["onChange", "onBlur"]}
rules={[
{
validator: (_, value) =>
value
? Promise.resolve()
: Promise.reject(new Error(messages["auth.tos_check_required"])),
},
]}
+ validateFirst={true}
>📝 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.
| <FormItem | |
| name="tos_check" | |
| valuePropName="checked" | |
| rules={[ | |
| { | |
| validator: (_, value) => | |
| value | |
| ? Promise.resolve() | |
| : Promise.reject(new Error(messages["auth.tos_check_required"])), | |
| }, | |
| ]} | |
| > | |
| <FormItem | |
| name="tos_check" | |
| valuePropName="checked" | |
| data-testid="tos-form-item" | |
| validateTrigger={["onChange", "onBlur"]} | |
| rules={[ | |
| { | |
| validator: (_, value) => | |
| value | |
| ? Promise.resolve() | |
| : Promise.reject(new Error(messages["auth.tos_check_required"])), | |
| }, | |
| ]} | |
| validateFirst={true} | |
| > |
| <Checkbox disabled={terms == null}> | ||
| I agree to the{" "} | ||
| {terms == null ? ( | ||
| "terms of service" | ||
| ) : ( | ||
| <a target="_blank" href={terms.url} rel="noopener noreferrer"> | ||
| terms of service | ||
| </a> | ||
| )} | ||
| . | ||
| </Checkbox> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility and security
The checkbox and link need better accessibility support and security enhancements.
- <Checkbox disabled={terms == null}>
+ <Checkbox
+ disabled={terms == null || loading}
+ aria-label="Terms of Service Agreement"
+ role="checkbox"
+ >
I agree to the{" "}
{terms == null ? (
- "terms of service"
+ <span aria-label="Terms of service link loading">terms of service</span>
) : (
<a
target="_blank"
href={terms.url}
- rel="noopener noreferrer"
+ rel="noopener noreferrer nofollow"
+ aria-label="Open terms of service in new tab"
+ data-testid="tos-link"
>
terms of service
</a>
)}
.Committable suggestion skipped: line range outside the PR's diff.
URL of deployed dev instance (used for testing):
Accept terms of service already at sign up / organization creation.
Backend is done. Frontend needs to call /api/termsOfService route and add a checkbox / iframe to the account creation page. Then set "acceptedTermsOfService" in the submitted form to /api/auth/createOrganizationWithAdmin to the TOS VERSION. The Terms of Service acceptance afterward can be removed.
Steps to test:
isWkOrgInstancein application.conf, it should behave the same regarding the ToSTODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)