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.

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

feat: [DHIS2-14799] Working list for follow up #3521

Merged
merged 11 commits into from
Mar 6, 2024

Conversation

eirikhaugstulen
Copy link
Contributor

TECH-summary:

  • Added a filter button with type TRUE_ONLY
  • Filters the API by followUp status

The design doc also mentions that this should be available to select in the ColumnSelector, but I'm not sure if this makes sense. FollowUp is an attribute of the enrollment itself, meaning we have no good way of displaying this. Not sure what you think.

@eirikhaugstulen eirikhaugstulen marked this pull request as ready for review February 2, 2024 08:28
@eirikhaugstulen eirikhaugstulen requested a review from a team as a code owner February 2, 2024 08:28
@simonadomnisoru
Copy link
Contributor

@eirikhaugstulen IMHO this feature needs more design clarification. Some doubts that came to mind:

  • Is Follow up one on the main filter and it is always visible just like the Enrollment status or is one of the secondary ones hidden under "More filters"?
  • Clarify if Follow up will appear in the ColumnSelector and in the table (for program stage WL we already combine events and TEIs metadata in the ColumnSelector and the table, therefore it should also be possible to add enrollment metadata too)

Thanks!

@eirikhaugstulen eirikhaugstulen marked this pull request as draft February 4, 2024 08:41
@eirikhaugstulen eirikhaugstulen marked this pull request as ready for review February 6, 2024 07:15
@eirikhaugstulen
Copy link
Contributor Author

Hey @simonadomnisoru! Thanks for the review. I had some discussions with Joakim and Karoline, and the consensus was that we put in More filters, but we don't add it as a column 😊 I added a flag on useFiltersOnly that would add the filter in the correct spot. Thank you!

Copy link
Contributor

@simonadomnisoru simonadomnisoru left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@JoakimSM JoakimSM left a comment

Choose a reason for hiding this comment

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

Couple of comments that might or might not be in the back of your head:

  • It seems like boolean value type filters have changed from a multi-select to a single-select. Great if we can keep multi-select for the value type.
  • Boolean value type filters are stored as a string representation in Redux when you add the filter (workingListsMeta/teiList/filters/). When the filter is loaded from a template, it is stored as a javascript boolean primitive. Great if we can always save them as javascript booleans. The idea was to store the "client-formatted" value here, and it would be great to continue doing so (boolean for boolean value type, number for number/integer value type, ISO-string for date etc. See the converters formToClient, clientToServer etc.)

The follow-up part seems to work well with an updated backend 👍

@eirikhaugstulen eirikhaugstulen marked this pull request as draft March 5, 2024 01:52
@JoakimSM JoakimSM marked this pull request as ready for review March 5, 2024 18:24
Copy link

github-actions bot commented Mar 5, 2024

@Bekkalizer Bekkalizer merged commit 0ad3891 into master Mar 6, 2024
37 checks passed
@Bekkalizer Bekkalizer deleted the eh/feat/DHIS2-14799_WorkingListForFollowUp branch March 6, 2024 00:28
dhis2-bot added a commit that referenced this pull request Mar 6, 2024
# [100.66.0](v100.65.0...v100.66.0) (2024-03-06)

### Features

* [DHIS2-14799] Working list for follow up ([#3521](#3521)) ([0ad3891](0ad3891))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.66.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants