feat(tools/risk-form-filler): automate filling out carleton's risk assessment form #179
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a comprehensive update to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (16)
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 (
|
❌ Deploy Preview for docs-cuhacking failed. Why did it fail? →
|
✅ Deploy Preview for eslint-cuhacking ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for website-cuhacking ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
✅ Deploy Preview for design-cuhacking ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
JeremyFriesenGitHub
force-pushed
the
jeremy/feat/145-automate-filling-out-Carleton's-risk-assesment-form
branch
3 times, most recently
from
5de2265 to
4518102
Compare
There was a problem hiding this comment.
Actionable comments posted: 38
🧹 Outside diff range and nitpick comments (19)
tools/risk-form-filler/src/pom.ts (1)
96-187: Consolidate locator initializations to enhance readability.In the constructor, you're initializing a large number of locators individually. Consider organizing them into grouped objects or mappings based on their form sections. This will improve code organization and make it easier to manage locators related to specific forms.
Example:
export class FormsLayout { readonly page: Page; + // Group locators into sections + readonly buttons = { + online: Locator, + inPerson: Locator, + hybrid: Locator, + submit: Locator, + }; + readonly onlineForm = { + expectedAttendees: Locator, + registrationRadio: Locator, + specialConcerns: Locator, + }; constructor(page: Page) { this.page = page; // Initialize buttons - this.onlineButton = page.getByRole('link', { name: 'Online' }); - this.inPersonButton = page.getByRole('link', { name: 'In-Person' }); - this.hybridButton = page.getByRole('link', { name: 'Hybrid' }); - this.submitButton = page.getByRole('button', { name: 'Submit' }); + this.buttons = { + online: page.getByRole('link', { name: 'Online' }), + inPerson: page.getByRole('link', { name: 'In-Person' }), + hybrid: page.getByRole('link', { name: 'Hybrid' }), + submit: page.getByRole('button', { name: 'Submit' }), + }; // Initialize online form locators - this.expectedOnlineAttendeesTextBox = page.getByLabel('Number of expected online'); - this.registrationRadioButton = page.locator('input[name="event_tix"]').nth(3); - this.specialConcernsOnlineTextBox = page.getByLabel('Are there any other special'); + this.onlineForm = { + expectedAttendees: page.getByLabel('Number of expected online'), + registrationRadio: page.locator('input[name="event_tix"]').nth(3), + specialConcerns: page.getByLabel('Are there any other special'), + }; // Repeat for other form sections } }This restructuring makes the code cleaner and aligns related locators, improving maintainability.
tools/risk-form-filler/helpers/logistics.ts (2)
3-10: Consider a more flexible form configuration approachThe current implementation hardcodes specific radio button selections. Consider making the function more flexible by accepting a configuration object.
Example approach:
interface LogisticsConfig { hasVendorContracts?: boolean; hasLiabilityContracts?: boolean; hasInsuranceRental?: boolean; hasInsuranceCertificate?: boolean; hasRightsImplications?: boolean; hasRightsRisks?: boolean; } export async function handleLogistics( formLayout: FormsLayout, config: LogisticsConfig = {} ): Promise<void> { try { await formLayout.setContractsVendorRadioButton(!config.hasVendorContracts) await formLayout.setContractsLiabilityRadioButton(!config.hasLiabilityContracts) // ... and so on } catch (error) { throw new Error(`Failed to handle logistics form: ${error.message}`) } }This would:
- Make the function more reusable for different scenarios
- Make the form configuration more explicit and maintainable
- Allow for future extensions without changing the function signature
1-1: Consider adding JSDoc documentationSince this is a public API for the form filler tool, adding JSDoc documentation would help users understand its purpose and usage.
+/** + * Handles the logistics section of the risk assessment form + * @param formLayout - The form layout instance to interact with + * @throws {Error} If any form interaction fails + */ import type { FormsLayout } from '../src/pom'tools/risk-form-filler/helpers/online-information.ts (2)
7-7: Fix typo in method name.There's a typo in the method name: "Attendence" should be "Attendance"
- await formLayout.fillOnlineOrganizersAttendenceFromOriginTextBox( + await formLayout.fillOnlineOrganizersAttendanceFromOriginTextBox(
1-9: Add JSDoc documentation.Consider adding JSDoc documentation to improve code maintainability and IDE support.
+/** + * Fills out the online event information section of the form. + * @param formLayout - The form layout instance to interact with + * @param params - The online event information parameters + * @throws {Error} When required fields are missing or form filling fails + */ export async function fillOnlineInformation( formLayout: FormsLayout, params: OnlineInformationParams )tools/risk-form-filler/helpers/emergency-response.ts (1)
3-11: Consider adding input validation.The function accepts string parameters without validation.
Consider adding input validation:
export async function EmergencyResponse(formLayout: FormsLayout, EMERGENCY_SAFETY_CONCERNS: string, EMERGENCY_SAFETY_RISKS: string) { + if (!formLayout) { + throw new Error('FormLayout is required') + } + if (!EMERGENCY_SAFETY_CONCERNS?.trim()) { + throw new Error('Emergency safety concerns cannot be empty') + } + if (!EMERGENCY_SAFETY_RISKS?.trim()) { + throw new Error('Emergency safety risks cannot be empty') + } try { // ... rest of the function } }tools/risk-form-filler/eslint.config.js (1)
25-26: Document the purpose of the empty rules object.The empty rules object should be documented to explain its purpose and guide future rule additions.
Apply this diff to add documentation:
- // Override or add rules here - rules: {}, + // This section is reserved for project-specific rule overrides. + // Add custom rules or override existing ones from the base config here. + rules: { + // Example: + // 'rule-name': ['error', { options }], + },tools/risk-form-filler/helpers/event-details.ts (1)
3-3: Consider improving type safety and parameter organization.The function signature could be enhanced in several ways:
- Add an explicit
Promise<void>return type- Group event-related parameters into an interface
- Use camelCase for parameter names to follow TypeScript conventions
Consider refactoring to:
+ interface EventDetailsParams { + title: string; + date: string; + startTime: string; + endTime: string; + description: string; + expectedAttendees: string; + } + - export async function EventDetails(formLayout: FormsLayout, EVENT_TITLE: string, EVENT_DATE: string, EVENT_START_TIME: string, EVENT_END_TIME: string, EVENT_DESCRIPTION: string, EVENT_EXPECTED_ATTENDEES: string) { + export async function fillEventDetails( + formLayout: FormsLayout, + eventDetails: EventDetailsParams + ): Promise<void> {tools/risk-form-filler/helpers/contacts.ts (1)
4-11: Add error handling and make organization name configurableTwo suggestions for improvement:
- Add try-catch blocks for form operations to handle potential failures gracefully
- Make the organization name configurable instead of hardcoding 'cuHacking'
Here's a suggested implementation:
+interface OrganizationConfig { + primary: string; + secondary: string; +} + +export async function fillContactForms( + formLayout: FormsLayout, + primaryContact: ContactInfo, + secondaryContact: ContactInfo, + orgConfig: OrganizationConfig +) { + try { await formLayout.fillPrimaryFirstNameTextBox(PRIMARY_FIRST_NAME) // ... other fields - await formLayout.fillPrimaryOrganizationTextBox('cuHacking') + await formLayout.fillPrimaryOrganizationTextBox(orgConfig.primary) + } catch (error) { + throw new Error(`Failed to fill primary contact form: ${error.message}`); + }tools/risk-form-filler/playwright.config.ts (3)
1-9: TODO comment needs more contextThe TODO comment lacks specificity about what aspects of node global process usage need investigation. Consider adding more details about the investigation scope and potential concerns.
Consider updating the TODO to be more specific:
-// TODO: Investigate node global process usage +// TODO: Investigate node global process usage for environment variables and CI/CD compatibility
23-23: Consider separating tests from source codeThe test directory is set to './src', which typically contains source code. Consider moving tests to a dedicated directory (e.g., './tests' or './e2e') to maintain better separation of concerns.
- ...nxE2EPreset(__filename, { testDir: './src' }), + ...nxE2EPreset(__filename, { testDir: './e2e' }),
32-38: Review webServer configuration timeoutThe commented webServer configuration includes a 120-second timeout, which seems excessive for most test scenarios. Consider reducing this value unless there's a specific need for such a long timeout.
When uncommenting the webServer configuration, consider adjusting the timeout:
- timeout: 120 * 1000, + timeout: 60 * 1000,tools/risk-form-filler/src/online.ts (1)
1-32: Consider adding resilience patterns for robust automationAs this is an automation tool interacting with external systems, consider implementing:
- Retry mechanism for flaky operations
- Rate limiting to prevent overwhelming the target system
- Configurable timeouts and wait strategies
- Screenshots on failure for debugging
Example implementation of a retry mechanism:
import { retry } from '../utils/retry'; // Implement a retry utility export async function scheduleOnline(params: OnlineEventParams): Promise<void> { // ... existing setup ... const retryOptions = { attempts: 3, delay: 1000, backoff: 'exponential', }; try { await retry(async () => { await formLayout.goto(); await formLayout.clickOnlineButton(); }, retryOptions); // ... rest of implementation with retry where appropriate ... } catch (error) { await page.screenshot({ path: `error-${Date.now()}.png` }); throw error; } }tools/risk-form-filler/src/in-person.ts (1)
1-47: Consider adding rate limiting and security measuresAs this is an automation tool handling sensitive information, consider implementing:
- Rate limiting to prevent overwhelming the form server
- Retry mechanism with exponential backoff for failed operations
- Audit logging for security tracking
- Environment-based configuration for different form endpoints
Would you like assistance in implementing any of these architectural improvements?
tools/risk-form-filler/src/hybrid.ts (1)
49-49: Document the reason for commented-out submissionThe form submission is commented out without explanation. If this is intentional (e.g., for testing), please document the reason.
Add a comment explaining why submission is disabled:
- // await formLayout.clickSubmitButton() + // Form submission disabled for testing purposes + // TODO: Enable submission after validation is implemented + // await formLayout.clickSubmitButton()tools/risk-form-filler/src/input/in-person-input.ts (2)
1-5: Consider renaming the function to better reflect its purposeThe current name
getInputAndRunPlaywrightdoesn't fully capture the function's purpose of collecting and submitting in-person event details. Consider a more descriptive name likesubmitInPersonEventFormorprocessInPersonEventDetails.
38-71: Refactor output formatting for better maintainabilityThe current output formatting uses repetitive template literals. Consider:
- Creating a reusable formatting function
- Using a structured data object for field definitions
Example refactor:
const formatField = (label: string, value: string): string => ` ${label.padEnd(30)} ${value}`; const fields = [ { label: "Primary Organizer's First Name:", value: organizer1FirstName }, { label: "Primary Organizer's Last Name:", value: organizer1LastName }, // ... other fields ]; const output = [ ' ===================== Event Details =====================', ...fields.map(f => formatField(f.label, f.value)), ' =========================================================' ].join('\n');tools/risk-form-filler/src/input/online-input.ts (1)
41-75: Enhance output readability and add null checksThe current output format could be improved for better user experience and reliability.
Consider:
- Adding color coding using a library like
chalk- Adding null checks for all fields
- Implementing a dedicated formatting function
import chalk from 'chalk'; function formatOutput(config: OnlineEventConfig): string { return ` ${chalk.bold('===================== Event Details =====================')} ${chalk.blue('Primary Organizer\'s First Name:')} ${config.organizer1FirstName ?? 'Not provided'} // ... rest of the formatting `; }tools/risk-form-filler/src/input/hybrid-input.ts (1)
45-84: Consider using a logging frameworkReplace direct stdout writes with a proper logging framework for better control over output formatting and log levels.
+import winston from 'winston' + +const logger = winston.createLogger({ + format: winston.format.simple(), + transports: [ + new winston.transports.Console() + ] +}) + -stdout.write(output) +logger.info(output)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
tools/risk-form-filler/README.md(1 hunks)tools/risk-form-filler/eslint.config.js(1 hunks)tools/risk-form-filler/helpers/contacts.ts(1 hunks)tools/risk-form-filler/helpers/emergency-response.ts(1 hunks)tools/risk-form-filler/helpers/event-details.ts(1 hunks)tools/risk-form-filler/helpers/logistics.ts(1 hunks)tools/risk-form-filler/helpers/online-information.ts(1 hunks)tools/risk-form-filler/helpers/risk-management.ts(1 hunks)tools/risk-form-filler/package.json(1 hunks)tools/risk-form-filler/playwright.config.ts(1 hunks)tools/risk-form-filler/project.json(1 hunks)tools/risk-form-filler/src/hybrid.ts(1 hunks)tools/risk-form-filler/src/in-person.ts(1 hunks)tools/risk-form-filler/src/input/hybrid-input.ts(1 hunks)tools/risk-form-filler/src/input/in-person-input.ts(1 hunks)tools/risk-form-filler/src/input/online-input.ts(1 hunks)tools/risk-form-filler/src/online.ts(1 hunks)tools/risk-form-filler/src/pom.ts(1 hunks)tools/risk-form-filler/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- tools/risk-form-filler/package.json
- tools/risk-form-filler/project.json
- tools/risk-form-filler/tsconfig.json
🧰 Additional context used
🪛 LanguageTool
tools/risk-form-filler/README.md
[uncategorized] ~11-~11: Possible missing preposition found.
Context: ... to change the input for that according workflow (for instance, change your hybrid input...
(AI_EN_LECTOR_MISSING_PREPOSITION)
🔇 Additional comments (17)
tools/risk-form-filler/src/pom.ts (1)
1-479: 🛠️ Refactor suggestion
Ensure locators are resilient to UI changes by using data attributes.
Some locators rely on text labels and the position of elements (e.g., nth()), which can be fragile if the UI changes. To make your tests more robust, consider using stable attributes like data-testid or id provided in the HTML elements.
Run the following script to check for available data-testid attributes in the application's pages:
By using data-testid attributes, you can create locators that are less likely to break when the UI changes, enhancing the reliability of your automation scripts.
tools/risk-form-filler/helpers/risk-management.ts (1)
1-1: LGTM!
Clean and specific type import following TypeScript best practices.
tools/risk-form-filler/helpers/online-information.ts (2)
1-2: LGTM!
The import statement and type usage are correct.
1-9: Verify form layout integration.
Let's verify that all the form layout methods being called actually exist in the FormsLayout class.
✅ Verification successful
Let me generate additional verification scripts to check for the remaining methods:
I'm searching for the actual method implementations rather than just the class definition to verify that all the required methods exist and are properly implemented in the FormsLayout class.
Let me try one more verification with a different pattern:
This will search for any method starting with "fillOnline" and ending with "TextBox" and show a few lines of context to verify the implementations.
Form layout integration verified successfully
All the required methods are properly implemented in the FormsLayout class:
- fillOnlinePlatformTextBox
- fillOnlineTopicsTextBox
- fillOnlineLocationTextBox
- fillOnlineOrganizersTextBox
- fillOnlineOrganizersAttendenceFromOriginTextBox
Each method correctly uses its corresponding Locator field to fill the form inputs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the FormsLayout class contains all the required methods
# Search for the FormsLayout class definition and its methods
ast-grep --pattern 'class FormsLayout {
$$$
fillOnlinePlatformTextBox($_) {
$$$
}
$$$
}'
ast-grep --pattern 'class FormsLayout {
$$$
fillOnlineTopicsTextBox($_) {
$$$
}
$$$
}'
ast-grep --pattern 'class FormsLayout {
$$$
fillOnlineLocationTextBox($_) {
$$$
}
$$$
}'
Length of output: 100640
Script:
#!/bin/bash
# Search for the remaining online form methods in FormsLayout class
ast-grep --pattern 'async fillOnlinePlatformTextBox($_) {
$$$
}'
ast-grep --pattern 'async fillOnlineTopicsTextBox($_) {
$$$
}'
ast-grep --pattern 'async fillOnlineLocationTextBox($_) {
$$$
}'
ast-grep --pattern 'async fillOnlineOrganizersTextBox($_) {
$$$
}'
ast-grep --pattern 'async fillOnlineOrganizersAttendenceFromOriginTextBox($_) {
$$$
}'
Length of output: 366
Script:
#!/bin/bash
# Search for the online form methods in FormsLayout class with implementation
rg -A 3 "async fill(Online.*TextBox)" tools/risk-form-filler/src/pom.ts
Length of output: 675
tools/risk-form-filler/helpers/emergency-response.ts (2)
1-2: LGTM! Clean import using type-only import.
The code correctly uses TypeScript's type-only import for the FormsLayout interface, which is good for bundle size optimization.
3-11: Verify form layout interface implementation.
The function assumes the existence of specific methods in the FormsLayout interface.
Let's verify the interface implementation:
✅ Verification successful
Let me search for the actual interface definition to complete the verification.
Let me try a different approach to find the interface definition.
All required methods are properly implemented in the FormsLayout class.
The verification shows that all methods used in the EmergencyResponse function are correctly implemented in the FormsLayout class in tools/risk-form-filler/src/pom.ts. Specifically:
checkNoEmergencyFirstAidRadioButton()checkYesEmergencyHospitalRadioButton()checkNoEmergencyEvacRadioButton()checkNoEmergencyFlamesRadioButton()checkNoEmergencyCrowdControlRadioButton()fillEmergencySafetyConcernsTextBox()fillEmergencySafetyRisksTextBox()
Each method is properly implemented with the corresponding Playwright locator interactions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the FormsLayout interface includes all required methods
# Search for the FormsLayout interface definition
echo "Searching for FormsLayout interface definition..."
ast-grep --pattern 'interface FormsLayout {
$$$
}'
# Search for the implementation of these methods
echo "Searching for implementation of form layout methods..."
rg -A 2 "checkNoEmergencyFirstAidRadioButton|checkYesEmergencyHospitalRadioButton|checkNoEmergencyEvacRadioButton|checkNoEmergencyFlamesRadioButton|checkNoEmergencyCrowdControlRadioButton|fillEmergencySafetyConcernsTextBox|fillEmergencySafetyRisksTextBox"
Length of output: 2902
Script:
#!/bin/bash
# Search for FormsLayout interface or class definition
fd FormsLayout --exec ast-grep --pattern 'interface FormsLayout {
$$$
}'
# Also search for any class that might implement this interface
fd pom.ts --exec ast-grep --pattern 'class $_ implements FormsLayout {
$$$
}'
Length of output: 158
Script:
#!/bin/bash
# Search for FormsLayout in all TypeScript files
rg -t typescript "FormsLayout" -B 2 -A 10
# Also check the specific file where it might be imported from
fd pom.ts --exec cat {}
Length of output: 22154
tools/risk-form-filler/eslint.config.js (1)
14-29: LGTM! The ES modules migration looks good.
The transition to ES modules and the async configuration setup is well implemented. The code correctly:
- Uses proper ES module imports
- Handles the async base configuration loading
- Maintains the existing ESLint configuration structure
tools/risk-form-filler/helpers/event-details.ts (1)
1-2: LGTM! Clean type import.
The import statement follows TypeScript best practices with proper type imports.
tools/risk-form-filler/helpers/contacts.ts (1)
1-2: LGTM! Good use of Page Object Model pattern
The import statement correctly utilizes TypeScript types and follows the Page Object Model pattern, which is a best practice for test automation.
tools/risk-form-filler/playwright.config.ts (1)
10-18: Verify environment configuration requirements
The commented sections suggest incomplete environment variable handling. This could impact the tool's functionality in different environments (local, CI/CD).
Let's check if there are any environment variable dependencies in the codebase:
tools/risk-form-filler/src/online.ts (2)
1-9: Well-organized imports following good practices!
The imports are logically grouped and the use of the Page Object Model pattern demonstrates good automation practices.
18-32:
Add error handling and logging to form filling implementation
The implementation lacks error handling for form interactions and has no way to track progress or debug issues. Additionally, the form submission is commented out without explanation.
Consider these improvements:
import { Logger } from '../utils/logger'; // Add a logging utility
export async function scheduleOnline(params: OnlineEventParams): Promise<void> {
const logger = new Logger('OnlineEventScheduler');
// ... browser setup ...
try {
logger.info('Starting online event scheduling');
logger.debug('Filling contact information');
await Contacts(formLayout, params.primaryContact, params.secondaryContact)
.catch(error => throw new Error(`Failed to fill contacts: ${error.message}`));
logger.debug('Setting attendance details');
await formLayout.fillExpectedOnlineAttendeesTextBox(params.eventDetails.expectedAttendees)
.catch(error => throw new Error(`Failed to set attendees: ${error.message}`));
await formLayout.checkYesRegistrationRadioButton();
// ... similar error handling for other sections ...
// TODO: Remove comment and implement form submission once testing is complete
// await formLayout.clickSubmitButton();
logger.info('Successfully completed form filling');
} catch (error) {
logger.error('Form filling failed', error);
throw error;
}
}Let's verify if there are any existing test files for this functionality:
tools/risk-form-filler/src/in-person.ts (2)
1-10: Well-structured imports with good separation of concerns!
The code follows good practices by:
- Using the Page Object Model pattern
- Separating form-filling logic into helper modules
- Maintaining clear module boundaries
16-47:
Add validation, waiting mechanisms, and error handling
The form filling implementation needs several improvements:
- Input validation is missing
- No waiting between sections could cause race conditions
- Form submission is commented out without explanation
- No progress logging
Consider implementing these improvements:
export async function scheduleInPerson(...) {
+ // Validate input data
+ validateContactInfo(primary);
+ validateEventInfo(event);
+
try {
// ... browser setup ...
+ console.log('Starting form fill for:', event.title);
+
await formLayout.goto();
+ await page.waitForLoadState('networkidle');
await formLayout.clickInPersonButton();
+ await page.waitForTimeout(1000); // Add small delay between sections
await Contacts(formLayout, ...);
+ console.log('Contact information filled');
// ... rest of form filling ...
- // await formLayout.clickSubmitButton()
+ if (process.env.SUBMIT_FORM === 'true') {
+ await formLayout.clickSubmitButton();
+ console.log('Form submitted successfully');
+ } else {
+ console.log('Form fill completed. Submission skipped (SUBMIT_FORM != true)');
+ }
}
}tools/risk-form-filler/src/hybrid.ts (2)
1-11: LGTM! Well-organized imports following the Page Object Model pattern.
The code demonstrates good separation of concerns by organizing form sections into separate helper modules.
1-50: Verify integration with other form types and helper functions
Let's verify the existence and implementation of all required helper functions and integration with other form types.
✅ Verification successful
Let me gather more information about the implementation details of the helper functions and form types to ensure proper integration.
Based on the gathered information, I can now provide the final response:
All helper functions and form types are properly integrated
The verification confirms:
- All required helper functions (
Contacts,EventDetails,EmergencyResponse,Logistics,OnlineInformation,RiskManagement) are properly implemented - The hybrid form implementation correctly integrates with other form types (in-person and online)
- The
FormsLayoutclass provides all necessary methods used across different form types - The implementation follows consistent patterns across all three form types (hybrid, in-person, and online)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other form types and helper implementations
echo "Checking for other form types..."
fd -e ts . tools/risk-form-filler/src
echo "Checking for all required helpers..."
fd -e ts . tools/risk-form-filler/helpers
echo "Checking FormsLayout implementation..."
rg -A 5 "class FormsLayout" tools/risk-form-filler/src/pom.ts
Length of output: 1094
Script:
#!/bin/bash
# Check implementation of helper functions
echo "Checking Contacts helper implementation..."
cat tools/risk-form-filler/helpers/contacts.ts
echo "Checking EventDetails helper implementation..."
cat tools/risk-form-filler/helpers/event-details.ts
echo "Checking other form type implementations..."
echo "In-person form:"
cat tools/risk-form-filler/src/in-person.ts
echo "Online form:"
cat tools/risk-form-filler/src/online.ts
echo "Checking FormsLayout class methods..."
rg -A 10 "class FormsLayout" tools/risk-form-filler/src/pom.ts
Length of output: 8150
tools/risk-form-filler/src/input/in-person-input.ts (1)
1-123: Verify security measures for automated form submission
Please ensure:
- Rate limiting is implemented to prevent abuse
- Proper logging is in place for audit trails
- Access controls are implemented for the form submission functionality
| export async function RiskManagement(formLayout: FormsLayout, RISK_SPEAKER_TOPICS: string, RISK_SPEAKER_FULL_NAMES: string, RISK_SPEAKER_WEBSITE_URL: string) { | ||
| await formLayout.checkYesSpeakersRadioButton() | ||
| await formLayout.speakersTopicsTextBox.fill(RISK_SPEAKER_TOPICS) | ||
| await formLayout.speakersFullNamesTextBox.fill(RISK_SPEAKER_FULL_NAMES) | ||
| await formLayout.speakersWebsiteUrlTextBox.fill(RISK_SPEAKER_WEBSITE_URL) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Essential improvements needed for robustness and maintainability.
The function implementation needs several improvements:
- Error handling for form operations
- Input validation for string parameters
- More specific function naming
- Documentation
Consider applying these improvements:
+/**
+ * Fills in speaker-related risk management form fields.
+ * @param formLayout - Page Object Model for the form
+ * @param RISK_SPEAKER_TOPICS - Topics to be discussed by speakers
+ * @param RISK_SPEAKER_FULL_NAMES - Full names of all speakers
+ * @param RISK_SPEAKER_WEBSITE_URL - Website URLs for speakers
+ * @throws {Error} If any form operation fails
+ */
-export async function RiskManagement(
+export async function fillSpeakerRiskManagementForm(
formLayout: FormsLayout,
RISK_SPEAKER_TOPICS: string,
RISK_SPEAKER_FULL_NAMES: string,
RISK_SPEAKER_WEBSITE_URL: string
) {
+ // Validate inputs
+ if (!RISK_SPEAKER_TOPICS?.trim()) {
+ throw new Error('Speaker topics cannot be empty');
+ }
+ if (!RISK_SPEAKER_FULL_NAMES?.trim()) {
+ throw new Error('Speaker names cannot be empty');
+ }
+ if (!RISK_SPEAKER_WEBSITE_URL?.trim()) {
+ throw new Error('Speaker website URL cannot be empty');
+ }
+
+ try {
await formLayout.checkYesSpeakersRadioButton()
await formLayout.speakersTopicsTextBox.fill(RISK_SPEAKER_TOPICS)
await formLayout.speakersFullNamesTextBox.fill(RISK_SPEAKER_FULL_NAMES)
await formLayout.speakersWebsiteUrlTextBox.fill(RISK_SPEAKER_WEBSITE_URL)
+ } catch (error) {
+ throw new Error(`Failed to fill speaker risk management form: ${error.message}`);
+ }
}📝 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.
| export async function RiskManagement(formLayout: FormsLayout, RISK_SPEAKER_TOPICS: string, RISK_SPEAKER_FULL_NAMES: string, RISK_SPEAKER_WEBSITE_URL: string) { | |
| await formLayout.checkYesSpeakersRadioButton() | |
| await formLayout.speakersTopicsTextBox.fill(RISK_SPEAKER_TOPICS) | |
| await formLayout.speakersFullNamesTextBox.fill(RISK_SPEAKER_FULL_NAMES) | |
| await formLayout.speakersWebsiteUrlTextBox.fill(RISK_SPEAKER_WEBSITE_URL) | |
| } | |
| /** | |
| * Fills in speaker-related risk management form fields. | |
| * @param formLayout - Page Object Model for the form | |
| * @param RISK_SPEAKER_TOPICS - Topics to be discussed by speakers | |
| * @param RISK_SPEAKER_FULL_NAMES - Full names of all speakers | |
| * @param RISK_SPEAKER_WEBSITE_URL - Website URLs for speakers | |
| * @throws {Error} If any form operation fails | |
| */ | |
| export async function fillSpeakerRiskManagementForm( | |
| formLayout: FormsLayout, | |
| RISK_SPEAKER_TOPICS: string, | |
| RISK_SPEAKER_FULL_NAMES: string, | |
| RISK_SPEAKER_WEBSITE_URL: string | |
| ) { | |
| // Validate inputs | |
| if (!RISK_SPEAKER_TOPICS?.trim()) { | |
| throw new Error('Speaker topics cannot be empty'); | |
| } | |
| if (!RISK_SPEAKER_FULL_NAMES?.trim()) { | |
| throw new Error('Speaker names cannot be empty'); | |
| } | |
| if (!RISK_SPEAKER_WEBSITE_URL?.trim()) { | |
| throw new Error('Speaker website URL cannot be empty'); | |
| } | |
| try { | |
| await formLayout.checkYesSpeakersRadioButton() | |
| await formLayout.speakersTopicsTextBox.fill(RISK_SPEAKER_TOPICS) | |
| await formLayout.speakersFullNamesTextBox.fill(RISK_SPEAKER_FULL_NAMES) | |
| await formLayout.speakersWebsiteUrlTextBox.fill(RISK_SPEAKER_WEBSITE_URL) | |
| } catch (error) { | |
| throw new Error(`Failed to fill speaker risk management form: ${error.message}`); | |
| } | |
| } |
| @@ -0,0 +1,9 @@ | |||
| import type { FormsLayout } from '../src/pom' | |||
|
|
|||
| export async function OnlineInformation(formLayout: FormsLayout, ONLINE_PLATFORM: string, ONLINE_TOPICS: string, ONLINE_LOCATION: string, ONLINE_ORGANIZERS: string, ONLINE_ORGANIZERS_ATTENDENCE_FROM_ORIGIN: string) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor function name and parameters to follow TypeScript conventions.
- Function name should be camelCase:
OnlineInformation→fillOnlineInformation - Parameters should be camelCase, not UPPERCASE
- Consider using an interface for the parameters to improve maintainability
-export async function OnlineInformation(
- formLayout: FormsLayout,
- ONLINE_PLATFORM: string,
- ONLINE_TOPICS: string,
- ONLINE_LOCATION: string,
- ONLINE_ORGANIZERS: string,
- ONLINE_ORGANIZERS_ATTENDENCE_FROM_ORIGIN: string
-) {
+interface OnlineInformationParams {
+ platform: string;
+ topics: string;
+ location: string;
+ organizers: string;
+ organizersAttendanceFromOrigin: string;
+}
+
+export async function fillOnlineInformation(
+ formLayout: FormsLayout,
+ params: OnlineInformationParams
+) {Committable suggestion skipped: line range outside the PR's diff.
| if (confirmation === 'yes' || confirmation === 'y') { | ||
| try { | ||
| await scheduleHybrid( | ||
| organizer1FirstName, | ||
| organizer1LastName, | ||
| organizer1StudentID, | ||
| organizer1Email, | ||
| organizer1Phone, | ||
| organizer2FirstName, | ||
| organizer2LastName, | ||
| organizer2StudentID, | ||
| organizer2Email, | ||
| organizer2Phone, | ||
| eventTitle, | ||
| eventDate, | ||
| eventStartTime, | ||
| eventEndTime, | ||
| eventDescription, | ||
| eventLocation, | ||
| expectedAttendees, | ||
| eventPlatform, | ||
| onlineEventTopics, | ||
| onlineEventLocation, | ||
| numberOfOrganizers, | ||
| numberOfOrganizersAttendingOnline, | ||
| speakerTopics, | ||
| speakersList, | ||
| speakerSites, | ||
| specialCircumstances, | ||
| risks, | ||
| cleanUpCrew, | ||
| additionalRemarks, | ||
| ) | ||
| stdout.write('Form filled successfully!\n') | ||
| } | ||
| catch (error) { | ||
| stdout.write(`Error filling the form: ${error.message}\n`) | ||
| } | ||
| } | ||
| else { | ||
| stdout.write('Operation cancelled.\n') | ||
| } | ||
| })() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor form submission to use a structured object
The current implementation passes too many parameters to scheduleHybrid. Consider using a structured object instead.
+interface HybridEventData {
+ organizer1: {
+ firstName: string
+ lastName: string
+ studentID: string
+ email: string
+ phone: string
+ }
+ organizer2: {
+ firstName: string
+ lastName: string
+ studentID: string
+ email: string
+ phone: string
+ }
+ event: {
+ title: string
+ date: string
+ startTime: string
+ endTime: string
+ description: string
+ location: string
+ expectedAttendees: number
+ }
+ online: {
+ platform: string
+ topics: string
+ location: string
+ organizersCount: number
+ onlineOrganizersCount: number
+ }
+ risk: {
+ speakerTopics: string
+ speakersList: string
+ speakerSites: string
+ specialCircumstances: string
+ risks: string
+ cleanUpCrew: string
+ additionalRemarks: string
+ }
+}Also, consider adding retry logic for transient failures:
async function submitFormWithRetry(data: HybridEventData, maxRetries = 3): Promise<void> {
for (let attempt = 1; attempt <= maxRetries; attempt++) {
try {
await scheduleHybrid(data)
return
} catch (error) {
if (attempt === maxRetries) throw error
await new Promise(resolve => setTimeout(resolve, 1000 * attempt))
}
}
}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (6)
tools/risk-form-filler/src/pom.ts (2)
96-187: Refactor repetitive locator initializations for better maintainabilityThe constructor initializes a large number of locators with repetitive patterns. Consider refactoring this code by creating helper functions or utilizing a configuration object to reduce duplication and enhance readability.
191-191: Avoid hardcoding URLs for flexibilityThe
gotomethod uses a hardcoded URL. Consider using environment variables or a configuration file to store URLs. This approach enhances flexibility and makes it easier to manage changes across different environments.tools/risk-form-filler/src/input/schedule-online-params.ts (1)
36-39: Use arrays for multiple speakers and websitesThe properties
speakersandspeakerSitesare defined asstring. If they are intended to handle multiple entries, consider changing their types tostring[]to accurately represent lists of speakers and their websites.tools/risk-form-filler/src/input/hybrid-input.ts (1)
95-95: Consider settingheadless: truewhen launching the browser in productionCurrently, the browser is launched with
headless: false, which opens a visible browser window. For automated scripts running in production or CI environments, it's recommended to run in headless mode by settingheadless: true.tools/risk-form-filler/helpers/get-user-confirmation.ts (1)
1-1: Avoid unnecessary import ofprocessThe
processobject is globally available in Node.js, so importing it is unnecessary. You can accessprocess.stdinandprocess.stdoutdirectly without importing.Apply this diff to remove the unnecessary import:
- import process from 'node:process'tools/risk-form-filler/src/in-person.ts (1)
19-30: Group related radio button operationsMultiple radio button operations are scattered throughout the code. Consider grouping them by functionality.
+ // Safety checks await formLayout.alcoholRadioButton.check() await formLayout.healthInsuranceRadioButton.check() await formLayout.photoIdRadioButton.check() + // Transportation and logistics await formLayout.transportationRadioButton.check() await formLayout.outOfProvinceRadioButton.check() + // Cleanup related await formLayout.garbageRadioButton.check() await formLayout.cleanupRadioButton.check() await formLayout.overnightRadioButton.check()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
tools/risk-form-filler/eslint.config.js(1 hunks)tools/risk-form-filler/helpers/get-user-confirmation.ts(1 hunks)tools/risk-form-filler/src/hybrid.ts(1 hunks)tools/risk-form-filler/src/in-person.ts(1 hunks)tools/risk-form-filler/src/input/hybrid-input.ts(1 hunks)tools/risk-form-filler/src/input/in-person-input.ts(1 hunks)tools/risk-form-filler/src/input/online-input.ts(1 hunks)tools/risk-form-filler/src/input/schedule-hybrid-params.ts(1 hunks)tools/risk-form-filler/src/input/schedule-in-person-params.ts(1 hunks)tools/risk-form-filler/src/input/schedule-online-params.ts(1 hunks)tools/risk-form-filler/src/online.ts(1 hunks)tools/risk-form-filler/src/pom.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/risk-form-filler/eslint.config.js
- tools/risk-form-filler/src/input/online-input.ts
🔇 Additional comments (7)
tools/risk-form-filler/src/online.ts (2)
9-9: Simplify method calls by passing organizer objects
The fillContacts method is called with numerous individual parameters from params. Refactor fillContacts to accept primaryOrganizer and secondaryOrganizer objects directly. This change will enhance readability and reduce the risk of errors.
14-16: Pass structured objects to methods to reduce complexity
Methods like fillEventDetails, fillOnlineInformation, and fillRiskManagement are invoked with multiple parameters. Refactoring these methods to accept their respective objects from params will simplify the function signatures and improve maintainability.
tools/risk-form-filler/src/input/hybrid-input.ts (2)
13-15:
Remove hardcoded sensitive information
Sensitive information such as student IDs and email addresses is hardcoded in the source code. This could lead to unauthorized exposure of personal data. Consider using environment variables or a secure configuration system to handle personal information.
Also applies to: 20-22
26-28:
Use proper date and time formats and validate input
The date, startTime, and endTime fields are using placeholders ('Today', 'Right Now') which do not conform to the specified formats. This may lead to errors during form submission. Please use valid date and time values in the correct formats ('YYYY/MM/DD' for date and 'HH:MM AM/PM' for time).
tools/risk-form-filler/src/input/schedule-hybrid-params.ts (1)
1-45: LGTM!
The ScheduleHybridParams interface is well-defined and provides a clear structure for hybrid event scheduling.
tools/risk-form-filler/src/in-person.ts (1)
4-34:
Add error handling and browser cleanup
The function lacks proper error handling and browser cleanup. This is particularly important for browser automation to prevent resource leaks.
Additionally, consider adding retry logic for flaky UI interactions.
tools/risk-form-filler/src/input/in-person-input.ts (1)
11-23:
Remove hardcoded personal information (PII)
Storing personal information directly in the code poses security risks and makes updates difficult.
Move sensitive information to environment variables or a secure configuration file.
tools/risk-form-filler/src/pom.ts
Outdated
| async fillContacts(formLayout: FormsLayout, PRIMARY_FIRST_NAME: string, PRIMARY_LAST_NAME: string, PRIMARY_CARLETON_ID: string, PRIMARY_EMAIL: string, PRIMARY_PHONE: string, SECONDARY_FIRST_NAME: string, SECONDARY_LAST_NAME: string, SECONDARY_CARLETON_ID: string, SECONDARY_EMAIL: string, SECONDARY_PHONE: string) { | ||
| await formLayout.primaryFirstNameTextBox.fill(PRIMARY_FIRST_NAME) | ||
| await formLayout.primaryLastNameTextBox.fill(PRIMARY_LAST_NAME) | ||
| await formLayout.primaryCarletonIdTextBox.fill(PRIMARY_CARLETON_ID) | ||
| await formLayout.primaryOrganizationTextBox.fill('cuHacking') | ||
| await formLayout.primaryStatusRadioButton.check() | ||
| await formLayout.primaryEmailTextBox.fill(PRIMARY_EMAIL) | ||
| await formLayout.primaryConfirmEmailTextBox.fill(PRIMARY_EMAIL) | ||
| await formLayout.primaryPhoneTextBox.fill(PRIMARY_PHONE) | ||
|
|
||
| await formLayout.secondaryFirstNameTextBox.fill(SECONDARY_FIRST_NAME) | ||
| await formLayout.secondaryLastNameTextBox.fill(SECONDARY_LAST_NAME) | ||
| await formLayout.secondaryCarletonIdTextBox.fill(SECONDARY_CARLETON_ID) | ||
| await formLayout.secondaryOrganizationTextBox.fill('Community Engagement') | ||
| await formLayout.secondaryStatusRadioButton.check() | ||
| await formLayout.secondaryEmailTextBox.fill(SECONDARY_EMAIL) | ||
| await formLayout.secondaryPhoneTextBox.fill(SECONDARY_PHONE) | ||
| } | ||
|
|
||
| async fillEmergencyResponse(formLayout: FormsLayout, EMERGENCY_SAFETY_CONCERNS: string, EMERGENCY_SAFETY_RISKS: string) { | ||
| await formLayout.emergencyFirstAidRadioButton.check() | ||
| await formLayout.emergencyHospitalRadioButton.check() | ||
| await formLayout.emergencyEvacRadioButton.check() | ||
| await formLayout.emergencyFlamesRadioButton.check() | ||
| await formLayout.emergencyCrowdControlRadioButton.check() | ||
| await formLayout.emergencySafetyConcernsTextBox.fill(EMERGENCY_SAFETY_CONCERNS) | ||
| await formLayout.emergencySafetyRisksTextBox.fill(EMERGENCY_SAFETY_RISKS) | ||
| } | ||
|
|
||
| async fillEventDetails(formLayout: FormsLayout, EVENT_TITLE: string, EVENT_DATE: string, EVENT_START_TIME: string, EVENT_END_TIME: string, EVENT_DESCRIPTION: string, EVENT_EXPECTED_ATTENDEES: string) { | ||
| await formLayout.eventTitleTextBox.fill(EVENT_TITLE) | ||
| await formLayout.eventDateTextBox.fill(EVENT_DATE) // date in yyyy/mm/dd format | ||
| await formLayout.eventStartTimeTextBox.fill(EVENT_START_TIME) // time in HH:MM AM/PM format | ||
| await formLayout.eventEndTimeTextBox.fill(EVENT_END_TIME) // time in HH:MM AM/PM format | ||
| await formLayout.eventDescriptionTextBox.fill(EVENT_DESCRIPTION) | ||
| await formLayout.expectedAttendeesTextBox.fill(EVENT_EXPECTED_ATTENDEES) | ||
| await formLayout.carletonStudentsRadioButton.check() | ||
| await formLayout.staffFacultyRadioButton.check() | ||
| await formLayout.alumniRadioButton.check() | ||
| await formLayout.minorsRadioButton.check() | ||
| await formLayout.vipRadioButton.check() | ||
| } | ||
|
|
||
| async fillLogistics(formLayout: FormsLayout) { | ||
| await formLayout.logisticsContractsVendorRadioButton.check() | ||
| await formLayout.logisticsContractsLiabilityRadioButton.check() | ||
| await formLayout.logisticsContractsInsuranceRentalRadioButton.check() | ||
| await formLayout.logisticsContractsInsuranceCertificateRadioButton.check() | ||
| await formLayout.logisticsRightsImplicationsRadioButton.check() | ||
| await formLayout.logisticsRightsRisksRadioButton.check() | ||
| } | ||
|
|
||
| async fillOnlineInformation(formLayout: FormsLayout, ONLINE_PLATFORM: string, ONLINE_TOPICS: string, ONLINE_LOCATION: string, ONLINE_ORGANIZERS: string, ONLINE_ORGANIZERS_ATTENDENCE_FROM_ORIGIN: string) { | ||
| await formLayout.onlinePlatformTextBox.fill(ONLINE_PLATFORM) | ||
| await formLayout.onlineTopicsTextBox.fill(ONLINE_TOPICS) | ||
| await formLayout.onlineLocationTextBox.fill(ONLINE_LOCATION) | ||
| await formLayout.onlineOrganizersTextBox.fill(ONLINE_ORGANIZERS) | ||
| await formLayout.onlineOrganizersAttendenceFromOriginTextBox.fill(ONLINE_ORGANIZERS_ATTENDENCE_FROM_ORIGIN) | ||
| } | ||
|
|
||
| async fillRiskManagement(formLayout: FormsLayout, RISK_SPEAKER_TOPICS: string, RISK_SPEAKER_FULL_NAMES: string, RISK_SPEAKER_WEBSITE_URL: string) { | ||
| await formLayout.speakersRadioButton.check() | ||
| await formLayout.speakersTopicsTextBox.fill(RISK_SPEAKER_TOPICS) | ||
| await formLayout.speakersFullNamesTextBox.fill(RISK_SPEAKER_FULL_NAMES) | ||
| await formLayout.speakersWebsiteUrlTextBox.fill(RISK_SPEAKER_WEBSITE_URL) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify method parameters by passing structured objects
The methods fillContacts, fillEmergencyResponse, fillEventDetails, and others have lengthy parameter lists, which can be error-prone and difficult to manage. Consider passing well-defined objects or interfaces as parameters to these methods to improve type safety and maintainability.
tools/risk-form-filler/src/hybrid.ts
Outdated
| await formLayout.goto() | ||
| await formLayout.hybridButton.click() | ||
|
|
||
| await formLayout.fillContacts(formLayout, params.primaryOrganizer.firstName, params.primaryOrganizer.lastName, params.primaryOrganizer.studentID.toString(), params.primaryOrganizer.email, params.primaryOrganizer.phone, params.secondaryOrganizer.firstName, params.secondaryOrganizer.lastName, params.secondaryOrganizer.studentID.toString(), params.secondaryOrganizer.email, params.secondaryOrganizer.phone) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor argument passing in fillContacts method
Currently, the fillContacts method receives many individual parameters, which can be error-prone and less maintainable. Consider passing the organizer objects directly or encapsulating them in an interface to improve code readability and maintainability.
Remove redundant formLayout parameter in method calls
The methods fillContacts, fillEventDetails, fillOnlineInformation, fillRiskManagement, fillEmergencyResponse, and fillLogistics are called on formLayout and also receive formLayout as a parameter. Since formLayout is already accessible within these methods via this, passing it as a parameter is redundant.
Apply this diff to remove the redundant parameter:
- await formLayout.fillContacts(formLayout, params.primaryOrganizer.firstName, params.primaryOrganizer.lastName, ...)
+ await formLayout.fillContacts(params.primaryOrganizer.firstName, params.primaryOrganizer.lastName, ...)Apply similar changes to the other method calls.
Also applies to: 19-19, 20-20, 24-24, 25-25, 34-34
tools/risk-form-filler/src/hybrid.ts
Outdated
| await formLayout.fillEventDetails(formLayout, params.eventDetails.title, params.eventDetails.date, params.eventDetails.startTime, params.eventDetails.endTime, params.eventDetails.description, params.eventDetails.expectedAttendees.toString()) | ||
| await formLayout.fillOnlineInformation(formLayout, params.onlineInformation.platform, params.onlineInformation.topics, params.onlineInformation.location, params.onlineInformation.numberOfOrganizers.toString(), params.onlineInformation.organizersAttendingOnline.toString()) | ||
|
|
||
| await formLayout.alcoholRadioButton.check() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize the 'alcohol' option instead of hardcoding
The code currently always selects the 'alcohol' option. Consider parameterizing this choice to reflect whether the event includes alcohol, based on input data.
tools/risk-form-filler/src/hybrid.ts
Outdated
| await formLayout.transportationRadioButton.check() | ||
| await formLayout.outOfProvinceRadioButton.check() | ||
| await formLayout.garbageRadioButton.check() | ||
| await formLayout.cleanupRadioButton.check() | ||
| await formLayout.cleanupTextBox.fill(params.riskManagement.cleanUpCrew) | ||
| await formLayout.overnightRadioButton.check() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize various options instead of hardcoding
Options for transportation, out-of-province travel, garbage disposal, cleanup, and overnight stays are currently hardcoded to always be checked or filled. To improve flexibility and accuracy, consider adding parameters to control these settings based on event-specific information in params.
| const browser = await chromium.launch({ headless: false }) | ||
| const page = await browser.newPage() | ||
| const formLayout = new FormsLayout(page) | ||
| await scheduleHybrid(params, formLayout) | ||
| stdout.write('Form filled successfully!\n') | ||
| } | ||
| catch (error) { | ||
| stdout.write(`Error filling the form: ${error.message}\n`) | ||
| exit(1) | ||
| } |
There was a problem hiding this comment.
Ensure browser instance is properly closed
The browser instance is not being properly closed after use, which may lead to resource leaks. Wrap the browser operations in a try...finally block to ensure that browser.close() is called, even if an error occurs.
Apply this diff to fix the issue:
if (isConfirmed) {
let browser;
try {
browser = await chromium.launch({ headless: false })
const page = await browser.newPage()
const formLayout = new FormsLayout(page)
await scheduleHybrid(params, formLayout)
stdout.write('Form filled successfully!\n')
}
catch (error) {
stdout.write(`Error filling the form: ${error.message}\n`)
exit(1)
}
+ finally {
+ if (browser) {
+ await browser.close()
+ }
+ }
}📝 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.
| const browser = await chromium.launch({ headless: false }) | |
| const page = await browser.newPage() | |
| const formLayout = new FormsLayout(page) | |
| await scheduleHybrid(params, formLayout) | |
| stdout.write('Form filled successfully!\n') | |
| } | |
| catch (error) { | |
| stdout.write(`Error filling the form: ${error.message}\n`) | |
| exit(1) | |
| } | |
| let browser; | |
| try { | |
| browser = await chromium.launch({ headless: false }) | |
| const page = await browser.newPage() | |
| const formLayout = new FormsLayout(page) | |
| await scheduleHybrid(params, formLayout) | |
| stdout.write('Form filled successfully!\n') | |
| } | |
| catch (error) { | |
| stdout.write(`Error filling the form: ${error.message}\n`) | |
| exit(1) | |
| } | |
| finally { | |
| if (browser) { | |
| await browser.close() | |
| } | |
| } |
| await formLayout.healthInsuranceRadioButton.check() | ||
| await formLayout.photoIdRadioButton.check() | ||
|
|
||
| await formLayout.fillEventDetails(formLayout, params.eventDetails.title, params.eventDetails.date, params.eventDetails.startTime, params.eventDetails.endTime, params.eventDetails.description, params.eventDetails.expectedAttendees.toString()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant formLayout parameter
The fillEventDetails method is being called with formLayout as both the instance and first parameter.
- await formLayout.fillEventDetails(formLayout, params.eventDetails.title, params.eventDetails.date, params.eventDetails.startTime, params.eventDetails.endTime, params.eventDetails.description, params.eventDetails.expectedAttendees.toString())
+ await formLayout.fillEventDetails(params.eventDetails)Committable suggestion skipped: line range outside the PR's diff.
| await formLayout.goto() | ||
| await formLayout.inPersonButton.click() | ||
|
|
||
| await formLayout.fillContacts(formLayout, params.primaryOrganizer.firstName, params.primaryOrganizer.lastName, params.primaryOrganizer.studentID.toString(), params.primaryOrganizer.email, params.primaryOrganizer.phone, params.secondaryOrganizer.firstName, params.secondaryOrganizer.lastName, params.secondaryOrganizer.studentID.toString(), params.secondaryOrganizer.email, params.secondaryOrganizer.phone) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor long parameter list in fillContacts method
The fillContacts method call has 10 parameters which makes it hard to maintain and error-prone.
Consider refactoring to use an object parameter:
- await formLayout.fillContacts(formLayout, params.primaryOrganizer.firstName, params.primaryOrganizer.lastName, params.primaryOrganizer.studentID.toString(), params.primaryOrganizer.email, params.primaryOrganizer.phone, params.secondaryOrganizer.firstName, params.secondaryOrganizer.lastName, params.secondaryOrganizer.studentID.toString(), params.secondaryOrganizer.email, params.secondaryOrganizer.phone)
+ await formLayout.fillContacts(formLayout, {
+ primary: params.primaryOrganizer,
+ secondary: params.secondaryOrganizer
+ })Committable suggestion skipped: line range outside the PR's diff.
| import { scheduleInPerson } from '../in-person' | ||
| import { FormsLayout } from '../pom' | ||
|
|
||
| (async function getInputAndRunPlaywright() { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert IIFE to named exported function
Using an IIFE makes the code harder to test and reuse.
- (async function getInputAndRunPlaywright() {
+ export async function getInputAndRunPlaywright() {📝 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.
| (async function getInputAndRunPlaywright() { | |
| export async function getInputAndRunPlaywright() { |
| try { | ||
| const browser = await chromium.launch({ headless: false }) | ||
| const page = await browser.newPage() | ||
| const formLayout = new FormsLayout(page) | ||
| await scheduleInPerson(params, formLayout) | ||
| stdout.write('Form filled successfully!\n') | ||
| } | ||
| catch (error) { | ||
| stdout.write(`Error filling the form: ${error.message}\n`) | ||
| exit(1) | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling in browser automation
The current error handling is too generic and doesn't properly clean up resources.
+ let browser;
try {
- const browser = await chromium.launch({ headless: false })
+ browser = await chromium.launch({ headless: false })
const page = await browser.newPage()
const formLayout = new FormsLayout(page)
await scheduleInPerson(params, formLayout)
stdout.write('Form filled successfully!\n')
}
catch (error) {
stdout.write(`Error filling the form: ${error.message}\n`)
exit(1)
}
+ finally {
+ await browser?.close()
+ }📝 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.
| try { | |
| const browser = await chromium.launch({ headless: false }) | |
| const page = await browser.newPage() | |
| const formLayout = new FormsLayout(page) | |
| await scheduleInPerson(params, formLayout) | |
| stdout.write('Form filled successfully!\n') | |
| } | |
| catch (error) { | |
| stdout.write(`Error filling the form: ${error.message}\n`) | |
| exit(1) | |
| } | |
| } | |
| let browser; | |
| try { | |
| browser = await chromium.launch({ headless: false }) | |
| const page = await browser.newPage() | |
| const formLayout = new FormsLayout(page) | |
| await scheduleInPerson(params, formLayout) | |
| stdout.write('Form filled successfully!\n') | |
| } | |
| catch (error) { | |
| stdout.write(`Error filling the form: ${error.message}\n`) | |
| exit(1) | |
| } | |
| finally { | |
| await browser?.close() | |
| } | |
| } |
| date: 'Today', // Format: 'YYYY/MM/DD' | ||
| startTime: 'Right Now', // Format: 'HH:MM AM/PM' | ||
| endTime: 'Right Now', // Format: 'HH:MM AM/PM' | ||
| description: 'cuHacking Event Description', | ||
| location: 'cuHacking Event Location', | ||
| expectedAttendees: 'Number of Expected Attendees', | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Based on the complete analysis, I can now provide the final response:
Fix placeholder date/time values and ensure proper format compliance
The review comment is correct. The code uses placeholder values that don't match the required formats, and the form implementation in pom.ts explicitly expects specific formats:
- Date must be in 'YYYY/MM/DD' format
- Time must be in 'HH:MM AM/PM' format
Required changes in both in-person-input.ts and hybrid-input.ts:
- date: 'Today', // Format: 'YYYY/MM/DD'
- startTime: 'Right Now', // Format: 'HH:MM AM/PM'
- endTime: 'Right Now', // Format: 'HH:MM AM/PM'
+ date: new Date().toISOString().split('T')[0].replace(/-/g, '/'),
+ startTime: new Date().toLocaleTimeString('en-US', { hour: '2-digit', minute: '2-digit', hour12: true }),
+ endTime: new Date().toLocaleTimeString('en-US', { hour: '2-digit', minute: '2-digit', hour12: true }),🔗 Analysis chain
Fix placeholder date/time values and add validation
The current implementation uses placeholder values ('Today', 'Right Now') instead of proper date/time formats.
Add proper validation:
- date: 'Today', // Format: 'YYYY/MM/DD'
- startTime: 'Right Now', // Format: 'HH:MM AM/PM'
- endTime: 'Right Now', // Format: 'HH:MM AM/PM'
+ date: new Date().toISOString().split('T')[0],
+ startTime: validateTime(startTime),
+ endTime: validateTime(endTime),Let me check the content of these files to understand the full context of date/time handling.
Let me check the type definitions and any related validation logic that might exist.
Let me check the form layout implementation to see how the date/time values are handled when filling the form.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other instances of improper date/time handling
rg -l "Today|Right Now" --type ts
Length of output: 136
Script:
#!/bin/bash
# Check the content of both files
echo "=== in-person-input.ts ==="
cat tools/risk-form-filler/src/input/in-person-input.ts
echo -e "\n=== hybrid-input.ts ==="
cat tools/risk-form-filler/src/input/hybrid-input.ts
# Also search for any existing date/time validation
rg -i "validate.*time|date.*validate" --type ts
Length of output: 8805
Script:
#!/bin/bash
# Check the type definitions and interfaces
cat tools/risk-form-filler/src/input/schedule-in-person-params.ts
cat tools/risk-form-filler/src/input/schedule-hybrid-params.ts
# Look for any date/time related utilities or validation
rg -A 5 "time|date" --type ts --glob '!*input.ts'
Length of output: 12359
Script:
#!/bin/bash
# Check the FormsLayout class implementation
cat tools/risk-form-filler/src/pom.ts
Length of output: 14268
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
tools/risk-form-filler/src/hybrid.ts (1)
11-13: Add validation for event locationThe event location is filled without validation.
+ // Add to validation.ts + export function validateEventLocation(location: string): boolean { + return location.trim().length > 0 && location.length <= 255 + } + // In hybrid.ts + if (!validateEventLocation(params.eventDetails.location)) { + throw new Error('Invalid event location') + } await formLayout.eventLocationTextBox.fill(params.eventDetails.location)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
tools/risk-form-filler/src/hybrid.ts(1 hunks)tools/risk-form-filler/src/in-person.ts(1 hunks)tools/risk-form-filler/src/input/hybrid-input.ts(1 hunks)tools/risk-form-filler/src/input/in-person-input.ts(1 hunks)tools/risk-form-filler/src/input/online-input.ts(1 hunks)tools/risk-form-filler/src/input/validation.ts(1 hunks)tools/risk-form-filler/src/online.ts(1 hunks)tools/risk-form-filler/src/pom.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/risk-form-filler/src/online.ts
- tools/risk-form-filler/src/input/online-input.ts
👮 Files not reviewed due to content moderation or server errors (3)
- tools/risk-form-filler/src/input/in-person-input.ts
- tools/risk-form-filler/src/input/hybrid-input.ts
- tools/risk-form-filler/src/pom.ts
🔇 Additional comments (6)
tools/risk-form-filler/src/input/validation.ts (1)
29-31: Verify student ID range constraints
Please verify if the student ID range (100000000-111111111) aligns with Carleton University's actual student ID format and range.
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to find any references to student IDs in the codebase, including test files that might contain example values or validation logic.
Based on the search results, I can now provide a final response:
Student ID validation range appears to be correct
The current validation range (100000000-111111111) appears to be correctly implemented and actively used across the codebase. This is evidenced by:
- The validation is consistently used across all form types (online, hybrid, in-person)
- Example student IDs in the test data (111211112) are flagged as invalid when they exceed the upper bound
- The format matches the 9-digit pattern commonly used for Carleton student numbers
- The validation is enforced for both primary and secondary organizers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other student ID validations in the codebase
rg -g '!node_modules' "student.*id.*valid" -B 2 -A 2
Length of output: 54
Script:
#!/bin/bash
# Let's try a broader search for student ID patterns and validations
rg -g '!node_modules' -i "studentid|student.+id" -B 2 -A 2
# Also search for any test files that might contain student ID examples
fd -g "*test*.ts" -g "*test*.js" -x cat {}
Length of output: 17581
tools/risk-form-filler/src/in-person.ts (2)
4-34:
Add error handling and browser cleanup
The implementation needs proper error handling and cleanup.
export async function scheduleInPerson(params: ScheduleInPersonParams, formLayout: InPersonFormLayout) {
+ try {
await formLayout.goto()
await formLayout.inPersonButton.click()
// ... rest of the function
+ } catch (error) {
+ console.error('Error during form filling:', error)
+ throw error
+ }
}8-8: 🛠️ Refactor suggestion
Remove redundant formLayout parameter in method calls
The formLayout parameter is redundantly passed to methods that are already being called on the formLayout instance.
- await formLayout.fillContacts(formLayout, ...)
+ await formLayout.fillContacts(...)
- await formLayout.fillEventDetails(formLayout, ...)
+ await formLayout.fillEventDetails(...)
- await formLayout.fillRiskManagement(formLayout, ...)
+ await formLayout.fillRiskManagement(...)
- await formLayout.fillLogistics(formLayout)
+ await formLayout.fillLogistics()Also applies to: 17-17, 21-22, 31-31
tools/risk-form-filler/src/hybrid.ts (3)
5-37: Add error handling and browser cleanup
The same error handling concerns from in-person.ts apply here.
9-9: Remove redundant formLayout parameter in method calls
The same redundant parameter issue from in-person.ts applies here.
Also applies to: 19-20, 24-25, 34-34
14-17: Parameterize radio button selections
The same hardcoded radio button selections issue from in-person.ts applies here.
Also applies to: 22-22, 27-32
| export function validateCarletonEmail(email: string): boolean { | ||
| const emailParts = email.split('@') | ||
| return emailParts.length === 2 && emailParts[1] === 'cmail.carleton.ca' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen email validation
The current implementation lacks:
- Validation of the local part format
- Basic email format checking
export function validateCarletonEmail(email: string): boolean {
+ const emailRegex = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/
+ if (!emailRegex.test(email)) {
+ return false
+ }
const emailParts = email.split('@')
return emailParts.length === 2 && emailParts[1] === 'cmail.carleton.ca'
}📝 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.
| export function validateCarletonEmail(email: string): boolean { | |
| const emailParts = email.split('@') | |
| return emailParts.length === 2 && emailParts[1] === 'cmail.carleton.ca' | |
| } | |
| export function validateCarletonEmail(email: string): boolean { | |
| const emailRegex = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/ | |
| if (!emailRegex.test(email)) { | |
| return false | |
| } | |
| const emailParts = email.split('@') | |
| return emailParts.length === 2 && emailParts[1] === 'cmail.carleton.ca' | |
| } |
| export function validateTime(time: string): boolean { | ||
| const timeRegex = /^(?:[01]\d|2[0-3]):[0-5]\d (?:AM|PM)$/ | ||
| return timeRegex.test(time) | ||
| } |
There was a problem hiding this comment.
Fix time validation logic
The current regex allows invalid times:
- Hours > 12 with AM/PM
- No validation of AM/PM logic
export function validateTime(time: string): boolean {
- const timeRegex = /^(?:[01]\d|2[0-3]):[0-5]\d (?:AM|PM)$/
+ const timeRegex = /^(0?[1-9]|1[0-2]):[0-5]\d\s(?:AM|PM)$/i
return timeRegex.test(time)
}📝 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.
| export function validateTime(time: string): boolean { | |
| const timeRegex = /^(?:[01]\d|2[0-3]):[0-5]\d (?:AM|PM)$/ | |
| return timeRegex.test(time) | |
| } | |
| export function validateTime(time: string): boolean { | |
| const timeRegex = /^(0?[1-9]|1[0-2]):[0-5]\d\s(?:AM|PM)$/i | |
| return timeRegex.test(time) | |
| } |
| export function validateDate(date: string): boolean { | ||
| const dateRegex = /^\d{4}\/\d{2}\/\d{2}$/ | ||
| if (!dateRegex.test(date)) { | ||
| return false | ||
| } | ||
|
|
||
| const [year, month, day] = date.split('/').map(Number) | ||
| if (year < 1900 || year > 2100 || month < 1 || month > 12 || day < 1 || day > 31) { | ||
| return false | ||
| } | ||
| if (month === 2) { | ||
| const isLeapYear = (year % 4 === 0 && year % 100 !== 0) || year % 400 === 0 | ||
| if (day > 29 || (day === 29 && !isLeapYear)) { | ||
| return false | ||
| } | ||
| } | ||
| if ([4, 6, 9, 11].includes(month) && day > 30) { | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen date validation
The date validation can be improved in two areas:
- The regex should enforce 2-digit months and days
- Add input sanitization before splitting
export function validateDate(date: string): boolean {
- const dateRegex = /^\d{4}\/\d{2}\/\d{2}$/
+ const dateRegex = /^[12]\d{3}\/(0[1-9]|1[0-2])\/(0[1-9]|[12]\d|3[01])$/
if (!dateRegex.test(date)) {
return false
}
+ // Sanitize input before splitting
+ const sanitizedDate = date.trim()
- const [year, month, day] = date.split('/').map(Number)
+ const [year, month, day] = sanitizedDate.split('/').map(Number)📝 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.
| export function validateDate(date: string): boolean { | |
| const dateRegex = /^\d{4}\/\d{2}\/\d{2}$/ | |
| if (!dateRegex.test(date)) { | |
| return false | |
| } | |
| const [year, month, day] = date.split('/').map(Number) | |
| if (year < 1900 || year > 2100 || month < 1 || month > 12 || day < 1 || day > 31) { | |
| return false | |
| } | |
| if (month === 2) { | |
| const isLeapYear = (year % 4 === 0 && year % 100 !== 0) || year % 400 === 0 | |
| if (day > 29 || (day === 29 && !isLeapYear)) { | |
| return false | |
| } | |
| } | |
| if ([4, 6, 9, 11].includes(month) && day > 30) { | |
| return false | |
| } | |
| return true | |
| } | |
| export function validateDate(date: string): boolean { | |
| const dateRegex = /^[12]\d{3}\/(0[1-9]|1[0-2])\/(0[1-9]|[12]\d|3[01])$/ | |
| if (!dateRegex.test(date)) { | |
| return false | |
| } | |
| // Sanitize input before splitting | |
| const sanitizedDate = date.trim() | |
| const [year, month, day] = sanitizedDate.split('/').map(Number) | |
| if (year < 1900 || year > 2100 || month < 1 || month > 12 || day < 1 || day > 31) { | |
| return false | |
| } | |
| if (month === 2) { | |
| const isLeapYear = (year % 4 === 0 && year % 100 !== 0) || year % 400 === 0 | |
| if (day > 29 || (day === 29 && !isLeapYear)) { | |
| return false | |
| } | |
| } | |
| if ([4, 6, 9, 11].includes(month) && day > 30) { | |
| return false | |
| } | |
| return true | |
| } |
| await formLayout.foodRadioButton.check() | ||
| await formLayout.healthInsuranceRadioButton.check() | ||
| await formLayout.photoIdRadioButton.check() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize radio button selections
The code hardcodes several radio button selections that should be configurable based on event requirements.
- await formLayout.foodRadioButton.check()
- await formLayout.healthInsuranceRadioButton.check()
- await formLayout.photoIdRadioButton.check()
+ await formLayout.foodRadioButton.check(params.eventDetails.hasFood)
+ await formLayout.healthInsuranceRadioButton.check(params.eventDetails.requiresHealthInsurance)
+ await formLayout.photoIdRadioButton.check(params.eventDetails.requiresPhotoId)
- await formLayout.alcoholRadioButton.check()
+ await formLayout.alcoholRadioButton.check(params.eventDetails.hasAlcohol)
- await formLayout.transportationRadioButton.check()
- await formLayout.outOfProvinceRadioButton.check()
- await formLayout.overnightRadioButton.check()
+ await formLayout.transportationRadioButton.check(params.logistics.requiresTransportation)
+ await formLayout.outOfProvinceRadioButton.check(params.logistics.isOutOfProvince)
+ await formLayout.overnightRadioButton.check(params.logistics.requiresOvernight)Also applies to: 19-20, 24-30
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (8)
tools/risk-form-filler/helpers/validation.ts (2)
29-31: Improve student ID validation with constants and documentationThe validation needs input type checking and would benefit from documented constants.
+const MIN_STUDENT_ID = 100000000; // 9-digit IDs starting with 1 +const MAX_STUDENT_ID = 111111111; // Maximum allowed student ID + export function validateStudentID(studentId: number): boolean { - return studentId >= 100000000 && studentId <= 111111111 + if (typeof studentId !== 'number' || !Number.isInteger(studentId)) { + return false; + } + return studentId >= MIN_STUDENT_ID && studentId <= MAX_STUDENT_ID; }
1-36: Consider enhancing validation architectureThe validation module could benefit from these architectural improvements:
- Add custom error messages for each validation failure
- Consider using a validation library (e.g., Zod, Yup) for more robust type checking
- Implement a common validation result type that includes error details
Example enhancement:
interface ValidationResult { isValid: boolean; error?: string; } function validateDate(date: string): ValidationResult { if (typeof date !== 'string') { return { isValid: false, error: 'Invalid input type' }; } // ... existing validation logic with specific error messages }tools/risk-form-filler/src/defs/in-person-input-defs.ts (2)
18-26: Enhance type safety for event detailsThe current implementation relies on string types and comments for format specifications. Consider:
- Using proper date/time types or string literals for format validation
- Adding structure to the location field
- Converting expectedAttendees to a numeric type with range validation
Here's a suggested implementation:
+type TimeFormat = `${number}:${number} ${'AM' | 'PM'}` +type DateFormat = `${number}/${number}/${number}` + +interface LocationDetails { + building: string + room: string + address?: string +} eventDetails: { title: string - date: string // Format: 'YYYY/MM/DD' + date: DateFormat - startTime: string // Format: 'HH:MM AM/PM' - endTime: string // Format: 'HH:MM AM/PM' + startTime: TimeFormat + endTime: TimeFormat description: string - location: string + location: LocationDetails - expectedAttendees: string + expectedAttendees: number }
28-37: Structure risk management data more effectivelyThe current implementation uses plain strings for all fields. Consider:
- Adding structure to speaker-related information
- Making fields optional where appropriate
- Adding enums for common risk types
Here's a suggested implementation:
+interface Speaker { + name: string + topic: string + website?: string +} + +enum CommonRiskTypes { + PHYSICAL = 'physical', + TECHNICAL = 'technical', + HEALTH = 'health', + OTHER = 'other' +} riskManagement: { - speakerTopics: string - speakers: string - speakerSites: string + speakers: Speaker[] specialCircumstances?: string - risks: string + risks: { + type: CommonRiskTypes + description: string + mitigation: string + }[] cleanUpCrew: string - additionalRemarks: string + additionalRemarks?: string }tools/risk-form-filler/src/defs/online-input-defs.ts (3)
18-25: Strengthen type safety for event detailsConsider using more specific types to enforce format validation at compile time:
- Create custom types for date and time formats
- Add validation for the expected attendees count
+type DateString = string // Consider using a branded type for YYYY/MM/DD +type TimeString = string // Consider using a branded type for HH:MM AM/PM +type PositiveNumber = number // Consider adding runtime validation eventDetails: { title: string - date: string // Format: 'YYYY/MM/DD' + date: DateString - startTime: string // Format: 'HH:MM AM/PM' - endTime: string // Format: 'HH:MM AM/PM' + startTime: TimeString + endTime: TimeString description: string - expectedAttendees: number + expectedAttendees: PositiveNumber }
27-33: Add type constraints for online informationConsider these improvements:
- Add validation to ensure organizersAttendingOnline doesn't exceed numberOfOrganizers
- Use an enum for supported platforms
+enum Platform { + Zoom = 'zoom', + Teams = 'teams', + // Add other supported platforms +} onlineInformation: { - platform: string + platform: Platform topics: string location: string numberOfOrganizers: number organizersAttendingOnline: number }
35-40: Structure speaker and resource data more formallyConsider using more specific types to better represent the data:
- Make speakers an array of speaker objects
- Validate speakerSites as URLs
+interface Speaker { + name: string + bio?: string + website?: URL +} speakerAndResources: { topics: string - speakers: string + speakers: Speaker[] - speakerSites: string + speakerSites: URL[] additionalRemarks: string }tools/risk-form-filler/src/defs/hybrid-input-defs.ts (1)
36-44: Enhance risk management structure and documentation.The current risk management section uses generic string types, which could lead to inconsistent or incomplete risk documentation.
+type RiskLevel = 'Low' | 'Medium' | 'High' +interface Risk { + description: string + level: RiskLevel + mitigationStrategy: string +} riskManagement: { speakerTopics: string speakers: string speakerSites: string specialCircumstances: string - risks: string + risks: Risk[] cleanUpCrew: string additionalRemarks: string }Also consider adding JSDoc comments to document expected content for each field.
/** * @property {string} speakerTopics - List of topics to be covered by speakers * @property {string} speakers - Names and credentials of speakers * @property {string} speakerSites - Relevant websites or platforms of speakers * ... */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
tools/risk-form-filler/helpers/validation.ts(1 hunks)tools/risk-form-filler/src/defs/hybrid-input-defs.ts(1 hunks)tools/risk-form-filler/src/defs/in-person-input-defs.ts(1 hunks)tools/risk-form-filler/src/defs/online-input-defs.ts(1 hunks)
🔇 Additional comments (2)
tools/risk-form-filler/src/defs/in-person-input-defs.ts (1)
1-37: Verify interface usage across the codebase
Let's ensure that all components using this interface are properly implementing the required fields.
✅ Verification successful
Interface implementation is correctly used across the codebase
The verification shows that the ScheduleInPersonParams interface is properly implemented and used:
- The interface definition in
schedule-in-person-params.tsmatches exactly with the reviewed file - The
scheduleInPersonfunction inin-person.tscorrectly uses all fields from the interface:- All organizer fields (primary and secondary)
- All event details fields including title, date, times, location, and attendees
- All risk management fields including speakers, risks, cleanup crew, and remarks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check implementation of ScheduleInPersonParams interface
# Find all files that might use this interface
echo "Files potentially using ScheduleInPersonParams:"
rg -l "ScheduleInPersonParams" --type typescript
# Check specific implementations mentioned in the summary
echo -e "\nChecking specific implementations:"
rg -A 10 "scheduleInPerson|getInputAndRunPlaywright" --type typescript
Length of output: 381
Script:
#!/bin/bash
# Let's try without typescript file type specification
echo "Files potentially using ScheduleInPersonParams:"
rg -l "ScheduleInPersonParams"
echo -e "\nChecking implementations:"
rg -A 10 "scheduleInPerson|getInputAndRunPlaywright"
# Also check for any potential imports of this interface
echo -e "\nChecking interface imports:"
rg "import.*ScheduleInPersonParams"
Length of output: 6265
Script:
#!/bin/bash
# Let's check the actual implementation in schedule-in-person-params.ts
echo "Content of schedule-in-person-params.ts:"
cat tools/risk-form-filler/src/input/schedule-in-person-params.ts
# Let's also check the actual usage in in-person.ts to verify all fields
echo -e "\nChecking full implementation in in-person.ts:"
cat tools/risk-form-filler/src/in-person.ts
Length of output: 3166
tools/risk-form-filler/src/defs/hybrid-input-defs.ts (1)
1-45: Verify interface alignment with form structure.
Please ensure this interface structure matches the actual form fields in the risk assessment system.
✅ Verification successful
Interface structure matches form implementation
The interface structure aligns perfectly with the actual form implementation as evidenced by:
- The interface fields match the form fields used in the
HybridFormLayoutclass - All fields are being properly mapped in the
scheduleHybridfunction - The field types and formats match the validation requirements (e.g., date format 'YYYY/MM/DD', time format 'HH:MM AM/PM')
- The structure covers all required sections: organizer details, event details, online information, and risk management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for form field references in the codebase
rg -t ts "risk.*form.*field|form.*field.*risk" ./tools/risk-form-filler/src/
Length of output: 78
Script:
#!/bin/bash
# Let's search for files that might contain form field definitions or usage
fd -e ts -e tsx . ./tools/risk-form-filler/src/ --exec cat {}
# This will help us see all TypeScript files in the risk-form-filler
# to verify the interface alignment with actual form implementation
Length of output: 47888
| export function validateCarletonEmail(email: string): boolean { | ||
| const emailParts = email.split('@') | ||
| return emailParts.length === 2 && emailParts[1] === 'cmail.carleton.ca' | ||
| } |
There was a problem hiding this comment.
Strengthen email validation with proper format checking
The current implementation needs input validation and proper email format checking.
+const CARLETON_EMAIL_DOMAIN = 'cmail.carleton.ca';
+
export function validateCarletonEmail(email: string): boolean {
+ if (typeof email !== 'string') return false;
+
+ // Basic email format validation
+ const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
+ if (!emailRegex.test(email)) return false;
+
const emailParts = email.split('@')
- return emailParts.length === 2 && emailParts[1] === 'cmail.carleton.ca'
+ return emailParts.length === 2 &&
+ emailParts[1].toLowerCase() === CARLETON_EMAIL_DOMAIN.toLowerCase();
}📝 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.
| export function validateCarletonEmail(email: string): boolean { | |
| const emailParts = email.split('@') | |
| return emailParts.length === 2 && emailParts[1] === 'cmail.carleton.ca' | |
| } | |
| const CARLETON_EMAIL_DOMAIN = 'cmail.carleton.ca'; | |
| export function validateCarletonEmail(email: string): boolean { | |
| if (typeof email !== 'string') return false; | |
| // Basic email format validation | |
| const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | |
| if (!emailRegex.test(email)) return false; | |
| const emailParts = email.split('@') | |
| return emailParts.length === 2 && | |
| emailParts[1].toLowerCase() === CARLETON_EMAIL_DOMAIN.toLowerCase(); | |
| } |
| export function validateTime(time: string): boolean { | ||
| const timeRegex = /^(?:[01]\d|2[0-3]):[0-5]\d (?:AM|PM)$/ | ||
| return timeRegex.test(time) | ||
| } |
There was a problem hiding this comment.
Fix time validation to handle invalid 12-hour format cases
The current regex allows invalid 12-hour format times (e.g., "13:00 PM"). Also missing input validation and case handling.
export function validateTime(time: string): boolean {
+ if (typeof time !== 'string') return false;
+ time = time.toUpperCase();
- const timeRegex = /^(?:[01]\d|2[0-3]):[0-5]\d (?:AM|PM)$/
+ const timeRegex = /^(0?[1-9]|1[0-2]):[0-5]\d (AM|PM)$/
return timeRegex.test(time)
}📝 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.
| export function validateTime(time: string): boolean { | |
| const timeRegex = /^(?:[01]\d|2[0-3]):[0-5]\d (?:AM|PM)$/ | |
| return timeRegex.test(time) | |
| } | |
| export function validateTime(time: string): boolean { | |
| if (typeof time !== 'string') return false; | |
| time = time.toUpperCase(); | |
| const timeRegex = /^(0?[1-9]|1[0-2]):[0-5]\d (AM|PM)$/ | |
| return timeRegex.test(time) | |
| } |
| export function validateDate(date: string): boolean { | ||
| const dateRegex = /^\d{4}\/\d{2}\/\d{2}$/ | ||
| if (!dateRegex.test(date)) { | ||
| return false | ||
| } | ||
|
|
||
| const [year, month, day] = date.split('/').map(Number) | ||
| if (year < 1900 || year > 2100 || month < 1 || month > 12 || day < 1 || day > 31) { | ||
| return false | ||
| } | ||
| if (month === 2) { | ||
| const isLeapYear = (year % 4 === 0 && year % 100 !== 0) || year % 400 === 0 | ||
| if (day > 29 || (day === 29 && !isLeapYear)) { | ||
| return false | ||
| } | ||
| } | ||
| if ([4, 6, 9, 11].includes(month) && day > 30) { | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and handle edge cases in date validation
The date validation logic is mostly correct but needs some improvements for robustness:
export function validateDate(date: string): boolean {
+ if (typeof date !== 'string') return false;
const dateRegex = /^\d{4}\/\d{2}\/\d{2}$/
if (!dateRegex.test(date)) {
return false
}
const [year, month, day] = date.split('/').map(Number)
+ if ([year, month, day].some(n => Number.isNaN(n))) {
+ return false
+ }
if (year < 1900 || year > 2100 || month < 1 || month > 12 || day < 1 || day > 31) {
return false
}
+
+ const isLeapYear = (year: number): boolean =>
+ (year % 4 === 0 && year % 100 !== 0) || year % 400 === 0
+
if (month === 2) {
- const isLeapYear = (year % 4 === 0 && year % 100 !== 0) || year % 400 === 0
- if (day > 29 || (day === 29 && !isLeapYear)) {
+ if (day > 29 || (day === 29 && !isLeapYear(year))) {
return false
}
}Committable suggestion skipped: line range outside the PR's diff.
| export interface ScheduleInPersonParams { | ||
| primaryOrganizer: { | ||
| firstName: string | ||
| lastName: string | ||
| studentID: number | ||
| email: string | ||
| phone: string | ||
| } | ||
|
|
||
| secondaryOrganizer: { | ||
| firstName: string | ||
| lastName: string | ||
| studentID: number | ||
| email: string | ||
| phone: string | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety and reducing duplication
A few suggestions to enhance the organizer sections:
- Consider using a string type for
studentIDto prevent potential number overflow issues with large IDs - Add validation patterns for email and phone fields
- Extract the common organizer structure into a reusable type
Here's the suggested implementation:
+type OrganizerInfo = {
+ firstName: string
+ lastName: string
+ studentID: string
+ email: string // Consider adding @type {email}
+ phone: string // Consider adding pattern validation
+}
export interface ScheduleInPersonParams {
- primaryOrganizer: {
- firstName: string
- lastName: string
- studentID: number
- email: string
- phone: string
- }
+ primaryOrganizer: OrganizerInfo
- secondaryOrganizer: {
- firstName: string
- lastName: string
- studentID: number
- email: string
- phone: string
- }
+ secondaryOrganizer: OrganizerInfo📝 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.
| export interface ScheduleInPersonParams { | |
| primaryOrganizer: { | |
| firstName: string | |
| lastName: string | |
| studentID: number | |
| email: string | |
| phone: string | |
| } | |
| secondaryOrganizer: { | |
| firstName: string | |
| lastName: string | |
| studentID: number | |
| email: string | |
| phone: string | |
| } | |
| type OrganizerInfo = { | |
| firstName: string | |
| lastName: string | |
| studentID: string | |
| email: string // Consider adding @type {email} | |
| phone: string // Consider adding pattern validation | |
| } | |
| export interface ScheduleInPersonParams { | |
| primaryOrganizer: OrganizerInfo | |
| secondaryOrganizer: OrganizerInfo | |
| primaryOrganizer: { | ||
| firstName: string | ||
| lastName: string | ||
| studentID: number | ||
| email: string | ||
| phone: string | ||
| } | ||
|
|
||
| secondaryOrganizer: { | ||
| firstName: string | ||
| lastName: string | ||
| studentID: number | ||
| email: string | ||
| phone: string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider creating a shared organizer type and improve type safety
The primary and secondary organizer structures are identical. Consider these improvements:
- Extract the shared structure to reduce duplication
- Add format validation for phone and email
- Use string type for studentID to prevent potential numeric precision issues
+type OrganizerInfo = {
+ firstName: string
+ lastName: string
+ studentID: string // Changed from number
+ email: string
+ phone: string // Consider adding format validation
+}
export interface ScheduleOnlineParams {
- primaryOrganizer: {
- firstName: string
- lastName: string
- studentID: number
- email: string
- phone: string
- }
+ primaryOrganizer: OrganizerInfo
- secondaryOrganizer: {
- firstName: string
- lastName: string
- studentID: number
- email: string
- phone: string
- }
+ secondaryOrganizer: OrganizerInfo📝 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.
| primaryOrganizer: { | |
| firstName: string | |
| lastName: string | |
| studentID: number | |
| email: string | |
| phone: string | |
| } | |
| secondaryOrganizer: { | |
| firstName: string | |
| lastName: string | |
| studentID: number | |
| email: string | |
| phone: string | |
| } | |
| type OrganizerInfo = { | |
| firstName: string | |
| lastName: string | |
| studentID: string // Changed from number | |
| email: string | |
| phone: string // Consider adding format validation | |
| } | |
| primaryOrganizer: OrganizerInfo | |
| secondaryOrganizer: OrganizerInfo |
| export interface ScheduleOnlineParams { | ||
| primaryOrganizer: { | ||
| firstName: string | ||
| lastName: string | ||
| studentID: number | ||
| email: string | ||
| phone: string | ||
| } | ||
|
|
||
| secondaryOrganizer: { | ||
| firstName: string | ||
| lastName: string | ||
| studentID: number | ||
| email: string | ||
| phone: string | ||
| } | ||
|
|
||
| eventDetails: { | ||
| title: string | ||
| date: string // Format: 'YYYY/MM/DD' | ||
| startTime: string // Format: 'HH:MM AM/PM' | ||
| endTime: string // Format: 'HH:MM AM/PM' | ||
| description: string | ||
| expectedAttendees: number | ||
| } | ||
|
|
||
| onlineInformation: { | ||
| platform: string | ||
| topics: string | ||
| location: string | ||
| numberOfOrganizers: number | ||
| organizersAttendingOnline: number | ||
| } | ||
|
|
||
| speakerAndResources: { | ||
| topics: string | ||
| speakers: string | ||
| speakerSites: string | ||
| additionalRemarks: string | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the results, I can now generate the final response as we have confirmed:
- No tests exist in the defs directory
- The README exists but only contains execution instructions
- No JSDoc documentation exists in the interface file
Add documentation and tests for the online form interface
The ScheduleOnlineParams interface needs documentation for maintainability:
- Add JSDoc comments to document:
- The interface purpose and usage
- Format requirements for date/time fields
- Required vs optional fields
- Constraints for numeric fields
- Create test cases in
tools/risk-form-filler/src/defs/__tests__/online-input-defs.test.ts - Update README.md to include interface usage examples
🔗 Analysis chain
Add documentation and tests
As per the PR objectives, documentation and tests are required. Please consider:
- Adding JSDoc comments to document the interface and its usage
- Creating test cases to validate the type constraints
- Adding example usage in the README
Let me check the content of the README to see if it contains any documentation about this interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if tests exist for this module
fd -e test.ts -e spec.ts . tools/risk-form-filler/src/defs/
# Check if documentation exists
fd README.md tools/risk-form-filler/
Length of output: 133
Script:
#!/bin/bash
# Check README content
cat tools/risk-form-filler/README.md
# Check if there are any existing JSDoc comments in the file
rg "@interface|@param|@example" tools/risk-form-filler/src/defs/online-input-defs.ts
Length of output: 954
| primaryOrganizer: { | ||
| firstName: string | ||
| lastName: string | ||
| studentID: number | ||
| email: string | ||
| phone: string | ||
| } | ||
|
|
||
| secondaryOrganizer: { | ||
| firstName: string | ||
| lastName: string | ||
| studentID: number | ||
| email: string | ||
| phone: string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor organizer definitions to reduce duplication and improve type safety.
The primary and secondary organizer structures are identical. Consider these improvements:
+type OrganizerDetails = {
+ firstName: string
+ lastName: string
+ studentID: number
+ email: `${string}@${string}` // Email format validation
+ phone: string // Consider regex pattern type
+}
export interface ScheduleHybridParams {
- primaryOrganizer: {
- firstName: string
- lastName: string
- studentID: number
- email: string
- phone: string
- }
+ primaryOrganizer: OrganizerDetails
- secondaryOrganizer: {
- firstName: string
- lastName: string
- studentID: number
- email: string
- phone: string
- }
+ secondaryOrganizer: OrganizerDetails📝 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.
| primaryOrganizer: { | |
| firstName: string | |
| lastName: string | |
| studentID: number | |
| email: string | |
| phone: string | |
| } | |
| secondaryOrganizer: { | |
| firstName: string | |
| lastName: string | |
| studentID: number | |
| email: string | |
| phone: string | |
| } | |
| type OrganizerDetails = { | |
| firstName: string | |
| lastName: string | |
| studentID: number | |
| email: `${string}@${string}` // Email format validation | |
| phone: string // Consider regex pattern type | |
| } | |
| export interface ScheduleHybridParams { | |
| primaryOrganizer: OrganizerDetails | |
| secondaryOrganizer: OrganizerDetails | |
| } |
| onlineInformation: { | ||
| platform: string | ||
| topics: string | ||
| location: string | ||
| numberOfOrganizers: number | ||
| organizersAttendingOnline: number | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve validation and clarity in online information section.
The current structure needs better validation and clarity improvements:
organizersAttendingOnlineshould not exceednumberOfOrganizersplatformcould be restricted to supported platformslocationfield purpose is unclear in online context
+type SupportedPlatforms = 'Zoom' | 'Teams' | 'Google Meet' | 'Other'
onlineInformation: {
- platform: string
+ platform: SupportedPlatforms
topics: string
- location: string
+ virtualLocation: string // Renamed for clarity
numberOfOrganizers: number
organizersAttendingOnline: number
+ // Add runtime validation to ensure organizersAttendingOnline <= numberOfOrganizers
}Committable suggestion skipped: line range outside the PR's diff.
| eventDetails: { | ||
| title: string | ||
| date: string // Format: 'YYYY/MM/DD' | ||
| startTime: string // Format: 'HH:MM AM/PM' | ||
| endTime: string // Format: 'HH:MM AM/PM' | ||
| description: string | ||
| location: string | ||
| expectedAttendees: number | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen type safety for event details.
The current string types for date and time fields could lead to format errors. Consider using more specific types and adding reasonable constraints.
+type DateString = `${number}/${number}/${number}` // YYYY/MM/DD
+type TimeString = `${number}:${number} ${'AM' | 'PM'}` // HH:MM AM/PM
eventDetails: {
title: string
- date: string // Format: 'YYYY/MM/DD'
+ date: DateString
- startTime: string // Format: 'HH:MM AM/PM'
- endTime: string // Format: 'HH:MM AM/PM'
+ startTime: TimeString
+ endTime: TimeString
description: string
location: string
- expectedAttendees: number
+ expectedAttendees: number & { __brand: 'PositiveInteger' } // Ensure positive numbers only
}Committable suggestion skipped: line range outside the PR's diff.
JeremyFriesenGitHub
force-pushed
the
jeremy/feat/145-automate-filling-out-Carleton's-risk-assesment-form
branch
from
3d413b7 to
fc5ad21
Compare
JeremyFriesenGitHub
force-pushed
the
jeremy/feat/145-automate-filling-out-Carleton's-risk-assesment-form
branch
from
fc5ad21 to
fdf5b8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
commitlint.config.ts (1)
47-47: Consider enforcing mandatory commit scopes.The
scope-emptyrule is currently commented out. Consider enabling it to ensure all commits have appropriate scopes, which would improve commit message consistency and make the change history more organized.- // 'scope-empty': [2, 'never'], // Scope should always be provided + 'scope-empty': [2, 'never'], // Scope should always be provided
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
commitlint.config.ts(1 hunks)
🔇 Additional comments (1)
commitlint.config.ts (1)
45-45: LGTM! The addition of 'tools' scope aligns with the project objectives.
The new scope enables proper categorization of commits related to the internal tools project.
JeremyFriesenGitHub
force-pushed
the
jeremy/feat/145-automate-filling-out-Carleton's-risk-assesment-form
branch
from
d8c2e77 to
06e2389
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
tools/risk-form-filler/src/pom.ts (2)
100-102: Add error handling for navigationThe
gotomethod should handle navigation failures.async goto() { + try { await this.page.goto('https://stuapps.carleton.ca/sarms/event-risk') + await this.page.waitForLoadState('networkidle') + } catch (error) { + throw new Error(`Failed to navigate to event risk page: ${error.message}`) + } }
137-144: Add validation for radio button statesThe
fillLogisticsmethod assumes all radio buttons can be checked without verifying their current state or availability.async fillLogistics(formLayout: FormsLayout) { + const buttons = [ + formLayout.logisticsContractsVendorRadioButton, + formLayout.logisticsContractsLiabilityRadioButton, + // ... other buttons + ]; + + for (const button of buttons) { + await button.waitFor({ state: 'visible' }); + if (!(await button.isDisabled())) { + await button.check(); + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
commitlint.config.ts(1 hunks)tools/risk-form-filler/README.md(1 hunks)tools/risk-form-filler/eslint.config.js(1 hunks)tools/risk-form-filler/helpers/get-user-confirmation.ts(1 hunks)tools/risk-form-filler/helpers/validation.ts(1 hunks)tools/risk-form-filler/package.json(1 hunks)tools/risk-form-filler/playwright.config.ts(1 hunks)tools/risk-form-filler/project.json(1 hunks)tools/risk-form-filler/src/defs/hybrid-input-defs.ts(1 hunks)tools/risk-form-filler/src/defs/in-person-input-defs.ts(1 hunks)tools/risk-form-filler/src/defs/online-input-defs.ts(1 hunks)tools/risk-form-filler/src/hybrid.ts(1 hunks)tools/risk-form-filler/src/in-person.ts(1 hunks)tools/risk-form-filler/src/input/hybrid-input.ts(1 hunks)tools/risk-form-filler/src/input/in-person-input.ts(1 hunks)tools/risk-form-filler/src/input/online-input.ts(1 hunks)tools/risk-form-filler/src/online.ts(1 hunks)tools/risk-form-filler/src/pom.ts(1 hunks)tools/risk-form-filler/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- tools/risk-form-filler/package.json
- commitlint.config.ts
- tools/risk-form-filler/tsconfig.json
- tools/risk-form-filler/helpers/get-user-confirmation.ts
- tools/risk-form-filler/eslint.config.js
- tools/risk-form-filler/README.md
- tools/risk-form-filler/playwright.config.ts
- tools/risk-form-filler/src/defs/online-input-defs.ts
- tools/risk-form-filler/src/online.ts
- tools/risk-form-filler/src/defs/in-person-input-defs.ts
- tools/risk-form-filler/src/defs/hybrid-input-defs.ts
- tools/risk-form-filler/project.json
- tools/risk-form-filler/src/input/online-input.ts
- tools/risk-form-filler/src/input/in-person-input.ts
- tools/risk-form-filler/src/input/hybrid-input.ts
- tools/risk-form-filler/helpers/validation.ts
- tools/risk-form-filler/src/hybrid.ts
🔇 Additional comments (6)
tools/risk-form-filler/src/pom.ts (1)
104-121: 🛠️ Refactor suggestion
Refactor method to use structured parameters
The fillContacts method has too many parameters, making it error-prone and hard to maintain.
+interface ContactInfo {
+ firstName: string;
+ lastName: string;
+ carletonId: string;
+ email: string;
+ phone: string;
+}
+
-async fillContacts(formLayout: FormsLayout, PRIMARY_FIRST_NAME: string, PRIMARY_LAST_NAME: string, PRIMARY_CARLETON_ID: string, PRIMARY_EMAIL: string, PRIMARY_PHONE: string, SECONDARY_FIRST_NAME: string, SECONDARY_LAST_NAME: string, SECONDARY_CARLETON_ID: string, SECONDARY_EMAIL: string, SECONDARY_PHONE: string)
+async fillContacts(formLayout: FormsLayout, contacts: {
+ primary: ContactInfo;
+ secondary: ContactInfo;
+})Likely invalid or redundant comment.
tools/risk-form-filler/src/in-person.ts (5)
1-4: Add error handling and browser cleanup
The function needs proper error handling and browser cleanup as mentioned in previous reviews.
8-8: Refactor long parameter list in fillContacts method
The method call has too many parameters and should be refactored to use an object parameter.
13-16: Parameterize radio button selections
The radio button selections should be configurable based on event requirements.
17-17: Remove redundant formLayout parameter
The fillEventDetails method is being called with redundant parameters.
28-29:
Add validation for cleanup crew information
Validate cleanup crew details before form submission:
+ if (!params.riskManagement.cleanUpCrew?.trim()) {
+ throw new Error('Cleanup crew information is required')
+ }
await formLayout.cleanupTextBox.fill(params.riskManagement.cleanUpCrew)Likely invalid or redundant comment.
tools/risk-form-filler/src/pom.ts
Outdated
| export class InPersonFormLayout extends FormsLayout { | ||
| // Buttons | ||
| readonly inPersonButton: Locator | ||
|
|
||
| // IN-PERSON + HYBRID FORM | ||
| readonly eventLocationTextBox: Locator | ||
| readonly eventLocationRadioButton: Locator | ||
| readonly expectedAttendeesTextBox: Locator | ||
| readonly foodRadioButton: Locator | ||
| readonly healthInsuranceRadioButton: Locator | ||
| readonly photoIdRadioButton: Locator | ||
| readonly alcoholRadioButton: Locator | ||
| readonly transportationRadioButton: Locator | ||
| readonly outOfProvinceRadioButton: Locator | ||
| readonly garbageRadioButton: Locator | ||
| readonly cleanupRadioButton: Locator | ||
| readonly cleanupTextBox: Locator | ||
| readonly overnightRadioButton: Locator | ||
| readonly rightsTextBox: Locator | ||
|
|
||
| // HELPER FUNCTIONS - EMERGENCY RESPONSE | ||
| readonly emergencyFirstAidRadioButton: Locator | ||
| readonly emergencyHospitalRadioButton: Locator | ||
| readonly emergencyEvacRadioButton: Locator | ||
| readonly emergencyFlamesRadioButton: Locator | ||
| readonly emergencyCrowdControlRadioButton: Locator | ||
| readonly emergencySafetyConcernsTextBox: Locator | ||
| readonly emergencySafetyRisksTextBox: Locator | ||
|
|
||
| constructor(page: Page) { | ||
| super(page) | ||
|
|
||
| // Buttons | ||
| this.inPersonButton = page.getByRole('link', { name: 'In-Person' }) | ||
|
|
||
| // in-person + hybrid form | ||
| this.eventLocationTextBox = page.getByLabel('Event LocationIf on campus,') | ||
| this.eventLocationRadioButton = page.locator('input[name="event_location_res"]').nth(1) | ||
| this.expectedAttendeesTextBox = page.getByLabel('Number of expected') | ||
| this.foodRadioButton = page.locator('input[name="event_food"]').first() | ||
| this.healthInsuranceRadioButton = page.locator('input[name="event_health_insurance"]').first() | ||
| this.photoIdRadioButton = page.locator('input[name="event_photo_id"]').first() | ||
| this.alcoholRadioButton = page.locator('input[name="risk_alcohol"]').first() | ||
| this.transportationRadioButton = page.locator('input[name="travel_transportation"]').first() | ||
| this.outOfProvinceRadioButton = page.locator('input[name="out_of_province"]').first() | ||
| this.garbageRadioButton = page.locator('input[name="fmp_garbage"]').first() | ||
| this.cleanupRadioButton = page.locator('input[name="fmp_cleanup"]').nth(1) | ||
| this.cleanupTextBox = page.getByLabel('Please provide details as to') | ||
| this.overnightRadioButton = page.locator('input[name="fmp_overnight"]').first() | ||
| this.rightsTextBox = page.locator('#rights_other') | ||
|
|
||
| // helper functions - emergency response | ||
| this.emergencyFirstAidRadioButton = page.locator('input[name="emergency_firstaid"]').first() | ||
| this.emergencyHospitalRadioButton = page.locator('input[name="emergency_hospital"]').nth(1) | ||
| this.emergencyEvacRadioButton = page.locator('input[name="emergency_evac"]').first() | ||
| this.emergencyFlamesRadioButton = page.locator('input[name="emergency_flames"]').first() | ||
| this.emergencyCrowdControlRadioButton = page.locator('fieldset').filter({ hasText: 'Will crowd control measures' }).getByLabel('No') | ||
| this.emergencySafetyConcernsTextBox = page.getByLabel('Are there any other special') | ||
| this.emergencySafetyRisksTextBox = page.locator('#safety_risk') | ||
| } | ||
|
|
||
| async fillEmergencyResponse(formLayout: InPersonFormLayout, EMERGENCY_SAFETY_CONCERNS: string, EMERGENCY_SAFETY_RISKS: string) { | ||
| await formLayout.emergencyFirstAidRadioButton.check() | ||
| await formLayout.emergencyHospitalRadioButton.check() | ||
| await formLayout.emergencyEvacRadioButton.check() | ||
| await formLayout.emergencyFlamesRadioButton.check() | ||
| await formLayout.emergencyCrowdControlRadioButton.check() | ||
| await formLayout.emergencySafetyConcernsTextBox.fill(EMERGENCY_SAFETY_CONCERNS) | ||
| await formLayout.emergencySafetyRisksTextBox.fill(EMERGENCY_SAFETY_RISKS) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication between form layouts
The InPersonFormLayout and HybridFormLayout classes share significant code duplication in their properties and methods.
Consider extracting common functionality into a shared base class:
class BaseInPersonFormLayout extends FormsLayout {
// Common properties and methods for in-person events
readonly eventLocationTextBox: Locator;
// ... other common properties
protected constructor(page: Page) {
super(page);
// Initialize common properties
}
async fillEmergencyResponse(params: EmergencyResponseParams) {
// Common emergency response implementation
}
}
export class InPersonFormLayout extends BaseInPersonFormLayout {
// In-person specific properties and methods
}
export class HybridFormLayout extends BaseInPersonFormLayout {
// Hybrid specific properties and methods
}Also applies to: 270-367
| await formLayout.eventLocationTextBox.fill(params.eventDetails.location) | ||
| await formLayout.eventLocationRadioButton.check() | ||
| await formLayout.expectedAttendeesTextBox.fill(params.eventDetails.expectedAttendees.toString()) |
There was a problem hiding this comment.
Add input validation for event details
Add validation for location and attendee count before filling the form:
+ if (!params.eventDetails.location?.trim()) {
+ throw new Error('Event location cannot be empty')
+ }
+ if (params.eventDetails.expectedAttendees <= 0) {
+ throw new Error('Expected attendees must be greater than 0')
+ }
await formLayout.eventLocationTextBox.fill(params.eventDetails.location)
await formLayout.eventLocationRadioButton.check()
await formLayout.expectedAttendeesTextBox.fill(params.eventDetails.expectedAttendees.toString())📝 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.
| await formLayout.eventLocationTextBox.fill(params.eventDetails.location) | |
| await formLayout.eventLocationRadioButton.check() | |
| await formLayout.expectedAttendeesTextBox.fill(params.eventDetails.expectedAttendees.toString()) | |
| if (!params.eventDetails.location?.trim()) { | |
| throw new Error('Event location cannot be empty') | |
| } | |
| if (params.eventDetails.expectedAttendees <= 0) { | |
| throw new Error('Expected attendees must be greater than 0') | |
| } | |
| await formLayout.eventLocationTextBox.fill(params.eventDetails.location) | |
| await formLayout.eventLocationRadioButton.check() | |
| await formLayout.expectedAttendeesTextBox.fill(params.eventDetails.expectedAttendees.toString()) |
| await formLayout.fillLogistics(formLayout) | ||
|
|
||
| await formLayout.rightsTextBox.fill(params.riskManagement.additionalRemarks) | ||
| } |
There was a problem hiding this comment.
Add form submission handling and final validation
Add proper form submission handling and final validation:
await formLayout.fillLogistics(formLayout)
await formLayout.rightsTextBox.fill(params.riskManagement.additionalRemarks)
+
+ // Verify all required fields are filled
+ const requiredFields = await formLayout.validateRequiredFields()
+ if (requiredFields.length > 0) {
+ throw new Error(`Missing required fields: ${requiredFields.join(', ')}`)
+ }
+
+ // Handle form submission
+ try {
+ await formLayout.submitButton.click()
+ await formLayout.page.waitForNavigation({ waitUntil: 'networkidle' })
+ return { success: true }
+ } catch (error) {
+ throw new Error(`Form submission failed: ${error.message}`)
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| await formLayout.goto() | ||
| await formLayout.inPersonButton.click() | ||
|
|
There was a problem hiding this comment.
Add navigation verification
Add checks to verify successful navigation and button clicks. Consider adding timeouts and retries for reliability.
export async function scheduleInPerson(params: ScheduleInPersonParams, formLayout: InPersonFormLayout) {
await formLayout.goto()
+ await formLayout.page.waitForLoadState('networkidle')
await formLayout.inPersonButton.click()
+ await formLayout.page.waitForNavigation({ waitUntil: 'networkidle' })📝 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.
| await formLayout.goto() | |
| await formLayout.inPersonButton.click() | |
| await formLayout.goto() | |
| await formLayout.page.waitForLoadState('networkidle') | |
| await formLayout.inPersonButton.click() | |
| await formLayout.page.waitForNavigation({ waitUntil: 'networkidle' }) | |
| await formLayout.fillRiskManagement(formLayout, params.riskManagement.speakerTopics, params.riskManagement.speakers, params.riskManagement.speakerSites) | ||
| await formLayout.fillEmergencyResponse(formLayout, params.riskManagement.specialCircumstances, params.riskManagement.risks) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and refactor risk management methods
Add validation for required fields and refactor method signatures:
+interface RiskManagementInput {
+ speakerTopics: string;
+ speakers: string;
+ speakerSites: string;
+}
+
+interface EmergencyResponseInput {
+ specialCircumstances: string;
+ risks: string;
+}
+
- await formLayout.fillRiskManagement(formLayout, params.riskManagement.speakerTopics, params.riskManagement.speakers, params.riskManagement.speakerSites)
- await formLayout.fillEmergencyResponse(formLayout, params.riskManagement.specialCircumstances, params.riskManagement.risks)
+ if (!params.riskManagement.risks?.trim()) {
+ throw new Error('Risk assessment is required')
+ }
+ await formLayout.fillRiskManagement({
+ speakerTopics: params.riskManagement.speakerTopics,
+ speakers: params.riskManagement.speakers,
+ speakerSites: params.riskManagement.speakerSites
+ })
+ await formLayout.fillEmergencyResponse({
+ specialCircumstances: params.riskManagement.specialCircumstances,
+ risks: params.riskManagement.risks
+ })📝 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.
| await formLayout.fillRiskManagement(formLayout, params.riskManagement.speakerTopics, params.riskManagement.speakers, params.riskManagement.speakerSites) | |
| await formLayout.fillEmergencyResponse(formLayout, params.riskManagement.specialCircumstances, params.riskManagement.risks) | |
| interface RiskManagementInput { | |
| speakerTopics: string; | |
| speakers: string; | |
| speakerSites: string; | |
| } | |
| interface EmergencyResponseInput { | |
| specialCircumstances: string; | |
| risks: string; | |
| } | |
| if (!params.riskManagement.risks?.trim()) { | |
| throw new Error('Risk assessment is required') | |
| } | |
| await formLayout.fillRiskManagement({ | |
| speakerTopics: params.riskManagement.speakerTopics, | |
| speakers: params.riskManagement.speakers, | |
| speakerSites: params.riskManagement.speakerSites | |
| }) | |
| await formLayout.fillEmergencyResponse({ | |
| specialCircumstances: params.riskManagement.specialCircumstances, | |
| risks: params.riskManagement.risks | |
| }) |
JeremyFriesenGitHub
force-pushed
the
jeremy/feat/145-automate-filling-out-Carleton's-risk-assesment-form
branch
from
fb54372 to
0593f0d
Compare
…er from Event class Commenting this out because this won't be able to apply to every enum. Check Old POM for more details.
JeremyFriesenGitHub
force-pushed
the
jeremy/feat/145-automate-filling-out-Carleton's-risk-assesment-form
branch
from
1592fb8 to
155edd8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tools/risk-form-filler/src/pom.ts (1)
31-73: Consider improving type safety and reducing code duplication.
- Type Safety: Consider creating an interface for the sections record to avoid using
any:interface FormSection { [key: string]: Field<any>; } private createSections(): Record<string, FormSection> { // ... existing code } getSection(sectionName: string): FormSection { return this.sections[sectionName]; }
- Code Duplication: Extract shared sections between InPerson and Hybrid forms:
private getInPersonSections() { return { emergencyPreparedness: EmergencyPreparedness.create(), safetyRiskAssessment: SafetyRiskAssessment.create(), travel: Travel.create(), outOfProvinceEvents: OutOfProvinceEvents.create(), }; } case FormType.InPerson: return { ...organizerSections, ...sharedSections, ...this.getInPersonSections(), maintenanceServices: MaintenanceServices.create(), }; case FormType.Hybrid: return { ...organizerSections, ...sharedSections, ...this.getInPersonSections(), };tools/risk-form-filler/src/object-models/sections.ts (1)
562-563: Address the FIXME comment about online event attendance.The comment indicates that online event attendance fields are missing. Consider adding fields for:
- Expected online attendees count
- Attendance tracking method
- Maximum capacity limits
Would you like me to help implement these missing fields?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
tools/risk-form-filler/src/action.spec.ts(1 hunks)tools/risk-form-filler/src/data/config.data.ts(1 hunks)tools/risk-form-filler/src/object-models/sections.ts(1 hunks)tools/risk-form-filler/src/pom-old.ts(1 hunks)tools/risk-form-filler/src/pom.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/risk-form-filler/src/action.spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
tools/risk-form-filler/src/data/config.data.ts
[error] 1-119: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
tools/risk-form-filler/src/object-models/sections.ts
[error] 19-54: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 57-82: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 85-169: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 170-211: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 212-258: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 259-308: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 309-459: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 460-493: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 494-532: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 533-562: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 564-611: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (14)
tools/risk-form-filler/src/pom.ts (2)
15-19: LGTM!
The FormType enum is well-defined with clear and descriptive values.
21-78: LGTM! Well-structured form management class.
The class effectively manages different form types and their sections with proper initialization and error handling.
tools/risk-form-filler/src/object-models/sections.ts (3)
1-16: LGTM! Well-designed generic Field class.
The class effectively encapsulates field properties with proper TypeScript generics and clear parameter types.
611-638: LGTM! Consistent section instantiation and export.
The sections are instantiated and exported in a clear and organized manner.
564-609: 🛠️ Refactor suggestion
Refactor class to a function for consistency.
Similar to other sections, this class contains only static members.
Apply this diff to refactor the class into a function:
-export class OnlineInformation {
- static create(): Record<string, Field<any>> {
+export function createOnlineInformation(): Record<string, Field<any>> {
return {
// ... existing fields
}
- }
}🧰 Tools
🪛 Biome (1.9.4)
[error] 564-611: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
tools/risk-form-filler/src/data/config.data.ts (4)
1-1: 🛠️ Refactor suggestion
Convert class to a plain object or namespace.
The class only contains static members, which is an anti-pattern in TypeScript.
-export class Config {
+export const Config = {
// ... rest of the code
-}
+} as const;🧰 Tools
🪛 Biome (1.9.4)
[error] 1-119: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
11-17:
Remove or anonymize personal information in test data.
The test data contains what appears to be real personal information (names, student IDs, email addresses). This poses a privacy risk and could violate data protection regulations.
- firstName: 'Raef',
- lastName: 'Sarofiem',
- carletonId: '101152872',
- email: '[email protected]',
+ firstName: 'John',
+ lastName: 'Doe',
+ carletonId: '000000001',
+ email: '[email protected]',31-31: 🛠️ Refactor suggestion
Use dynamic dates for event testing.
The hardcoded future date could cause tests to fail once we pass that date.
- eventDate: '2024-12-15',
+ eventDate: getFutureDate(),Add this utility function at the top of the file:
const getFutureDate = (daysFromNow = 30) => {
const date = new Date();
date.setDate(date.getDate() + daysFromNow);
return date.toISOString().split('T')[0];
};46-58:
Remove or obfuscate sensitive operational details.
The configuration contains sensitive operational information about security measures, alcohol service, and emergency procedures.
Consider:
- Moving sensitive operational details to a separate, non-versioned configuration file
- Using environment variables for sensitive values
- Creating separate test and production configurations
tools/risk-form-filler/src/pom-old.ts (5)
114-114: Parameterize hard-coded organization name.
The organization name "cuHacking" is hard-coded in the fillContacts method.
- await formLayout.primaryOrganizationTextBox.fill('cuHacking')
+ await formLayout.primaryOrganizationTextBox.fill(PRIMARY_ORGANIZATION)106-108:
Add URL validation and configuration.
The hardcoded URL in the goto method could pose security risks and makes the code less maintainable.
+import { Config } from './data/config.data';
async goto() {
- await this.page.goto('https://stuapps.carleton.ca/sarms/event-risk')
+ const url = new URL(Config.LINK.BASE);
+ if (!url.hostname.endsWith('carleton.ca')) {
+ throw new Error('Invalid form URL');
+ }
+ await this.page.goto(url.toString());
}110-127: 🛠️ Refactor suggestion
Remove redundant formLayout parameter from instance methods.
The methods are already instance methods, so passing the formLayout parameter is redundant.
- async fillContacts(formLayout: FormsLayout, PRIMARY_FIRST_NAME: string, ...) {
+ async fillContacts(PRIMARY_FIRST_NAME: string, ...) {
- await formLayout.primaryFirstNameTextBox.fill(PRIMARY_FIRST_NAME)
+ await this.primaryFirstNameTextBox.fill(PRIMARY_FIRST_NAME)
// Apply similar changes to other lines
}204-274: 🛠️ Refactor suggestion
Reduce code duplication between InPersonFormLayout and HybridFormLayout.
There's significant duplication of properties and methods between these classes.
Create an intermediate base class:
abstract class InPersonBaseFormLayout extends FormsLayout {
// Shared properties and methods for in-person events
readonly eventLocationTextBox: Locator;
// ... other shared properties
constructor(page: Page) {
super(page);
// Initialize shared properties
}
async fillEmergencyResponse(EMERGENCY_SAFETY_CONCERNS: string, EMERGENCY_SAFETY_RISKS: string): Promise<void> {
// Shared implementation
}
}
// Update existing classes
export class InPersonFormLayout extends InPersonBaseFormLayout {
// In-person specific properties and methods
}
export class HybridFormLayout extends InPersonBaseFormLayout {
// Hybrid-specific properties and methods
// Include online-related properties and methods
}Also applies to: 277-374
129-157: 🛠️ Refactor suggestion
Add input sanitization for form data.
The code should sanitize input data before filling forms to prevent XSS attacks.
Add a utility class for input sanitization:
class InputSanitizer {
static sanitize(input: string): string {
// Remove potentially dangerous characters
return input.replace(/[<>'"]/g, '');
}
}
// Use in form filling methods
async fillEventDetails(details: EventDetails): Promise<void> {
await this.eventTitleTextBox.fill(InputSanitizer.sanitize(details.title));
// ... sanitize other inputs
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tools/risk-form-filler/src/data/config.data.ts (1)
99-122: Restructure configuration for better maintainabilityConsider splitting this large configuration file into smaller, focused modules:
- Base configuration (URLs, environment-specific settings)
- Test data module (anonymized data for testing)
- Form templates (reusable form structures)
Example structure:
// config/ // ├── base.config.ts // ├── test-data/ // │ ├── organizers.ts // │ ├── events.ts // │ └── forms.ts // └── templates/ // ├── risk-assessment.ts // └── event-registration.ts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tools/risk-form-filler/src/action.spec.ts(1 hunks)tools/risk-form-filler/src/data/config.data.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/risk-form-filler/src/action.spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
tools/risk-form-filler/src/data/config.data.ts
[error] 1-123: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (5)
tools/risk-form-filler/src/data/config.data.ts (5)
29-45: Use dynamic dates and verify location validity
- The hardcoded future date could cause tests to fail
- The location "NN 374" should be verified as a valid room number
#!/bin/bash
# Description: Verify if the room number exists in any documentation
# Search for room number references
rg -i "NN\s*374|NN\s+room\s+374" --type=md --type=txt1-8: 🛠️ Refactor suggestion
Convert class to a plain object and use environment variables for URLs
The current implementation has two issues:
- Using a class with only static members is an anti-pattern in TypeScript
- Hardcoded URLs should be configurable for different environments
-export class Config {
- static LINK = {
- BASE: 'https://stuapps.carleton.ca/sarms/event-risk',
- IN_PERSON: 'in-person',
- HYBRID: 'hybrid',
- ONLINE: 'online',
- }
+export const LINK = {
+ BASE: process.env.RISK_FORM_BASE_URL ?? 'https://stuapps.carleton.ca/sarms/event-risk',
+ IN_PERSON: 'in-person',
+ HYBRID: 'hybrid',
+ ONLINE: 'online',
+} as const;🧰 Tools
🪛 Biome (1.9.4)
[error] 1-123: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
10-28:
Remove or anonymize personal information in test data
The test data contains what appears to be real personal information (names, student IDs, email addresses) which poses privacy risks.
46-62:
Secure sensitive operational details and anonymize volunteer information
This section contains:
- Security-related configurations that should not be in version control
- Real names of volunteers that should be anonymized
Consider moving sensitive data to environment variables or a secure configuration management system:
riskManagement: {
- alcohol: 'Yes',
- smartServe: 'Yes',
+ alcohol: process.env.TEST_ALCOHOL ?? 'No',
+ smartServe: process.env.TEST_SMART_SERVE ?? 'No',
volunteers: {
- volunteer1: 'Alice',
- volunteer2: 'Bob',
- volunteer3: 'Charlie',
+ volunteer1: 'Volunteer A',
+ volunteer2: 'Volunteer B',
+ volunteer3: 'Volunteer C',
},Likely invalid or redundant comment.
63-74: Verify compliance with official safety guidelines
The emergency preparedness and safety risk assessment configurations should be validated against the university's official safety guidelines.
| travel: { | ||
| transportationRequired: 'Yes', | ||
| groupTransportation: 'No', | ||
| rentingVehicles: 'Yes', | ||
| insurance: 'Yes', | ||
| rentalDetails: { | ||
| companyName: 'Rent-A-Van Inc.', | ||
| companyAddress: '123 Main St, Ottawa, ON', | ||
| companyPhone: '123-456-7892', | ||
| additionalInfo: 'Insurance is fully covered in the rental fee.', | ||
| }, | ||
| personalVehicles: 'No', | ||
| timesCommunicated: 'Yes', | ||
| contingencyPlan: 'Yes', | ||
| contingencyDetails: 'Plan includes backup transportation via taxis.', | ||
| alcoholOnboard: 'No', | ||
| intoxicatedIndividuals: 'No', | ||
| }, |
There was a problem hiding this comment.
Secure vendor information and transportation details
The travel section contains sensitive vendor information and detailed transportation plans that should not be in version control.
Consider using a secure configuration pattern:
interface VendorConfig {
companyName: string;
companyAddress: string;
companyPhone: string;
additionalInfo: string;
}
// Load from environment or secure config service
const getVendorConfig = (): VendorConfig => ({
companyName: process.env.VENDOR_NAME ?? 'Test Vendor',
companyAddress: process.env.VENDOR_ADDRESS ?? 'Test Address',
companyPhone: process.env.VENDOR_PHONE ?? '000-000-0000',
additionalInfo: process.env.VENDOR_INFO ?? 'Test Info'
});…dded OnlineInformation section
…and add onlineInformation with humanRights section for online action suite
…ccordingly in all action suites
…s and add online topic
…in event and humanRights fields and add online topic
JeremyFriesenGitHub
force-pushed
the
jeremy/feat/145-automate-filling-out-Carleton's-risk-assesment-form
branch
from
d5fedac to
420c3df
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
tools/risk-form-filler/src/object-models/sections.ts (2)
562-564: Remove outdated FIXME commentThe FIXME comment suggests that the online event attendance section is incomplete, but the implementation below is complete with all necessary fields.
-// FIXME: This section is incomplete -// needs to add online event attendance
611-638: Consider lazy instantiation of form sectionsThe current implementation creates all section instances at module level, which could impact tree-shaking. Consider implementing a factory function that creates sections based on the form type.
export function createFormSections(formType: 'online' | 'inPerson' | 'hybrid'): Record<string, Record<string, Field<any>>> { const sections: Record<string, Record<string, Field<any>>> = { primaryOrganizer: createOrganizer('primary', true), event: createEvent(), // ... common sections } if (formType === 'online' || formType === 'hybrid') { sections.onlineInformation = createOnlineInformation() } return sections }tools/risk-form-filler/src/pom-old.ts (1)
195-201: Add input validation for form data.The form filling methods should validate input data to prevent XSS attacks.
class InputSanitizer { static sanitize(input: string): string { return input.replace(/[<>'"]/g, ''); } } async fillOnlineInformation(ONLINE_PLATFORM: string, ...) { await this.onlinePlatformTextBox.fill(InputSanitizer.sanitize(ONLINE_PLATFORM)); // ... sanitize other inputs }Also applies to: 366-372
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tools/risk-form-filler/src/action.spec.ts(1 hunks)tools/risk-form-filler/src/data/config.data.ts(1 hunks)tools/risk-form-filler/src/object-models/sections.ts(1 hunks)tools/risk-form-filler/src/pom-old.ts(1 hunks)tools/risk-form-filler/src/pom.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/risk-form-filler/src/action.spec.ts
- tools/risk-form-filler/src/pom.ts
🧰 Additional context used
🪛 Biome (1.9.4)
tools/risk-form-filler/src/data/config.data.ts
[error] 1-123: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
tools/risk-form-filler/src/object-models/sections.ts
[error] 19-54: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 57-82: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 85-169: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 170-211: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 212-258: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 259-308: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 309-459: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 460-493: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 494-532: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 533-562: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 564-611: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (8)
tools/risk-form-filler/src/object-models/sections.ts (2)
1-16: Well-structured Field class implementation!
The generic Field class effectively encapsulates form field properties with comprehensive support for validation, nested fields, and field matching.
18-561: 🛠️ Refactor suggestion
Refactor section classes into standalone functions
The current implementation uses classes with only static methods, which adds unnecessary complexity. Based on the established pattern in this project and TypeScript best practices, these should be standalone factory functions.
Apply this pattern to all section classes:
-export class SectionName {
- static create(): Record<string, Field<any>> {
+export function createSectionName(): Record<string, Field<any>> {
return {
// field definitions
}
- }
}🧰 Tools
🪛 Biome (1.9.4)
[error] 19-54: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 57-82: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 85-169: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 170-211: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 212-258: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 259-308: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 309-459: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 460-493: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 494-532: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
tools/risk-form-filler/src/data/config.data.ts (5)
31-31: Use dynamic date for event testing.
The hardcoded future date could cause tests to fail once we pass that date.
81-98: Secure vendor and transportation information.
The travel section contains sensitive vendor details that should be protected.
1-8: 🛠️ Refactor suggestion
Convert class to a const object.
The class only contains static members which is an anti-pattern in TypeScript.
-export class Config {
- static LINK = {
+export const Config = {
+ LINK: {
BASE: 'https://stuapps.carleton.ca/sarms/event-risk',
IN_PERSON: 'in-person',
HYBRID: 'hybrid',
ONLINE: 'online',
- }
+ },Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-123: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
21-28:
Remove secondary organizer's personal information.
Similar to the primary organizer, this contains real personal data.
secondaryOrganizer: {
- firstName: 'Ajaan',
- lastName: 'Nalliah',
- carletonId: '101151873',
- email: '[email protected]',
+ firstName: 'TestUser',
+ lastName: 'Secondary',
+ carletonId: '000000002',
+ email: '[email protected]',Likely invalid or redundant comment.
11-17:
Remove personal information from test data.
The configuration contains real personal information (names, student IDs, emails) which poses privacy risks.
primaryOrganizer: {
- firstName: 'Raef',
- lastName: 'Sarofiem',
- carletonId: '101152872',
- email: '[email protected]',
+ firstName: 'TestUser',
+ lastName: 'Primary',
+ carletonId: '000000001',
+ email: '[email protected]',Likely invalid or redundant comment.
tools/risk-form-filler/src/pom-old.ts (1)
106-108: Add URL validation and configuration.
The hardcoded URL should be moved to configuration and validated.
| alcohol: 'Yes', | ||
| smartServe: 'Yes', | ||
| entranceTicket: 'No', | ||
| food: 'Yes', | ||
| volunteers: { | ||
| volunteer1: 'Alice', | ||
| volunteer2: 'Bob', | ||
| volunteer3: 'Charlie', | ||
| }, | ||
| campusSafety: 'Yes', | ||
| privateSecurity: 'No', | ||
| speaker: 'Yes', | ||
| topics: 'cuHacking', | ||
| names: 'Raef Sarofiem', | ||
| website: 'https://cuhacking.ca/', | ||
| }, |
There was a problem hiding this comment.
Move sensitive operational details to environment variables.
Risk management configuration contains sensitive details about security measures and alcohol service that shouldn't be in version control.
Consider using environment variables:
export const Config = {
riskManagement: {
alcohol: process.env.RISK_ALCOHOL ?? 'No',
smartServe: process.env.RISK_SMART_SERVE ?? 'No',
// ... other fields
}
};| async fillContacts(formLayout: FormsLayout, PRIMARY_FIRST_NAME: string, PRIMARY_LAST_NAME: string, PRIMARY_CARLETON_ID: string, PRIMARY_EMAIL: string, PRIMARY_PHONE: string, SECONDARY_FIRST_NAME: string, SECONDARY_LAST_NAME: string, SECONDARY_CARLETON_ID: string, SECONDARY_EMAIL: string, SECONDARY_PHONE: string) { | ||
| await formLayout.primaryFirstNameTextBox.fill(PRIMARY_FIRST_NAME) | ||
| await formLayout.primaryLastNameTextBox.fill(PRIMARY_LAST_NAME) | ||
| await formLayout.primaryCarletonIdTextBox.fill(PRIMARY_CARLETON_ID) | ||
| await formLayout.primaryOrganizationTextBox.fill('cuHacking') | ||
| await formLayout.primaryStatusRadioButton.check() | ||
| await formLayout.primaryEmailTextBox.fill(PRIMARY_EMAIL) | ||
| await formLayout.primaryConfirmEmailTextBox.fill(PRIMARY_EMAIL) | ||
| await formLayout.primaryPhoneTextBox.fill(PRIMARY_PHONE) | ||
|
|
||
| await formLayout.secondaryFirstNameTextBox.fill(SECONDARY_FIRST_NAME) | ||
| await formLayout.secondaryLastNameTextBox.fill(SECONDARY_LAST_NAME) | ||
| await formLayout.secondaryCarletonIdTextBox.fill(SECONDARY_CARLETON_ID) | ||
| await formLayout.secondaryOrganizationTextBox.fill('Community Engagement') | ||
| await formLayout.secondaryStatusRadioButton.check() | ||
| await formLayout.secondaryEmailTextBox.fill(SECONDARY_EMAIL) | ||
| await formLayout.secondaryPhoneTextBox.fill(SECONDARY_PHONE) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant formLayout parameter.
These methods are instance methods and don't need the formLayout parameter as they can use this.
Example fix for fillContacts:
- async fillContacts(formLayout: FormsLayout, PRIMARY_FIRST_NAME: string, ...
+ async fillContacts(PRIMARY_FIRST_NAME: string, ...
- await formLayout.primaryFirstNameTextBox.fill(PRIMARY_FIRST_NAME)
+ await this.primaryFirstNameTextBox.fill(PRIMARY_FIRST_NAME)Also applies to: 129-141, 143-150, 152-157
| export class InPersonFormLayout extends FormsLayout { | ||
| // Buttons | ||
| readonly inPersonButton: Locator | ||
|
|
||
| // IN-PERSON + HYBRID FORM | ||
| readonly eventLocationTextBox: Locator | ||
| readonly eventLocationRadioButton: Locator | ||
| readonly expectedAttendeesTextBox: Locator | ||
| readonly foodRadioButton: Locator | ||
| readonly healthInsuranceRadioButton: Locator | ||
| readonly photoIdRadioButton: Locator | ||
| readonly alcoholRadioButton: Locator | ||
| readonly transportationRadioButton: Locator | ||
| readonly outOfProvinceRadioButton: Locator | ||
| readonly garbageRadioButton: Locator | ||
| readonly cleanupRadioButton: Locator | ||
| readonly cleanupTextBox: Locator | ||
| readonly overnightRadioButton: Locator | ||
| readonly rightsTextBox: Locator | ||
|
|
||
| // HELPER FUNCTIONS - EMERGENCY RESPONSE | ||
| readonly emergencyFirstAidRadioButton: Locator | ||
| readonly emergencyHospitalRadioButton: Locator | ||
| readonly emergencyEvacRadioButton: Locator | ||
| readonly emergencyFlamesRadioButton: Locator | ||
| readonly emergencyCrowdControlRadioButton: Locator | ||
| readonly emergencySafetyConcernsTextBox: Locator | ||
| readonly emergencySafetyRisksTextBox: Locator | ||
|
|
||
| constructor(page: Page) { | ||
| super(page) | ||
|
|
||
| // Buttons | ||
| this.inPersonButton = page.getByRole('link', { name: 'In-Person' }) | ||
|
|
||
| // in-person + hybrid form | ||
| this.eventLocationTextBox = page.getByLabel('Event LocationIf on campus,') | ||
| this.eventLocationRadioButton = page.locator('input[name="event_location_res"]').nth(1) | ||
| this.expectedAttendeesTextBox = page.getByLabel('Number of expected') | ||
| this.foodRadioButton = page.locator('input[name="event_food"]').first() | ||
| this.healthInsuranceRadioButton = page.locator('input[name="event_health_insurance"]').first() | ||
| this.photoIdRadioButton = page.locator('input[name="event_photo_id"]').first() | ||
| this.alcoholRadioButton = page.locator('input[name="risk_alcohol"]').first() | ||
| this.transportationRadioButton = page.locator('input[name="travel_transportation"]').first() | ||
| this.outOfProvinceRadioButton = page.locator('input[name="out_of_province"]').first() | ||
| this.garbageRadioButton = page.locator('input[name="fmp_garbage"]').first() | ||
| this.cleanupRadioButton = page.locator('input[name="fmp_cleanup"]').nth(1) | ||
| this.cleanupTextBox = page.getByLabel('Please provide details as to') | ||
| this.overnightRadioButton = page.locator('input[name="fmp_overnight"]').first() | ||
| this.rightsTextBox = page.locator('#rights_other') | ||
|
|
||
| // helper functions - emergency response | ||
| this.emergencyFirstAidRadioButton = page.locator('input[name="emergency_firstaid"]').first() | ||
| this.emergencyHospitalRadioButton = page.locator('input[name="emergency_hospital"]').nth(1) | ||
| this.emergencyEvacRadioButton = page.locator('input[name="emergency_evac"]').first() | ||
| this.emergencyFlamesRadioButton = page.locator('input[name="emergency_flames"]').first() | ||
| this.emergencyCrowdControlRadioButton = page.locator('fieldset').filter({ hasText: 'Will crowd control measures' }).getByLabel('No') | ||
| this.emergencySafetyConcernsTextBox = page.getByLabel('Are there any other special') | ||
| this.emergencySafetyRisksTextBox = page.locator('#safety_risk') | ||
| } | ||
|
|
||
| async fillEmergencyResponse(formLayout: InPersonFormLayout, EMERGENCY_SAFETY_CONCERNS: string, EMERGENCY_SAFETY_RISKS: string) { | ||
| await formLayout.emergencyFirstAidRadioButton.check() | ||
| await formLayout.emergencyHospitalRadioButton.check() | ||
| await formLayout.emergencyEvacRadioButton.check() | ||
| await formLayout.emergencyFlamesRadioButton.check() | ||
| await formLayout.emergencyCrowdControlRadioButton.check() | ||
| await formLayout.emergencySafetyConcernsTextBox.fill(EMERGENCY_SAFETY_CONCERNS) | ||
| await formLayout.emergencySafetyRisksTextBox.fill(EMERGENCY_SAFETY_RISKS) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication between InPersonFormLayout and HybridFormLayout.
There's significant duplication of properties and methods between these classes.
Create a base class for shared functionality:
abstract class InPersonBaseFormLayout extends FormsLayout {
// Shared properties and methods
readonly eventLocationTextBox: Locator;
// ... other shared properties
constructor(page: Page) {
super(page);
this.eventLocationTextBox = page.getByLabel('Event LocationIf on campus,');
// ... initialize other shared properties
}
async fillEmergencyResponse(EMERGENCY_SAFETY_CONCERNS: string, EMERGENCY_SAFETY_RISKS: string): Promise<void> {
// Shared implementation
}
}Also applies to: 276-373
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tools/risk-form-filler/src/object-models/sections.ts (1)
562-564: Address FIXME comment for online event attendanceThe comment indicates that online event attendance functionality is incomplete. This could affect the proper handling of online events.
Would you like me to help implement the missing online event attendance functionality?
tools/risk-form-filler/src/action.spec.ts (2)
1-67: Reduce code duplication in form fillingThe form filling code for common sections (primary/secondary organizer, event info, etc.) is duplicated across tests. Consider extracting these into helper functions.
Example refactor:
async function fillOrganizerSection(page: Page, organizer: any, data: any) { await page.locator(`[name=${organizer.firstName.name}]`).fill(data.firstName) await page.locator(`[name=${organizer.lastName.name}]`).fill(data.lastName) // ... other fields } // Usage in tests await fillOrganizerSection(page, primaryOrganizer, Config.testData.primaryOrganizer)
165-170: Remove or implement commented codeThe commented code for
intoxicatedIndividualssubfields should either be removed if not needed or properly implemented if required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tools/risk-form-filler/src/action.spec.ts(1 hunks)tools/risk-form-filler/src/object-models/sections.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tools/risk-form-filler/src/object-models/sections.ts
[error] 19-54: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 57-82: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 85-169: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 170-211: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 212-258: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 259-308: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 309-459: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 460-493: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 494-532: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 533-562: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 564-611: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (4)
tools/risk-form-filler/src/object-models/sections.ts (3)
1-16: Well-structured field model implementation!
The Field class provides a robust foundation with comprehensive properties for form fields.
19-561: Convert static-only classes to functions
These classes consist only of static create methods. As previously suggested, they should be converted to standalone functions for better maintainability and adherence to functional programming principles.
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-54: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 57-82: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 85-169: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 170-211: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 212-258: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 259-308: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 309-459: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 460-493: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 494-532: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
611-638: 🛠️ Refactor suggestion
Update section instantiations after refactoring
Once the classes are converted to functions, these instantiations will need to be updated to use the new function-based approach.
Apply this diff after converting the classes to functions:
-const primaryOrganizer = Organizer.create('primary', true)
-const secondaryOrganizer = Organizer.create('secondary', false)
-const event = Event.create()
-const riskManagement = RiskManagement.create()
-const emergencyPreparedness = EmergencyPreparedness.create()
-const safetyRiskAssessment = SafetyRiskAssessment.create()
-const contractsInsurance = ContractsInsurance.create()
-const travel = Travel.create()
-const outOfProvinceEvents = OutOfProvinceEvents.create()
-const maintenanceServices = MaintenanceServices.create()
-const humanRights = HumanRights.create()
-const onlineInformation = OnlineInformation.create()
+const primaryOrganizer = createOrganizer('primary', true)
+const secondaryOrganizer = createOrganizer('secondary', false)
+const event = createEvent()
+const riskManagement = createRiskManagement()
+const emergencyPreparedness = createEmergencyPreparedness()
+const safetyRiskAssessment = createSafetyRiskAssessment()
+const contractsInsurance = createContractsInsurance()
+const travel = createTravel()
+const outOfProvinceEvents = createOutOfProvinceEvents()
+const maintenanceServices = createMaintenanceServices()
+const humanRights = createHumanRights()
+const onlineInformation = createOnlineInformation()Likely invalid or redundant comment.
tools/risk-form-filler/src/action.spec.ts (1)
199-331: Well-structured form type specific tests!
The tests are properly separated by form type and navigate to the correct URLs. Good job on implementing comprehensive test coverage for each form variant.
MFarabi619
changed the title
MFarabi619
changed the title
|
🎉 This PR is included in version 1.9.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Pull Request Template
Linked issue #145
PR Requirements Checklist
Summary by CodeRabbit
New Features
Formclass to manage event registration forms (In-Person, Hybrid, Online) with dynamic section creation.Configclass for managing event-related data and URLs.Documentation
Tests
Chores