Add the openfn package to platform. Also fixed CU-86bzwyh69 #322
Conversation
WalkthroughThis update introduces the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Docker
participant OpenFn
participant Database
User->>Docker: Deploy OpenFn stack
Docker->>OpenFn: Start openfn service
OpenFn->>Database: Connect and initialize
Database-->>OpenFn: Confirmation
OpenFn-->>Docker: Health check status
Docker-->>User: Deployment complete!
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Task linked: CU-86bzwyh69 Incorrect Config Import - Kafka Mapper |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (7)
openfn/importer/workflows/docker-compose.config.yml (1)
19-22: Consider a more robust restart policy for production.The current restart policy (
condition: none) is suitable for development but consider using a more robust policy likeon-failureoralwaysfor production environments to ensure service reliability.- condition: none + condition: on-failureopenfn/importer/postgres/docker-compose.config.yml (1)
23-26: Consider a more robust restart policy for production.The current restart policy (
condition: none) is suitable for development but consider using a more robust policy likeon-failureoralwaysfor production environments to ensure service reliability.- condition: none + condition: on-failuredocumentation/packages/openfn/README.md (1)
5-11: Specify language for code block in usage section.To improve readability and follow best practices, specify the language for the code block in the usage section.
-`instant package init -n openfn --dev` +```bash +instant package init -n openfn --dev +```openfn/swarm.sh (3)
66-68: Consider logging the stack destruction.In the
destroy_packagefunction, consider adding a log statement before destroying the stack to provide more context in the logs.log info "Destroying stack $STACK"
70-89: Enhance error messaging inmain.The
mainfunction logs an error if an invalid action is provided. Consider providing more detailed guidance on valid options.log error "Invalid action: $ACTION. Valid options are: init, up, down, or destroy."
1-1: Add Executable Permissions toswarm.sh.The file
openfn/swarm.shcurrently lacks executable permissions. To execute this script directly, set the executable bit usingchmod +x openfn/swarm.sh.
- Current permissions:
-rw-r--r--- Required action:
chmod +x openfn/swarm.shAnalysis chain
Ensure executable permissions.
Since this is a shell script, ensure that it has executable permissions set in the repository.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the swarm.sh script has executable permissions. # Test: Check file permissions. Expect: Executable bit set. ls -l openfn/swarm.shLength of output: 88
openfn/package-metadata.json (1)
4-4: Remove HTML entities in the description.The description contains HTML entities (
') which should be replaced with the appropriate character (')."description": "Lightning ⚡ (OpenFn v2) is a workflow automation platform that's used to automate critical business processes and integrate information systems.",
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
openfn/importer/postgres/package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (12)
- config.yaml (1 hunks)
- documentation/packages/openfn/README.md (1 hunks)
- documentation/packages/openfn/environment-variables.md (1 hunks)
- kafka-mapper-consumer/docker-compose.config.yml (1 hunks)
- openfn/docker-compose.dev.yml (1 hunks)
- openfn/docker-compose.yml (1 hunks)
- openfn/importer/postgres/create-db.js (1 hunks)
- openfn/importer/postgres/docker-compose.config.yml (1 hunks)
- openfn/importer/postgres/package.json (1 hunks)
- openfn/importer/workflows/docker-compose.config.yml (1 hunks)
- openfn/package-metadata.json (1 hunks)
- openfn/swarm.sh (1 hunks)
Files skipped from review due to trivial changes (3)
- config.yaml
- openfn/docker-compose.dev.yml
- openfn/importer/postgres/package.json
Additional context used
Markdownlint
documentation/packages/openfn/README.md
24-24: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
Gitleaks
openfn/package-metadata.json
31-31: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
Additional comments not posted (14)
openfn/importer/workflows/docker-compose.config.yml (4)
1-6: Use specific Node.js version and global npm package installation.Using a specific Node.js version (
node:18-alpine3.20) ensures consistency across environments. Installing npm packages globally (npm install -g @openfn/cli) is suitable for CLI tools but be mindful of potential version conflicts.
14-18: Good use of environment variables and external network.Using environment variables for sensitive information like API keys and URLs is a best practice. The external network
postgres_publicensures connectivity with the database.
24-44: Dynamic config names and external network definition are well-implemented.Using dynamic names for configuration files based on digests ensures uniqueness and traceability. The external network
postgres_publicis correctly defined.
7-13: Verify configuration files availability and correctness.Ensure that the configuration files (
cdr-ndr.yaml,config.json,project.yaml) are present in the specified paths and contain the correct configurations for your deployment.openfn/importer/postgres/docker-compose.config.yml (4)
1-6: Use specific Node.js version and npm package installation.Using a specific Node.js version (
node:erbium-alpine) ensures consistency across environments. Installing npm packages (npm i) is necessary for running thecreate-db.jsscript.
12-22: Good use of environment variables and external network.Using environment variables for sensitive information like database credentials is a best practice. The external network
postgres_publicensures connectivity with the database.
28-43: Dynamic config names and external network definition are well-implemented.Using dynamic names for configuration files based on digests ensures uniqueness and traceability. The external network
postgres_publicis correctly defined.
7-11: Verify configuration files availability and correctness.Ensure that the configuration files (
package.json,create-db.js) are present in the specified paths and contain the correct configurations for your deployment.documentation/packages/openfn/README.md (2)
29-31: Documentation section is well-structured.The link to the official documentation is appropriate and provides additional resources for users.
15-27: Exercise caution with sensitive information in documentation.Providing demo credentials in documentation can be risky. Ensure these credentials are intended for public use and consider alternative methods for sharing sensitive information.
Tools
Markdownlint
24-24: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
openfn/docker-compose.yml (1)
1-40: Configuration looks good!The Docker Compose file is well-structured and uses environment variables effectively for configuration. Ensure that all environment variables used are defined in the environment where this Docker Compose file will be deployed.
kafka-mapper-consumer/docker-compose.config.yml (1)
30-30: Change approved!The update to the digest variable name aligns with the service's context, which should improve clarity in the configuration.
Verification successful
Verification complete: No issues found with the updated digest variable usage.
The variable
kafka_mapper_consumer_openhimConfig_js_DIGESTis used only in the specified line within thedocker-compose.config.ymlfile, indicating consistent usage within this context. No further occurrences were found in thekafka-mapper-consumerdirectory.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the updated digest variable. # Test: Search for the updated digest variable usage. # Expect: The new variable should be used consistently across the codebase. rg --type yaml 'kafka_mapper_consumer_openhimConfig_js_DIGEST' kafka-mapper-consumer/Length of output: 236
openfn/importer/postgres/create-db.js (1)
60-72: Overall logic is sound.The script correctly handles database and user creation, checking for existence before attempting to create them. Ensure that error handling covers all potential failure points.
openfn/swarm.sh (1)
28-32: Verify the existence of utility scripts.The
import_sourcesfunction assumes that the utility scripts exist at the specified paths. Ensure these files are present and accessible.
| <td></td> | ||
| </tr> | ||
| <tr> | ||
| <td>POSTGRES_USER</td> | ||
| <td>The username for the PostgreSQL database</td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| <tr> | ||
| <td>POSTGRES_SERVICE</td> | ||
| <td>The service name for the PostgreSQL database</td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| <tr> | ||
| <td>POSTGRES_DATABASE</td> | ||
| <td>The name of the PostgreSQL database</td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| <tr> | ||
| <td>POSTGRES_PASSWORD</td> | ||
| <td>The password for the PostgreSQL database</td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| <tr> | ||
| <td>POSTGRES_PORT</td> | ||
| <td>The port number for the PostgreSQL database</td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| <tr> | ||
| <td>OpenFn_POSTGRESQL_DB</td> | ||
| <td>The name of the OpenFn PostgreSQL database</td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| <tr> | ||
| <td>OpenFn_POSTGRESQL_USERNAME</td> | ||
| <td>The username for the OpenFn PostgreSQL database</td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| <tr> | ||
| <td>OpenFn_POSTGRESQL_PASSWORD</td> | ||
| <td>The password for the OpenFn PostgreSQL database</td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| <tr> | ||
| <td>WORKER_LIGHTNING_PUBLIC_KEY</td> | ||
| <td>The public key for the worker lightning</td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| <tr> | ||
| <td>WORKER_SECRET</td> | ||
| <td>The secret key for the worker</td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| <tr> | ||
| <td>OpenFn_IMAGE</td> | ||
| <td>The image name for OpenFn</td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| <tr> | ||
| <td>OpenFn_WORKER_IMAGE</td> | ||
| <td>The image name for OpenFn worker</td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| </tbody> | ||
| </table> |
There was a problem hiding this comment.
Complete the environment variables documentation.
The table is missing information in several columns: Type, Relevance, Required, and Default. Providing this information will help users understand how to configure these variables.
| Variable Name | Description | Type | Relevance | Required | Default |
|-------------------------|-------------------------------------------------|---------|-----------|----------|-------------------|
| DATABASE_URL | The URL of the PostgreSQL database | String | Critical | Yes | None |
| DISABLE_DB_SSL | Whether to disable SSL for the database connection | Boolean | Optional | No | false |
| ... | ... | ... | ... | ... | ... |
drizzentic
requested review from
rcrichton,
ItsMurumba,
bradsawadye and
brett-onions
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
openfn/importer/postgres/create-db.js (1)
67-67: Improve error logging for better debugging.Consider logging the full error object instead of just the message to get more context about the error.
- console.error("Error in db operations:", error.message); + console.error("Error in db operations:", error);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- openfn/importer/postgres/create-db.js (1 hunks)
Additional context used
Biome
openfn/importer/postgres/create-db.js
[error] 52-52: expected
,but instead foundawaitRemove await
(parse)
[error] 54-54: expected
,but instead found;Remove ;
(parse)
Additional comments not posted (3)
openfn/importer/postgres/create-db.js (3)
3-10: Securely handle sensitive data.Avoid using hardcoded fallback values for sensitive information like passwords. Use environment variables or secret management tools to manage these values securely.
35-35: Use parameterized queries to prevent SQL injection.The current use of string interpolation for creating the database is vulnerable to SQL injection. Use parameterized queries instead.
51-51: Fix syntax error and use parameterized queries.There is a syntax error due to a missing closing parenthesis. Additionally, use parameterized queries to prevent SQL injection.
- await client.query( - 'CREATE USER $1 WITH ENCRYPTED PASSWORD $2;', [newUser, newUserPassword] + await client.query( + 'CREATE USER $1 WITH ENCRYPTED PASSWORD $2;', [newUser, newUserPassword] + );Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (4)
openfn/importer/workflows/example/cdr-ndr.yaml (2)
16-20: Remove trailing spaces.Trailing spaces are present in multiple lines, which can lead to formatting inconsistencies. Consider removing them for cleaner code.
- const parsedData = JSON.parse(state.data.body); - console.log(parsedData); - state.resource = parsedData.entry[0].resource; - console.log('show only the resource:: ', state.resource); - return state; + const parsedData = JSON.parse(state.data.body); + console.log(parsedData); + state.resource = parsedData.entry[0].resource; + console.log('show only the resource:: ', state.resource); + return state;Also applies to: 34-35, 37-37, 84-84, 86-87, 92-93, 115-116, 118-123
Tools
yamllint
[error] 16-16: trailing spaces
(trailing-spaces)
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 18-18: trailing spaces
(trailing-spaces)
[error] 20-20: trailing spaces
(trailing-spaces)
29-29: Typographical Error: Correct comment spelling.The comment contains a typographical error: "Removig" should be "Removing."
- //Removig PII here before sending it to NDR + //Removing PII here before sending it to NDRopenfn/importer/workflows/example/project.yaml (2)
16-20: Remove trailing spaces.Trailing spaces are present in multiple lines, which can lead to formatting inconsistencies. Consider removing them for cleaner code.
- const parsedData = JSON.parse(state.data.body); - console.log(parsedData); - state.resource = parsedData.entry[0].resource; - console.log('show only the resource:: ', state.resource); - return state; + const parsedData = JSON.parse(state.data.body); + console.log(parsedData); + state.resource = parsedData.entry[0].resource; + console.log('show only the resource:: ', state.resource); + return state;Also applies to: 34-35, 37-37, 84-84, 86-87, 92-93, 115-116, 118-123
Tools
yamllint
[error] 16-16: trailing spaces
(trailing-spaces)
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 18-18: trailing spaces
(trailing-spaces)
[error] 20-20: trailing spaces
(trailing-spaces)
29-29: Typographical Error: Correct comment spelling.The comment contains a typographical error: "Removig" should be "Removing."
- //Removig PII here before sending it to NDR + //Removing PII here before sending it to NDR
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- openfn/importer/workflows/example/cdr-ndr.yaml (1 hunks)
- openfn/importer/workflows/example/config.json (1 hunks)
- openfn/importer/workflows/example/project.yaml (1 hunks)
Additional context used
Gitleaks
openfn/importer/workflows/example/config.json
4-4: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
yamllint
openfn/importer/workflows/example/cdr-ndr.yaml
[error] 16-16: trailing spaces
(trailing-spaces)
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 18-18: trailing spaces
(trailing-spaces)
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 84-84: trailing spaces
(trailing-spaces)
[error] 86-86: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 92-92: trailing spaces
(trailing-spaces)
[error] 93-93: trailing spaces
(trailing-spaces)
[error] 115-115: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
[error] 118-118: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
[error] 120-120: trailing spaces
(trailing-spaces)
[error] 123-123: trailing spaces
(trailing-spaces)
openfn/importer/workflows/example/project.yaml
[error] 16-16: trailing spaces
(trailing-spaces)
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 18-18: trailing spaces
(trailing-spaces)
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 84-84: trailing spaces
(trailing-spaces)
[error] 86-86: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 92-92: trailing spaces
(trailing-spaces)
[error] 93-93: trailing spaces
(trailing-spaces)
[error] 115-115: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
[error] 118-118: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
[error] 120-120: trailing spaces
(trailing-spaces)
[error] 123-123: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
openfn/docker-compose.yml (3)
6-6: Consider simplifying the command for readability.The command for the
openfnservice is complex and could be broken down into a script or multiple lines for better readability and maintainability.command: > sh -c ' /app/bin/lightning eval "Lightning.Release.migrate()" && /app/bin/lightning eval "Lightning.Demo.reset_demo()" && /app/bin/lightning start '
34-44: Environment Variables Not Set forworkerServiceThe following environment variables required for the
workerservice are not set, which may lead to issues in its operation:
OpenFn_WORKER_IMAGEWORKER_LIGHTNING_PUBLIC_KEYWORKER_SECRETNODE_ENVEnsure these variables are properly defined in your environment or in a
.envfile used by Docker Compose. This is crucial for the correct functioning of theworkerservice.Analysis chain
workerservice configuration looks good. Verify environment variables.The configuration for the
workerservice is clear and concise. Ensure that all environment variables are correctly set.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all environment variables for the worker service are set correctly. # Test: List all environment variables used and check if they are set. env_vars=( "OpenFn_WORKER_IMAGE" "WORKER_LIGHTNING_PUBLIC_KEY" "WORKER_SECRET" "NODE_ENV" ) for var in "${env_vars[@]}"; do if [ -z "${!var}" ]; then echo "Warning: $var is not set." fi doneLength of output: 621
12-22: Environment Variables Not SetThe following environment variables are not set in the current environment, which might lead to issues if they are required for the application to function properly:
DATABASE_URLDISABLE_DB_SSLIS_RESETTABLE_DEMOLISTEN_ADDRESSLOG_LEVELORIGINSPRIMARY_ENCRYPTION_KEYSECRET_KEY_BASEWORKER_RUNS_PRIVATE_KEYWORKER_SECRETKAFKA_TRIGGERS_ENABLED(note: this variable is not listed in thedocker-compose.ymlsnippet provided)Please ensure these variables are correctly set in the environment where the Docker Compose file will be used.
Analysis chain
Verify environment variables for correctness.
Ensure that all environment variables are correctly set in the environment where this Docker Compose file will be used. This includes checking for any missing or incorrectly named variables.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all environment variables are set correctly. # Test: List all environment variables used and check if they are set. env_vars=( "DATABASE_URL" "DISABLE_DB_SSL" "IS_RESETTABLE_DEMO" "LISTEN_ADDRESS" "LOG_LEVEL" "ORIGINS" "PRIMARY_ENCRYPTION_KEY" "SECRET_KEY_BASE" "WORKER_RUNS_PRIVATE_KEY" "WORKER_SECRET" "KAFKA_TRIGGERS_ENABLED" ) for var in "${env_vars[@]}"; do if [ -z "${!var}" ]; then echo "Warning: $var is not set." fi doneLength of output: 1641
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- openfn/docker-compose.dev.yml (1 hunks)
- openfn/docker-compose.yml (1 hunks)
- openfn/importer/postgres/create-db.js (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- openfn/docker-compose.dev.yml
- openfn/importer/postgres/create-db.js
Additional comments not posted (2)
openfn/docker-compose.yml (2)
1-1: Version declaration is correct.The version '3.9' is a valid and recent Docker Compose version.
49-55: Network settings are appropriate. Verify network existence.The use of external networks
kafka_publicandpostgresis appropriate. Ensure these networks exist and are correctly configured in the Docker environment.
drizzentic
force-pushed
the
add-openfn-to-platform
branch
from
6131eae
to
9819edb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
test/cucumber/package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (4)
- config.yaml (2 hunks)
- test/cucumber/features/single-mode/openfn.feature (1 hunks)
- test/cucumber/features/single-mode/recipe.feature (1 hunks)
- test/cucumber/package.json (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- config.yaml
Additional comments not posted (3)
test/cucumber/features/single-mode/openfn.feature (2)
4-13: ScenarioInit OpenFnis well-defined.The scenario comprehensively tests the initialization of the OpenFn package, including service startups and network connections.
14-24: ScenarioDestroy OpenFnis well-defined.The scenario thoroughly tests the destruction of the OpenFn package, ensuring all services, volumes, configs, and networks are removed.
test/cucumber/package.json (1)
34-34: New test commandtest:single:openfnis correctly added.The command for testing the OpenFn feature is well-integrated and follows the existing pattern.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (8)
openfn/package-metadata.json (2)
1-7: LGTM! Consider version number strategy.The basic metadata looks good. However, given that this seems to be a configuration for a production-ready platform (OpenFn v2), you might want to reconsider the version number "0.0.1".
Consider adopting a more meaningful versioning strategy that aligns with your release cycle and the maturity of the platform.
28-29: Specify a version for the worker image.Good job on using a specific version for the OpenFn image. This practice ensures consistency across different environments.
However, for the worker image, consider specifying a version instead of using the 'latest' tag. Using 'latest' can lead to inconsistencies and unexpected behavior in production environments.
Example:
"OpenFn_WORKER_IMAGE": "openfn/ws-worker:v1.2.3"Replace "v1.2.3" with the appropriate version number. This ensures that you have control over which version of the worker is being used in your environment.
openfn/importer/workflows/example/project.yaml (6)
8-40: LGTM: parse-bundle job is well-structured.The job effectively parses and sorts incoming data bundles. The logging of resource type counts is a helpful feature for monitoring and debugging.
Consider adding more detailed error logging for the JSON parsing step. For example:
if (typeof data === "string") { try { data = JSON.parse(data); } catch (error) { console.error(`Error parsing JSON: ${error.message}`); // Handle the error appropriately } }This will provide more context if there are issues with malformed JSON input.
41-111: LGTM with suggestions: Register-Organization job is functional but could be improved.The job successfully creates and registers an organization based on the first patient's managing organization. However, there are a few areas for potential improvement:
Using the first patient's managing organization might not be suitable for all scenarios. Consider adding a comment explaining this choice or implementing a more robust selection method.
Error handling for the HTTP request is missing.
Consider adding error handling for the HTTP request:
put(`/fhir/Organization/${$.orgId}`, { body: $.mappedOrganization, headers: { 'content-type': 'application/json' }, }).then((response) => { console.log('Organization registered successfully!'); return response; }).catch((error) => { console.error('Error registering organization:', error); // Handle the error appropriately });This will provide better visibility into any issues that occur during the organization registration process.
113-1129: LGTM with suggestions: Comprehensive mapping jobs with room for improvement.The map-encounters, map-patients, and map-medications jobs provide thorough mapping of resources to FHIR format. The use of utility functions and constants for mapping is commendable. However, there are areas for potential improvement:
Some parts of the code, especially in the map-medications job, are quite complex. Consider breaking down large functions into smaller, more manageable pieces.
Additional inline comments would be helpful for understanding the more complex mapping logic.
There's some code duplication in error handling and logging that could be refactored into utility functions.
Consider creating utility functions for common operations. For example:
const logWarning = (message, context) => { console.log(`WARNING: ${message}`, context); }; const findResourceById = (resources, id, type) => { const resource = resources.find(e => e.resource.id === id)?.resource; if (!resource) { logWarning(`COULD NOT FIND MATCHING ${type} FOR ${id}`); } return resource; };These functions could be used throughout the code to reduce duplication and improve readability.
🧰 Tools
yamllint
[error] 158-158: trailing spaces
(trailing-spaces)
[error] 401-401: trailing spaces
(trailing-spaces)
1131-1164: LGTM with a minor suggestion: send-to-NDR job is well-implemented.The job successfully sends the final bundle to the NDR and includes error handling. This is crucial for ensuring data integrity and tracking issues.
Consider enhancing the error logging to provide more context:
.catch(async (response, state) => { const err = JSON.parse(response.body) console.error('Error from NDR FHIR:', { status: response.status, statusText: response.statusText, error: err }); state.issue = err.issue; return state })This will provide more detailed information about the HTTP response in case of an error, which can be helpful for debugging.
1165-1189: LGTM with suggestions: send-errors-to-kafka-topic job is a good error handling mechanism.The job effectively sends error messages and failed bundles to separate Kafka topics, which is excellent for error tracking and potential retry mechanisms.
Consider adding error handling for the Kafka POST requests:
post('/openfn/fhir-errors', { body: JSON.stringify($.issue), headers: { 'content-type': 'application/json' }, }).then(() => { console.log('Successfully sent error to Kafka topic fhir-errors'); }).catch((error) => { console.error('Failed to send error to Kafka topic fhir-errors:', error); }); post('/openfn/dead-queue', { body: $.bundle, headers: { 'content-type': 'application/json' }, }).then(() => { console.log('Successfully sent failed bundle to Kafka topic dead-queue'); }).catch((error) => { console.error('Failed to send bundle to Kafka topic dead-queue:', error); });This will provide visibility into any issues that occur when sending data to Kafka topics.
158-158: Minor: Remove trailing spaces.The static analysis tool yamllint reported trailing spaces on lines 158 and 401. While these don't affect functionality, it's good practice to remove them for code cleanliness.
Please remove the trailing spaces from lines 158 and 401.
Also applies to: 401-401
🧰 Tools
yamllint
[error] 158-158: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- openfn/importer/workflows/docker-compose.config.yml (1 hunks)
- openfn/importer/workflows/example/config.json (1 hunks)
- openfn/importer/workflows/example/project.yaml (1 hunks)
- openfn/package-metadata.json (1 hunks)
🧰 Additional context used
Gitleaks
openfn/importer/workflows/example/config.json
4-4: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
openfn/package-metadata.json
31-31: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
yamllint
openfn/importer/workflows/example/project.yaml
[error] 158-158: trailing spaces
(trailing-spaces)
[error] 401-401: trailing spaces
(trailing-spaces)
🔇 Additional comments not posted (5)
openfn/importer/workflows/docker-compose.config.yml (3)
17-20: Review deployment configuration for production readinessThe current deployment configuration might not be suitable for a production environment.
- The single replica without a restart policy could lead to downtime if the container fails.
- For a configuration service, you might want to consider using a
deploymode that ensures the task runs once and then stops.Consider the following adjustments:
If this is intended for production use, add a restart policy:
restart_policy: condition: on-failure max_attempts: 3If this is a one-time setup task, consider using a separate Docker Compose file for initialization tasks, or use a
deploymode that runs once:deploy: mode: global placement: constraints: - node.role == managerPlease clarify the intended use of this service (one-time setup vs. long-running service) to determine the most appropriate configuration.
27-37:⚠️ Potential issueReview config file paths and consider environment-specific configurations
The current configuration uses files from an
./exampledirectory, which may not be suitable for production use.Consider the following improvements:
Use environment-specific paths instead of
./example:configs: config.json: file: ${CONFIG_PATH:-./config/production}/config.json project.yaml: file: ${PROJECT_PATH:-./config/production}/project.yamlImplement a strategy for managing different configurations across environments (development, staging, production).
Ensure that sensitive information in these config files is properly secured, possibly by using Docker secrets for sensitive data instead of config files.
To ensure the correct files are being used, you can add a step in your deployment process to verify the content of the configs:
#!/bin/bash # Verify config files echo "Content of config.json:" cat ${CONFIG_PATH:-./config/production}/config.json echo "Content of project.yaml:" cat ${PROJECT_PATH:-./config/production}/project.yamlReview the output to confirm that the correct configuration files are being used for your target environment.
22-25: Verify the security of thepostgres_publicnetworkThe use of an external network named
postgres_publicraises potential security concerns.Please confirm that:
- This network is not actually exposed to the public internet.
- Appropriate security measures (like firewalls, VPNs, or other network isolation techniques) are in place to protect this network.
Consider renaming the network to something less ambiguous, like
postgres_internal, if it's not meant to be public.Review the output to ensure the network configuration aligns with your security requirements.
openfn/importer/workflows/example/project.yaml (2)
1-7: LGTM: Initial configuration looks good.The workflow name and description are clear and concise. The credential setup appears to be correct for the intended purpose.
1190-1231: LGTM: Workflow configuration is well-structured.The workflow configuration is logically organized with a clear sequence of job executions. The use of a conditional edge for error handling (send-to-NDR -> send-errors-to-kafka-topic) is a good practice that ensures errors are properly captured and processed.
The webhook trigger and job sequence align well with the overall purpose of the workflow. The configuration provides a robust pipeline for processing and validating healthcare data.
| "specPath": "project.yaml", | ||
| "statePath": "state.json", | ||
| "apiKey": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJKb2tlbiIsImlhdCI6MTY4ODAzNzE4NSwiaXNzIjoiSm9rZW4iLCJqdGkiOiIydG1ocG8zYm0xdmR0MDZvZDgwMDAwdTEiLCJuYmYiOjE2ODgwMzcxODUsInVzZXJfaWQiOiIzZjM3OGU2Yy02NjBhLTRiOTUtYWI5Ni02YmQwZGMyNjNkMzMifQ.J1FnACGpqtQbmXNvyUCwCY4mS5S6CohRU3Ey-N0prP4", | ||
| "endpoint": "http://openfn:4000" |
There was a problem hiding this comment.
Use HTTPS for the endpoint URL
The current endpoint URL uses the insecure HTTP protocol. This can lead to potential security vulnerabilities, especially when transmitting sensitive data like JWT tokens.
Replace the HTTP URL with HTTPS:
- "endpoint": "http://openfn:4000"
+ "endpoint": "https://openfn:4000"Ensure that the OpenFn server is configured to support HTTPS connections on port 4000.
📝 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.
| "endpoint": "http://openfn:4000" | |
| "endpoint": "https://openfn:4000" |
There was a problem hiding this comment.
This is configurable from the package-metadata.json
There was a problem hiding this comment.
@drizzentic, thank you for the clarification. I'll note that the endpoint URL is configurable via the package-metadata.json.
(_/)
(•_•)
/>💡
✏️ Learnings added
Learnt from: drizzentic
PR: jembi/platform#322
File: openfn/importer/workflows/example/config.json:5-5
Timestamp: 2024-10-01T09:41:30.506Z
Learning: The endpoint URL in `openfn/importer/workflows/example/config.json` is configurable via `package-metadata.json`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| { | ||
| "specPath": "project.yaml", | ||
| "statePath": "state.json", | ||
| "apiKey": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJKb2tlbiIsImlhdCI6MTY4ODAzNzE4NSwiaXNzIjoiSm9rZW4iLCJqdGkiOiIydG1ocG8zYm0xdmR0MDZvZDgwMDAwdTEiLCJuYmYiOjE2ODgwMzcxODUsInVzZXJfaWQiOiIzZjM3OGU2Yy02NjBhLTRiOTUtYWI5Ni02YmQwZGMyNjNkMzMifQ.J1FnACGpqtQbmXNvyUCwCY4mS5S6CohRU3Ey-N0prP4", | ||
| "endpoint": "http://openfn:4000" | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Additional Hardcoded API Key Found
Another instance of a hardcoded JWT token was detected in openfn/package-metadata.json. To maintain security best practices, please address this as follows:
-
Replace the hardcoded JWT token with an environment variable:
- "OPENFN_API_KEY": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9..." + "OPENFN_API_KEY": "${OPENFN_API_KEY}"
-
Ensure that the environment variable
API_KEYis securely managed and injected during the deployment process.
🔗 Analysis chain
Security and best practices improvements needed
The configuration file structure looks good, but there are some security concerns that need to be addressed:
- The
apiKeycontains a hardcoded JWT token, which is a security risk. This issue was previously flagged and still needs to be resolved. - The
endpointuses an insecure HTTP protocol.
To resolve these issues:
- Replace the hardcoded JWT token with an environment variable:
- "apiKey": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJKb2tlbiIsImlhdCI6MTY4ODAzNzE4NSwiaXNzIjoiSm9rZW4iLCJqdGkiOiIydG1ocG8zYm0xdmR0MDZvZDgwMDAwdTEiLCJuYmYiOjE2ODgwMzcxODUsInVzZXJfaWQiOiIzZjM3OGU2Yy02NjBhLTRiOTUtYWI5Ni02YmQwZGMyNjNkMzMifQ.J1FnACGpqtQbmXNvyUCwCY4mS5S6CohRU3Ey-N0prP4",
+ "apiKey": "${API_KEY}"- Use HTTPS for the endpoint (addressed in a separate comment).
To ensure that the API key is not hardcoded elsewhere in the codebase, run the following script:
Action Required: Re-run JWT Token Search with Corrected Script
Please execute the following script to verify that no hardcoded JWT tokens exist outside of openfn/importer/workflows/example/config.json:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for hardcoded JWT tokens in the codebase
# Test: Look for JWT patterns in all files. Expect: No results (excluding this config file).
rg 'eyJ[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]+\.?[A-Za-z0-9-_.+/=]*' --type-not json
Length of output: 79
Script:
#!/bin/bash
# Description: Search for hardcoded JWT tokens in the codebase, excluding config.json
rg 'eyJ[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]+\.?[A-Za-z0-9-_.+/=]*' --glob '!openfn/importer/workflows/example/config.json'
Length of output: 449
🧰 Tools
Gitleaks
4-4: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
| services: | ||
| openfn_workflow_config: | ||
| image: node:18-alpine3.20 | ||
| command: sh -c "cd /workflows && ls && npm install -g @openfn/cli && ls && openfn deploy -c config.json --no-confirm" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider splitting the command for better maintainability
The current command combines multiple operations into a single line. While this works, it can make debugging and maintenance more challenging.
Consider using a separate shell script for these operations. This would improve readability and make it easier to modify or troubleshoot individual steps. For example:
- command: sh -c "cd /workflows && ls && npm install -g @openfn/cli && ls && openfn deploy -c config.json --no-confirm"
+ command: sh /scripts/deploy-openfn.shThen create a deploy-openfn.sh script with the following content:
#!/bin/sh
set -e
cd /workflows
ls
npm install -g @openfn/cli
ls
openfn deploy -c config.json --no-confirmDon't forget to add this script to your Docker image and make it executable.
| "DATABASE_URL": "postgresql://openfn:instant101@postgres-1:5432/lightning_dev", | ||
| "DISABLE_DB_SSL": "true", | ||
| "IS_RESETTABLE_DEMO": "true", |
There was a problem hiding this comment.
Secure database configuration and credentials.
There are several security concerns in the database configuration:
- Database credentials are hardcoded, which is a security risk.
- SSL is disabled for the database connection (
DISABLE_DB_SSL: "true"), which can be a security concern in production environments. - There's a mix of general PostgreSQL settings and OpenFn-specific settings, which might lead to confusion.
Consider the following improvements:
- Use environment variables or a secure secrets management system for database credentials.
- Enable SSL for database connections in production environments.
- Separate general PostgreSQL settings from OpenFn-specific settings for clarity.
Example:
"DATABASE_URL": "${DATABASE_URL}",
"DISABLE_DB_SSL": "${DISABLE_DB_SSL:-false}",
"POSTGRES_USER": "${POSTGRES_USER}",
"POSTGRES_PASSWORD": "${POSTGRES_PASSWORD}",
"OpenFn_POSTGRESQL_USERNAME": "${OpenFn_POSTGRESQL_USERNAME}",
"OpenFn_POSTGRESQL_PASSWORD": "${OpenFn_POSTGRESQL_PASSWORD}",This approach allows for different configurations in development and production environments while keeping sensitive information secure.
Also applies to: 19-26
| "PRIMARY_ENCRYPTION_KEY": "KLu/IoZuaf+baDECd8wG4Z6auwNe6VAmwh9N8lWdJ1A=", | ||
| "SECRET_KEY_BASE": "jGDxZj2O+Qzegm5wcZ940RfWO4D6RyU8thNCr5BUpHNwa7UNV52M1/Sn+7RxiP+f", | ||
| "WORKER_RUNS_PRIVATE_KEY": "LS0tLS1CRUdJTiBQUklWQVRFIEtFWS0tLS0tCk1JSUV2Z0lCQURBTkJna3Foa2lHOXcwQkFRRUZBQVNDQktnd2dnU2tBZ0VBQW9JQkFRREVtR3drUW5pT0hqVCsKMnkyRHFvRUhyT3dLZFI2RW9RWG9DeDE4MytXZ3hNcGthTFZyOFViYVVVQWNISGgzUFp2Z2UwcEIzTWlCWWR5Kwp1ajM1am5uK2JIdk9OZGRldWxOUUdpczdrVFFHRU1nTSs0Njhldm5RS0h6R29DRUhabDlZV0s0MUd5SEZCZXppCnJiOGx2T1A1NEtSTS90aE5pVGtHaUIvTGFLMldLcTh0VmtoSHBvaFE3OGIyR21vNzNmcWtuSGZNWnc0ZE43d1MKdldOamZIN3QwSmhUdW9mTXludUxSWmdFYUhmTDlnbytzZ0thc0ZUTmVvdEZIQkYxQTJjUDJCakwzaUxad0hmdQozTzEwZzg0aGZlTzJqTWlsZlladHNDdmxDTE1EZWNrdFJGWFl6V0dWc25FcFNiOStjcWJWUXRvdEU4QklON09GClRmaEx2MG9uQWdNQkFBRUNnZ0VBV3dmZyt5RTBSVXBEYThiOVdqdzNKdUN4STE1NzFSbmliRUhKVTZzdzNyS0EKck9HM0w5WTI0cHhBdlVPSm5GMFFzbThrUVQ4RU1MU3B6RDdjdDVON2RZMngvaGY4TThhL0VSWXM4cFlYcXI5Vwpnbnh3NldGZ0R6elFHZ0RIaW0raXNudk5ucFdEbTRGVTRObG02d2g5MzVSZlA2KzVaSjJucEJpZjhFWDJLdE9rCklOSHRVbFcwNFlXeDEwS0pIWWhYNFlydXVjL3MraXBORzBCSDZEdlJaQzQxSWw0N1luaTg1OERaL0FaeVNZN1kKWTlTamNKQ0QvUHBENTlNQjlSanJDQjhweDBjWGlsVXBVZUJSYndGalVwbWZuVmhIa1hiYlM1U0hXWWM4K3pLRQp2ajFqSEpxc2UyR0hxK2lHL1V3NTZvcHNyM2x3dHBRUXpVcEJGblhMMFFLQmdRRDM5bkV3L1NNVGhCallSd1JGCkY2a2xOYmltU2RGOVozQlZleXhrT0dUeU5NSCtYckhsQjFpOXBRRHdtMit3V2RvcWg1ZFRFbEU5K1crZ0FhN0YKbXlWc2xPTW4wdnZ2cXY2Wkp5SDRtNTVKU0lWSzBzRjRQOTRMYkpNSStHUW5VNnRha3Y0V0FSMkpXaURabGxPdAp3R01EQWZqRVIrSEFZeUJDKzNDL25MNHF5d0tCZ1FESzk3NERtV0c4VDMzNHBiUFVEYnpDbG9oTlQ2UldxMXVwCmJSWng4ZGpzZU0vQ09kZnBUcmJuMnk5dVc3Q1pBNFVPQ2s4REcxZ3ZENVVDYlpEUVdMaUp5RzZGdG5OdGgvaU8KT1dJM0UyczZOS0VMMU1NVzh5QWZwNzV4Ung5cnNaQzI2UEtqQ0pWL2lTVjcyNlQ1ZTFzRG5sZUtBb0JFZnlDRgpvbEhhMmhybWxRS0JnUURHT1YyOWd1K1NmMng1SVRTWm8xT1ZxbitGZDhlZno1d3V5YnZ3Rm1Fa2V1YUdXZDh1CnJ4UFM3MkJ6K0Y1dUJUWngvMWtLa0w4Zm94TUlQN0FleW1zOWhUeWVybnkyMk9TVlBJSmN3dExqMUxTeDN3L0kKK0kyaVpsYVl1akVlZXpXbHY1S2R0cUNORjk3Zzh0ck1NTnMySVZKa1h1NXFwUk82V0ZXRzZGL2h4d0tCZ0hnNApHYUpFSFhIT204ekZTU2lYSW5FWGZKQmVWZmJIOUxqNzFrbVRlR3RJZTdhTlVHZnVxY1BYUGRiZUZGSHRsY2ZsCkx6dWwzS3V6VFExdEhGTnIyWkl5MTlQM1o1TSs4R2c5Y1FFeVRWYmlpV2xha2x0cmttRnRtQTI4bE0zVEZPWmkKUUNWMUZpZStjaWRVeC9qRnFma1F0c1VXQ2llSUxSazZOY1d0WGpXcEFvR0JBTGN6Y210VGlUUEFvWnk0MFV1QQpTOXpUd3RsamhmUWJEVTVjb21EcnlKcnFRU0VOdmQ2VW5HdW0zYVNnNk13dDc0NGxidDAyMC9mSGI0WTJkTGhMCmx4YWJ5b1dQUElRRUpLL1NNOGtURFEvYTRyME5tZzhuV3h5bGFLcHQ5WUhmZ2NYMkYzSzUrc0VSUGNFcVZlWFMKdWZkYXdYQVlFampZK3V2UHZ2YzU3RU1aCi0tLS0tRU5EIFBSSVZBVEUgS0VZLS0tLS0K", | ||
| "WORKER_SECRET": "secret_here", |
There was a problem hiding this comment.
Implement secure key management practices.
I understand from your previous comment that these are default keys. However, including default keys in the configuration file can lead to security risks if accidentally used in production.
Consider the following recommendations:
- Remove all hardcoded keys and tokens from the configuration file.
- Use environment variables or a secure vault system to manage these sensitive values.
- Provide clear documentation on how to set up these keys securely in different environments.
Example:
"PRIMARY_ENCRYPTION_KEY": "${PRIMARY_ENCRYPTION_KEY}",
"SECRET_KEY_BASE": "${SECRET_KEY_BASE}",
"WORKER_SECRET": "${WORKER_SECRET}",
"WORKER_LIGHTNING_PUBLIC_KEY": "${WORKER_LIGHTNING_PUBLIC_KEY}",
"OPENFN_API_KEY": "${OPENFN_API_KEY}"This approach ensures that sensitive information is not exposed in the configuration file while allowing for easy setup in development environments and secure usage in production.
Also applies to: 27-27, 31-31
| "LISTEN_ADDRESS": "0.0.0.0", | ||
| "LOG_LEVEL": "debug", | ||
| "ORIGINS": "http://localhost:4000", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance configuration flexibility for different environments.
The current configuration seems tailored for a development environment. To improve flexibility across different deployment scenarios:
- Consider using environment variables for settings that might change between environments:
"LISTEN_ADDRESS": "${LISTEN_ADDRESS:-0.0.0.0}",
"LOG_LEVEL": "${LOG_LEVEL:-debug}",
"ORIGINS": "${ORIGINS:-http://localhost:4000}",
"KAFKA_TRIGGERS_ENABLED": "${KAFKA_TRIGGERS_ENABLED:-true}",
"OPENFN_ENDPOINT": "${OPENFN_ENDPOINT:-http://localhost:4000}"- For the
IS_RESETTABLE_DEMOflag, consider if this should be enabled in all environments or just development:
"IS_RESETTABLE_DEMO": "${IS_RESETTABLE_DEMO:-false}"These changes will make your configuration more adaptable to different environments while maintaining sensible defaults for development.
Also applies to: 30-30, 32-32
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
openfn/importer/workflows/example/state.json (5)
1-17: Consider enhancing project metadata and reviewing retention policy.While the project configuration is well-structured, consider the following improvements:
- The project name "jembi-cdc-ahie" could be more descriptive to better reflect its purpose.
- The description "POC test project for ART patient use case" is brief. Consider expanding it to provide more context about the project's goals and scope.
- The retention policy is set to "retain_all". Review if this is appropriate for long-term data management, as it may lead to unnecessary data storage.
18-151: Enhance workflow naming and documentation.The workflow configuration is comprehensive, but consider the following improvements:
- The workflow name "1-process-cdr-bundle" uses a numeric prefix. Consider adopting a more descriptive naming convention that doesn't rely on ordering, e.g., "process-cdr-bundle".
- Some edges could benefit from more descriptive condition labels. For example, the edge "send-to-NDR->send-errors-to-kafka-topic" has a condition label "import errors", which could be more specific.
- The kafka trigger is enabled, but there's no description of its purpose or expected payload. Consider adding documentation about the trigger's role in the workflow.
86-92: Enhance "parse-bundle" job documentation and error handling.The "parse-bundle" job could benefit from the following improvements:
- Add comments explaining the purpose and expected input/output of the job.
- Implement error handling for the JSON parsing operation.
- Consider moving the complex logic into a separate, testable function.
128-134: Improve error handling in "send-errors-to-kafka-topic" job.The error handling job could be enhanced:
- Add more detailed logging of the error information.
- Implement retries for the Kafka topic posts.
- Consider adding alerting or monitoring for critical errors.
1-155: Overall, the workflow configuration is comprehensive but has room for improvement.The
state.jsonfile provides a detailed configuration for the CDR bundle processing workflow. While it's functional, consider the following overall improvements:
- Adopt more consistent and descriptive naming conventions across the project, workflow, and jobs.
- Enhance documentation throughout, especially for complex job logic and trigger configurations.
- Refactor inline JavaScript code in job bodies into separate, testable functions.
- Implement more robust error handling and logging across all jobs.
- Review and standardize the use of project credentials across jobs.
- Consider breaking down larger jobs into smaller, more focused tasks for better maintainability.
These enhancements will improve the overall quality, maintainability, and reliability of the workflow.
openfn/importer/workflows/example/project.yaml (1)
1209-1310: LGTM: Bundle building and workflow configuration look good.The "build-bundle" job effectively constructs the final FHIR bundle, and the workflow configuration properly defines the job sequence and execution conditions.
Consider adding more detailed logging in the "build-bundle" job to facilitate debugging and monitoring. For example:
console.log(`Bundle construction started. Total resources: ${Object.values(state).flat().length}`); // ... existing bundle building logic ... console.log(`Bundle construction completed. Final bundle size: ${JSON.stringify(bundle).length} bytes`);This additional logging would provide more insights into the bundle building process, which could be helpful for performance monitoring and troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- openfn/docker-compose.yml (1 hunks)
- openfn/importer/workflows/docker-compose.config.yml (1 hunks)
- openfn/importer/workflows/example/config.json (1 hunks)
- openfn/importer/workflows/example/project.yaml (1 hunks)
- openfn/importer/workflows/example/state.json (1 hunks)
- openfn/package-metadata.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- openfn/importer/workflows/docker-compose.config.yml
- openfn/importer/workflows/example/config.json
- openfn/package-metadata.json
🧰 Additional context used
🪛 yamllint
openfn/importer/workflows/example/project.yaml
[error] 1205-1205: trailing spaces
(trailing-spaces)
🔇 Additional comments (7)
openfn/docker-compose.yml (5)
1-3: LGTM: Docker Compose version and services structure.The Docker Compose file version 3.9 is appropriate for modern features, and the services structure is well-defined.
14-23: Caution: Potential exposure of sensitive information.Some environment variables (e.g.,
SECRET_KEY_BASE,PRIMARY_ENCRYPTION_KEY,WORKER_SECRET) likely contain sensitive information. Ensure these are not logged or exposed in any way.Consider using Docker secrets for sensitive information. Here's a script to check if any of these variables are currently set in the environment:
#!/bin/bash # Check for sensitive environment variables sensitive_vars=("SECRET_KEY_BASE" "PRIMARY_ENCRYPTION_KEY" "WORKER_SECRET" "WORKER_RUNS_PRIVATE_KEY") for var in "${sensitive_vars[@]}"; do if env | grep -q "^$var="; then echo "Warning: $var is set in the environment" fi done
34-48: Verify the WebSocket connection to the OpenFn service.The worker service connects to the OpenFn service using a WebSocket. Ensure that the URL and port are correctly configured and that the OpenFn service is set up to accept this connection.
#!/bin/bash # Check if the OpenFn service is exposing the expected port grep -q "LISTEN_ADDRESS=.*:4000" openfn/docker-compose.yml && echo "OpenFn service is configured to listen on port 4000" || echo "Warning: OpenFn service may not be configured to listen on port 4000" # Verify that the worker's command uses the correct WebSocket URL grep -q "ws://openfn:\${URL_PORT-4000}/worker" openfn/docker-compose.yml && echo "Worker is configured to connect to OpenFn service" || echo "Warning: Worker may not be configured to connect to OpenFn service correctly"
50-56: LGTM: Network configuration. Verify external networks.The use of external networks for Kafka and PostgreSQL is a good practice for modularity. However, ensure that these networks are properly set up in your environment.
Run the following script to verify the existence of the external networks:
#!/bin/bash # Check for the existence of the required external networks networks=("kafka_public" "postgres_public") for network in "${networks[@]}"; do if docker network ls | grep -q "$network"; then echo "Network $network exists" else echo "Warning: Network $network does not exist" fi done
25-30: Verify the health check test command.The health check test uses an environment variable
${DOCKER_WEB_HEALTHCHECK_TEST}with a fallback. Ensure this variable is properly set in your environment or consider using a fixed value for consistency.✅ Verification successful
Health check test command is correctly using the fallback value when the environment variable is not set.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if DOCKER_WEB_HEALTHCHECK_TEST is set in the environment if env | grep -q DOCKER_WEB_HEALTHCHECK_TEST; then echo "DOCKER_WEB_HEALTHCHECK_TEST is set" else echo "DOCKER_WEB_HEALTHCHECK_TEST is not set, using default value" fiLength of output: 843
Script:
#!/bin/bash # Directly check if DOCKER_WEB_HEALTHCHECK_TEST is set if [ -z "${DOCKER_WEB_HEALTHCHECK_TEST}" ]; then echo "DOCKER_WEB_HEALTHCHECK_TEST is not set" else echo "DOCKER_WEB_HEALTHCHECK_TEST is set to: ${DOCKER_WEB_HEALTHCHECK_TEST}" fiLength of output: 102
openfn/importer/workflows/example/project.yaml (2)
1-7: LGTM: Initial configuration looks good.The workflow name and description are clear, and the credential for "openhim ndr" is properly defined.
1-1310: Overall, the workflow is well-structured with room for some improvements.This YAML file defines a comprehensive workflow for processing and submitting healthcare data, particularly for ART patients. The workflow effectively handles data parsing, mapping, and submission to the NDR.
Key strengths:
- Detailed mapping of various healthcare resources to FHIR format.
- Error handling and reporting to Kafka topics.
- Well-defined job sequence and execution conditions.
Suggested improvements:
- Enhance error handling in data parsing and organization registration.
- Refactor complex mapping jobs for better readability and maintainability.
- Implement a retry mechanism for NDR submissions.
- Optimize Kafka error reporting by combining messages.
- Add more detailed logging throughout the workflow.
Implementing these suggestions would further improve the robustness, maintainability, and reliability of the workflow.
🧰 Tools
🪛 yamllint
[error] 1205-1205: trailing spaces
(trailing-spaces)
openfn/docker-compose.yml
Outdated
| command: > | ||
| sh -c "/app/bin/lightning eval 'Lightning.Release.migrate()' && /app/bin/lightning eval 'Lightning.Setup.setup_user(%{first_name: \"Derrick\",last_name: \"Rono\",email: \"[email protected]\",password: \"welcome12345\", role: :superuser}, \"apiKey\" ,[%{name: \"openhim ndr\", schema: \"http\", body: %{username: \"openfn\", password: \"PasswordHere\", baseUrl: \"http://urlHere\"}}])' && /app/bin/lightning start" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider separating the setup steps.
The current command combines database migration, user setup, and application start. Consider separating these steps for better maintainability and easier troubleshooting.
You could create separate scripts for each step and call them sequentially:
command: >
sh -c "/app/bin/migrate.sh && /app/bin/setup_user.sh && /app/bin/start_app.sh"| 1-process-cdr-bundle: | ||
| name: 1-process-cdr-bundle | ||
| jobs: | ||
| parse-bundle: | ||
| name: parse bundle | ||
| adaptor: "@openfn/[email protected]" | ||
| credential: null | ||
| body: | | ||
| // this will take an incoming bundle and sort it into different resource types | ||
| fn((state) => { | ||
| let data = state.data.body; | ||
| if (typeof data === "string") { | ||
| data = JSON.parse(data) | ||
| } | ||
|
|
||
| const next = { | ||
| entry: data.entry | ||
| } | ||
|
|
||
| console.log(`Loading ${next.entry.length} bundle items from input`) | ||
|
|
||
| return next; | ||
| }) | ||
|
|
||
| group($.entry, 'resource.resourceType') | ||
|
|
||
| fn((state) => { | ||
| const counts = Object.keys(state.data).map((k) => `${k} (${state.data[k].length})`); | ||
| console.log(`Sorted bundles into the following groups: ${counts.join('\n ')}`) | ||
| return { | ||
| inputs: state.data, | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for unexpected input formats.
The "parse-bundle" job effectively sorts incoming data into different resource types. However, it might be beneficial to add error handling for cases where the input data is not in the expected format. This could prevent potential runtime errors and improve the robustness of the workflow.
Consider adding a try-catch block around the parsing logic, for example:
fn((state) => {
try {
let data = state.data.body;
if (typeof data === "string") {
data = JSON.parse(data)
}
// ... rest of the parsing logic ...
} catch (error) {
console.error(`Error parsing input data: ${error.message}`);
return { error: 'Invalid input format' };
}
})|
|
||
| Register-Organization: | ||
| name: Register Organization | ||
| adaptor: "@openfn/[email protected]" | ||
| credential: [email protected] | ||
| body: | | ||
| //For the POC test only we need to register the Organization first | ||
| //... for each Bundle... and AFTER we can process the Bundle | ||
| fn(state => { | ||
| const orgId = | ||
| state.inputs.Patient[0].resource.managingOrganization.reference.replace( | ||
| 'Organization/', | ||
| '' | ||
| ); | ||
| console.log('Setting up test organization', orgId); | ||
|
|
||
| const mappedOrganization = { | ||
| resourceType: 'Organization', | ||
| id: orgId, | ||
| meta: { | ||
| profile: ['http://moh.gov.et/fhir/hiv/StructureDefinition/organization'], | ||
| }, | ||
| text: { | ||
| status: 'generated', | ||
| div: '<div xmlns="http://www.w3.org/1999/xhtml"><p class="res-header-id"><b>Generated Narrative: Organization CurrentServiceProviderExample</b></p><a name="CurrentServiceProviderExample"> </a><a name="hcCurrentServiceProviderExample"> </a><a name="CurrentServiceProviderExample-en-US"> </a><p><b>identifier</b>: <code>http://moh.gov.et/fhir/hiv/identifier/mohid</code>/MOH001, <code>http://moh.gov.et/fhir/hiv/identifier/hfuid</code>/HFUID001</p><p><b>active</b>: true</p><p><b>type</b>: <span title="Codes:{http://snomed.info/sct 257622000}">Current Healthcare Provider</span></p><p><b>name</b>: Meshwalkiya Health Center</p><p><b>address</b>: Meshulekia Kirkos Addis Ababa </p></div>', | ||
| }, | ||
| identifier: [ | ||
| { | ||
| system: 'http://moh.gov.et/fhir/hiv/identifier/mohid', | ||
| value: 'MOH001', | ||
| }, | ||
| { | ||
| system: 'http://moh.gov.et/fhir/hiv/identifier/hfuid', | ||
| value: 'HFUID001', | ||
| }, | ||
| ], | ||
| active: true, | ||
| type: [ | ||
| { | ||
| coding: [ | ||
| { | ||
| system: 'http://snomed.info/sct', | ||
| code: '257622000', | ||
| }, | ||
| ], | ||
| text: 'Current Healthcare Provider', | ||
| }, | ||
| ], | ||
| name: `Organization ${orgId}`, | ||
| address: [ | ||
| { | ||
| line: ['Meshulekia'], | ||
| city: 'Kirkos', | ||
| district: 'Kirkos woreda 9', | ||
| state: 'Addis Ababa', | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| return { ...state, mappedOrganization, orgId }; | ||
| }); | ||
|
|
||
| // basic HTTP put should do it | ||
| put(`/fhir/Organization/${$.orgId}`, { | ||
| body: $.mappedOrganization, | ||
| headers: { 'content-type': 'application/json' }, | ||
| }); | ||
|
|
||
| fn(state => { | ||
| console.log('Organization registered!'); | ||
| return state; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a more robust approach for organization registration.
The current implementation registers an organization based on the managing organization of the first patient in the bundle. This approach might not be representative of all patients, especially if the bundle contains patients from different organizations.
Consider the following improvements:
- Iterate through all patients in the bundle to identify unique organizations.
- Register each unique organization separately.
- Handle potential conflicts if multiple patients have different information for the same organization.
Example pseudocode:
const uniqueOrgs = new Set();
state.inputs.Patient.forEach(patient => {
const orgId = patient.resource.managingOrganization.reference.replace('Organization/', '');
uniqueOrgs.add(orgId);
});
for (const orgId of uniqueOrgs) {
// Register organization logic here
}| send-to-NDR: | ||
| name: send to NDR | ||
| adaptor: "@openfn/[email protected]" | ||
| credential: [email protected] | ||
| body: | | ||
| fn(state => { | ||
| console.log('Sending final bundle to NDR'); | ||
| return state; | ||
| }); | ||
|
|
||
| post('/fhir', { | ||
| body: $.bundle, | ||
| headers: { 'content-type': 'application/json' }, | ||
| parseAs: 'json', | ||
| }).catch(async (response, state) => { | ||
| const err = JSON.parse(response.body) | ||
| console.error('Error from NDR FHIR') | ||
| console.error(err) | ||
| state.issue = err.issue; | ||
| return state | ||
| }) | ||
|
|
||
| fn(state => { | ||
| console.log('Done!'); | ||
| return { | ||
| issue: state.issue, | ||
| bundle: state.bundle, | ||
| data: state.data, | ||
| response: state.response, | ||
| }; | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a retry mechanism for failed NDR submissions.
The "send-to-NDR" job effectively sends the final processed bundle to the NDR and includes error handling. However, adding a retry mechanism could improve the reliability of the data submission process, especially in cases of temporary network issues.
Consider implementing a retry mechanism with exponential backoff. For example:
const axios = require('axios');
const axiosRetry = require('axios-retry');
axiosRetry(axios, {
retries: 3,
retryDelay: axiosRetry.exponentialDelay,
retryCondition: (error) => {
return axiosRetry.isNetworkOrIdempotentRequestError(error) || error.response.status === 500;
},
});
post('/fhir', {
body: $.bundle,
headers: { 'content-type': 'application/json' },
parseAs: 'json',
}).catch(async (response, state) => {
// ... existing error handling ...
});This approach would attempt to retry the request up to 3 times with increasing delays between attempts, improving the chances of successful submission in case of temporary issues.
| send-errors-to-kafka-topic: | ||
| name: send errors to kafka topic | ||
| adaptor: "@openfn/[email protected]" | ||
| credential: [email protected] | ||
| body: | | ||
| fn(state => { | ||
| console.log('Catching errors to send to kafka topic fhir-errors...'); | ||
| console.log(state.issue) | ||
| return state; | ||
| }); | ||
|
|
||
| //send error message from FHIR API | ||
| post('/openfn/fhir-errors', { | ||
| body: JSON.stringify($.issue), | ||
| headers: { 'content-type': 'application/json' }, | ||
| }); | ||
|
|
||
| fn(state => { | ||
| console.log('Sending failed bundle to kafka topic dead-queue...'); | ||
| return state; | ||
| }); | ||
|
|
||
| //send final Bundle we mapped | ||
| post('/openfn/dead-queue', { | ||
| body: $.bundle, | ||
| headers: { 'content-type': 'application/json' }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove trailing space and consider combining Kafka messages.
The error handling and reporting to Kafka topics are implemented well. However, there's a minor formatting issue and a potential optimization:
- Remove the trailing space on line 1205 to address the yamllint warning.
- Consider combining the error message and failed bundle into a single Kafka message to reduce the number of API calls and ensure atomicity of the error reporting.
Fix the trailing space issue:
- body: $.bundle,
+ body: $.bundle,Consider combining the Kafka messages:
post('/openfn/error-reporting', {
body: JSON.stringify({
error: $.issue,
failedBundle: $.bundle
}),
headers: { 'content-type': 'application/json' },
});This approach would send both the error and the failed bundle in a single request, potentially improving performance and ensuring that both pieces of information are always sent together.
🧰 Tools
🪛 yamllint
[error] 1205-1205: trailing spaces
(trailing-spaces)
This project's new adaptor has a new release and is under a different adaptor name ("fhir-ndr-et")
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (7)
documentation/packages/openfn/README.md (4)
7-11: Consider expanding the Usage sectionWhile the provided command is clear, consider adding more context to enhance user understanding:
- Briefly explain what the
instantCLI tool is and how to install it if not already available.- Clarify what the
--devflag does in this context.- Provide a brief explanation of what happens after running this command.
These additions would help users, especially those new to the package or the
instanttool, to get started more easily.
29-36: Enhance Kafka trigger configuration instructionsThe Kafka trigger configuration instructions are helpful, but they could be improved for clarity and completeness.
Consider the following enhancements:
- Use a numbered list for the configuration steps to make them easier to follow.
- Provide more context for the configuration details (e.g., explain what CDR and NDR stand for).
- Consider using a table or bullet points for the configuration parameters (topic, hosts, etc.) to improve readability.
- Expand on the warning about disabling the trigger, explaining the potential consequences of not doing so.
38-40: Enhance the Documentation sectionWhile the link to the official documentation is helpful, this section could be expanded to provide more value to users.
Consider the following additions:
- Briefly list some of the key topics covered in the official documentation to give users an idea of what they can expect to find.
- If applicable, mention any other resources available (e.g., API reference, tutorials, community forums).
- Encourage users to contribute to the documentation if it's open-source.
24-27: Specify language for fenced code blockTo improve the formatting and potential syntax highlighting of the README, specify a language for the fenced code block containing the demo credentials.
Update the fenced code block as follows:
-``` +```plaintext username: [email protected] password: instant101<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 24-24: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> </blockquote></details> <details> <summary>openfn/importer/workflows/example/state.json (3)</summary><blockquote> `15-17`: **Consider reviewing the data retention policy.** The current retention policy is set to "retain_all". While this ensures no data loss, it may have implications for storage costs and data privacy compliance (e.g., GDPR). Consider implementing a more granular retention policy based on data criticality and regulatory requirements. --- `58-66`: **Document the error handling condition.** The error handling edge uses a JavaScript expression condition (`state.issue`). While this is likely checking for the presence of an error, it would be beneficial to add a comment explaining the specific conditions under which errors are sent to the Kafka topic. This will improve maintainability and make it easier for other developers to understand the error handling logic. --- `154-155`: **Consider enabling Multi-Factor Authentication (MFA).** Given the sensitive nature of healthcare data being processed in this workflow, it's recommended to enable MFA for additional security. This can help prevent unauthorized access and comply with healthcare data protection regulations. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Files that changed from the base of the PR and between 97daf986687866c791fbcde9e4c291c95f12eed3 and 2490ccd4eb42f7d0b22810811d05799bb04b588c. </details> <details> <summary>📒 Files selected for processing (5)</summary> * documentation/packages/openfn/README.md (1 hunks) * openfn/docker-compose.yml (1 hunks) * openfn/importer/workflows/example/project.yaml (1 hunks) * openfn/importer/workflows/example/state.json (1 hunks) * openfn/package-metadata.json (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * openfn/docker-compose.yml * openfn/package-metadata.json </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> <details> <summary>documentation/packages/openfn/README.md</summary><blockquote> 24-24: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </blockquote></details> <details> <summary>🪛 yamllint</summary><blockquote> <details> <summary>openfn/importer/workflows/example/project.yaml</summary><blockquote> [error] 1205-1205: trailing spaces (trailing-spaces) </blockquote></details> </blockquote></details> </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>documentation/packages/openfn/README.md (1)</summary><blockquote> `1-5`: **LGTM: Clear and concise introduction** The introduction effectively communicates the purpose of the `openfn` package and its target audience. It provides a good overview for users new to the package. </blockquote></details> <details> <summary>openfn/importer/workflows/example/project.yaml (4)</summary><blockquote> `1-11`: **LGTM: Well-structured workflow configuration.** The initial configuration for the "jembi-cdc-ahie" workflow is well-organized and clearly defined. It includes appropriate credential setup and a logical job structure for processing ART patient data. --- `1209-1262`: **LGTM: Well-implemented bundle building process.** The "build-bundle" job effectively constructs the final FHIR bundle from the processed resources. It includes proper handling of resource creation vs. update, and the logging of bundle statistics is helpful for monitoring. The implementation is clean and efficient. --- `1264-1267`: **LGTM: Appropriate trigger configuration.** The Kafka webhook trigger is properly defined and enabled, providing a suitable entry point for the workflow. --- `1268-1310`: **LGTM: Well-defined workflow sequence.** The edges section effectively defines the workflow sequence with appropriate conditions for job execution. The use of a custom JS expression for error handling adds flexibility to the workflow, allowing for proper routing of errors to the Kafka topic. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
|
||
| To access the demo, follow these steps: | ||
|
|
||
| 1. Visit the [OpenFn Demo](http://localhost:4000) website. |
There was a problem hiding this comment.
Update the demo URL
The current demo URL (http://localhost:4000) appears to be a local development URL. For the public documentation, this should be updated to the actual public demo URL.
Please replace http://localhost:4000 with the correct public demo URL.
| ``` | ||
| username: [email protected] | ||
| password: instant101 | ||
| ``` |
There was a problem hiding this comment.
Security concern: Exposed credentials
Exposing login credentials in public documentation is a security risk, even if they are for a demo account.
Consider one of the following alternatives:
- Use placeholder text (e.g.,
<your_username>and<your_password>) and provide the actual credentials through a secure channel. - Implement a system to generate temporary demo credentials on demand.
- If these are truly public demo credentials, add a clear note stating that these are shared demo credentials and should not be used for sensitive information.
🧰 Tools
🪛 Markdownlint
24-24: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
| "Register-Organization": { | ||
| "id": "d9f273dc-5233-403a-afaa-c923e4c1d647", | ||
| "name": "Register Organization", | ||
| "body": "//For the POC test only we need to register the Organization first\n//... for each Bundle... and AFTER we can process the Bundle\nfn(state => {\n const orgId =\n state.inputs.Patient[0].resource.managingOrganization.reference.replace(\n 'Organization/',\n ''\n );\n console.log('Setting up test organization', orgId);\n\n const mappedOrganization = {\n resourceType: 'Organization',\n id: orgId,\n meta: {\n profile: ['http://moh.gov.et/fhir/hiv/StructureDefinition/organization'],\n },\n text: {\n status: 'generated',\n div: '<div xmlns=\"http://www.w3.org/1999/xhtml\"><p class=\"res-header-id\"><b>Generated Narrative: Organization CurrentServiceProviderExample</b></p><a name=\"CurrentServiceProviderExample\"> </a><a name=\"hcCurrentServiceProviderExample\"> </a><a name=\"CurrentServiceProviderExample-en-US\"> </a><p><b>identifier</b>: <code>http://moh.gov.et/fhir/hiv/identifier/mohid</code>/MOH001, <code>http://moh.gov.et/fhir/hiv/identifier/hfuid</code>/HFUID001</p><p><b>active</b>: true</p><p><b>type</b>: <span title=\"Codes:{http://snomed.info/sct 257622000}\">Current Healthcare Provider</span></p><p><b>name</b>: Meshwalkiya Health Center</p><p><b>address</b>: Meshulekia Kirkos Addis Ababa </p></div>',\n },\n identifier: [\n {\n system: 'http://moh.gov.et/fhir/hiv/identifier/mohid',\n value: 'MOH001',\n },\n {\n system: 'http://moh.gov.et/fhir/hiv/identifier/hfuid',\n value: 'HFUID001',\n },\n ],\n active: true,\n type: [\n {\n coding: [\n {\n system: 'http://snomed.info/sct',\n code: '257622000',\n },\n ],\n text: 'Current Healthcare Provider',\n },\n ],\n name: `Organization ${orgId}`,\n address: [\n {\n line: ['Meshulekia'],\n city: 'Kirkos',\n district: 'Kirkos woreda 9',\n state: 'Addis Ababa',\n },\n ],\n };\n\n return { ...state, mappedOrganization, orgId };\n});\n\n// basic HTTP put should do it\nput(`/fhir/Organization/${$.orgId}`, {\n body: $.mappedOrganization,\n headers: { 'content-type': 'application/json' },\n});\n\nfn(state => {\n console.log('Organization registered!');\n return state;\n});\n", | ||
| "adaptor": "@openfn/[email protected]", | ||
| "project_credential_id": "9ef53b55-f50b-4b3e-a610-5dd71ecf441d" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor "Register-Organization" job for better maintainability.
The "Register-Organization" job contains a large amount of inline JavaScript code. Consider:
- Moving the organization mapping logic to a separate, testable function.
- Using configuration files for the hardcoded values (e.g., URLs, codes).
- Implementing error handling for the HTTP PUT request.
| "build-bundle": { | ||
| "id": "db6a52fc-caac-4607-81df-caa5a28ffd1c", | ||
| "name": "build bundle", | ||
| "body": "const wrapResource = (res) => {\n const { __new__, ...resource } = res;\n return {\n request: {\n method: __new__ ? \"POST\" : \"PUT\",\n url: __new__ ? res.resourceType : `${res.resourceType}/${res.id}`\n },\n resource,\n }\n}\n\nfn((state) => {\n console.log('Building final bundle...')\n\n delete state.inputs\n\n const bundle = {\n \"resourceType\": \"Bundle\",\n \"type\": \"transaction\",\n \"entry\": []\n };\n\n // Write all data onto state\n for (const k in state) {\n const arr = state[k]\n if (Array.isArray(arr)) {\n bundle.entry.push(...arr.filter(a => a.resourceType).map(wrapResource))\n }\n }\n\n console.log(`Bundle built with ${bundle.entry.length} items`)\n\n const counts = {}\n for (const res of bundle.entry ) {\n try {\n const type = res.resource.meta.profile[0].split('StructureDefinition/')[1]\n counts[type] ??= 0\n counts[type] += 1\n } catch(e) {\n console.log(res)\n throw e\n }\n }\n\n console.log(counts)\n\n return { bundle };\n\n})\n", | ||
| "adaptor": "@openfn/[email protected]", | ||
| "project_credential_id": null | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize "build-bundle" job for performance and readability.
The "build-bundle" job could be improved:
- Consider using a more efficient data structure for counting resource types.
- Add comments explaining the purpose of the
wrapResourcefunction. - Implement error handling for cases where
meta.profilemight be undefined.
| Register-Organization: | ||
| name: Register Organization | ||
| adaptor: "@openfn/[email protected]" | ||
| credential: [email protected] | ||
| body: | | ||
| //For the POC test only we need to register the Organization first | ||
| //... for each Bundle... and AFTER we can process the Bundle | ||
| fn(state => { | ||
| const orgId = | ||
| state.inputs.Patient[0].resource.managingOrganization.reference.replace( | ||
| 'Organization/', | ||
| '' | ||
| ); | ||
| console.log('Setting up test organization', orgId); | ||
|
|
||
| const mappedOrganization = { | ||
| resourceType: 'Organization', | ||
| id: orgId, | ||
| meta: { | ||
| profile: ['http://moh.gov.et/fhir/hiv/StructureDefinition/organization'], | ||
| }, | ||
| text: { | ||
| status: 'generated', | ||
| div: '<div xmlns="http://www.w3.org/1999/xhtml"><p class="res-header-id"><b>Generated Narrative: Organization CurrentServiceProviderExample</b></p><a name="CurrentServiceProviderExample"> </a><a name="hcCurrentServiceProviderExample"> </a><a name="CurrentServiceProviderExample-en-US"> </a><p><b>identifier</b>: <code>http://moh.gov.et/fhir/hiv/identifier/mohid</code>/MOH001, <code>http://moh.gov.et/fhir/hiv/identifier/hfuid</code>/HFUID001</p><p><b>active</b>: true</p><p><b>type</b>: <span title="Codes:{http://snomed.info/sct 257622000}">Current Healthcare Provider</span></p><p><b>name</b>: Meshwalkiya Health Center</p><p><b>address</b>: Meshulekia Kirkos Addis Ababa </p></div>', | ||
| }, | ||
| identifier: [ | ||
| { | ||
| system: 'http://moh.gov.et/fhir/hiv/identifier/mohid', | ||
| value: 'MOH001', | ||
| }, | ||
| { | ||
| system: 'http://moh.gov.et/fhir/hiv/identifier/hfuid', | ||
| value: 'HFUID001', | ||
| }, | ||
| ], | ||
| active: true, | ||
| type: [ | ||
| { | ||
| coding: [ | ||
| { | ||
| system: 'http://snomed.info/sct', | ||
| code: '257622000', | ||
| }, | ||
| ], | ||
| text: 'Current Healthcare Provider', | ||
| }, | ||
| ], | ||
| name: `Organization ${orgId}`, | ||
| address: [ | ||
| { | ||
| line: ['Meshulekia'], | ||
| city: 'Kirkos', | ||
| district: 'Kirkos woreda 9', | ||
| state: 'Addis Ababa', | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| return { ...state, mappedOrganization, orgId }; | ||
| }); | ||
|
|
||
| // basic HTTP put should do it | ||
| put(`/fhir/Organization/${$.orgId}`, { | ||
| body: $.mappedOrganization, | ||
| headers: { 'content-type': 'application/json' }, | ||
| }); | ||
|
|
||
| fn(state => { | ||
| console.log('Organization registered!'); | ||
| return state; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a more robust approach for organization registration.
The current implementation registers an organization based on the managing organization of the first patient in the bundle. This approach might not be representative of all patients, especially if the bundle contains patients from different organizations.
Consider the following improvements:
- Iterate through all patients in the bundle to identify unique organizations.
- Register each unique organization separately.
- Handle potential conflicts if multiple patients have different information for the same organization.
Example pseudocode:
const uniqueOrgs = new Set();
state.inputs.Patient.forEach(patient => {
const orgId = patient.resource.managingOrganization.reference.replace('Organization/', '');
uniqueOrgs.add(orgId);
});
for (const orgId of uniqueOrgs) {
// Register organization logic here
}| util.concept([ | ||
| reason, | ||
| 'http://moh.gov.et/fhir/hiv/CodeSystem/cd4-vl-classification-for-treatment-failure-code-system', | ||
| ]) | ||
| ); | ||
|
|
||
| changeReasonObs = b.observation('arv-regimen-change-reason-observation', { | ||
| id: createId(input.id, 'ARVRegimenChangeReason'), | ||
| status: 'final', | ||
| subject: input.subject, | ||
| encounter, | ||
| effective: encounter?.period?.start, | ||
| performer: findServiceProvider(encounter, patient), | ||
| value, | ||
| //hasMember: changeObs, //reference moved to Obs arv-regimen-changed-observation | ||
| }); | ||
| markNew(changeReasonObs); | ||
| state.observations.push(changeReasonObs); | ||
| } | ||
|
|
||
| if (changeObs) { | ||
| changeObs.hasMember = [changeTypeObs, changeReasonObs] | ||
| .filter(Boolean) | ||
| .map(util.ref); | ||
| } | ||
|
|
||
| return result; | ||
| }; | ||
|
|
||
| each($.inputs.MedicationDispense, state => { | ||
| const input = state.data.resource; | ||
|
|
||
| console.log(`Processing medication dispense ${input.id}`); | ||
|
|
||
| let encounter; | ||
| if (input.context) { | ||
| // Extract the actual ID out of the reference | ||
| const [_, encounterId] = input.context.reference.split('Encounter/'); | ||
| encounter = state.inputs.Encounter.find( | ||
| e => e.resource.id === encounterId | ||
| )?.resource; | ||
| if (encounter) { | ||
| console.log(` Found encounter ${input.context.reference} in bundle`); | ||
| } else { | ||
| console.log( | ||
| 'WARNING: COULD NOT FIND MATCHING ENCOUNTER FOR ', | ||
| input.context | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const [_, patientId] = input.subject.reference.split('Patient/'); | ||
| const patient = state.inputs.Patient.find( | ||
| e => e.resource.id === patientId | ||
| )?.resource; | ||
|
|
||
| const dispense = mapDispense(state, input, encounter); | ||
|
|
||
| state.medicationDispense.push(dispense); | ||
|
|
||
| const med = extractMedication(state, input); | ||
| dispense.medicationReference = util.reference(med); | ||
| state.medication.push(med); | ||
|
|
||
| const plan = extractCareplan(state, input, encounter, patient); | ||
| state.careplans.push(plan); | ||
|
|
||
| const o = extractObservations(state, input, encounter, patient); | ||
|
|
||
| const request = extractRequest( | ||
| state, | ||
| input, | ||
| plan, | ||
| med, | ||
| dispense, | ||
| encounter, | ||
| o.followUpStatus | ||
| ); | ||
| dispense.authorizingPrescription = [util.ref(request)]; | ||
| state.requests.push(request); | ||
|
|
||
| // care plan and request have circular dependencies, so sort out plan.activity here | ||
| plan.activity = [{ reference: util.reference(request) }]; | ||
| extractCareplanExtensions(state, input, plan, encounter); | ||
|
|
||
| const admin = extractAdmin(state, input, encounter, med, request); | ||
| if (admin) { | ||
| state.admin.push(admin); | ||
| } | ||
|
|
||
| return state; | ||
| }); | ||
|
|
||
| fn(state => { | ||
| console.log( | ||
| `Finished parsing medications! Created ${state.medicationDispense.length} Patients` | ||
| ); | ||
|
|
||
| return state; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring for improved readability and maintainability.
The mapping jobs for encounters, patients, and medications are comprehensive and handle various scenarios. However, the code complexity could be reduced to improve readability and maintainability.
Consider the following improvements:
- Break down large functions into smaller, more focused functions with clear responsibilities.
- Add more inline comments explaining complex logic or business rules.
- Consider creating separate modules for different resource types (e.g., encounters.js, patients.js, medications.js) to improve code organization.
- Use consistent naming conventions for variables and functions across all mapping jobs.
- Consider using TypeScript to add type safety and improve code documentation.
Example of breaking down a function:
const mapMedication = (input) => {
const medication = extractMedicationDetails(input);
const dispense = createMedicationDispense(input, medication);
const request = createMedicationRequest(input, medication, dispense);
const observations = createRelatedObservations(input, medication);
return { medication, dispense, request, observations };
};
const extractMedicationDetails = (input) => {
// Logic to extract medication details
};
const createMedicationDispense = (input, medication) => {
// Logic to create medication dispense
};
// ... other helper functions ...This approach would make the code more modular and easier to maintain.
upgrade adaptor version project.yaml
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (1)
openfn/importer/workflows/example/project.yaml (1)
1-1310: Summary of review for openfn/importer/workflows/example/project.yamlThis workflow for processing ART patient data is comprehensive and well-structured. However, there are several areas where improvements can be made:
Code Organization: Many jobs, particularly "map-patients" and "map-medications", are overly complex and could benefit from being broken down into smaller, more manageable functions or separate jobs.
Error Handling: While some error handling is present, it could be more robust and consistent across all jobs. Implementing a global error handling strategy would improve the workflow's resilience.
Logging: More detailed and structured logging throughout the workflow would enhance observability and make debugging easier.
Performance: Some jobs, like "build-bundle", could benefit from performance optimizations.
Configurability: Consider making the workflow more configurable by moving hardcoded values to configuration files where appropriate.
Testing: While not directly addressed in the review, implementing unit tests for individual mapping functions and integration tests for the workflow would greatly improve reliability.
Addressing these points will result in a more maintainable, robust, and efficient workflow. Despite these areas for improvement, the overall structure and approach of the workflow are sound and provide a good foundation for processing ART patient data.
🧰 Tools
🪛 yamllint
[error] 1205-1205: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- openfn/importer/workflows/example/project.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
openfn/importer/workflows/example/project.yaml
[error] 1205-1205: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
openfn/importer/workflows/example/project.yaml (1)
1-11: Workflow setup looks good!The initial configuration for the "jembi-cdc-ahie" workflow is well-structured. It clearly defines the purpose (ART patient use case), includes necessary credentials, and sets up the first job "parse-bundle".
| name: parse bundle | ||
| adaptor: "@openfn/[email protected]" | ||
| credential: null | ||
| body: | | ||
| // this will take an incoming bundle and sort it into different resource types | ||
| fn((state) => { | ||
| let data = state.data.body; | ||
| if (typeof data === "string") { | ||
| data = JSON.parse(data) | ||
| } | ||
|
|
||
| const next = { | ||
| entry: data.entry | ||
| } | ||
|
|
||
| console.log(`Loading ${next.entry.length} bundle items from input`) | ||
|
|
||
| return next; | ||
| }) | ||
|
|
||
| group($.entry, 'resource.resourceType') | ||
|
|
||
| fn((state) => { | ||
| const counts = Object.keys(state.data).map((k) => `${k} (${state.data[k].length})`); | ||
| console.log(`Sorted bundles into the following groups: ${counts.join('\n ')}`) | ||
| return { | ||
| inputs: state.data, | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling and logging in the parse-bundle job.
While the current implementation is functional, we can improve its robustness and readability:
- Add more specific error handling for JSON parsing.
- Implement structured logging for better traceability.
- Consider adding input validation to ensure the expected data structure.
Here's a suggested improvement for the parsing logic:
fn((state) => {
console.log('Starting parse-bundle job');
let data = state.data.body;
try {
if (typeof data === "string") {
data = JSON.parse(data);
}
if (!data.entry || !Array.isArray(data.entry)) {
throw new Error('Invalid data structure: missing or invalid entry array');
}
const next = { entry: data.entry };
console.log(`Successfully loaded ${next.entry.length} bundle items from input`);
return next;
} catch (error) {
console.error(`Error parsing input data: ${error.message}`);
throw error; // Re-throw to halt the workflow if parsing fails
}
})
// ... rest of the function remains the sameThis refactored version includes better error handling, input validation, and more informative logging.
| Register-Organization: | ||
| name: Register Organization | ||
| adaptor: "@openfn/[email protected]" | ||
| credential: [email protected] | ||
| body: | | ||
| //For the POC test only we need to register the Organization first | ||
| //... for each Bundle... and AFTER we can process the Bundle | ||
| fn(state => { | ||
| const orgId = | ||
| state.inputs.Patient[0].resource.managingOrganization.reference.replace( | ||
| 'Organization/', | ||
| '' | ||
| ); | ||
| console.log('Setting up test organization', orgId); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve organization registration logic
The current implementation registers an organization based on the first patient's managing organization. This approach might not be suitable for all cases, especially if the bundle contains patients from different organizations.
Consider implementing a more robust approach:
- Iterate through all patients to identify unique organizations.
- Register each unique organization separately.
- Handle potential conflicts if multiple patients have different information for the same organization.
Example pseudocode:
const uniqueOrgs = new Set();
state.inputs.Patient.forEach(patient => {
const orgId = patient.resource.managingOrganization.reference.replace('Organization/', '');
uniqueOrgs.add(orgId);
});
for (const orgId of uniqueOrgs) {
// Register organization logic here
}| const mappedOrganization = { | ||
| resourceType: 'Organization', | ||
| id: orgId, | ||
| meta: { | ||
| profile: ['http://moh.gov.et/fhir/hiv/StructureDefinition/organization'], | ||
| }, | ||
| text: { | ||
| status: 'generated', | ||
| div: '<div xmlns="http://www.w3.org/1999/xhtml"><p class="res-header-id"><b>Generated Narrative: Organization CurrentServiceProviderExample</b></p><a name="CurrentServiceProviderExample"> </a><a name="hcCurrentServiceProviderExample"> </a><a name="CurrentServiceProviderExample-en-US"> </a><p><b>identifier</b>: <code>http://moh.gov.et/fhir/hiv/identifier/mohid</code>/MOH001, <code>http://moh.gov.et/fhir/hiv/identifier/hfuid</code>/HFUID001</p><p><b>active</b>: true</p><p><b>type</b>: <span title="Codes:{http://snomed.info/sct 257622000}">Current Healthcare Provider</span></p><p><b>name</b>: Meshwalkiya Health Center</p><p><b>address</b>: Meshulekia Kirkos Addis Ababa </p></div>', | ||
| }, | ||
| identifier: [ | ||
| { | ||
| system: 'http://moh.gov.et/fhir/hiv/identifier/mohid', | ||
| value: 'MOH001', | ||
| }, | ||
| { | ||
| system: 'http://moh.gov.et/fhir/hiv/identifier/hfuid', | ||
| value: 'HFUID001', | ||
| }, | ||
| ], | ||
| active: true, | ||
| type: [ | ||
| { | ||
| coding: [ | ||
| { | ||
| system: 'http://snomed.info/sct', | ||
| code: '257622000', | ||
| }, | ||
| ], | ||
| text: 'Current Healthcare Provider', | ||
| }, | ||
| ], | ||
| name: `Organization ${orgId}`, | ||
| address: [ | ||
| { | ||
| line: ['Meshulekia'], | ||
| city: 'Kirkos', | ||
| district: 'Kirkos woreda 9', | ||
| state: 'Addis Ababa', | ||
| }, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding organization details
The current implementation uses hardcoded values for most of the organization details. This may not be flexible enough for different scenarios.
Consider the following improvements:
- Fetch organization details from a configuration file or database.
- Use more dynamic values based on the actual data in the bundle.
- Implement a fallback mechanism for missing data.
Example:
const getOrganizationDetails = (orgId) => {
// Fetch from config or database
// Return default values if not found
};
const mappedOrganization = {
resourceType: 'Organization',
id: orgId,
// ... other fields
name: getOrganizationDetails(orgId).name || `Organization ${orgId}`,
address: getOrganizationDetails(orgId).address || [],
};| put(`/fhir/Organization/${$.orgId}`, { | ||
| body: $.mappedOrganization, | ||
| headers: { 'content-type': 'application/json' }, | ||
| }); | ||
|
|
||
| fn(state => { | ||
| console.log('Organization registered!'); | ||
| return state; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the HTTP request
The current implementation doesn't handle potential errors from the HTTP PUT request.
Implement error handling for the HTTP request:
put(`/fhir/Organization/${$.orgId}`, {
body: $.mappedOrganization,
headers: { 'content-type': 'application/json' },
}).then(() => {
console.log('Organization registered successfully!');
}).catch(error => {
console.error('Error registering organization:', error);
throw error; // Re-throw to halt the workflow if registration fails
});| map-encounters: | ||
| name: map encounters | ||
| adaptor: "@openfn/[email protected]" | ||
| credential: null | ||
| body: | | ||
| fn(state => { | ||
| console.log('Parsing encounters...'); | ||
| state.encounters = []; | ||
| return state; | ||
| }); | ||
|
|
||
| each($.inputs.Encounter, state => { | ||
| const input = state.data.resource; | ||
| console.log('Mapping encounter ', input.id); | ||
|
|
||
| util.setSystemMap({ | ||
| 'http://cdr.aacahb.gov.et/Encounter': | ||
| 'http://moh.gov.et/fhir/hiv/identifier/encounter', | ||
| }); | ||
|
|
||
| const visitType = util.findExtension( | ||
| input, | ||
| 'http://cdr.aacahb.gov.et/visit-type' | ||
| ); | ||
|
|
||
| const [_, patientId] = input.subject.reference.split('Patient/'); | ||
| const patient = state.inputs.Patient.find( | ||
| e => e.resource.id === patientId | ||
| )?.resource; | ||
| if (!patient) { | ||
| console.log( | ||
| `WARNING: COULD NOT FIND PATIENT ${patientId ?? input.subject} IN BUNDLE` | ||
| ); | ||
| } | ||
|
|
||
| // TODO: Future phase work out whether this is a target-facility-encounter or a entry-from-outside-target-facility-encounter | ||
| if (visitType) { | ||
| const serviceType = input.serviceType?.coding.find( | ||
| item => | ||
| item.system === 'http://terminology.hl7.org/CodeSystem/service-type' | ||
| ); | ||
|
|
||
| const result = builders.encounter('target-facility-encounter', { | ||
| id: input.id, | ||
| status: input.status, | ||
|
|
||
| class: | ||
| input.class ?? | ||
| util.coding( | ||
| 'Absent-or-unknown', | ||
| 'http://moh.gov.et/fhir/hiv/CodeSystem/absent-unknown-code-system' | ||
| ), | ||
| identifier: input.identifier, | ||
| serviceType: serviceType && { | ||
| coding: serviceType, | ||
| }, | ||
| period: input.period, | ||
| subject: input.subject, | ||
| serviceProvider: patient.managingOrganization, | ||
| type: input.type, | ||
| }); | ||
|
|
||
| // Handle the visit type extension manually | ||
| util.addExtension( | ||
| result.type[0], | ||
| 'http://moh.gov.et/fhir/hiv/StructureDefinition/encounter-visit-type', | ||
| util.concept([ | ||
| visitType.valueString.toLowerCase(), | ||
| 'http://moh.gov.et/fhir/hiv/CodeSystem/encounter-visit-type-code-system', | ||
| ]) | ||
| ), | ||
| state.encounters.push(result); | ||
| } else { | ||
| console.log('WARNING: no visit type found for Encounter ', input.id); | ||
| } | ||
|
|
||
| return state; | ||
| }); | ||
|
|
||
| fn(state => { | ||
| console.log( | ||
| `Finished parsing Encounters! Created ${state.encounters.length} Encounters` | ||
| ); | ||
|
|
||
| return state; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance encounter mapping with improved organization and error handling
The current implementation of the "map-encounters" job is comprehensive, but we can improve its organization and error handling:
- Consider breaking down the large
eachfunction into smaller, more focused functions. - Implement more robust error handling, especially for cases where required data might be missing.
- Add more detailed logging to aid in debugging and monitoring.
Here's a suggested refactoring approach:
const mapEncounter = (input, patient, state) => {
// ... existing mapping logic ...
};
const handleMissingPatient = (patientId, input) => {
console.error(`WARNING: COULD NOT FIND PATIENT ${patientId ?? input.subject} IN BUNDLE`);
// Consider throwing an error or returning a special value to indicate the problem
};
each($.inputs.Encounter, state => {
const input = state.data.resource;
console.log('Mapping encounter ', input.id);
try {
const [_, patientId] = input.subject.reference.split('Patient/');
const patient = state.inputs.Patient.find(e => e.resource.id === patientId)?.resource;
if (!patient) {
handleMissingPatient(patientId, input);
return state; // Skip this encounter
}
const mappedEncounter = mapEncounter(input, patient, state);
state.encounters.push(mappedEncounter);
console.log(`Successfully mapped encounter ${input.id}`);
} catch (error) {
console.error(`Error mapping encounter ${input.id}:`, error);
// Consider how to handle this error (skip, throw, etc.)
}
return state;
});This refactored version improves code organization, error handling, and logging, making the job more robust and easier to maintain.
| util.concept([ | ||
| reason, | ||
| 'http://moh.gov.et/fhir/hiv/CodeSystem/cd4-vl-classification-for-treatment-failure-code-system', | ||
| ]) | ||
| ); | ||
|
|
||
| changeReasonObs = b.observation('arv-regimen-change-reason-observation', { | ||
| id: createId(input.id, 'ARVRegimenChangeReason'), | ||
| status: 'final', | ||
| subject: input.subject, | ||
| encounter, | ||
| effective: encounter?.period?.start, | ||
| performer: findServiceProvider(encounter, patient), | ||
| value, | ||
| //hasMember: changeObs, //reference moved to Obs arv-regimen-changed-observation | ||
| }); | ||
| markNew(changeReasonObs); | ||
| state.observations.push(changeReasonObs); | ||
| } | ||
|
|
||
| if (changeObs) { | ||
| changeObs.hasMember = [changeTypeObs, changeReasonObs] | ||
| .filter(Boolean) | ||
| .map(util.ref); | ||
| } | ||
|
|
||
| return result; | ||
| }; | ||
|
|
||
| each($.inputs.MedicationDispense, state => { | ||
| const input = state.data.resource; | ||
|
|
||
| console.log(`Processing medication dispense ${input.id}`); | ||
|
|
||
| let encounter; | ||
| if (input.context) { | ||
| // Extract the actual ID out of the reference | ||
| const [_, encounterId] = input.context.reference.split('Encounter/'); | ||
| encounter = state.inputs.Encounter.find( | ||
| e => e.resource.id === encounterId | ||
| )?.resource; | ||
| if (encounter) { | ||
| console.log(` Found encounter ${input.context.reference} in bundle`); | ||
| } else { | ||
| console.log( | ||
| 'WARNING: COULD NOT FIND MATCHING ENCOUNTER FOR ', | ||
| input.context | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const [_, patientId] = input.subject.reference.split('Patient/'); | ||
| const patient = state.inputs.Patient.find( | ||
| e => e.resource.id === patientId | ||
| )?.resource; | ||
|
|
||
| const dispense = mapDispense(state, input, encounter); | ||
|
|
||
| state.medicationDispense.push(dispense); | ||
|
|
||
| const med = extractMedication(state, input); | ||
| dispense.medicationReference = util.reference(med); | ||
| state.medication.push(med); | ||
|
|
||
| const plan = extractCareplan(state, input, encounter, patient); | ||
| state.careplans.push(plan); | ||
|
|
||
| const o = extractObservations(state, input, encounter, patient); | ||
|
|
||
| const request = extractRequest( | ||
| state, | ||
| input, | ||
| plan, | ||
| med, | ||
| dispense, | ||
| encounter, | ||
| o.followUpStatus | ||
| ); | ||
| dispense.authorizingPrescription = [util.ref(request)]; | ||
| state.requests.push(request); | ||
|
|
||
| // care plan and request have circular dependencies, so sort out plan.activity here | ||
| plan.activity = [{ reference: util.reference(request) }]; | ||
| extractCareplanExtensions(state, input, plan, encounter); | ||
|
|
||
| const admin = extractAdmin(state, input, encounter, med, request); | ||
| if (admin) { | ||
| state.admin.push(admin); | ||
| } | ||
|
|
||
| return state; | ||
| }); | ||
|
|
||
| fn(state => { | ||
| console.log( | ||
| `Finished parsing medications! Created ${state.medicationDispense.length} Patients` | ||
| ); | ||
|
|
||
| return state; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Significant refactoring needed for the map-medications job
The "map-medications" job is extremely complex and long, making it difficult to maintain and understand. Here are some suggestions for improvement:
- Break down the job into smaller, more focused functions or even separate jobs.
- Improve error handling and logging throughout the job.
- Consider using a more declarative approach for mapping operations.
- Reduce code duplication and improve code reuse.
Here's a high-level approach to refactoring:
- Extract utility functions and constants to separate modules:
// medicationMappingUtils.js
export const createId = (base, type) => { /* ... */ };
export const markNew = (resource) => { /* ... */ };
export const findServiceProvider = (encounter, patient) => { /* ... */ };
// medicationMappingConstants.js
export const categoryTypeMap = { /* ... */ };
export const treatmentFailureMap = { /* ... */ };
export const substituteReasonMap = { /* ... */ };
export const stoppedReasonMap = { /* ... */ };- Create separate functions for each major mapping operation:
const mapMedication = (input) => { /* ... */ };
const mapMedicationDispense = (input, encounter) => { /* ... */ };
const mapMedicationRequest = (input, careplan, medication, dispense, encounter, observation) => { /* ... */ };
const mapCarePlan = (input, encounter, patient) => { /* ... */ };
const mapObservations = (input, encounter, patient) => { /* ... */ };- Implement more robust error handling and logging in each function:
const mapMedication = (input) => {
try {
// ... mapping logic ...
console.log(`Successfully mapped medication for ${input.id}`);
return mappedMedication;
} catch (error) {
console.error(`Error mapping medication for ${input.id}:`, error);
// Consider how to handle this error (skip, throw, etc.)
}
};- Use a more declarative approach for mapping operations:
const medicationMappers = {
medication: mapMedication,
dispense: mapMedicationDispense,
request: mapMedicationRequest,
carePlan: mapCarePlan,
observations: mapObservations,
};
const mapMedicationResource = (type, input, ...args) => {
const mapper = medicationMappers[type];
if (!mapper) {
console.error(`No mapper found for type: ${type}`);
return null;
}
return mapper(input, ...args);
};- Reorganize the main job structure:
fn(state => {
console.log('Starting medication mapping process');
state.medicationDispense = [];
state.medication = [];
state.observations = [];
state.careplans = [];
state.requests = [];
state.admin = [];
return state;
});
each($.inputs.MedicationDispense, state => {
const input = state.data.resource;
console.log(`Processing medication dispense ${input.id}`);
try {
const encounter = findEncounter(input.context, state);
const patient = findPatient(input.subject, state);
const medication = mapMedicationResource('medication', input);
const dispense = mapMedicationResource('dispense', input, encounter);
const carePlan = mapMedicationResource('carePlan', input, encounter, patient);
const observations = mapMedicationResource('observations', input, encounter, patient);
const request = mapMedicationResource('request', input, carePlan, medication, dispense, encounter, observations.followUpStatus);
// Update state with mapped resources
updateState(state, { medication, dispense, carePlan, observations, request });
} catch (error) {
console.error(`Error processing medication dispense ${input.id}:`, error);
// Consider how to handle this error (skip, continue, etc.)
}
return state;
});
fn(state => {
console.log(`Finished parsing medications! Created ${state.medicationDispense.length} Medication Dispenses`);
return state;
});These changes will significantly improve the maintainability, readability, and potentially the performance of the "map-medications" job. It will be easier to understand, debug, and extend in the future.
| send-to-NDR: | ||
| name: send to NDR | ||
| adaptor: "@openfn/[email protected]" | ||
| credential: [email protected] | ||
| body: | | ||
| fn(state => { | ||
| console.log('Sending final bundle to NDR'); | ||
| return state; | ||
| }); | ||
|
|
||
| post('/fhir', { | ||
| body: $.bundle, | ||
| headers: { 'content-type': 'application/json' }, | ||
| parseAs: 'json', | ||
| }).catch(async (response, state) => { | ||
| const err = JSON.parse(response.body) | ||
| console.error('Error from NDR FHIR') | ||
| console.error(err) | ||
| state.issue = err.issue; | ||
| return state | ||
| }) | ||
|
|
||
| fn(state => { | ||
| console.log('Done!'); | ||
| return { | ||
| issue: state.issue, | ||
| bundle: state.bundle, | ||
| data: state.data, | ||
| response: state.response, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and logging in the send-to-NDR job
The current implementation of the "send-to-NDR" job is functional but could benefit from improved error handling and logging. Here are some suggestions:
- Add more detailed logging throughout the process.
- Implement more robust error handling, including specific error types.
- Consider adding retry logic for transient errors.
Here's a suggested improvement:
const axios = require('axios');
const axiosRetry = require('axios-retry');
axiosRetry(axios, {
retries: 3,
retryDelay: axiosRetry.exponentialDelay,
retryCondition: (error) => {
return axiosRetry.isNetworkOrIdempotentRequestError(error) || error.response.status === 500;
},
});
fn(state => {
console.log('Preparing to send final bundle to NDR');
return state;
});
post('/fhir', {
body: $.bundle,
headers: { 'content-type': 'application/json' },
parseAs: 'json',
})
.then(response => {
console.log('Successfully sent bundle to NDR');
console.log(`Response status: ${response.status}`);
console.log(`Response data: ${JSON.stringify(response.data)}`);
return response;
})
.catch(async (error) => {
console.error('Error sending bundle to NDR');
if (error.response) {
console.error(`Response status: ${error.response.status}`);
console.error(`Response data: ${JSON.stringify(error.response.data)}`);
state.issue = error.response.data.issue;
} else if (error.request) {
console.error('No response received from NDR');
} else {
console.error(`Error message: ${error.message}`);
}
throw error; // Re-throw to trigger error handling workflow
});
fn(state => {
console.log('Completed NDR submission process');
return {
issue: state.issue,
bundle: state.bundle,
data: state.data,
response: state.response,
};
});This refactored version includes:
- More detailed logging at each step
- Improved error handling with specific error types
- Retry logic for transient errors using axios-retry
- Clearer separation of concerns between sending the request and handling the response
These changes will make the job more robust and easier to debug in case of issues.
| send-errors-to-kafka-topic: | ||
| name: send errors to kafka topic | ||
| adaptor: "@openfn/[email protected]" | ||
| credential: [email protected] | ||
| body: | | ||
| fn(state => { | ||
| console.log('Catching errors to send to kafka topic fhir-errors...'); | ||
| console.log(state.issue) | ||
| return state; | ||
| }); | ||
|
|
||
| //send error message from FHIR API | ||
| post('/openfn/fhir-errors', { | ||
| body: JSON.stringify($.issue), | ||
| headers: { 'content-type': 'application/json' }, | ||
| }); | ||
|
|
||
| fn(state => { | ||
| console.log('Sending failed bundle to kafka topic dead-queue...'); | ||
| return state; | ||
| }); | ||
|
|
||
| //send final Bundle we mapped | ||
| post('/openfn/dead-queue', { | ||
| body: $.bundle, | ||
| headers: { 'content-type': 'application/json' }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and logging in the send-errors-to-kafka-topic job
The current implementation of the "send-errors-to-kafka-topic" job could benefit from enhanced error handling and logging. Additionally, there's a minor formatting issue. Here are some suggestions:
- Add more detailed logging throughout the process.
- Implement error handling for the HTTP requests.
- Consider combining the error message and failed bundle into a single Kafka message.
- Fix the trailing space on line 1205.
Here's a suggested improvement:
fn(state => {
console.log('Preparing to send errors to Kafka topics');
return state;
});
// Send error message and failed bundle in a single request
post('/openfn/error-reporting', {
body: JSON.stringify({
error: $.issue,
failedBundle: $.bundle
}),
headers: { 'content-type': 'application/json' },
})
.then(response => {
console.log('Successfully sent error report to Kafka');
console.log(`Response status: ${response.status}`);
return response;
})
.catch(error => {
console.error('Error sending report to Kafka:', error.message);
// Consider how to handle this error (retry, alert, etc.)
});
fn(state => {
console.log('Completed error reporting process');
return state;
});This refactored version:
- Combines the error message and failed bundle into a single Kafka message
- Adds more detailed logging
- Implements basic error handling for the HTTP request
- Removes the trailing space issue
Fix the trailing space on line 1205:
- body: $.bundle,
+ body: $.bundle,These changes will make the job more robust and easier to debug in case of issues.
🧰 Tools
🪛 yamllint
[error] 1205-1205: trailing spaces
(trailing-spaces)
| build-bundle: | ||
| name: build bundle | ||
| adaptor: "@openfn/[email protected]" | ||
| credential: null | ||
| body: | | ||
| const wrapResource = (res) => { | ||
| const { __new__, ...resource } = res; | ||
| return { | ||
| request: { | ||
| method: __new__ ? "POST" : "PUT", | ||
| url: __new__ ? res.resourceType : `${res.resourceType}/${res.id}` | ||
| }, | ||
| resource, | ||
| } | ||
| } | ||
|
|
||
| fn((state) => { | ||
| console.log('Building final bundle...') | ||
|
|
||
| delete state.inputs | ||
|
|
||
| const bundle = { | ||
| "resourceType": "Bundle", | ||
| "type": "transaction", | ||
| "entry": [] | ||
| }; | ||
|
|
||
| // Write all data onto state | ||
| for (const k in state) { | ||
| const arr = state[k] | ||
| if (Array.isArray(arr)) { | ||
| bundle.entry.push(...arr.filter(a => a.resourceType).map(wrapResource)) | ||
| } | ||
| } | ||
|
|
||
| console.log(`Bundle built with ${bundle.entry.length} items`) | ||
|
|
||
| const counts = {} | ||
| for (const res of bundle.entry ) { | ||
| try { | ||
| const type = res.resource.meta.profile[0].split('StructureDefinition/')[1] | ||
| counts[type] ??= 0 | ||
| counts[type] += 1 | ||
| } catch(e) { | ||
| console.log(res) | ||
| throw e | ||
| } | ||
| } | ||
|
|
||
| console.log(counts) | ||
|
|
||
| return { bundle }; | ||
|
|
||
| }) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the build-bundle job with improved error handling and performance
The "build-bundle" job is well-structured, but we can make some improvements to enhance its robustness and efficiency:
- Add more comprehensive error handling.
- Optimize the bundle building process for performance.
- Improve logging for better traceability.
Here's a suggested improvement:
const wrapResource = (res) => {
const { __new__, ...resource } = res;
return {
request: {
method: __new__ ? "POST" : "PUT",
url: __new__ ? res.resourceType : `${res.resourceType}/${res.id}`
},
resource,
}
}
fn((state) => {
console.log('Starting to build final bundle...');
try {
delete state.inputs;
const bundle = {
"resourceType": "Bundle",
"type": "transaction",
"entry": []
};
const counts = {};
// Use Object.entries for better performance
Object.entries(state).forEach(([k, arr]) => {
if (Array.isArray(arr)) {
arr.forEach(resource => {
if (resource.resourceType) {
bundle.entry.push(wrapResource(resource));
try {
const type = resource.meta.profile[0].split('StructureDefinition/')[1];
counts[type] = (counts[type] || 0) + 1;
} catch (e) {
console.warn(`Unable to determine resource type for ${resource.id}:`, e.message);
}
}
});
}
});
console.log(`Bundle built with ${bundle.entry.length} items`);
console.log('Resource counts:', counts);
return { bundle };
} catch (error) {
console.error('Error building bundle:', error);
throw error; // Re-throw to trigger error handling workflow
}
});This refactored version includes:
- More comprehensive error handling with try-catch blocks
- Performance optimization using
Object.entries()instead of multiplefor...inloops - Improved logging with more detailed information about the bundle building process
- A more concise way of counting resources by type
These changes will make the job more robust, potentially faster, and easier to debug in case of issues.
| triggers: | ||
| kafka: | ||
| type: webhook | ||
| enabled: true | ||
| edges: | ||
| kafka->parse-bundle: | ||
| source_trigger: kafka | ||
| target_job: parse-bundle | ||
| condition_type: always | ||
| enabled: true | ||
| parse-bundle->Register-Organization: | ||
| source_job: parse-bundle | ||
| target_job: Register-Organization | ||
| condition_type: on_job_success | ||
| enabled: true | ||
| Register-Organization->map-encounters: | ||
| source_job: Register-Organization | ||
| target_job: map-encounters | ||
| condition_type: on_job_success | ||
| enabled: true | ||
| map-encounters->map-patients: | ||
| source_job: map-encounters | ||
| target_job: map-patients | ||
| condition_type: on_job_success | ||
| enabled: true | ||
| map-patients->map-medications: | ||
| source_job: map-patients | ||
| target_job: map-medications | ||
| condition_type: on_job_success | ||
| enabled: true | ||
| send-to-NDR->send-errors-to-kafka-topic: | ||
| source_job: send-to-NDR | ||
| target_job: send-errors-to-kafka-topic | ||
| condition_type: js_expression | ||
| condition_label: import errors | ||
| condition_expression: state.issue | ||
| enabled: true | ||
| map-medications->build-bundle: | ||
| source_job: map-medications | ||
| target_job: build-bundle | ||
| condition_type: on_job_success | ||
| enabled: true | ||
| build-bundle->send-to-NDR: | ||
| source_job: build-bundle | ||
| target_job: send-to-NDR | ||
| condition_type: on_job_success | ||
| enabled: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing workflow error handling and logging
The workflow edges configuration is well-structured and covers the necessary connections between jobs. However, we can improve the overall robustness and observability of the workflow:
- Add more detailed condition logging for each edge.
- Implement global error handling for the entire workflow.
- Consider adding retry mechanisms for certain job failures.
Here are some suggestions for improvement:
- Add logging to edge conditions:
Register-Organization->map-encounters:
source_job: Register-Organization
target_job: map-encounters
condition_type: on_job_success
enabled: true
on_condition_met: |
console.log('Starting map-encounters job after successful organization registration');- Implement a global error handling job:
global-error-handler:
name: global error handler
adaptor: "@openfn/[email protected]"
body: |
fn(state => {
console.error('Workflow error occurred:', state.error);
// Implement error reporting logic (e.g., send to monitoring system)
return state;
});
# Add edges to the global error handler from each job
map-encounters->global-error-handler:
source_job: map-encounters
target_job: global-error-handler
condition_type: on_job_failure
enabled: true- Add retry mechanism for certain jobs:
send-to-NDR->send-to-NDR:
source_job: send-to-NDR
target_job: send-to-NDR
condition_type: js_expression
condition_label: retry on network error
condition_expression: state.error && state.error.isNetworkError && state.retryCount < 3
on_condition_met: |
state.retryCount = (state.retryCount || 0) + 1;
console.log(`Retrying send-to-NDR job. Attempt ${state.retryCount}`);
enabled: trueThese enhancements will improve the workflow's resilience, observability, and ability to recover from transient failures. It will also provide better insights into the workflow's execution path and any issues that may occur.
| @@ -47,6 +48,7 @@ profiles: | |||
| - kafka-unbundler-consumer | |||
| - fhir-ig-importer | |||
| - reprocess-mediator | |||
| - openfn | |||
There was a problem hiding this comment.
I don't think this should be deployed by default for CDR, let's leave it as optional.
Summary by CodeRabbit
New Features
openfnpackage to enhance functionality and integration capabilities.openfnpackage, including setup instructions and demo access.openfnand related services for streamlined development and deployment.Documentation
openfnpackage outlining its purpose and usage.Bug Fixes
Tests
Chores