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

GSoC 2018 Project: PDF generator code for Patient Report section #1165

Merged
merged 6 commits into from
Jan 22, 2019

Conversation

2208Abhinav
Copy link
Contributor

This code will create the PDFs for the Patient Report section which will be tested using Test Download button.
screenshot from 2018-06-04 11-18-28

@2208Abhinav
Copy link
Contributor Author

2208Abhinav commented Jun 6, 2018

An error for Demographics option. Working on it.

@aethelwulffe
Copy link
Contributor

Need any help?
-You can always post errors here.

@aethelwulffe
Copy link
Contributor

Sorry, I have not tested this yet myself. I have been working on the PR's from bottom up.

@aethelwulffe
Copy link
Contributor

Is this ready to test?

@2208Abhinav
Copy link
Contributor Author

yes @aethelwulffe its ready to test, just Demographics option is having some problems.

@aethelwulffe
Copy link
Contributor

What are the dems issues? Not finding them. Was this procedures report inclusion the fix?

@muarachmann
Copy link
Member

muarachmann commented Jun 11, 2018

@2208Abhinav just looked at your code cool. Went through the code and saw the dowload button didnt have a class of cp-output just make sure you dont forget to add that we are going to do a revamping of those stuffs after gsoc

@aethelwulffe
Copy link
Contributor

aethelwulffe commented Jun 11, 2018

Mua, if you do a "looking at cool code" then WRITE A REVIEW. K? We need you here, you are valuable, but we also have you on the hook for this GSOC thing, and we defined community involvement as "writing reviews" among other things. 😛

-This here is supposed to be a lateral organization. Everybody should be worked up to the point where they are part of the integration process. Right now, the dumbest three old turds in the project are the integrators and handle all the reviews. That is silly. 😉

@2208Abhinav
Copy link
Contributor Author

@aethelwulffe the error correction is not included yet in the code. Is the Procedures Order code working fine?

@2208Abhinav
Copy link
Contributor Author

I think for the error that there are some HTML tags not closed properly that is why data is leaking. I will solve it.

@aethelwulffe
Copy link
Contributor

Oh...We have always had HTML errors in this junk. I tend to overlook such things, as none of this has ever worked right. I have always had...low expectations with this part of the application. I will adjust the bar up a notch or two!

