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.

Sign up for GitHub

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

Jump to bottom

Custom patient report cleanup (Fixes: #5744, #5765, #5766) #5751

Merged
merged 6 commits into from
Oct 1, 2022
Merged

Custom patient report cleanup (Fixes: #5744, #5765, #5766) #5751

merged 6 commits into from
Oct 1, 2022

Conversation

tsimonq2
Copy link
Contributor

@tsimonq2 tsimonq2 commented Sep 14, 2022 •
edited
Loading

Fixes: #5744, #5765, #5766

(Throwing the WIP flag back up on this, since we have an up for grabs demo available here. I'll update this PR once it's all ready again.)

Changes proposed in this pull request:

  • Hide unchecked boxes in reports by default, allowing the user to override this setting per-layout.
  • Add a grp_unchecked column to layout_group_properties
  • Use the Unicode reference for checkboxes instead of [ x ] / [ ]
  • Make the patient name larger in printed reports, ensuring the practice information is right-aligned

@tsimonq2 tsimonq2 changed the title [WIP] Add option to show checkboxes in LBF reports, hiding them by default (Fixes: #5744) Add option to show checkboxes in LBF reports, hiding them by default (Fixes: #5744) Sep 16, 2022
@tsimonq2
Copy link
Contributor Author

@sjpadgett @bradymiller No rush, just wanted to let you know this is now ready to review. Thanks.

@stephenwaite
Copy link
Member

created a PR for the demo at openemr/demo_farm_openemr#41

@stephenwaite
Copy link
Member

up for grabs demo is up at https://four.openemr.io/c/openemr/interface/login/login.php?site=default

@tsimonq2
Copy link
Contributor Author

Thank you very much! $job has requested some more layout modifications, so I'll file the issues and link over the PR

@tsimonq2 tsimonq2 changed the title Add option to show checkboxes in LBF reports, hiding them by default (Fixes: #5744) Custom patient report cleanup (Fixes: #5744) Sep 20, 2022
@tsimonq2 tsimonq2 changed the title Custom patient report cleanup (Fixes: #5744) [WIP] Custom patient report cleanup (Fixes: #5744) Sep 20, 2022
@bradymiller
Copy link
Member

For the Patient History layout which is impacted by this, toggling on the 'Show Unchecked Boxes' does not work:
image

image

btw, here is look in current openemr just for reference:
image

@bradymiller bradymiller changed the title [WIP] Custom patient report cleanup (Fixes: #5744) Custom patient report cleanup (Fixes: #5744) Sep 20, 2022
@tsimonq2 tsimonq2 changed the title Custom patient report cleanup (Fixes: #5744) Custom patient report cleanup (Fixes: #5744, #5766) Sep 20, 2022
@tsimonq2
Copy link
Contributor Author

@bradymiller It should be functional now with my latest commit. There's some spammy PHP warnings that could probably fix, but, well, one step at a time :)

Please do let me know if you have any further questions. Probably looking at next week Mon or Tues it should be ready for a review cycle.

Thanks.

@tsimonq2 tsimonq2 changed the title Custom patient report cleanup (Fixes: #5744, #5766) Custom patient report cleanup (Fixes: #5744, #5765, #5766) Sep 21, 2022
bradymiller
bradymiller reviewed Sep 24, 2022
View reviewed changes
library/options.inc.php Outdated
{
if ($sel != '*' && strpos($sel, 'grp_group_id') === false) {
$sel = "grp_group_id, $sel";
}
$gres = sqlStatement("SELECT $sel FROM layout_group_properties WHERE grp_form_id = ? " .
"ORDER BY grp_group_id", array($formtype));
" ORDER BY grp_group_id " .
($limit ? "LIMIT " . $limit : ""), array($formtype));
Copy link
Member

Choose a reason for hiding this comment

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

need to escape this for sql with the following:

($limit ? "LIMIT " . escape_limit($limit) : ""), array($formtype));

@tsimonq2 tsimonq2 force-pushed the 5744 branch 3 times, most recently from 4ae339a to 5da960b Compare September 24, 2022 16:37
@bradymiller
Copy link
Member

bradymiller commented Sep 29, 2022 •
edited
Loading

hi @tsimonq2 , Code looks good and testing well. After the merge conflict is addressed, can then bring this in (if you give us access to the PR branch, then we can do that since pretty sure just a conflict in the sql upgrade script; see below screenshot for toggle that can select for this in the PR (bottom of right column)).

image

@tsimonq2
Copy link
Contributor Author

tsimonq2 commented Sep 30, 2022 •
edited
Loading

Heya @bradymiller - can I still add some additional features to this PR or would you rather merge it now as-is and ask me to do the other issues in a different PR? Totally your call.

I appreciate it!

@bradymiller
Copy link
Member

hi @tsimonq2 , gonna bring this in and then would use separate PR's for other issues. Thanks for the improvements!

@bradymiller bradymiller merged commit 758a42b into openemr:master Oct 1, 2022
@tsimonq2
Copy link
Contributor Author

tsimonq2 commented Oct 3, 2022

Thank you very much @bradymiller @stephenwaite et al. for your help!

tsimonq2 added a commit to AltispeedTechnologies/openemr that referenced this pull request Oct 3, 2022
@tsimonq2
Revert "Custom patient report cleanup (Fixes: openemr#5744, openemr#5765
7672342 
, openemr#5766) (openemr#5751)"

This reverts commit 758a42b.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradymiller bradymiller bradymiller left review comments

@sjpadgett sjpadgett Awaiting requested review from sjpadgett

Assignees
No one assigned
Labels
Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Many empty checkboxes in reports make things cluttered
4 participants
@tsimonq2 @stephenwaite @bradymiller @tywrenn