fix(#8815): fix contact-summary calculation to use max (500) fetched reports #8984

Benmuiruri · 2024-04-02T12:28:02Z

Description

This PR fixes the unintended bug where the number of reports that would be retrieved for the contact and passed to the contact-summary calculation was capped at 50. So, the contact-summary for contacts associated with >50 reports, would not have the full context of all that contacts reports.

With this fix, for the sake of accuracy, we provide all the reports associated with that contact (limit of 500 reports).

Before the fix

Video showing created woman has pregnancy card when reports are less than 50

Screen.Recording.2024-03-27.at.21.04.44.mov

Video showing pregnancy card for woman does not appear when reports than 50 reports and the pregnancy report is not part of the 50

Screen.Recording.2024-03-27.at.21.13.38.mov

After the fix

Closes #8815

Code review checklist

Readable: Concise, well named, follows the style guide, documented if necessary.
Documented: Configuration and user documentation on cht-docs
Tested: Unit and/or e2e where appropriate
Internationalised: All user facing text
Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

Compose URLs

If Build CI hasn't passed, these may 404:

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

Benmuiruri · 2024-04-03T15:04:47Z

This fix adopts the limit of reports for a user used in reports.component.ts which sets the limit to 500.

However, unlike the reports.component.ts this fix does not implement lazy loading of the reports. Therefore, it does not fetch reports in batches of 50, instead if a user has 500 reports, the spinner will wait until all reports are fetched before displaying them in the contact-detail.

Below is a performance test on Samsung Galaxy S23+ using a CHW with just 1 household with 5 people household used in the video above.

Performance (Telemetry) for contact with 2 reports

"contact_detail:contact:load": {
      "sum": 2046,
      "min": 86,
      "max": 557,
      "count": 13,
      "sumsqr": 509392
    },
    "contact_detail:contact:load:apdex:satisfied": {
      "sum": 2046,
      "min": 86,
      "max": 557,
      "count": 13,
      "sumsqr": 509392
    },
    "contact_detail:contact:load:contact_data": {
      "sum": 316,
      "min": 12,
      "max": 52,
      "count": 13,
      "sumsqr": 8754
    },
    "contact_detail:contact:load:load_contact_summary": {
      "sum": 36,
      "min": 2,
      "max": 6,
      "count": 13,
      "sumsqr": 120
    },
    "contact_detail:contact:load:load_descendants": {
      "sum": 352,
      "min": 13,
      "max": 82,
      "count": 13,
      "sumsqr": 13754
    },
    "contact_detail:contact:load:load_reports": {
      "sum": 506,
      "min": 14,
      "max": 138,
      "count": 13,
      "sumsqr": 32494
    },
    "contact_detail:contact:load:load_targets": {
      "sum": 179,
      "min": 6,
      "max": 22,
      "count": 13,
      "sumsqr": 2953
    },
    "contact_detail:contact:load:load_tasks": {
      "sum": 645,
      "min": 20,
      "max": 297,
      "count": 13,
      "sumsqr": 99193
    }

Performance (Telemetry) for contact with 52 reports

"contact_detail:contact:load": {
      "sum": 11957,
      "min": 167,
      "max": 7325,
      "count": 11,
      "sumsqr": 57385949
    },
    "contact_detail:contact:load:apdex:satisfied": {
      "sum": 4632,
      "min": 167,
      "max": 1607,
      "count": 10,
      "sumsqr": 3730324
    },
    "contact_detail:contact:load:apdex:tolerable": {
      "sum": 7325,
      "min": 7325,
      "max": 7325,
      "count": 1,
      "sumsqr": 53655625
    },
    "contact_detail:contact:load:contact_data": {
      "sum": 1690,
      "min": 15,
      "max": 775,
      "count": 11,
      "sumsqr": 826898
    },
    "contact_detail:contact:load:load_contact_summary": {
      "sum": 79,
      "min": 3,
      "max": 18,
      "count": 11,
      "sumsqr": 825
    },
    "contact_detail:contact:load:load_descendants": {
      "sum": 983,
      "min": 12,
      "max": 448,
      "count": 11,
      "sumsqr": 330535
    },
    "contact_detail:contact:load:load_reports": {
      "sum": 2437,
      "min": 26,
      "max": 1188,
      "count": 11,
      "sumsqr": 1818973
    },
    "contact_detail:contact:load:load_targets": {
      "sum": 1735,
      "min": 32,
      "max": 578,
      "count": 11,
      "sumsqr": 516949
    },
    "contact_detail:contact:load:load_tasks": {
      "sum": 5017,
      "min": 25,
      "max": 4312,
      "count": 11,
      "sumsqr": 18658219
    }

Performance (Telemetry) for contact with 490 reports

"contact_detail:contact:load": {
      "sum": 741392,
      "min": 8435,
      "max": 111124,
      "count": 12,
      "sumsqr": 52127429276
    },
    "contact_detail:contact:load:apdex:frustrated": {
      "sum": 732957,
      "min": 45465,
      "max": 111124,
      "count": 11,
      "sumsqr": 52056280051
    },
    "contact_detail:contact:load:apdex:tolerable": {
      "sum": 8435,
      "min": 8435,
      "max": 8435,
      "count": 1,
      "sumsqr": 71149225
    },
    "contact_detail:contact:load:contact_data": {
      "sum": 2650,
      "min": 2650,
      "max": 2650,
      "count": 1,
      "sumsqr": 7022500
    },
    "contact_detail:contact:load:load_contact_summary": {
      "sum": 24,
      "min": 24,
      "max": 24,
      "count": 1,
      "sumsqr": 576
    },
    "contact_detail:contact:load:load_descendants": {
      "sum": 1658,
      "min": 1658,
      "max": 1658,
      "count": 1,
      "sumsqr": 2748964
    },
    "contact_detail:contact:load:load_reports": {
      "sum": 2669,
      "min": 2669,
      "max": 2669,
      "count": 1,
      "sumsqr": 7123561
    },
    "contact_detail:contact:load:load_targets": {
      "sum": 561,
      "min": 561,
      "max": 561,
      "count": 1,
      "sumsqr": 314721
    },
    "contact_detail:contact:load:load_tasks": {
      "sum": 713553,
      "min": 872,
      "max": 101141,
      "count": 12,
      "sumsqr": 48683546167
    }

The performance when loading all 500 reports would be a huge concern (It would be frustrating every time). Given we want to include ALL records for accuracy's sake, I am investigating whether it is possible to use All 500 records to calculate the contact-summary but use lazy loading to load the contacts in batches.

cc @latin-panda @michaelkohn

michaelkohn · 2024-04-03T17:42:16Z

The performance when loading all 500 reports would be a huge concern (It would be frustrating every time). Given we want to include ALL records for accuracy's sake, I am investigating whether it is possible to use All 500 records to calculate the contact-summary but use lazy loading to load the contacts in batches.

Not sure if this is any easier but it would probably also be fine to limit the "3 month" view to 50 (since it is selected by default) but have "6 month" and "View all" show everything applicable.

latin-panda · 2024-04-04T02:03:42Z

This fix adopts the limit of reports for a user used in reports.component.ts which sets the limit to 500.

Why 500 and not another number? Just curious how it was picked

Benmuiruri · 2024-04-04T06:10:46Z

This fix adopts the limit of reports for a user used in reports.component.ts which sets the limit to 500.

Why 500 and not another number? Just curious how it was picked

@latin-panda as I was exploring how the search function works I came across the limit set in reports.component.ts. Git shows you added the 500 limit 16 months ago. I'd also be interested to know why 500.

latin-panda · 2024-04-04T06:33:13Z

@Benmuiruri If you use git-blame to track down the changes, it will take you to 7y ago (commit) and this is the conversation how they came up with this number, previously the report list page was loading 1000 reports but that's too much for the bulk delete api.

Bulk delete isn't related to what we're doing here. I'm unsure if this number is still relevant for this case - is it too high or too low?

Benmuiruri · 2024-04-04T09:10:16Z

I have adopted @michaelkohn suggestion.

When user clicks on the 3 months or 6 month filter for the Reports they can only see 50 reports.
Same for tasks, when the user clicks 1 week or 2 weeks filter for Tasks, they can only see 50 tasks.

I added the constraint for Tasks for consistency. In master, if a contact has 400 tasks and 400 reports. The contact detail page displays all 400 tasks but only 50 reports (whether there is a filter of the reports or not, you cannot see reports > 50).

With my implementation, if a contact has 400 tasks and 400 reports.

The filtered Tasks when 1 week or 2 weeks is selected will display 50 tasks, when View all is selected, it will display all 400 tasks.
The filtered Reports when 3 months or 6 months is selected will display 50 reports, when View all is selected, it will display all 400 reports.

Video demo

Screen.Recording.2024-04-04.at.11.25.12.mov

Two points for discussion.

@latin-panda I am also unsure whether 500 is the right number, perhaps the answer might be in looking at how many reports users typically have in PostgreSQL to come up with the number? @michaelkohn.
We will need to open another issue to implement pagination / lazy loading when View all is selected.

Benmuiruri · 2024-04-05T06:40:42Z

Hi @dianabarsan I have requested you for a review of my implementation of the fix while we explore the right number to represent All Reports. I put 500 for now.

The review request is to get your thoughts on

limiting filtered reports to 50 and loading All reports when View all is clicked
Setting a fixed number to represent All Reports
e2e test

michaelkohn · 2024-04-05T18:25:52Z

FWIW... I don't have a problem setting a max of 500, though if it were super easy to implement, it would probably be better to show 50 and if there are more than 50, have a "Show more" link that redirects them to the Reports page, prefiltered to that Contact. It wouldn't be possible to do that for Tasks since the Tasks page doesn't have a filter, but it's also a lot less likely to have that many tasks (unless something is mis-configured).

Benmuiruri · 2024-04-08T09:30:53Z

Hi @michaelkohn based on your comment, here is my suggestion.

Let's limit the scope of this task to fix the bug of including All Reports when calculating contact-summary.
Let's agree on what number represents All Reports. Currently I have set it to 500.
Let's limit the display of Reports and Tasks on the Contact Detail page to 50. Regardless of whether someone has clicked 3 months, 6 months or View All. (The reason to limit is we do not want the app to freeze attempting to fetch All Reports )
Open another issue to implement efficiently loading All Reports if user clicks View All

michaelkohn · 2024-04-08T14:52:43Z

Let's limit the scope of this task to fix the bug of including All Reports when calculating contact-summary.

Good idea

Let's agree on what number represents All Reports. Currently I have set it to 500.

Feels weird to call it "All Reports" when it's actually "max reports" (since we are limiting it to 500). FWIW, from a functional perspective I think 500 is totally fine. I ran some quick queries and in one big project only 0.2% of patients had more than 500 reports associated to them and that's server-side (and probably not intentional) so I'd imagine client-side, after purging, there are even fewer patients, if any, that have more than 500.

Let's limit the display of Reports and Tasks on the Contact Detail page to 50. Regardless of whether someone has clicked 3 months, 6 months or View All. (The reason to limit is we do not want the app to freeze attempting to fetch All Reports )

Cool.

Open another issue to implement efficiently loading All Reports if user clicks View All

I appreciate the awareness around scope-creep on this ticket, but we aren't going to do the efficient loading work unless we hear the need so I'd actually rather just not even create the ticket since it'll just sit there open/un-addressed until we hear about it.

Benmuiruri · 2024-04-11T07:48:09Z

Hi @ralfudx when you get some time, please review this, let's try and merge it this week. Thanks.

ralfudx

@Benmuiruri it looks good to me

Benmuiruri · 2024-04-12T04:57:55Z

Hi @latin-panda given Rafa's QA approval. Please give this PR a once over before I merge it. Thanks.

latin-panda

Well done! Just a feedback below