modules/pdf_generator/report/billing.php Outdated Show resolved Hide resolved
global $content_insu;
if (@array_key_exists($key,$retar)) {
$length = sizeof($retar{$key});
if ($retar{$key}[$length]{"value"} != "0000-00-00 00:00:00") {

Choose a reason for hiding this comment

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

Be careful with Date/Time strings in INNODB default MYSQL that format is a bad date, an empty date will be NULL not zero filled. LibreEHR has some old code (and data) that may already have ZERO dates, so all tests need to support both formats for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmccormi So am I required to use condition like if(($retar{$key}[$length]{"value"} != "0000-00-00 00:00:00")&&($retar{$key}[$length]{"value"} != NULL))?

"WHERE po.procedure_order_id = ?",
$orderid);

$content_pro .= "(" . oeFormatShortDate(substr($orow['date'], 0, 10)) . ")<br><table border=\"1\" style=\"background-color: silver\"><tr><td>Patient ID</td>".

Choose a reason for hiding this comment

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

ALL Strings need to be wrapped in xlt() translation functions...

Copy link
Contributor

Choose a reason for hiding this comment

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

...and they should be checked for standard terms. If something exists in the translations tables, you should use that (or change it everywhere). Not a bad idea when those who can provide translations actually go and do so. We have not ...ever... checked or updated the xlt() tables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmccormi should I use xlt() only with strings like xlt("Procedures Order") or also with the variables like xlt($facility['name']) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only for labels/instructions that are hard-coded. Never for variables. The translation is for the application, not any of the contents. The translations come from an old-school table lookup, so any new text added to applications must also be added to the translations table.


$pdf = new EHRPDF('p', 'mm', array(216, 279), true, 'UTF-8', false);//potrait, pdf unit in mm and height=279mm, width=216mm
$pdf->AddPage();
$pdf->SetFont('dejavusans', '', 19);

Choose a reason for hiding this comment

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

Seems odd to set the font over and over, perhaps this should be a higher level setting or at least a defined variable at the top?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be needed with every call, BUUUT, I think that dejavusans is the asset's default font anyway....which is not saying that you don't have to specify...I don't know.

@muarachmann
Copy link
Member

muarachmann commented Jun 19, 2018

@2208Abhinav please edit your PR title with a GSOC 2018 prefix or suffix had a hard time finding this

@aethelwulffe
Copy link
Contributor

@muarachmann You can filter issues/PR's by choosing the tag "GSOC 2018"

@2208Abhinav 2208Abhinav changed the title PDF generator code for Patient Report section GSoC 2018 Project: PDF generator code for Patient Report section Jun 20, 2018
@2208Abhinav
Copy link
Contributor Author

I think @aethelwulffe is right but still I changed the title @muarachmann 😄

@2208Abhinav
Copy link
Contributor Author

@aethelwulffe test the code its ready 🤓 but just leave the demographics for now, it will be corrected in next submission.

@2208Abhinav
Copy link
Contributor Author

@tmccormi due to merge conflict I removed the second download button, but still you can test by using previous single download button.

if($orow['specimen_type']) $content_pro .= "<td>Specimen Type</td>";
if($orow['specimen_type']) $content_pro .= "<td>" . xlt("Specimen Type") . "</td>";

$content_pro .= "<td>" . $orow['specimen_type'] . "</td></tr></table><br><br>";
Copy link
Member

Choose a reason for hiding this comment

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

Good that you included the xlt functions but these green lines and red stuffs makes my eyes go 🙄 will test and get back to a line id needed

@muarachmann
Copy link
Member

Good you included the xlt functions those green and red lines make my eyes 👀 will test and get back to the line in case.

@muarachmann
Copy link
Member

muarachmann commented Jul 7, 2018

Worry not much about merge conflict for your GSoC project at the end it would be resolved

@2208Abhinav
Copy link
Contributor Author

@teryhill did you check the code? I want the output PDF of the options selected collectively that I mentioned on the chat.

@aethelwulffe
Copy link
Contributor

Checks worked fine, but need to address the zero-date concern from Tony.

@2208Abhinav
Copy link
Contributor Author

@aethelwulffe I will do it in the end first I am making the whole base then minor issues will be solved.
When I will complete the report section I will do that.

@aethelwulffe
Copy link
Contributor

@2208Abhinav OH, I know all that. Just documenting the status of a test...for the at-the-moment. I have been really doing poorly keeping up with testing of branches for the last two weeks, and I am just trying to get on top of things.
For the day-to-day operations of a clinic, retro wrong-headed ideas like exporting things to a PDF document instead of using a data exchange API are actually vital to how they do business in the here and now. The stuff you are working on here directly affects the average user's performance and basic quality of life (less frustration) in a major daily way. I should be keeping up with testing to help out and verify.

@2208Abhinav
Copy link
Contributor Author

2208Abhinav commented Jul 21, 2018

@tmccormi @teryhill @aethelwulffe I pushed some new code, removed error for Demographics please test the code.

@aethelwulffe
Copy link
Contributor

Testing will commence at 15:00 EST

@aethelwulffe
Copy link
Contributor

Hi folks,
Torn shoulder keeping me off the computer a lot right now.
-Testing the Who is Online branch piled on my error log to the point everything shut down. This branch does it too.
About 100,000 error sets per second are generated repeating this:

[Tue Jul 31 13:09:57.485137 2018] [:error] [pid 1068:tid 1956] [client ::1:50491] PHP Notice:  Undefined offset: 1 in C:\\xampp\\htdocs\\branchtest\\library\\appointments.inc.php on line 174, referer: http://localhost/branchtest/modules/calendar/index.php?&framewidth=953
[Tue Jul 31 13:09:57.485137 2018] [:error] [pid 1068:tid 1956] [client ::1:50491] PHP Notice:  Undefined offset: 2 in C:\\xampp\\htdocs\\branchtest\\library\\appointments.inc.php on line 174, referer: http://localhost/branchtest/modules/calendar/index.php?&framewidth=953
[Tue Jul 31 13:09:57.485137 2018] [:error] [pid 1068:tid 1956] [client ::1:50491] PHP Notice:  Only variable references should be returned by reference in C:\\xampp\\htdocs\\branchtest\\library\\encounter_events.inc.php on line 403, referer: http://localhost/branchtest/modules/calendar/index.php?&framewidth=953

Sure, those are warnings, but why is it getting called so much? Basically this seems to be killing my system. I dare not push the current branch to production.

@aethelwulffe
Copy link
Contributor

The errors I am getting are not related to this.
The demographics section looks great....BUUUT it should not (meaning our dems default content) have the social security number....
-Nothing to do with this though.

Copy link
Contributor

@aethelwulffe aethelwulffe left a comment

Choose a reason for hiding this comment

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

PDF prototype in there, but this commit tests well.
When you need another test (or you declare victory) I am ready to do so.

@2208Abhinav
Copy link
Contributor Author

@teryhill @aethelwulffe @tmccormi @nileshprasad137 I did the final commit please check it. I will integrate the PDF generation code for @Trodrige project after GSOC. I have writen the code for it and I am waiting for its integration in main EHR software.

@2208Abhinav
Copy link
Contributor Author

PDF code for Review of Systems and Review of Systems Checks is left due due to previous bugs.

@2208Abhinav
Copy link
Contributor Author

NOTE : The code is left for Review of Systems and Review of Systems Checks.
And integration with @Trodrige project is left.

@aethelwulffe
Copy link
Contributor

Tested as-is

@aethelwulffe
Copy link
Contributor

Review of Systems and Review of Systems Checks is due for replacement itself. Long overdue. There are contrib repo chunks related to a possible replacement.

@aethelwulffe
Copy link
Contributor

@2208Abhinav Could you please address @tmccormi request to change the comment headers so we can merge this? It is an awesome and much needed thing. Pretty please?

@2208Abhinav
Copy link
Contributor Author

@aethelwulffe I added the new comment headers like it was asked long ago.

@2208Abhinav
Copy link
Contributor Author

@aethelwulffe what other changes do I need to add for this to be mergeable

@aethelwulffe
Copy link
Contributor

I was looking at an old commit.
Yes, this can be merged.
I will do the testing of the PDF's immediately. If someone wants to review this again, they may do so, but I thing 4-5 months of just sitting here is plenty long enough, and unless they post protests, I am going to merge this pending on my test results.

@aethelwulffe
Copy link
Contributor

OK.
Pulled down a pretty extensive PDF which you can find to look at at https://suncoastconnection.com/downloads/screenies/test_rat.pdf.
If anything, there is only some possible translation or date transformation issues in the code, and even those might not have any real reason to be addressed in this commit.
Merging this and moving on before the NEXT YEAR GSOC starts!
This fixes a major problem for the EHR.

@aethelwulffe aethelwulffe merged commit 3f8b1d3 into LibreHealthIO:master Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants