Skip to content

Conversation

Wambere
Copy link
Contributor

@Wambere Wambere commented Feb 4, 2025

IMPORTANT: Where possible all PRs must be linked to a Github issue

Fixes #322

Engineer Checklist

  • I have run ./gradlew spotlessApply to check my code follows the project's style guide
  • I have built using the command ./gradlew clean assemble and run the efsity jar to verify my change fixes the issue and does not break the application

@Wambere
Copy link
Contributor Author

Wambere commented Feb 4, 2025

Will be adding tests in a bit

@dubdabasoduba dubdabasoduba requested a review from Copilot March 25, 2025 10:27
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds new functionalities for importing flags from OpenSRP1 by extending resource types and defining new payload builder functions, while updating the CLI options and documentation accordingly.

  • Added "Encounter" and "Location" to the valid resource types and implemented new functions (build_single_resource, build_resources, check_location, and build_flag_payload) to support flag imports.
  • Updated the main CLI to accept additional parameters (encounter_id, practitioner_id, visit_location_id) and branch logic for flags.
  • Revised the README with detailed usage examples and improved the token refresh logic in the FHIR Keycloak API service.

Reviewed Changes

Copilot reviewed 4 out of 9 changed files in this pull request and generated 2 comments.

File Description
importer/importer/builder.py Extended valid resource types and added new payload builder functions for flag resources.
importer/main.py Introduced new CLI options and flags-specific processing branch.
importer/README.md Updated documentation and examples for flag imports.
importer/importer/services/fhir_keycloak_api.py Modified token refresh condition for improved reliability.
Files not reviewed (5)
  • importer/csv/import/flags.csv: Language not supported
  • importer/json_payloads/flag_encounter_payload.json: Language not supported
  • importer/json_payloads/flag_observation_payload.json: Language not supported
  • importer/json_payloads/flag_payload.json: Language not supported
  • importer/json_payloads/visit_encounter_payload.json: Language not supported
Comments suppressed due to low confidence (1)

importer/importer/builder.py:1165

  • Ensure that json_template is not None before using it to open a file. Adding a check and raising a meaningful error may prevent runtime failures if no matching template is found.
json_template = next((template for key, template in template_map.items() if key in resource_type), None)

@dubdabasoduba dubdabasoduba requested a review from Copilot April 11, 2025 17:27
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 11 changed files in this pull request and generated 1 comment.

Files not reviewed (6)
  • importer/csv/import/flags.csv: Language not supported
  • importer/json_payloads/flag_encounter_payload.json: Language not supported
  • importer/json_payloads/flag_observation_payload.json: Language not supported
  • importer/json_payloads/flag_payload.json: Language not supported
  • importer/json_payloads/inventory_group_payload.json: Language not supported
  • importer/json_payloads/visit_encounter_payload.json: Language not supported
Comments suppressed due to low confidence (2)

importer/importer/services/fhir_keycloak_api.py:100

  • The condition to detect a login page uses a generic 'login' substring, which might inadvertently capture other response messages. Consider using a more specific identifier to avoid false positives.
if response.status_code == 401 or 'login' in response.text:

importer/importer/builder.py:1265

  • The variable 'json_path' appears to be used without a clear definition. Ensure that 'json_path' is defined or imported so that the file path concatenation works as intended.
with open(json_path + json_template) as json_file:

if len(sub_list) < 1:
sub_list = ""

entries.append(sub_list)
Copy link

Copilot AI Apr 11, 2025

Choose a reason for hiding this comment

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

The 'entries' list is being appended to without an explicit definition. Please initialize the 'entries' variable (e.g., entries = []) before the loop to ensure proper collection of list entries.

Copilot uses AI. Check for mistakes.

@dubdabasoduba dubdabasoduba requested a review from Copilot April 15, 2025 13:48
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 9 changed files in this pull request and generated 2 comments.

Files not reviewed (5)
  • importer/csv/import/flags.csv: Language not supported
  • importer/json_payloads/flag_encounter_payload.json: Language not supported
  • importer/json_payloads/flag_observation_payload.json: Language not supported
  • importer/json_payloads/flag_payload.json: Language not supported
  • importer/json_payloads/visit_encounter_payload.json: Language not supported

def request(self, **kwargs):
response = self.api_service.oauth.request(**kwargs)
if response.status_code == 401 or '<html class="login-pf">' in response.text:
if response.status_code == 401 or 'login' in response.text:
Copy link

Copilot AI Apr 15, 2025

Choose a reason for hiding this comment

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

Using the generic 'login' keyword to detect authentication pages may cause false positives. Consider using a more specific marker from the response to reliably identify authentication failures.

Copilot uses AI. Check for mistakes.

if len(sub_list) < 1:
sub_list = ""

entries.append(sub_list)
Copy link

Copilot AI Apr 15, 2025

Choose a reason for hiding this comment

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

The variable 'entries' is used without prior initialization. Initialize it (e.g., 'entries = []') before appending to ensure the code runs as intended.

Copilot uses AI. Check for mistakes.

@dubdabasoduba dubdabasoduba requested a review from Copilot April 16, 2025 07:54
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 9 changed files in this pull request and generated no comments.

Files not reviewed (5)
  • importer/csv/import/flags.csv: Language not supported
  • importer/json_payloads/flag_encounter_payload.json: Language not supported
  • importer/json_payloads/flag_observation_payload.json: Language not supported
  • importer/json_payloads/flag_payload.json: Language not supported
  • importer/json_payloads/visit_encounter_payload.json: Language not supported
Comments suppressed due to low confidence (3)

importer/main.py:45

  • When the 'flags' setup is specified, the encounter_id, practitioner_id, and visit_location_id options become critical. Consider either enforcing these options as required or adding validation logic to ensure they are provided.
@click.option("--encounter_id", required=False)

importer/importer/services/fhir_keycloak_api.py:100

  • Using the substring 'login' to determine an authentication issue might be too broad and could trigger token refreshes on unrelated responses. Consider using a more specific pattern to detect the login page.
if response.status_code == 401 or 'login' in response.text:

importer/importer/builder.py:1265

  • The variable 'json_path' is used in the file opening but is not defined within the diff context. Ensure that 'json_path' is properly defined or imported to avoid runtime errors.
with open(json_path + json_template) as json_file:

@dubdabasoduba dubdabasoduba merged commit 8db4b0c into main Apr 22, 2025
4 checks passed
@dubdabasoduba dubdabasoduba deleted the import-flags branch April 22, 2025 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Importer] Add the ability to import flags from a CSV file generated from OpenSRP 1 data.

2 participants