Migrate sample's DateOfBirth field to AgeDateOfBirthField #74

xispa · 2023-05-16T16:18:18Z

Description of the issue/feature this PR addresses

Warning
Merge senaite/senaite.core#2310 and senaite/senaite.core#2311 first

This Pull Request makes senaite.patient compatible with senaite/senaite.core#2307 and senaite/senaite.core#2311 by replacing the DateTimeField type for sample's DateOfBirth to a new field AgeDateOfBirthField, fully supported by the AgeDoBWidget.

Current behavior before PR

The "DateOfBirth" field was only storing the datetime, while other attributes required for the field and widget to work properly were stored as independent attributes in the instance itself.

Desired behavior after PR is merged

The "DateOfBirth" field stores a tuple (dob, from_age, estimated).

--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.

…field

ramonski

Phew, what a brain excursion... I left some comments on my journey through the code.

ramonski · 2023-05-17T19:32:10Z

src/senaite/patient/api.py

@@ -186,6 +188,7 @@ def update_patient(patient, **values):
    patient.reindexObject()


+@deprecated("Us senaite.core.api.dtime.to_dt instead")


Small typo: Us -> Use

resolved with c06ae2b

ramonski · 2023-05-17T19:42:51Z

src/senaite/patient/api.py

    if not any([years, months, days]):
-        raise AttributeError("No valid ymd: {}".format(age_ymd))
+        if default is _marker:
+            raise AttributeError("No valid ymd: {}".format(ymd))


Better use a TypeError or ValueError instead of a AttributeError. But probably not needed at all if we just return (0, 0, 0).

ramonski · 2023-05-17T20:58:41Z

src/senaite/patient/api.py


    ymd = list("ymd")
-    diff = map(str, (delta.years, delta.months, delta.days))
+    diff = map(str, (val.years, val.months, val.days))
    age = filter(lambda it: int(it[0]), zip(diff, ymd))
    return " ".join(map("".join, age))


Man, you drive me nuts with constructs like this 🙈

Just to understand:
You are converting the years, months and days to strings. Then you zip them together with ["y", "m", "d"] to filter out empties, e.g. to get 5y2d to finally " ".join(map("".join, age)), which returns 5y2d again?

Ok, I've seen it in the doctest now, it generates something like 43y 2m 3d.
As proposed further down, please encapsulate it in an own function, e.g. format_ymd to do this.

ramonski · 2023-05-17T21:02:59Z

src/senaite/patient/api.py

-    valid = map(lambda p: p in ymd, "ymd")
-    return any(valid)
+    values = get_years_months_days(ymd, default=None)
+    return values is not None


Drop the default handling (also below) in favor of this construct:

def is_ymd(ymd): try: get_years_months_days(ymd) except TypeError: return False return True

ramonski · 2023-05-17T21:05:11Z

src/senaite/patient/api.py

-def get_birth_date(age_ymd, on_date=None):
-    """Returns the birth date given an age in ymd format and the date when age
-    was recorded or current datetime if None
+def get_years_months_days(ymd, default=_marker):


remove the default handling for simplicity (only used in is_ymd and get_birth_date)

ramonski · 2023-05-17T21:11:16Z

src/senaite/patient/api.py

-    days = extract_period(age_ymd, "d")
+    years = extract_period(ymd, "y")
+    months = extract_period(ymd, "m")
+    days = extract_period(ymd, "d")
    if not any([years, months, days]):


Why can't we just return (0, 0, 0)?
E.g. for a new born baby with 2 hours.

yes, with latest changes it always returns (0, 0, 0) unless format or type are not valid

ramonski · 2023-05-17T21:23:49Z

src/senaite/patient/api.py

+        years, months, days = get_years_months_days(ymd)
+    except AttributeError:
+        if default is _marker:
+            raise AttributeError("No valid ymd: {}".format(age_ymd))


Phew, catch an AttributeError (which is actually none) to raise an AttributeError again to (probably) bypass the IndexError that might happen here years, months, days = get_years_months_days(ymd) ...

I would maybe completely skip the handling here and leave it to the get_birth_date to either raise a TypeError or return (0, 0, 0).

I simplified the whole thing and now it raises TypeError or ValueError only if the format is not correct

ramonski · 2023-05-17T21:28:20Z

src/senaite/patient/api.py

+        delta = dtime.get_relative_delta(birth_date, on_date)
+        return to_ymd(delta)
+    except (ValueError, TypeError):
+        return None


You do everywhere the default handling except of here you return None? What happened? 😅

Maybe you can skip here the ValueError. At least I see only the explicit TypeError that is raised by to_ymd.

ramonski · 2023-05-17T21:35:45Z

src/senaite/patient/browser/widgets/agedob.py

            ymd = map(lambda p: value.get(p), ["years", "months", "days"])
            if not any(ymd):
                # No values set
-                return None
+                return None, {}

            # Age in ymd format
            ymd = filter(lambda p: p[0], zip(ymd, 'ymd'))
            ymd = "".join(map(lambda p: "".join(p), ymd))


Ok, once again this dance. If it is really required, I would somehow encapsulate it in a format_ymd function with a good docstring / comments.

Done with e77df12 . Is now using to to_ymd function

ramonski · 2023-05-17T21:41:00Z

src/senaite/patient/content/fields.py

+        super(AgeDateOfBirthField, self).set(instance, val)
+
+    def get_date_of_birth(self, instance):
+        """Returns whether the date of birth


incomplete/confusing docstring

xispa · 2023-05-18T16:45:49Z

Phew, what a brain excursion... I left some comments on my journey through the code.

Hehe I think so. Apologies for these excursions

ramonski · 2023-05-18T20:22:59Z

src/senaite/patient/tests/doctests/API.rst

+
+    >>> dob = dtime.to_dt("19791207")
+    >>> api.get_age_ymd(dob, on_date="20230518")
+    '43y 5m 11d'


Happy birthday:)

xispa added 7 commits May 16, 2023 16:01

Sample's DateTimeField for birthdate to AgeDoBField

9a8da93

Compatibility with core#2311

3c14565

Fix the value returned on submit is a list instead of a dict

d9d3a92

Upgrade step 1411

ec3ea35

Cleanup

4916a3b

Remove unused import

83bcd9b

Changelog

fb3002d

xispa requested a review from ramonski May 16, 2023 16:53

xispa added 4 commits May 16, 2023 19:43

Merge branch 'master' of github.com:senaite/senaite.patient into age-…

cd6d305

…field

Added tests in PatientSample.rst

5ab48a7

Added patch for sample's setDateOfBirth function

3c933f2

Handle ValueError and TypeError exceptions properly

3134074

xispa marked this pull request as draft May 17, 2023 07:50

xispa added 2 commits May 17, 2023 19:27

Added doctest for AgeDateOfBirthField

8027573

Additional doctests to SamplePatient

62ced84

xispa marked this pull request as ready for review May 17, 2023 17:44

Fix doctets

4fa47da

xispa mentioned this pull request May 17, 2023

Use Github actions instead of travis #75

Merged

ramonski requested changes May 17, 2023

View reviewed changes

xispa added 7 commits May 18, 2023 07:36

Fix typo

c06ae2b

Make get_years_months_days to return (0, 0, 0)

11b8c1e

Refactor ymd-related functions from the api

628aa0f

Added doctests for age/ymd functions from the API

499002f

Additional doctests

0f4ec7d

Simplify

e77df12

fix tests

19b9aae

ramonski approved these changes May 18, 2023

View reviewed changes

ramonski merged commit 113c974 into master May 18, 2023

ramonski deleted the age-field branch May 18, 2023 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate sample's DateOfBirth field to AgeDateOfBirthField #74

Migrate sample's DateOfBirth field to AgeDateOfBirthField #74

xispa commented May 16, 2023 •

edited

Loading

ramonski left a comment

ramonski May 17, 2023

xispa May 18, 2023

ramonski May 17, 2023

ramonski May 17, 2023

ramonski May 17, 2023

ramonski May 17, 2023

ramonski May 17, 2023

xispa May 18, 2023

ramonski May 17, 2023

xispa May 18, 2023

ramonski May 17, 2023

xispa May 18, 2023

ramonski May 17, 2023

xispa May 18, 2023

ramonski May 17, 2023

xispa May 18, 2023

ramonski May 17, 2023

xispa commented May 18, 2023

ramonski May 18, 2023

		@@ -186,6 +188,7 @@ def update_patient(patient, **values):
		patient.reindexObject()


		@deprecated("Us senaite.core.api.dtime.to_dt instead")

Migrate sample's DateOfBirth field to AgeDateOfBirthField #74

Migrate sample's DateOfBirth field to AgeDateOfBirthField #74

Conversation

xispa commented May 16, 2023 • edited Loading

Description of the issue/feature this PR addresses

Current behavior before PR

Desired behavior after PR is merged

ramonski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xispa commented May 18, 2023

Choose a reason for hiding this comment

xispa commented May 16, 2023 •

edited

Loading