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

HERACLES-33: Historical & Lab Results range/unit revisions #1807

Open
wants to merge 7 commits into
base: CARDS-2580
Choose a base branch
from
Open

HERACLES-33: Historical & Lab Results range/unit revisions #1807

wants to merge 7 commits into from
+559 −8

Conversation

sashaandjic
Copy link
Contributor

@sashaandjic sashaandjic commented Sep 4, 2024 •
edited by marta-
Loading

@sashaandjic

This comment was marked as outdated.

Sign in to view
@marta- marta- dismissed their stale review September 5, 2024 03:54

The comments were addressed

@sashaandjic

This comment was marked as outdated.

Sign in to view
@sashaandjic

This comment was marked as outdated.

Sign in to view
@marta-
Copy link
Contributor

marta- commented Sep 13, 2024

Rebased on the latest dev, squashed commits for cleaner history.

sdumitriu
sdumitriu approved these changes Sep 13, 2024
View reviewed changes
Copy link
Member

@sdumitriu sdumitriu left a comment

Choose a reason for hiding this comment

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

I fixed the < and > not showing up. Are they sure that no value will ever be out of range? Because a value out of range makes the form invalid, which means it will not be exported.

...ces/clinical-data/src/main/resources/SLING-INF/content/Questionnaires/Laboratory Results.xml Show resolved Hide resolved
...ces/clinical-data/src/main/resources/SLING-INF/content/Questionnaires/Laboratory Results.xml Show resolved Hide resolved
@Hajany
Copy link
Collaborator

Hajany commented Sep 17, 2024

@Hajany here is the look of the modified Laboratory Results form with min, max and ranges being added: Laboratoty Results

If the ranges are making it look too busy I can remove them.

Leave the ranges as descriptions, study team wants them to visible in the database as a reference

@sashaandjic

This comment was marked as outdated.

Sign in to view
@sashaandjic sashaandjic added the Test me! Ready for testing label Sep 17, 2024
@sdumitriu
Copy link
Member

Hemoglobin A1c seems to be missing newlines in both forms. Is that intentional?

@sashaandjic
Copy link
Contributor Author

Hemoglobin A1c seems to be missing newlines in both forms. Is that intentional?

no. i missed that one. and red blood cells too.

sashaandjic
sashaandjic commented Sep 17, 2024
View reviewed changes
Copy link
Contributor Author

@sashaandjic sashaandjic left a comment

Choose a reason for hiding this comment

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

all spaces seem to be back now

@sashaandjic

This comment was marked as resolved.

Sign in to view
@sashaandjic
Copy link
Contributor Author

tested. all seems fine.

@marta- marta- changed the base branch from dev to CARDS-2580 September 18, 2024 19:23
@marta-
Copy link
Contributor

marta- commented Sep 18, 2024

@sashaandjic
Copy link
Contributor Author

sashaandjic commented Sep 18, 2024 •
edited
Loading

Tested, looks good. Min/Max ranges are now hidden.
In the attached pdfs, all fields were populated with a number outside min/max range. Both forms were not marked neither as Draft not Invalid.
Historical LAB.pdf
LAB.pdf

In edit mode, the proper warning message is displayed if value entered is outside of the min/max range.
image

image

@sashaandjic

This comment was marked as resolved.

Sign in to view
sashaandjic and others added 5 commits September 18, 2024 17:32
@sashaandjic @marta-
HERACLES-33: Historical & Lab Results range/unit revisions
92d26b1 
Updated Laboratory Results questionnaire
@sashaandjic @marta-
HERACLES-33: Historical & Lab Results range/unit revisions
b8409f3 
Updated Historical Laboratory Results questionnaire
* added min and max values
* added value ranges in the description of the question
* added units of measurement to pro_BNP question
@sdumitriu @marta-
HERACLES-33: Historical & Lab Results range/unit revisions
bc0146a 
Bugfix: <  and > aren't displayed
@sashaandjic @marta-
HERACLES-33: Historical & Lab Results range/unit revisions
8b26499 
Restored spaces that were removed in the previous commit from markdown-formatted descriptions
@sashaandjic @marta-
HERACLES-33: Historical & Lab Results range/unit revisions
83da257 
* Corrected unit of measurements for Lab Results CRP
* Added unit of measurements to Hisotrical Lab Results CRP
* Corrected pro-BNP range in Historical Lab Results
@marta-
HERACLES-33: Historical & Lab Results range/unit revisions
2032ab8 
Disabled the enforcement of min and max values for lab results
@marta-
Copy link
Contributor

marta- commented Sep 18, 2024

When I enabled the reinforcement for Cholesterol, the error message was displayed in 1 line.
Also, we no longer have ability to show min/max values.

Updated branch CARDS-2580 to display the default message ("Please enter values between...") instead of the custom one when min/max are enforced. Rebased this branch for testing.

@sashaandjic sashaandjic added the bug Something isn't working label Sep 18, 2024
@sashaandjic
Copy link
Contributor Author

sashaandjic commented Sep 18, 2024 •
edited
Loading

TESTED - looks good

One very minor thing ( and out of scope) that we can address later. If enforcing the min/max, display the message bellow the input line.
I really like the new message being displayed under the input line, it would improve the look if we always do it.
image
image

@sashaandjic sashaandjic added tested Passed manual testing, needs code review and removed bug Something isn't working Test me! Ready for testing labels Sep 18, 2024
@Hajany
Copy link
Collaborator

Hajany commented Sep 20, 2024

Tested - minor changes:
image
image
image

@sashaandjic
Copy link
Contributor Author

sashaandjic commented Sep 20, 2024 •
edited
Loading

Tested - minor changes: image image image

Addressed in 20a2a9f

@marta- marta- added the on hold To be reviewed at a later time, awaiting other fixes, decisions, etc. label Sep 20, 2024
@marta-
Copy link
Contributor

marta- commented Sep 20, 2024

@sdumitriu Hold the merge until #1810 is approved and merged.

acrowthe
acrowthe approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@acrowthe acrowthe left a comment

Choose a reason for hiding this comment

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

Looks good, tested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdumitriu sdumitriu sdumitriu approved these changes

@acrowthe acrowthe acrowthe approved these changes

@Hajany Hajany Hajany approved these changes

@marta- marta- marta- left review comments

Assignees
No one assigned
Labels
on hold To be reviewed at a later time, awaiting other fixes, decisions, etc. tested Passed manual testing, needs code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sashaandjic @marta- @Hajany @sdumitriu @acrowthe