Skip to content
This repository has been archived by the owner on May 10, 2022. It is now read-only.

GSoC2018 report generator #1

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

Trodrige
Copy link
Collaborator

Ability to connect a second database (report generator database), and use.

@aethelwulffe
Copy link

aethelwulffe commented May 28, 2018

@pri2si17-1997 Hit it dude! :)

Copy link

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

Nice work. Much love here.

@@ -57,7 +57,8 @@ Instructions:
'php artisan migrate'

12. You can seed the database width
'php artisan db:seed'
'php artisan db:seed' // This shouldn't work for now.

Choose a reason for hiding this comment

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

This is obviously a WIP comment. Making a comment here just so we make sure to check for these when it is final review time.

@@ -0,0 +1,44 @@
<?php

Choose a reason for hiding this comment

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

I assume these names are for WIP versioning for now, or are they intended to serve in a meaningful way to version components later?

Copy link
Collaborator Author

@Trodrige Trodrige Jun 8, 2018

Choose a reason for hiding this comment

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

@aethelwulffe that's the Laravel convention. I am trying to make the table names as obvious as possible.

*/
public function report_formats()
{
return $this->belongsToMany('ReportFormat')->withTimestamps();

Choose a reason for hiding this comment

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

I like the naming convention for many-to-one, one-to-many elements. Nice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's the beauty of Laravel!

@@ -81,7 +81,7 @@
</div>
<div id="collapseOne" class="collapse show" aria-labelledby="headingOne" data-parent="#accordion">
<div class="card-body" id="second">
<p class="note">Why am I stil empty?</p>
<p class="note">Why am I still empty?</p>

Choose a reason for hiding this comment

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

I feel the same way. 😸

Choose a reason for hiding this comment

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

Why is my still empty? 🍷

@@ -6,7 +6,7 @@
text-align: center;
}

#draggable{
.draggable{

Choose a reason for hiding this comment

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

ID to class? Was that an error correction or just a refactor of the approach?

$table->boolean('is_default')->default(0)->comment = "0 -> False, 1 -> True.";
$table->float('option_value', 8, 2)->default(0)->comment = "";
$table->string('mapping', 31)->comment = "";
$table->string('notes', 255)->comment = "This stores the meaning or usefulness of the component.";
Copy link
Member

Choose a reason for hiding this comment

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

Limiting notes to string is not a good idea. You can use text instead. Notes may exceed 255 char.

$table->string('title', 255)->comment = "This is the text on the component that end users see.";
$table->integer('order', 0)->comment = "The order in which components appear in the list.";
$table->boolean('active')->default(1)->comment = "0 -> False, 1 -> True whether the component should be active or not.";
$table->json('notes', 255)->comment = "This stores the fields that the component relates to in the database.";
Copy link
Member

Choose a reason for hiding this comment

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

Why notes to be of json type? I can't understand from your comment. Any example will be appreciated.

$table->string('title', 255)->comment = "This is the report name e.g Patient List.";
$table->string('system_feature', 255)->comment = "The system feature under which the report belongs.";
$table->text('description')->comment = "This describes the report format briefly.";
$table->json('draggable_components_id')->comment = "This stores the id of all the components that belong to this report format";
Copy link
Member

Choose a reason for hiding this comment

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

Not able to get draggable_components_id field's type.

@pri2si17-1997
Copy link
Member

This is a general feedback looking at one go. I would test your PR explicitly and let you know @Trodrige .

Sorry @aethelwulffe , been very busy last week, with academic major and all.

@teryhill
Copy link
Contributor

teryhill commented Jun 6, 2018

image

@Trodrige Trodrige closed this Jun 11, 2018
@Trodrige Trodrige deleted the GSoC2017_Report_Generator branch June 11, 2018 09:35
@Trodrige Trodrige changed the title GSoC2017 report generator GSoC2018 report generator Jun 11, 2018
@aethelwulffe
Copy link

I assume this is open elsewhere now?
I got a data set, and I wannaa play with it.
Evals are due ya know. :)

@Trodrige Trodrige restored the GSoC2017_Report_Generator branch June 11, 2018 17:39
@Trodrige Trodrige reopened this Jun 11, 2018
@Trodrige
Copy link
Collaborator Author

@aethelwulffe I'm sorry for the inconvenience. I renamed the branch to show 2018 instead of 2017. I didn't realize this happened.

@Trodrige
Copy link
Collaborator Author

@aethelwulffe please let me know if everything is fine now

@aethelwulffe
Copy link

Looks good to me!
I just want to play with the code. I don't (as much) much care what it is called!

@pri2si17-1997
Copy link
Member

Hi @Trodrige , You need to add new instructions, from scratch, how to handle two databases. It would be easier to follow.

return url('reportgenerator/showReport');
$option_ids = serialize($option_ids);
return response()->json([
'redirecturl' => 'http:https://localhost:8000/reportgenerator/report/'.$option_ids,

Choose a reason for hiding this comment

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

Shoutcast port? :)

Choose a reason for hiding this comment

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

Not sure what this affects, but might need to be configurable/discoverable. For instance, this would not work in my production setup. localhost does one thing. 127.0.0.1 does something completely different.
-I am sure you have this under consideration, or it is actually a non-issue...I am just making sure you know I am here! 😺

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have this @aethelwulffe I'll use the $DB_HOST global.

<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1">
<meta name="author" content="Tigpezeghe Rodrige K. @ [email protected]">
<metan name="description" content="GSoC2018: Building a report generator for LibreHealthEHR">

Choose a reason for hiding this comment

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

oops

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Taken care of!

data: { ids: option_ids },
dataType: 'json',
success: function(response){
alert("You've successfully sent the unique ids");
console.log(response);
//alert("You've successfully sent the unique ids");

Choose a reason for hiding this comment

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

Debug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, debug.

}
});
}

$(document).ready(function() {
$('#generate-button').click(function() {
$('#generate-button').click(function(e) {
e.preventDefault();
if (jQuery.isEmptyObject(IDs)) { // check if the user has dropped any component before trying to generate a report
alert("Please drag and drop components before you can generate a report!");

Choose a reason for hiding this comment

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

"Cannot create a blank report. Please drag and drop at least one component into your new report before attempting to generate it."
OR
"Cannot generate a blank report! You must add at least one component."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought of that, and it escaped me. Handling it now!

@@ -0,0 +1,46 @@
<?php
/**
* This file creates draggable_components for the report-generator.
Copy link
Member

Choose a reason for hiding this comment

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

It should be "This file created report_format for report-generator." . Just a guess.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checked!

$table->foreign('draggable_component_id')->references('id')->on('draggable_components');
$table->integer('report_format_id')->unsigned()->comment = "Ussed to access the report_formats.";
$table->foreign('report_format_id')->references('id')->on('report_formats');

Copy link
Member

Choose a reason for hiding this comment

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

There should be something, that what happens if it is deleted. Like onDelete('cascade').

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

checked!

<div class="col-sm-3" id="draggable"><p>I am seven</p></div>
<div class="col-sm-3" id="draggable"><p>I am eight</p></div>
<div class="row" id="draggable-column">
@foreach ($draggableComponents as $draggableComponent)
Copy link
Member

Choose a reason for hiding this comment

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

This is good use of foreach to write compact code. 👍

Choose a reason for hiding this comment

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

foreach is indeed a beautiful construct. One that sometimes leaves me thinking "My code can't work! There isn't enough....CODE!" 😜

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My mentors catch everything!!!

@aethelwulffe
Copy link

Pri is your mentor here. I am the Laravel noob peanut gallery blowing the Vuvuzela.

@Trodrige
Copy link
Collaborator Author

Trodrige commented Jul 2, 2018

@pri2si17-1997 in order to these recent changes, you have to run 'php artisan module:migrate-refresh ReportGenerator' in the project folder.
Use the 'librereportgenerator.sql' file in the Documentation folder to populate the draggable_components table. BANG!

  1. Generate reports
  2. Add System Feature
  3. Save Report Format
  4. Leave your comments!!!

@pri2si17-1997
Copy link
Member

Hi! @Trodrige

I got your code working, please see attached screenshot :
screenshot from 2018-07-02 22-59-53

General Comments :

[1] Search button is not visible.
[2] Not able to find any draggable componets, probably because I haven't imported from your sql file. I can see if I import your data. So in this case, if your table is empty, you should have some relevant message there, like 'Oops! No components available.'
[3] Why query part is disabled?

Specific Comments:

[1] You should have detailed instruction, on how to make it up. And add what is relevant to you. Like if you do not need base db, just remove its all contents, it should not throw error in your migrate command.

[2] Do not take dump of your specific table. Take dump of database.

[3] I am not able to search my created system feature. Nothing happens.

[4] Is there any specific order of dragging components or it will handle automatically?

[5] I am getting this error, when trying to generate report.
screenshot from 2018-07-02 23-03-28

These are my findings. I would suggest you to modify instruction, considering a new bie with only laptop in his hand is going to build your code and request from @aethelwulffe @teryhill @tmccormi to review it and give their feedback.

-- --------------------------------------------------------

--
-- Dumping data for table `report_formats`
Copy link
Member

Choose a reason for hiding this comment

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

Take dump of whole database. What if user do not run migrations or somehow migration fails? He/she do not know your table structure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took these when I had no entries for report_formats, and system features in the database. I'll populate those tables with some entries and dump again!

Copy link
Member

Choose a reason for hiding this comment

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

I meant that you are taking dump of one table draggable_components. Instead take dump of whole database librereportgenerator. I think now I'm clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I get you!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I export the entire database, then it'll have CREATE TABLE xyz, and migration command takes care of creating tables.
We need only the INSERT sql statements in this file.

@foreach ($draggable_components as $draggable_component)
<div class="col-sm-2 wordwrap draggable" id="{{ $draggable_component->option_id }}">
<p data-toggle="tooltip" data-placement="top" title="{{ $draggable_component->title }}">
{{ $draggable_component->title }}
Copy link
Member

Choose a reason for hiding this comment

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

I like templating. 👍

1. Create your own .env file from the .env.example fill
2. Edit the database variables as for your report generator database connection.
3. Create a new database with the database name you specified in DB_REPORT_GENERATOR_DATABASE
4. Run php artisan module:seed ReportGenerator
Copy link
Member

Choose a reason for hiding this comment

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

No need to have a separate file. Merge this and instructions.txt to Readme.

* @TODO: Uncomment the line below when EHR is in full Laravel mode.
* That is, when the EHR has been completely ported to Laravel.
*/
//$this->createDB(env('DB_DATABASE'), env('DB_HOST'), env('DB_PORT'), env('DB_USERNAME'), env('DB_PASSWORD'));
Copy link
Member

Choose a reason for hiding this comment

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

I have uncommented this line to make it working.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I uncomment and push?

@Trodrige
Copy link
Collaborator Author

Trodrige commented Jul 2, 2018

Hello @pri2si17-1997

General answers

  1. Hover over the search button. I'll make it colourful if it's trouble!

  2. For now, you have to import the draggable components. Subsequently, I'll populate the tables with default draggable components, system features and report formats automatically. For now, let's roll this way. I know it's painful, but I'll take care of this!

  3. The query part is an aside from my project. That is not necessary, but we'll implement it after the main report generator we had in mind is up and running.

Specific answers

  1. I've taken care of the instructions for setting up the database.

  2. I don't think I understand!!! Anyways, this is what I think. You want the librereportgenerator.sql file to have INSERTs for all tables. The report generator will have the opportunity for the user to create all those things. In addition, we'll have default draggable components, system features, and report formats. I've not yet added enough data for the last 2.

  3. The search isn't working yet. The next thing on my TODO list is creating a page to edit/delete system feature and reports. The system features also have to show up as menu items in the navbar. Then the report formats under each system feature will be displayed in the list under each menu (system feature) item.

  4. There is no specific order of dragging components. The generated report displays in the order the user dragged the components. I'm not sure if this answers your question.

  5. For the error you're getting, please make sure you enter correct details for the main libreehr database in your .env file. The laravel application doesn't see your database. Maybe you've specified a wrong database name. Check port, username and password.

@aethelwulffe
Copy link

Teacher: "Little boy, what is 24 divided by 8?"
Little Boy: "I don't know. What do you think?"
Teacher: "I don't THINK, I KNOW!"
Little Boy: "Well I don't think I know either!"

On 2. above (sort of): The Layout Engine allows users to alter tables like patient_data. This includes deletions. That is not great, and should at least have a "blacklist" of fields that cannot be modified through the GUI at very least. Many installations have been bricked by someone doing a general cleanup and deleting something useless appearing like "genericval2". It may be worth looking at having a robust enough "report troubleshooter" so that if, after the report is created, if something gets broken, each query involved can be run sequentially to report to the user in plain language where the problem is.

@Trodrige
Copy link
Collaborator Author

Trodrige commented Jul 5, 2018

@aethelwulffe I don't think I'm getting you clearly here. Which '2 above' are you referring to?
Let me just explain this here.

A user of the report generator will have default draggable components, system features, and report formats that they can't edit or delete. The users can only edit and/or delete those that they have created. There is no action done in this report generator that will delete data in the patient_data table. NONE. The users of the report generator can only view these data. The only data they have permission to edit/delete are those for the draggable components, system features and report formats that they created.

I don't know if this is what you were referring to, but I think it's something we all should know.

@aethelwulffe
Copy link

Right. You can't use the report gen to delete core things. Other things can delete things. Then your report tries to run.
-The thought above was to see if there was something simple we could do to let the user know why the report they built and have been using has suddenly stopped working. They might get an error message that means something to them, depending on how the server is set up.

-I am just thinking about how everything can fall to pieces. I am a systems engineer. That is my job. 😜

@Trodrige Trodrige force-pushed the GSoC2017_Report_Generator branch 2 times, most recently from c88eeec to 425b965 Compare July 28, 2018 15:12
@@ -1 +0,0 @@
<?php echo 'yes it worked!!!'; ?>

Choose a reason for hiding this comment

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

error_log("Yes, it still does work")
👍

@aethelwulffe
Copy link

Component pool, or component ocean? :)

@Trodrige
Copy link
Collaborator Author

Trodrige commented Aug 6, 2018

https://tigrodrige.wordpress.com/2018/08/06/gsoc-2018-final-submission/

If @pri2si17-1997 and @aethelwulffe can confirm, then I can submit my final evaluation and continue work here.

@pri2si17-1997
Copy link
Member

Hi @Trodrige ,

bug_report_gen

Please see point 7 and 8. System features are of same name. So is it desirable? IMO system features should be unique.

@Trodrige
Copy link
Collaborator Author

@pri2si17-1997 thanks for observing. I'll fix that ASAP.

@Trodrige
Copy link
Collaborator Author

Trodrige commented Aug 17, 2018

@pri2si17-1997 @aethelwulffe hope y'all doing great. I moved the work to the main repo in order to work on installation. This is the pr below. just head to localhost:'port'/modules/report_generator/public/reportgenerator while trying to test the PR in the link below.
LibreHealthIO/lh-ehr#1225

Pri, I fixed the duplicate entries for system features and report formats too.

@robbyoconnor
Copy link
Member

Shall this be closed?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants