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.

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

refactor: test and refactor populateTimezones function in (ccdaservice) #6818

Merged

Conversation

raskolnikov-rodion
Copy link
Contributor

No description provided.

@sjpadgett
Copy link
Sponsor Member

Hi @raskolnikov-rodion
I like what you are doing with ccda but I'm concerned that you are touching some of the main builder components. I assume you're doing most of the refactor to support automated testing which is appreciated however building CDA's is part of certification so we have to be careful not to introduce new issues with the document build. Are you testing full builds as you go?
The cert test is pretty extensive but I think it'd be safe if you test with a patient dataset that includes the Summary of Care required templates. This will exercise any code changes to what's included in build.

@raskolnikov-rodion
Copy link
Contributor Author

Hi @raskolnikov-rodion I like what you are doing with ccda but I'm concerned that you are touching some of the main builder components. I assume you're doing most of the refactor to support automated testing which is appreciated however building CDA's is part of certification so we have to be careful not to introduce new issues with the document build. Are you testing full builds as you go? The cert test is pretty extensive but I think it'd be safe if you test with a patient dataset that includes the Summary of Care required templates. This will exercise any code changes to what's included in build.

Hi @sjpadgett , thanks for chiming in! Besides facilitating automated testing, the refactors help with code maintainability and modernization. Here's an overview of the process:

  1. I update my branch with the lastest changes
  2. I pick the functions I want to include in the refactor and copy the code to a separate file, without modifying the original script
  3. I read the code and write tests, leveraging the coverage report to verify the code branches I'm hitting
  4. only after the tests are written, I refactor the functions inside the new file; this way, I can confirm that I'm getting the same output for the same input before and after the refactor
  5. I remove the functions from the original script and import the refactored code
  6. I run the OpenEMR app and insert random patients
  7. I generate CCDA reports with the patient data

Occasionally, I also modify some of the generated data and see if there are any surprises in the report.

Steps 2 and 4 is where most of the fun is. The other parts are often quite dry, but is the best way I know to do refactors with confidence. It follows ideas from books on refactor and working with legacy code.

If you have any suggestions on how to improve the testing or if there are any other datasets available, I'd be happy to include in the process. It makes sense to be careful, especially with a critical feature for certification 🙂

@sjpadgett
Copy link
Sponsor Member

Thanks for the effort to give me a detailed reply and it put me at ease. A sound engineering approach.
Your efforts are appreciated and will help get us ready for our next cert go around which mainly involves document parsing and user auditing for import. That code could use some serious look see if you wish to attack that part!:)

If you get into the node apps template builder engine we should have a conversation so I can help you understand where we started and how we have had to modify over the years to get to where we are today. Builder engine came from a Blue Button project based on c32/hl7 rel 1.0 templates. We'd been better served to have developed from scratch using twig, mustache or similar.
We developed the front end you're into now.

@bradymiller
Copy link
Sponsor Member

another great improvement! bringing in.

@bradymiller bradymiller merged commit 408ff7c into openemr:master Sep 2, 2023
23 checks passed
@raskolnikov-rodion raskolnikov-rodion deleted the refactor/ccda-timezones branch September 3, 2023 08:05
@raskolnikov-rodion
Copy link
Contributor Author

Thanks for the effort to give me a detailed reply and it put me at ease. A sound engineering approach. Your efforts are appreciated and will help get us ready for our next cert go around which mainly involves document parsing and user auditing for import. That code could use some serious look see if you wish to attack that part!:)

If you get into the node apps template builder engine we should have a conversation so I can help you understand where we started and how we have had to modify over the years to get to where we are today. Builder engine came from a Blue Button project based on c32/hl7 rel 1.0 templates. We'd been better served to have developed from scratch using twig, mustache or similar. We developed the front end you're into now.

sounds cool! It will take me a while to get through the ccda script, but I'd be happy to work on other parts of the project as well

@adunsulag adunsulag changed the title refactor(ccdaservice): test and refactor populateTimezones function refactor: test and refactor populateTimezones function in (ccdaservice) Nov 16, 2023
@adunsulag adunsulag added this to the 7.0.2 milestone Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants