-
Notifications
You must be signed in to change notification settings - Fork 14
GSoC2018 report generator #1
base: master
Are you sure you want to change the base?
GSoC2018 report generator #1
Conversation
…ed a report generator table connection
@pri2si17-1997 Hit it dude! :) |
…lationship. Updated instructions
There was a problem hiding this 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.
Documentation/instructions.txt
Outdated
@@ -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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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. 😸
There was a problem hiding this comment.
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{ |
There was a problem hiding this comment.
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."; |
There was a problem hiding this comment.
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."; |
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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.
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. |
I assume this is open elsewhere now? |
@aethelwulffe I'm sorry for the inconvenience. I renamed the branch to show 2018 instead of 2017. I didn't realize this happened. |
@aethelwulffe please let me know if everything is fine now |
Looks good to me! |
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoutcast port? :)
There was a problem hiding this comment.
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! 😺
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken care of!
public/assets/js/master.js
Outdated
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, debug.
public/assets/js/master.js
Outdated
} | ||
}); | ||
} | ||
|
||
$(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!"); |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'); | ||
|
There was a problem hiding this comment.
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')
.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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. 👍
There was a problem hiding this comment.
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!" 😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mentors catch everything!!!
Pri is your mentor here. I am the Laravel noob peanut gallery blowing the Vuvuzela. |
@pri2si17-1997 in order to these recent changes, you have to run 'php artisan module:migrate-refresh ReportGenerator' in the project folder.
|
Hi! @Trodrige I got your code working, please see attached screenshot : General Comments :[1] Search button is not visible. 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. 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` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get you!
There was a problem hiding this comment.
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 }} |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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')); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Hello @pri2si17-1997 General answers
Specific answers
|
Teacher: "Little boy, what is 24 divided by 8?" 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. |
@aethelwulffe I don't think I'm getting you clearly here. Which '2 above' are you referring to? 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. |
Right. You can't use the report gen to delete core things. Other things can delete things. Then your report tries to run. -I am just thinking about how everything can fall to pieces. I am a systems engineer. That is my job. 😜 |
c88eeec
to
425b965
Compare
425b965
to
7f33e3f
Compare
public/pdf_report.php
Outdated
@@ -1 +0,0 @@ | |||
<?php echo 'yes it worked!!!'; ?> |
There was a problem hiding this comment.
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")
👍
Component pool, or component ocean? :) |
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. |
Hi @Trodrige , Please see point 7 and 8. System features are of same name. So is it desirable? IMO system features should be unique. |
@pri2si17-1997 thanks for observing. I'll fix that ASAP. |
@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. Pri, I fixed the duplicate entries for system features and report formats too. |
Shall this be closed? |
Ability to connect a second database (report generator database), and use.