Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CU-86c083kqv - Add config fields for names for Kafka and Notifications #325

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

CU-86c083kqv - Add config fields for names for Kafka and Notifications #325

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5499bd
CU-86c083kqv - Add config fields for names for Kafka and Notifications
sushantpatil1214 Sep 25, 2024
fc2ac46
Merge branch 'dev' into CU-86c083kqv_Create-Config-Flag-for-Kafka-Key
sushantpatil1214 Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions JeMPI_Apps/JeMPI_AsyncReceiver/src/main/java/org/jembi/jempi/async_receiver/Main.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,13 +153,15 @@ private void apacheReadCSV(

for (CSVRecord csvRecord : csvParser) {
final var patientRecord = demographicData(csvRecord);
String givenNameField = FIELDS_CONFIG.nameValidationFields.get(0);
String familyNameField = FIELDS_CONFIG.nameValidationFields.get(1);
String givenName = patientRecord.fields.stream()
.filter(field -> "given_name".equals(field.ccTag()))
.filter(field -> givenNameField.equals(field.ccTag()))
.map(DemographicData.DemographicField::value)
.findFirst()
.orElse("");
String familyName = patientRecord.fields.stream()
.filter(field -> "family_name".equals(field.ccTag()))
.filter(field -> familyNameField.equals(field.ccTag()))
.map(DemographicData.DemographicField::value)
.findFirst()
.orElse("");
Expand Down
3 changes: 2 additions & 1 deletion JeMPI_Apps/JeMPI_EM_Scala/src/main/scala/org/jembi/jempi/em/config/Config.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,5 +65,6 @@ case class Config(
auxGoldenRecordFields: Option[Array[auxField]],
additionalNodes: Option[Array[AdditionalNode]],
demographicFields: Array[DemographicField],
rules: Rules
rules: Rules,
nameValidationFields: Option[List[String]]
)
2 changes: 2 additions & 0 deletions JeMPI_Apps/JeMPI_LibShared/src/main/java/org/jembi/jempi/shared/config/FieldsConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public final class FieldsConfig {
public final List<AuxField> userAuxInteractionFields;
public final List<AuxField> userAuxGoldenRecordFields;
public final List<DemographicField> demographicFields;
public final List<String> nameValidationFields;
public final List<AdditionalNode> additionalNodes;

public FieldsConfig(final JsonConfig jsonConfig) {
Expand Down Expand Up @@ -88,6 +89,7 @@ public FieldsConfig(final JsonConfig jsonConfig) {
);
optionalInteractionAuxIdIdx = auxInteractionAuxIdIdx[0];

nameValidationFields = jsonConfig.nameValidationFields().stream().toList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

LGTM with a suggestion: Consider enhancing null safety and immutability.

The initialization of nameValidationFields is consistent with other fields and uses a concise stream operation. However, consider the following enhancement:

- nameValidationFields = jsonConfig.nameValidationFields().stream().toList();
+ nameValidationFields = jsonConfig.nameValidationFields() != null
+     ? List.copyOf(jsonConfig.nameValidationFields())
+     : List.of();

This change would:

  1. Handle potential null return from jsonConfig.nameValidationFields().
  2. Ensure immutability by using List.copyOf().
  3. Provide an empty list as a fallback if the source is null.
📝 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.

Suggested change
nameValidationFields = jsonConfig.nameValidationFields().stream().toList();
nameValidationFields = jsonConfig.nameValidationFields() != null
? List.copyOf(jsonConfig.nameValidationFields())
: List.of();


demographicFields = jsonConfig.demographicFields()
.stream()
Expand Down
1 change: 1 addition & 0 deletions JeMPI_Apps/JeMPI_LibShared/src/main/java/org/jembi/jempi/shared/config/input/JsonConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public record JsonConfig(
@JsonProperty("auxGoldenRecordFields") List<AuxGoldenRecordField> auxGoldenRecordFields,
List<AdditionalNode> additionalNodes,
List<DemographicField> demographicFields,
List<String> nameValidationFields,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding @JsonProperty annotation and documentation for the new field.

The addition of List<String> nameValidationFields aligns with the PR objective to add config fields for names. However, there are a couple of suggestions to improve this change:

  1. Add a @JsonProperty annotation to the field for consistency with other fields and to ensure proper JSON serialization/deserialization.
  2. Include a brief comment explaining the purpose and usage of this field.

Here's a suggested improvement:

+     /**
+      * List of field names to be used for name validation.
+      */
+     @JsonProperty("nameValidationFields")
      List<String> nameValidationFields,

This change will maintain consistency with other fields and provide clarity for future developers working with this configuration.

📝 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.

Suggested change
List<String> nameValidationFields,
/**
* List of field names to be used for name validation.
*/
@JsonProperty("nameValidationFields")
List<String> nameValidationFields,

Rules rules) {

private static final Logger LOGGER = LogManager.getLogger(JsonConfig.class);
Expand Down
10 changes: 10 additions & 0 deletions ...MPI_LibShared/src/main/java/org/jembi/jempi/shared/config/input/NameValidationFields.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package org.jembi.jempi.shared.config.input;
import com.fasterxml.jackson.annotation.JsonInclude;

import java.util.List;

@JsonInclude(JsonInclude.Include.NON_NULL)
public record NameValidationFields(
List<String> nameValidationFields
) {
}
Comment on lines +8 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

PR does not fully address Kafka and Notifications configuration objectives.

The current implementation adds a List<String> nameValidationFields, which does not directly relate to the PR's stated goal of adding configuration fields for Kafka and Notifications. To align with the PR objectives:

  1. Introduce specific configuration fields for Kafka, such as kafkaBootstrapServers and kafkaApplicationId.
  2. Add configuration settings for Notifications to ensure seamless integration with Kafka.
  3. Update relevant documentation and Javadoc comments to reflect these new configurations.
🔗 Analysis chain

Improve field naming and add documentation.

The use of a List for nameValidationFields is appropriate, but there are some areas for improvement:

  1. The field name is somewhat redundant given the class name. Consider renaming it to simply fields or rules.
  2. There's no documentation explaining the purpose or expected content of this field. Adding a Javadoc comment would greatly improve clarity.
  3. It's unclear how this relates to the PR objective of "Add config fields for names for Kafka and Notifications". Could you clarify the connection?

Consider applying these changes:

 @JsonInclude(JsonInclude.Include.NON_NULL)
 public record NameValidationFields(
-        List<String> nameValidationFields
+        /**
+         * List of validation rules or field names for name validation.
+         * Each string in this list represents a specific validation criterion or a field to be validated.
+         */
+        List<String> rules
 ) {
 }

To ensure this change aligns with the PR objectives, could you provide more context on how this relates to Kafka and Notifications configuration? Are there any other files changed in this PR that implement the Kafka-specific aspects?

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for Kafka-related changes in other files
rg --type java -i 'kafka|notification' --glob '!JeMPI_Apps/JeMPI_LibShared/src/main/java/org/jembi/jempi/shared/config/input/NameValidationFields.java'

Length of output: 105040

1 change: 1 addition & 0 deletions ..._Apps/JeMPI_LibShared/src/main/java/org/jembi/jempi/shared/models/ConfigurationModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ public record Configuration(
List<Field> auxGoldenRecordFields,
List<Node> additionalNodes,
List<DemographicField> demographicFields,
List<String> nameValidationFields,
Rules rules) {
}

Expand Down
6 changes: 4 additions & 2 deletions JeMPI_Apps/JeMPI_Linker/src/main/java/org/jembi/jempi/linker/backend/LinkerDWH.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,15 @@ private static boolean helperUpdateGoldenRecordField(

private static String patientName(final Interaction interaction) {
var patientRecord = interaction.demographicData();
String givenNameField = FIELDS_CONFIG.nameValidationFields.get(0);
String familyNameField = FIELDS_CONFIG.nameValidationFields.get(1);
String givenName = patientRecord.fields.stream()
.filter(field -> "given_name".equals(field.ccTag()))
.filter(field -> givenNameField.equals(field.ccTag()))
.map(DemographicData.DemographicField::value)
.findFirst()
.orElse("");
String familyName = patientRecord.fields.stream()
.filter(field -> "family_name".equals(field.ccTag()))
.filter(field -> familyNameField.equals(field.ccTag()))
.map(DemographicData.DemographicField::value)
.findFirst()
.orElse("");
Expand Down
4 changes: 4 additions & 0 deletions devops/linux/docker/data-config/config-reference-auto-generate-fields.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@
"indexGoldenRecord": "@index(exact)"
}
],
"nameValidationFields": [
"given_name",
"family_name"
],
"rules": {
"link": {
"deterministic": [
Expand Down
4 changes: 4 additions & 0 deletions devops/linux/docker/data-config/config-reference-link-d-validate-dp-match-dp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,10 @@
"matchMetaData": null
}
],
"nameValidationFields": [
"given_name",
"family_name"
],
Comment on lines +225 to +228
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Update the rules section to utilize nameValidationFields.

The nameValidationFields is not currently referenced in the rules section, which means the new field isn't being utilized as intended. Please update the rules to incorporate nameValidationFields or provide documentation explaining its intended use.

🔗 Analysis chain

Verify if rules need updating to use nameValidationFields.

The addition of nameValidationFields introduces a new concept for name-specific validation. However, the existing rules in the rules section don't explicitly reference this new field.

Please verify if any changes are needed in the rules section to utilize the new nameValidationFields. If not, consider adding a comment explaining how this new field is intended to be used.

To help with the verification, you can run the following script to check for any references to nameValidationFields in the codebase:

This script will help identify if and where nameValidationFields is being used, and list any files that might be related to name validation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for references to nameValidationFields in the codebase

# Test: Search for nameValidationFields usage
echo "Searching for nameValidationFields usage:"
rg --type-add 'config:*.{json,js,ts,py}' -t config 'nameValidationFields'

# Test: List files that might be related to name validation
echo -e "\nListing files that might be related to name validation:"
fd -e py -e js -e ts 'name.*valid'

Length of output: 820

Script:

#!/bin/bash
# Description: Check if the 'rules' section references 'nameValidationFields' in the specified config file

echo "Checking for 'nameValidationFields' usage in the 'rules' section of config-reference-link-d-validate-dp-match-dp.json:"
jq '.rules' devops/linux/docker/data-config/config-reference-link-d-validate-dp-match-dp.json | grep 'nameValidationFields' || echo "'nameValidationFields' is not referenced in the 'rules' section."

Length of output: 531

"rules": {
"link": {
"deterministic": [
Expand Down
4 changes: 4 additions & 0 deletions devops/linux/docker/data-config/config-reference-link-d.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@
"indexGoldenRecord": "@index(exact)"
}
],
"nameValidationFields": [
"given_name",
"family_name"
],
"rules": {
"link": {
"deterministic": [
Expand Down
4 changes: 4 additions & 0 deletions devops/linux/docker/data-config/config-reference-link-dp-minimal.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,10 @@
}
}
],
"nameValidationFields": [
"given_name",
"family_name"
],
"rules": {
"link": {
"deterministic": [
Expand Down
4 changes: 4 additions & 0 deletions devops/linux/docker/data-config/config-reference-link-dp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@
}
}
],
"nameValidationFields": [
"given_name",
"family_name"
],
"rules": {
"link": {
"deterministic": [
Expand Down
2 changes: 0 additions & 2 deletions devops/linux/docker/deployment/install_from_scratch/d-stack-1-create-db-build-all-reboot.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,3 @@ source ../scripts/d-stack-up-keycloak-containers.sh
sleep 2
source ../scripts/d-stack-up-app-containers.sh
sleep 2
source ../../helper/keycloak/start-keycloak-test-server.sh
sleep 2